Re: [PATCH v2 4/4] rtc: pcf8525: Add temperature sensor support via hwmon

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The hwmon interface is exposed to userspace before the hardware is configured, and remains permanently exposed in a broken state if hardware configuration fails.
- [Medium] Hardware monitoring functionality is implemented and registered directly from the RTC driver instead of using an auxiliary device, violating HWMON subsystem layering guidelines.
- [Medium] The PCF8525_CTRL5_TEMP_RD_EN bit is enabled during probe but is never disabled when the driver is removed or unbound.
--

--- Patch [4]: [PATCH v2 4/4] rtc: pcf8525: Add temperature sensor support via hwmon ---
commit 78a53fcd5265022e4a59c395bbca42f812fe19b7
Author: Shiv Prakash Gupta <shivprakash.gupta@nxp.com>

rtc: pcf8525: Add temperature sensor support via hwmon

Register the PCF8525 internal temperature sensor through the hwmon
interface when CONFIG_RTC_DRV_PCF8525_HWMON is enabled.

Exposes temp1_input (millidegrees Celsius, read-only) and
update_interval (milliseconds, read/write).

> diff --git a/drivers/rtc/rtc-pcf8525.c b/drivers/rtc/rtc-pcf8525.c
> index 415615ea0a406..4b8ac0c6fb23e 100644
> --- a/drivers/rtc/rtc-pcf8525.c
> +++ b/drivers/rtc/rtc-pcf8525.c

[ ... ]

> +static void pcf8525_hwmon_register(struct device *dev,
> +				   struct pcf8525 *pcf8525)
> +{
> +	struct device *hwmon_dev;
> +	int ret;
> +
> +	if (!IS_ENABLED(CONFIG_RTC_DRV_PCF8525_HWMON))
> +		return;
> +
> +	/*
> +	 * Register the hwmon device first.  Only enable the hardware
> +	 * temperature readout afterwards so that the device is not left
> +	 * enabled if registration fails.
> +	 */
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, "pcf8525",
> +							 pcf8525,
> +							 &pcf8525_hwmon_chip_info,
> +							 NULL);

[Severity: Medium]
Should this hardware monitoring functionality be implemented as an auxiliary
device driver residing in drivers/hwmon/ instead?

According to the hwmon subsystem guidelines, if a chip's primary function is
not hardware monitoring, its hwmon functionality should be implemented as an
auxiliary device, rather than registering directly from another subsystem like
drivers/rtc/.

> +	if (IS_ERR(hwmon_dev)) {
> +		dev_warn(dev, "failed to register HWMON device: %ld\n",
> +			 PTR_ERR(hwmon_dev));
> +		return;
> +	}
> +
> +	/* Enable digital readout; preserve TSIE, CL and XTL_TYP. */
> +	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL5,
> +				 PCF8525_CTRL5_TEMP_RD_EN,
> +				 PCF8525_CTRL5_TEMP_RD_EN);

[Severity: High]
Does registering the hwmon device before configuring the hardware expose a
broken interface to userspace?

If userspace reads the sysfs attributes immediately after
devm_hwmon_device_register_with_info() completes but before this
regmap_update_bits() call, it might read invalid data. 

Also, if this regmap_update_bits() fails, the hwmon device is left
registered but permanently broken since there's no rollback of the hwmon
registration.

[Severity: Medium]
Does this code leak the hardware state when the driver is unbound?

The PCF8525_CTRL5_TEMP_RD_EN bit is enabled here, but there doesn't appear
to be a corresponding devm_add_action_or_reset() or similar cleanup to
disable it upon driver removal.

Leaving internal analog sensors active on an RTC can cause excessive
power drain from the backup battery when the driver is unloaded.

> +	if (ret)
> +		dev_warn(dev, "failed to enable temperature readout: %d\n", ret);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824065531.693701-1-shivprakash.gupta@nxp.com?part=4