Re: [PATCH] hwmon: Add driver for MSI Mystic Light controller voltage sensors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] hwmon: msi_mystic_light: probe fails in msi_ml_probe() when
  CONFIG_HIDRAW=n
- [Low] hwmon: msi_mystic_light: 32-bit jiffies wraparound in
  msi_ml_update()
--

commit e43662ecc3664e6bdfc441610eb41666bfc00492
Author: Tomasz Bojanowski <tomasz@bojanowski.me>

hwmon: Add driver for MSI Mystic Light controller voltage sensors

This commit adds a hwmon driver for the voltage sensors exposed via HID
feature report 0xf0 on the MSI Mystic Light USB controller found on MSI
800-series motherboards.

> diff --git a/drivers/hwmon/msi_mystic_light.c b/drivers/hwmon/msi_mystic_light.c
> new file mode 100644
> index 0000000000000..39dde9d9e82d9
> --- /dev/null
> +++ b/drivers/hwmon/msi_mystic_light.c
[ ... ]
> +struct msi_ml_data {
> +	struct hid_device *hdev;
> +	struct device *hwmon_dev;
> +	const struct msi_ml_board *board;
> +	u8 *buf;			/* report buffer, must be DMA safe */
> +	u16 in[MSI_ML_MAX_IN];		/* mV, 0 = no reading */
> +	unsigned long updated;		/* jiffies */
> +	bool valid;
> +};
[ ... ]
> +static int msi_ml_update(struct msi_ml_data *priv)
> +{
> +	const struct msi_ml_board *board = priv->board;
> +	u8 *buf = priv->buf;
> +	int i, ret;
> +
> +	if (priv->valid && time_before(jiffies, priv->updated + MSI_ML_CACHE_TIME))
> +		return 0;

[Severity: Low]
Can the 32-bit jiffies comparison in msi_ml_update() wrap around on 32-bit
kernels?  If the sensor is read once and then not polled for 2^31 jiffies
(~24.8 days at HZ=1000), time_before() evaluates to true again and
msi_ml_update() returns stale cached readings for the next ~24.8 days.
Using u64 with get_jiffies_64() and time_before64() avoids the wraparound.

[ ... ]

> +static int msi_ml_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
[ ... ]
> +	ret = hid_parse(hdev);
> +	if (ret)
> +		return ret;
> +
> +	/* Keep the hidraw interface for user space LED control */
> +	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> +	if (ret)
> +		return ret;

[Severity: Medium]
Does msi_ml_probe() fail with -ENODEV when CONFIG_HIDRAW is disabled?

SENSORS_MSI_MYSTIC_LIGHT only depends on USB_HID in Kconfig.  When
CONFIG_HIDRAW=n, hidraw_connect() fails and leaves hdev->claimed as 0;
because msi_ml_driver does not define a .raw_event callback and
HID_CONNECT_DRIVER was not passed to hid_hw_start(), hid_connect() rejects
the device:

drivers/hid/hid-core.c:hid_connect() {
    ...
	if (!hdev->claimed && !hdev->driver->raw_event) {
		hid_err(hdev, "device has no listeners, quitting\n");
		return -ENODEV;
	}
    ...
}

As a result, the hwmon device is never registered even though reading the
voltage sensors via hid_hw_raw_request() does not require hidraw.  Passing
HID_CONNECT_HIDRAW | HID_CONNECT_DRIVER to hid_hw_start() avoids this
failure.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106184357.27763-1-tomasz@bojanowski.me?part=1