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

[Tomer Maimon]

Hi Paul,

Thanks for your comments.

On Fri, 21 Jul 2023 at 12:22, Winiarska, Iwona
<iwona.winiarska@xxxxxxxxx> wrote:
>
> 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@xxxxxxxxx>
> > > > > > >
> > > > > > > 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.
As Iwona mentions in the cover letter,
The series adds support for PECI on Nuvoton-based BMC boards.
It is based on patches that were sent by Nuvoton and we checking Iwona
upstream NPCM PECI driver on NPCM systems.
Iwona, in case you like to have the NPCM BMC datasheet, it can be
provided under NDA to relevant companies.
>
> >
> > > > > > 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@xxxxxxxxxxxxxx/
> I can link this in the cover letter.
>
> >
> > > > > > > Signed-off-by: Tomer Maimon <tmaimon77@xxxxxxxxx>
> > > > > > > Signed-off-by: Tyrone Ting <warp5tw@xxxxxxxxx>
> > > > > > > Co-developed-by: Iwona Winiarska <iwona.winiarska@xxxxxxxxx>
> > > > > > > Signed-off-by: Iwona Winiarska <iwona.winiarska@xxxxxxxxx>
> > > > > > > ---
> > > > > > >     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
>
Iwona, we highly appreciate that you upstream Nuvoton NPCM PECI driver :-)

Thanks,

Tomer