Re: [PATCH v2 2/2] arm64: dts: qcom: purwa-iot-evk: Add Embedded controller node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Configuring the EC interrupt as IRQ_TYPE_EDGE_FALLING causes a stuck IRQ line and lost events.

Pre-existing issues:
- [High] The driver registers all cooling devices with cdev_id = 0, breaking multi-fan thermal device tree bindings.
- [High] Probe function fails to disable EC SCI events on error paths, leading to corrupted EC state and unhandled interrupts.
- [High] qcom_ec_read() ignores partial I2C block reads, causing callers to use uninitialized stack memory as hardware state.
--

commit 30f797509dd26f2c0981b525ba3d62d1173ae857
Author: Anvesh Jain P <anvesh.p@oss.qualcomm.com>

arm64: dts: qcom: purwa-iot-evk: Add Embedded controller node

This commit adds the embedded controller node for Purwa IOT EVK boards.
It configures fan control, temperature sensors, access to EC internal state
changes, and suspend notifications to the EC.

> diff --git a/arch/arm64/boot/dts/qcom/purwa-iot-evk.dts b/arch/arm64/boot/dts/qcom/purwa-iot-evk.dts
> index ad503beec1d3d..491ea7769cd71 100644
> --- a/arch/arm64/boot/dts/qcom/purwa-iot-evk.dts
> +++ b/arch/arm64/boot/dts/qcom/purwa-iot-evk.dts
> @@ -837,6 +837,16 @@ eusb6_repeater: redriver@4f {
[ ... ]
> +
> +	embedded-controller@76 {
> +		compatible = "qcom,purwa-iot-evk-ec", "qcom,hamoa-crd-ec";
> +		reg = <0x76>;
> +
> +		interrupts-extended = <&tlmm 66 IRQ_TYPE_EDGE_FALLING>;

[Severity: High]
Does configuring the EC interrupt as IRQ_TYPE_EDGE_FALLING cause a stuck
IRQ line and lost events?

The EC asserts an interrupt while multiple events are pending, but the driver's
interrupt handler reads exactly one event and returns without looping. 

Because the trigger is configured as edge-falling, the persistent low state
of the line does not trigger another interrupt, causing the line to stall
permanently.  Should this use level-triggered interrupts instead?

> +
> +		pinctrl-0 = <&ec_int_n_default>;
> +		pinctrl-names = "default";
> +	};
>  };


While reviewing this patch, I also noticed a few pre-existing issues in
drivers/platform/arm64/qcom-hamoa-ec.c that handles this device tree node:

drivers/platform/arm64/qcom-hamoa-ec.c:qcom_ec_probe()
[ ... ]
> 	ret = qcom_ec_sci_evt_control(dev, true);
> 	if (ret < 0)
> 		return dev_err_probe(dev, ret, "Failed to enable SCI events\n");
> 
> 	ret = qcom_ec_thermal_capabilities(dev);
> 	if (ret < 0)
> 		return dev_err_probe(dev, ret, "Failed to read thermal capabilities\n");

[Severity: High]
This is a pre-existing issue, but does the probe function fail to disable
EC SCI events on error paths?

If qcom_ec_thermal_capabilities() or the subsequent memory allocation fails,
probe returns an error. The previously enabled SCI events are never disabled
because no devm action was registered and remove() is not called.

The EC is left actively generating events, but the OS interrupt handler is
freed by devres. The EC will assert the IRQ line, which goes unhandled,
potentially stalling the EC and causing battery drain.


drivers/platform/arm64/qcom-hamoa-ec.c:qcom_ec_probe()
[ ... ]
> 	for (i = 0; i < ec->thermal_cap.fan_cnt; i++) {
> 		struct qcom_ec_cooling_dev *ec_cdev = &ec->ec_cdev[i];
> 		char name[EC_FAN_NAME_SIZE];
> 
> 		scnprintf(name, sizeof(name), "qcom_ec_fan_%u", i);
> 		ec_cdev->fan_id = i + 1;
> 		ec_cdev->parent_dev = dev;
> 
> 		ec_cdev->cdev = devm_thermal_of_cooling_device_register(dev, 0, NULL, ec_cdev,
> 									&qcom_ec_thermal_ops);

[Severity: High]
This isn't a bug introduced by this patch, but does hardcoding the cdev_id 
to 0 break multi-fan thermal device tree bindings?

By looping over fan_cnt and passing 0 for every fan instance rather than
the iteration index, the OF thermal core cannot distinguish between multiple
cooling devices under the same node.  

This breaks thermal zone mappings and can cause undefined behavior when
binding thermal states.


drivers/platform/arm64/qcom-hamoa-ec.c:qcom_ec_read()
[ ... ]
> static int qcom_ec_read(struct qcom_ec *ec, u8 cmd, u8 resp_len, u8 *resp)
> {
> 	int ret;
> 
> 	ret = i2c_smbus_read_i2c_block_data(ec->client, cmd, resp_len, resp);
> 	if (ret < 0)
> 		return ret;
> 	else if (ret == 0 || ret == 0xff)
> 		return -EOPNOTSUPP;
> 
> 	if (resp[0] >= resp_len)
> 		return -EINVAL;
> 
> 	return 0;
> }

[Severity: High]
This isn't a bug introduced by this patch, but does qcom_ec_read() ignore
partial I2C block reads?

i2c_smbus_read_i2c_block_data() returns the number of bytes read. If a short
read occurs, this returns success (0) because it only checks if ret < 0,
ret == 0, or ret == 0xff. It does not verify if ret matches the expected
resp_len.

Callers are then exposed to using uninitialized stack memory as hardware
state for firmware version data, fan counts, or thermistor masks. This can
lead to out-of-bounds memory allocations if a bogus fan count is read.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-ec_support_for_purwa_evk-v2-0-c59aac5c6aac@oss.qualcomm.com?part=2