Re: [PATCH v2 2/2] hwmon: raspberrypi: Add voltage input support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Unconditional self-rescheduling of the background workqueue leads to a use-after-free on device removal.
- [Medium] The driver incorrectly detects and exposes unsupported voltage channels due to unverified firmware responses.
--

commit aa874218705dba1c84bc41a7c3b61abd93bbf0a2
Author: Shubham Chakraborty <chakrabortyshubham66@gmail.com>

hwmon: raspberrypi: Add voltage input support

This commit extends the raspberrypi-hwmon driver to expose firmware-provided
voltage measurements. It adds core and SDRAM voltage inputs, converting
the values from microvolts to millivolts for the hwmon subsystem.
Inputs are only exposed if the firmware reports support for the corresponding
voltage ID.

> diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
> index a2938881ccd2f..4f96f37116f32 100644
> --- a/drivers/hwmon/raspberrypi-hwmon.c
> +++ b/drivers/hwmon/raspberrypi-hwmon.c

[ ... ]

> @@ -56,6 +70,23 @@ static void rpi_firmware_get_throttled(struct rpi_hwmon_data *data)
>  	hwmon_notify_event(data->hwmon_dev, hwmon_in, hwmon_in_lcrit_alarm, 0);
>  }
>  
> +static int rpi_firmware_get_voltage(struct rpi_hwmon_data *data, u32 id,
> +				    long *val)
> +{
> +	struct rpi_firmware_get_value packet;
> +	int ret;
> +
> +	packet.id = cpu_to_le32(id);
> +	packet.val = 0;
> +	ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_VOLTAGE,
> +				    &packet, sizeof(packet));

If the firmware does not support the RPI_FIRMWARE_GET_VOLTAGE tag or the
specific voltage ID, could the mailbox transaction still return 0 without
modifying the tag buffer?

If so, packet.val would remain 0 and this function would return success.
During probe, this would cause the channel to be incorrectly flagged as
supported, exposing sysfs files that permanently read 0mV.

Would it be safer to initialize packet.val to a sentinel value like ~0
before querying the firmware to verify the voltage ID is actually supported?

> +	if (ret)
> +		return ret;
> +
> +	*val = le32_to_cpu(packet.val) / 1000;
> +	return 0;
> +}
> +
>  static void get_values_poll(struct work_struct *work)
>  {
>  	struct rpi_hwmon_data *data;

Does the unconditional rescheduling in get_values_poll() cause a
use-after-free regression during driver removal?

If the work function executes concurrently with driver unbind,
cancel_delayed_work_sync() will wait for it to finish. However,
get_values_poll() currently reschedules itself just before returning:

get_values_poll() {
        ...
        schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
}

After cancellation completes, devres frees the memory for data. When the
rescheduled work executes later, it accesses the freed data pointer.

Could a shutdown flag be added to safely prevent rescheduling during
teardown?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516191555.17978-1-chakrabortyshubham66@gmail.com?part=2