Re: [PATCH v2 1/1] hwmon: Add driver for the NCT6801D EC sensor window

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] hwmon: lenovo-shm-sensors: lenovo_shm_init() ignores probe
  failure and DMI
- [Medium] hwmon: lenovo-shm-sensors: unchecked chip ID and 0xffffffff
  base in probe
- [Low] hwmon: lenovo-shm-sensors: missing Documentation/hwmon/ driver
  doc
--

commit 9f416da4c0f5af4f99bd963bf60bcd4e26a13658
Author: Jiqi Li <lijq9@lenovo.com>

hwmon: Add driver for the NCT6801D EC sensor window

This commit adds the lenovo-shm-sensors hwmon driver to read fan and
temperature sensors published in an EC shared-memory window on supported
Lenovo systems.

> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 8d397fa089fac..d020a2a25b98d 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1100,6 +1100,24 @@ config SENSORS_LENOVO_EC
>            This driver can also be built as a module. If so, the module
>            will be called lenovo_ec_sensors.
>  
> +config SENSORS_LENOVO_SHM
> +	tristate "Sensor reader for Lenovo EC sensor windows"

[Severity: Low]
This adds a new hwmon driver without corresponding documentation in
Documentation/hwmon/lenovo-shm-sensors.rst as required by
Documentation/hwmon/submitting-patches.rst.

> diff --git a/drivers/hwmon/lenovo-shm-sensors.c b/drivers/hwmon/lenovo-shm-sensors.c
> new file mode 100644
> index 0000000000000..0aaf27eade57d
> --- /dev/null
> +++ b/drivers/hwmon/lenovo-shm-sensors.c

[ ... ]

> +static phys_addr_t lenovo_shm_find_window(const struct lenovo_shm_layout *layout,
> +					  const struct lenovo_shm_chip **chip)
> +{
> +	static const int ports[] = { SIO_PORT_2E, SIO_PORT_4E };
> +	struct lenovo_shm_data probe = { .layout = layout };
> +	u32 bases[ARRAY_SIZE(lenovo_shm_shaw_reg)];
> +	phys_addr_t found = 0;
> +	int p, i, n;
> +
> +	for (p = 0; p < ARRAY_SIZE(ports) && !found; p++) {
> +		const struct lenovo_shm_chip *c;
> +		int port = ports[p];
> +
> +		if (lenovo_shm_sio_enter(port))
> +			continue;
> +
> +		/*
> +		 * The chip ID is only used for the log message. DMI has
> +		 * already selected the layout and the window content check
> +		 * below is what guards against a wrong mapping, so an
> +		 * unexpected ID is not a reason to give up here.
> +		 */
> +		c = lenovo_shm_identify(port);
> +
> +		lenovo_shm_sio_select(port, SIO_LD_SHM);
> +		n = 0;
> +		for (i = 0; i < ARRAY_SIZE(lenovo_shm_shaw_reg); i++) {
> +			u32 base = lenovo_shm_read_shaw(port, lenovo_shm_shaw_reg[i]);
> +
> +			/*
> +			 * SHAWnBA_3-0 == 0 means the window is disabled
> +			 * (Datasheet 5.3.3). The Core side WIN_BASEn
> +			 * register has its own disabled marker, but it is
> +			 * not reachable from the host, so the only further
> +			 * check available here is the content check below.
> +			 */
> +			if (base)
> +				bases[n++] = base;
> +		}
> +		lenovo_shm_sio_exit(port);
> +
> +		/* Validate outside the configuration-region lock. */
> +		for (i = 0; i < n && !found; i++) {
> +			/*
> +			 * Claim the window only for the content check and drop
> +			 * it again right after; probe takes a devm-managed claim
> +			 * for the lifetime of the device.
> +			 */
> +			if (!request_mem_region(bases[i], SHM_SIZE, DRVNAME))
> +				continue;
> +			probe.regs = ioremap(bases[i], SHM_SIZE);
> +			if (probe.regs && lenovo_shm_window_valid(&probe)) {

[Severity: Medium]
Does lenovo_shm_find_window() end up modifying LDN selection on other
Super-I/O chips and mapping 0xffffffff when a probed port does not match
lenovo_shm_identify()?

Because lenovo_shm_find_window() does not skip the port when c is NULL and
only checks if (base) is non-zero, an unpopulated port (or an unused SHAW
register) reading 0xff produces base == 0xffffffff, which is then passed to
request_mem_region() and ioremap(), while a different Super-I/O chip at 0x2e
has its LDN 0x0f selected and its registers treated as physical addresses to
map and read.

[ ... ]

> +static struct platform_driver lenovo_shm_driver = {
> +	.driver = {
> +		.name = DRVNAME,
> +	},
> +	.probe = lenovo_shm_probe,
> +};
> +
> +static struct platform_device *lenovo_shm_pdev;
> +
> +static int __init lenovo_shm_init(void)
> +{
> +	int ret;
> +
> +	ret = platform_driver_register(&lenovo_shm_driver);
> +	if (ret)
> +		return ret;
> +
> +	lenovo_shm_pdev = platform_device_register_simple(DRVNAME, -1, NULL, 0);
> +	if (IS_ERR(lenovo_shm_pdev)) {
> +		ret = PTR_ERR(lenovo_shm_pdev);
> +		platform_driver_unregister(&lenovo_shm_driver);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
Does lenovo_shm_init() succeed and leave lenovo_shm_pdev and
lenovo_shm_driver registered even when DMI does not match or
lenovo_shm_probe() fails?

platform_device_register_simple() does not propagate errors from the driver's
.probe callback because __device_attach_driver() ignores probe failures on
the bus. When lenovo_shm_probe() returns -ENODEV on non-matching hardware or
-EBUSY when FAAS bit 3 is set, lenovo_shm_init() still returns 0, keeping the
module loaded and leaving an unbound platform device and driver registered.
Checking dmi_check_system() in lenovo_shm_init() and using
platform_create_bundle() (as in drivers/hwmon/lenovo-ec-sensors.c) would
propagate probe failures.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113102544.893158-2-lijq9@lenovo.com?part=1