On Tue, Aug 11, 2026 at 10:31:47AM +0200, Krzysztof Kozlowski <[email protected]>
wrote:
> > Reviewed-by: Joel Wittenauer <[email protected]>
> > Reviewed-by: Thi Nguyen <[email protected]>
>
> These two reviews did not happen, drop the tags. The code has trivial
> mistakes from downstream code. Internal review would point these trivial
> issues, so I do not believe you received valid, meaningful internal
> review which would justify the tags.
We will drop them. Any future Reviewed-by will come from on-list review.
> > +MODULE_ALIAS("platform:cmh");
>
> Why?
Redundant with the OF match table; we will remove it.
> > +MODULE_IMPORT_NS("CRYPTO_INTERNAL");
>
> Why all this is in completely odd place, not next to the module init
> code?
We will move it next to the module code.
> > +static struct cmh_device *g_cmh_dev;
>
> Eh, no. Don't implement singletons.
We will remove the singleton and use dev_get_drvdata() /
platform_get_drvdata().
> > + dev_info(cmh_dev(), "CMH eSW fully operational ...\n", sw_boot);
>
> Drop
We will remove the dev_info() logging across the driver,
keeping only genuine error paths.
> > + if (g_cmh_dev)
> > + return -EBUSY;
>
> NAK
We will drop the single-instance guard together with the singleton.
> > + dev_info(&pdev->dev, "loading v%s\n", CMH_VERSION);
>
> NAK and internal review should tell you that
Will remove.
> > + dev_info(cmh_dev(), "sic_base=0x%llx size=0x%zx mbx_count=%u\n",
>
> How many dev_info are you going to print?
Will remove.
> > + dev_info(cmh_dev(), "enabled %d clock(s)\n", ret);
>
> Again?
Will remove.
> > + dev_err(cmh_dev(), "failed to acquire reset GPIO (rc=%d)\n",
> > ret);
>
> Syntax is ret = dev_err_probe and internal review should tell you that.
We will use return dev_err_probe(...) on the probe error paths.
> > + (void)cmh_debugfs_init(cfg);
>
> Why the cast?
We will drop the cast and check the return.
> > + g_cmh_dev = dev;
>
> Nope (and we do not call things "g" from globals)
Will be gone with the singleton removal.
> > + dev_info(cmh_dev(), "initialized successfully\n");
>
> NAK
Will remove.
> > + dev_info(&pdev->dev, "unloaded successfully\n");
>
> NAK
Will remove.
> > + .name = CMH_DRV_NAME,
>
> Use name directly.
We will use "cmh" directly.
> > + * g_cmh_dev -- that would defeat deferred probe.
> > + */
>
> Why do you need this? Why your driver is done differently than all
> others?
We will drop the custom init/exit and use module_platform_driver().
> > +module_init(cmh_init);
> > +module_exit(cmh_exit);
>
> And this is not module_platform_driver because...?
Will switch to module_platform_driver() in v4.
> > + dev_info(cmh_dev(), "MQI init complete (%u mailboxes)\n",
> > cfg->mbx_count);
>
> NAK
Will remove.
> > + dev_info(cmh_dev(), "MQI cleanup complete\n");
>
> NAK
Will remove.
> > + dev_info(cmh_dev(), "rh: no mailbox IRQs -- polling mode\n");
>
> So a warning or info?
Will remove -- polling is a normal configuration, not worth logging.
> > + dev_warn(cmh_dev(), "rh: no IRQs -- using watchdog polling
> > ...\n",
>
> And here it is a warning?
Will remove.
> > + {
>
> What is with this indentation?
We will drop the naked block and restructure.
> > + dev_info(cmh_dev(), "rh: initialized (%u IRQs, ...)\n",
>
> One more dev_info....
Will remove.
> > + dev_dbg(cmh_dev(), "rh: suspended\n");
>
> No, you cannot have even debug messages which re-implement and
> duplicated standard tracing and standard debugging code.
We will remove these debug messages.
> > +static unsigned int backlog_max_depth = 1024;
> > +module_param(backlog_max_depth, uint, 0444);
>
> Drop all module params, not really right way to do things.
We will drop all module parameters and move the genuinely useful knobs
to debugfs.
> > +#define CMH_VERSION "1.0.0"
>
> NAK, this is not allowed in Linux kernel code. Do you see drivers
> (except complete junk from AMD) doing this?
We will remove the driver-version define entirely.
Thanks for the review.
Regards,
Alex
