Date:   Fri, 21 Jul 2023 08:30:51 +0200
From:   Paul Menzel <pmenzel@...gen.mpg.de>
To:     Iwona Winiarska <iwona.winiarska@...el.com>
Cc:     tmaimon77@...il.com, avifishman70@...il.com,
        Benjamin Fair <benjaminfair@...gle.com>,
        devicetree@...r.kernel.org, venture@...gle.com,
        linux-kernel@...r.kernel.org, warp5tw@...il.com,
        conor+dt@...nel.org, openbmc@...ts.ozlabs.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org
Subject: Re: [PATCH 2/4] peci: Add peci-npcm controller driver

Dear Iwona,


Am 20.07.23 um 22:20 schrieb Winiarska, Iwona:
> On Thu, 2023-07-20 at 16:47 +0200, Paul Menzel wrote:

>> Am 20.07.23 um 10:38 schrieb Winiarska, Iwona:
>>> On Thu, 2023-07-20 at 08:20 +0200, Paul Menzel wrote:
>>
>>>> Am 20.07.23 um 00:08 schrieb Iwona Winiarska:
>>>>> From: Tomer Maimon <tmaimon77@...il.com>
>>>>>
>>>>> Add support for Nuvoton NPCM BMC hardware to the Platform Environment
>>>>> Control Interface (PECI) subsystem.
>>>>
>>>> Please elaborate on the implementation, and document the used datasheets.
>>>
>>> As far as I know, there is no publicly available documentation.
>>
>> Too bad. Documenting the datasheet name and version is still important,
>> so developers could request it, and it can be mapped, once they are made
>> public.
> 
> Sorry, unfortunately I can't help with that, I don't have access to any Nuvoton
> docs. Perhaps Tomer can provide more information?

Hopefully. But I wonder, how can you develop and review the patch then?

>>>> Additionally, please document how you tested this.
>>>
>>> Are you asking to include this information in the commit message?
>>
>> Yes.
>>
>>> That would be unusual.
>>> But in general - it's a controller driver, it allows PECI subsystem to detect
>>> devices behind it and for PECI drivers to bind to those devices.
>>
>> Having a test line in the commit message is not unusual at. So people
>> with systems where it doesn’t work, could replicate the test setup to at
>> least verify that it works in that configuration.
> 
> It's unusual as almost none of the commits in Linux kernel contain "how to test
> it" description.

I saw some commits document on what hardware it was tested.

> The explanation body in the commit message should explain *why* the patch was
> created, not how to test it.

I disagree. It should of course the why, but sometimes also the how 
(implementation), the used datasheets, and all other details making it 
easy to review and give reviewers without the hardware confidence, that 
the patch is good.

> And when taken as a series - it's self documenting. There's a Kconfig which
> allows to enable/disable the driver, and there are bindings which show what
> platform contains the hardware that is compatible with it.

I just meant: Tested on server X with BMC Y using Nuvoton Z. Driver 
registered correctly, and devices A were discovered.

>>>>> Signed-off-by: Tomer Maimon <tmaimon77@...il.com>
>>>>> Signed-off-by: Tyrone Ting <warp5tw@...il.com>
>>>>> Co-developed-by: Iwona Winiarska <iwona.winiarska@...el.com>
>>>>> Signed-off-by: Iwona Winiarska <iwona.winiarska@...el.com>
>>>>> ---
>>>>>     drivers/peci/controller/Kconfig     |  16 ++
>>>>>     drivers/peci/controller/Makefile    |   1 +
>>>>>     drivers/peci/controller/peci-npcm.c | 298 ++++++++++++++++++++++++++++
>>>>>     3 files changed, 315 insertions(+)
>>>>>     create mode 100644 drivers/peci/controller/peci-npcm.c
>>>>>
>>>>> diff --git a/drivers/peci/controller/Kconfig
>>>>> b/drivers/peci/controller/Kconfig
>>>>> index 2fc5e2abb74a..4f9c245ad042 100644
>>>>> --- a/drivers/peci/controller/Kconfig
>>>>> +++ b/drivers/peci/controller/Kconfig

[…]

>>>>> +#if IS_ENABLED(CONFIG_DYNAMIC_DEBUG)
>>>>> +       dev_dbg(priv->dev, "addr : %#02x, tx.len : %#02x, rx.len : %#02x\n",
>>>>> +               addr, req->tx.len, req->rx.len);
>>>>> +       print_hex_dump_bytes("TX : ", DUMP_PREFIX_NONE, req->tx.buf, req-tx.len);
>>>>> +#endif
>>>>
>>>> The preprocessor guards are not needed, as it’s taken care of in
>>>> `include/linux/printk.h`. Also in other parts of the patch.
>>>
>>> Since this is dumping the raw contents of PECI messages, it's going to be quite
>>> verbose. The idea behind preprocessor guard is that we don't ever want this to
>>> be converted to regular printk. If there's no dynamic debug available - this
>>> won't be printed unconditionally (even with -DDEBUG).
>>
>> How will it be converted to a regular printk?
>>
>>       #if defined(CONFIG_DYNAMIC_DEBUG) || \
>>           (defined(CONFIG_DYNAMIC_DEBUG_CORE) && defined(DYNAMIC_DEBUG_MODULE))
>>       #define print_hex_dump_debug(prefix_str, prefix_type, rowsize,      \
>>                                groupsize, buf, len, ascii)        \
>>           dynamic_hex_dump(prefix_str, prefix_type, rowsize,      \
>>                            groupsize, buf, len, ascii)
>>       #elif defined(DEBUG)
>>       #define print_hex_dump_debug(prefix_str, prefix_type, rowsize,      \
>>                                groupsize, buf, len, ascii)                \
>>           print_hex_dump(KERN_DEBUG, prefix_str, prefix_type, rowsize,    \
>>                          groupsize, buf, len, ascii)
>>       #else
>>       static inline void print_hex_dump_debug(const char *prefix_str, int prefix_type,
>>                                           int rowsize, int groupsize,
>>                                           const void *buf, size_t len, bool ascii)
>>       {
>>       }
>>       #endif
> 
> Let's consider 3 scenarios
> 1) Dynamic debug is available
> 2) Dynamic debug is not available, but we're built with -DDEBUG
> 3) Dynamic debug is not available, we're built without -DDEBUG
> 
> For 1), print_hex_dump_debug is dynamic - it can be controlled
> (enabled/disabled) using dynamic debug knobs (debugfs / dyndbg kernel arg).
> For 2), print_hex_dump_debug is using print_hex_dump, which is just using printk
> with KERN_DEBUG level under the hood.
> For 3), it's compiled out.
> 
> And it's scenario 2) that we would like to avoid, as hex-dumping all PECI
> communication into dmesg is likely going to make dmesg output unusable (can
> overflow, printing that to terminal is going to be slow, etc).
> 
> The dump can be useful, it's just that in order to be useful it needs the
> dynamic debug facilities :)

Thank you for the explanation. Currently, this is only used in the PECI 
subsystem:

     $ git grep 'if IS_ENABLED(CONFIG_DYNAMIC_DEBUG)'
     drivers/mtd/nand/raw/nand_base.c:#if 
IS_ENABLED(CONFIG_DYNAMIC_DEBUG) || defined(DEBUG)
     drivers/peci/controller/peci-aspeed.c:#if 
IS_ENABLED(CONFIG_DYNAMIC_DEBUG)
     drivers/peci/controller/peci-aspeed.c:#if 
IS_ENABLED(CONFIG_DYNAMIC_DEBUG)
     include/linux/mtd/rawnand.h:#if IS_ENABLED(CONFIG_DYNAMIC_DEBUG) || 
defined(DEBUG)

I think, it will only cause confusing for users, wondering why it does 
not show up with `-DDEBUG`. I assume the Linux kernel offers other ways 
to do what you are trying to achieve. Maybe using a dump_traffic knob or 
so in `/sys`.


Kind regards,

Paul

Powered by blists - more mailing lists