Re: [PATCH 2/4] peci: Add peci-npcm controller driver
From: Paul Menzel
Date: Fri Jul 21 2023 - 02:32:10 EST
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?
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@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:
$ 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