Re: [PATCH v2 7/8] Add Advantech EIO Thermal driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The driver registers thermal zones in a loop but fails to unregister them on probe failure and completely lacks a `.remove` callback.
- [High] The `.set_trip_temp` callback incorrectly uses the abstract trip ID as a control register address, overwriting critical read-only state registers.
- [Medium] Multi-byte variable addresses (`int`, `enum`) are passed directly to hardware access functions which read bytes in host memory order, breaking on big-endian architectures.
- [Low] The child driver communicates with the parent using a bespoke inter-driver function instead of utilizing standard MFD mechanisms.
- [Low] Error codes from `pmc_write` are masked during the trip iteration in `eio_tz_change_mode`.
--

--- Patch [7]: [PATCH v2 7/8] Add Advantech EIO Thermal driver ---
commit 001e79fa1d8c0bd96fb7bffbb91c6090c53b08a4
Author: Ramiro Oliveira <ramiro.oliveira@advantech.com>

Add Advantech EIO Thermal driver

This commit adds the driver to control the Advantech EIO Thermal block,
this block is included in the Advantech EIO Embedded Controller.

> diff --git a/drivers/thermal/eio_thermal.c b/drivers/thermal/eio_thermal.c
> index 0000000000000..ad923acaf1389 100644
> --- /dev/null
> +++ b/drivers/thermal/eio_thermal.c
[ ... ]
> +static int pmc_write(struct device *mfd, u8 ctrl, u8 dev_id, void *data)
> +{
> +	if (ctrl >= ARRAY_SIZE(pmc_len))
> +		return -EINVAL;
> +
> +	struct pmc_op op = {
> +		.cmd       = CMD_THERM_WRITE,
> +		.control   = ctrl,
> +		.device_id = dev_id,
> +		.payload   = (u8 *)data,
> +		.size      = pmc_len[ctrl],
> +	};
> +
> +	return eio_core_pmc_operation(mfd, &op);

[Severity: Low]
This isn't a bug, but does calling a bespoke helper function exported by
the parent MFD driver conflict with the MFD subsystem guidelines? The MFD
documentation suggests child drivers should use standard APIs and avoid
custom accessors or helper functions in the parent to pass state.

> +}
[ ... ]
> +static int eio_tz_set_trip_temp(struct thermal_zone_device *tzd,
> +				const struct thermal_trip *trip, int temp)
> +{
> +	struct eio_thermal_dev *eio_thermal = thermal_zone_device_priv(tzd);
> +	const u8 ctl = (uintptr_t)trip->priv;
> +	u16 val;
> +
> +	if (temp < 1000)
> +		return -EINVAL;
> +
> +	val = MILLI_CELSIUS_TO_DECI_KELVIN(temp);
> +	return pmc_write(eio_thermal->mfd, ctl, eio_thermal->ch, &val);

[Severity: High]
Does this write to the wrong register?

During probe, trip->priv is initialized with logical indices like
TRIP_SHUTDOWN (which is 0). Here, the ctl variable is passed directly to
pmc_write() as the control register address. Will this overwrite state
registers like CTRL_STATE (0x00) instead of the limit registers like
CTRL_SHUTDOWN_HI (0x41)?

> +}
> +
> +static int eio_tz_change_mode(struct thermal_zone_device *tzd,
> +			      enum thermal_device_mode mode)
> +{
> +	struct eio_thermal_dev *eio_thermal = thermal_zone_device_priv(tzd);
> +	int trip;
> +	int ret = 0;
> +
> +	for (trip = 0; trip < TRIP_NUM; trip++) {
> +		ret = pmc_write(eio_thermal->mfd, ctrl_map[trip], eio_thermal->ch, &mode);

[Severity: Medium]
Will this fail to set the mode correctly on big-endian architectures?

The mode variable is an enum (typically 4 bytes), but it's passed as a byte
array payload to pmc_write(). Because pmc_write() only copies 1 or 2 bytes,
won't it read the most significant bytes (usually zeros) on big-endian
systems?

[Severity: Low]
Could an early error in this loop be masked? If pmc_write() fails on the
first iteration but succeeds on the next, ret will be overwritten with 0,
hiding the failure from the caller.

> +		if (ret)
> +			dev_err(eio_thermal->dev, "Error when %s trip num %d\n",
> +				mode == THERMAL_DEVICE_ENABLED ? "enabling" : "disabling",
> +				trip);
> +	}
> +
> +	return ret;
> +}
[ ... ]
> +static int eio_thermal_probe(struct platform_device *pdev)
> +{
[ ... ]
> +		/* Throttle starts a 1C increase it */
> +		int throttle_temp = MILLI_CELSIUS_TO_DECI_KELVIN(60000);
> +
> +		pmc_write(dev->parent, CTRL_THROTTLE_HI, (u8)ch, &throttle_temp);

[Severity: Medium]
Does this have the same big-endian issue as eio_tz_change_mode()?

The throttle_temp variable is a 32-bit int, and taking its address for a
1 or 2 byte payload write in pmc_write() might read the zeroed upper bytes
on big-endian systems.

[ ... ]
> +		tzd = thermal_zone_device_register_with_trips(therm_name[name],
> +							      trips,
> +							      ntrips,
> +							      eio_th,
> +							      &zone_ops,
> +							      &zone_params,
> +							      THERMAL_PASSIVE_DELAY,
> +							      THERMAL_POLLING_DELAY);
> +		if (IS_ERR(tzd))
> +			return PTR_ERR(tzd);

[Severity: High]
Does this path leak resources if a failure occurs after some zones have been
successfully registered?

If the loop successfully registers a thermal zone on ch=0 but fails on ch=1,
it returns immediately without unregistering the thermal zone created for
ch=0.

> +		/* Make sure zones start disabled */
> +		thermal_zone_device_disable(tzd);
> +
> +		dev_info(dev, "%s thermal up (ch=%d)\n", therm_name[name], ch);
> +	}
> +
> +	return 0;
> +}
> +
> +static struct platform_driver eio_thermal_driver = {
> +	.probe  = eio_thermal_probe,
> +	.driver = {
> +		.name = "eio_thermal",
> +	},
> +};

[Severity: High]
Are there use-after-free and resource leak bugs when this driver is unbound?

Because there is no .remove callback in the platform driver, unbinding it via
sysfs will leave the registered thermal zones active in the thermal core
while devres frees the eio_th private data. Could this lead to a
use-after-free when the core polls the zones?

> +module_platform_driver(eio_thermal_driver);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-upstream-v2-v2-0-76e5e41026db@advantech.com?part=7