Re: [PATCH 2/4] peci: Add peci-npcm controller driver

["Winiarska, Iwona" <iwona.winiarska@intel.com>]

On Fri, 2023-07-21 at 08:30 +0200, Paul Menzel wrote:
> 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@gmail.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?

It is explained in the cover letter.
Also, the review is not only about verifying driver/hardware interactions.
In fact - we often have to trust the author, because the docs are not available.
Interactions between software (other kernel components), whether the driver is a
good fit within its subsystem, etc. are just as important, and it's something
that we can review without the docs.

> 
> > > > > 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.

But it will be persisted for eternity in the git log.
And it is only about the review of the series as a whole, which means that
ultimately, having this information in individual commits is just adding noise.

> 
> > 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.

The series (after my modifications) was tested by Tomer from Nuvoton:
https://lore.kernel.org/openbmc/CAP6Zq1h1if4hyubyh6N8EOdGOu+zp0qVUimF-9L2eXZ-QFAYjw@mail.gmail.com/
I can link this in the cover letter.

> 
> > > > > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> > > > > > Signed-off-by: Tyrone Ting <warp5tw@gmail.com>
> > > > > > Co-developed-by: Iwona Winiarska <iwona.winiarska@intel.com>
> > > > > > Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.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:

That's simply not true.
The same approach is used in other subsystems as well, sometimes it covers
individual printk:
https://elixir.bootlin.com/linux/v6.4/source/drivers/rpmsg/rpmsg_ns.c#L40
In other cases it covers custom wrappers:
https://elixir.bootlin.com/linux/v6.4/source/drivers/usb/host/ehci-dbg.c#L8

There are more examples in the tree, but the general idea is the same - if the
log is verbose and printed often (lies on a hotpath), and we can't ratelimit,
hiding it behind dynamic debug availability is an option to consider.

> 
>      $ 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`.

Adding a new sysfs ABI for debug prints? No.
Alternative would be to use tracepoints, but that's semi-stable and until now we
only had one controller driver, so, for now, I would prefer to postpone any PECI
tracepoint conversions.

Thanks
-Iwona

> 
> 
> Kind regards,
> 
> Paul