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