Re: [PATCH 2/8] platform: arm64: qcom-hamoa-ec: Add SoC junction temperature reporting

On 7/28/2026 11:32 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> - [High] Race condition in probe leading to NULL pointer dereference in IRQ handler
> - [High] Incorrect error code on device_link_add failure causes permanent probe failure
> - [Medium] Missing `io_lock` acquisition in fan control functions breaks EC command serialization
> - [Medium] Thermal polling delayed work unconditionally requeues itself even if no thermal zones were found
> - [Medium] Suspend error path permanently disables thermal polling
> - [Medium] Integer overflow when accumulating args.args[0] into total leads to heap buffer overflow
> --
> 
> --- Patch [2]: [PATCH 2/8] platform: arm64: qcom-hamoa-ec: Add SoC junction temperature reporting ---
> commit d5d00275c6c452ac414dd45c11ca38e64c17b24a
> Author: Anvesh Jain P <[email protected]>
> 
> platform: arm64: qcom-hamoa-ec: Add SoC junction temperature reporting
>     
> Add the EC command definitions and handler function for reporting the
> SoC junction temperature (Tj) to the EC.
> 
> [ ... ]
> 
>> diff --git a/drivers/platform/arm64/qcom-hamoa-ec.c b/drivers/platform/arm64/qcom-hamoa-ec.c
>> index 5ca7308c60774..4c745b78322c4 100644
>> --- a/drivers/platform/arm64/qcom-hamoa-ec.c
>> +++ b/drivers/platform/arm64/qcom-hamoa-ec.c
> 
> [ ... ]
> 
>> @@ -77,10 +89,14 @@ struct qcom_ec_cooling_dev {
>>  };
>>  
>>  struct qcom_ec {
>> +	struct i2c_client *client;
>>  	struct qcom_ec_cooling_dev *ec_cdev;
>> +	struct thermal_zone_device **soc_tj_zones;
>> +	struct delayed_work soc_tj_work;
>> +	struct mutex io_lock; /* serializes EC command sequences */
> 
> [Severity: Medium]
> Does this mutex also need to be acquired in existing fan control functions
> like qcom_ec_set_fan_profile()?
> 
> Because the thermal framework can invoke fan control at any time, these
> commands might interleave with the new background thermal polling delayed
> work, which could violate the intended EC firmware serialization.
>

Yeah, both callers already hold io_lock around that call, same as
everywhere else in the driver.

>>  	struct qcom_ec_thermal_cap thermal_cap;
>>  	struct qcom_ec_version version;
>> -	struct i2c_client *client;
>> +	int num_soc_tj_zones;
>>  };
> 
> [ ... ]
> 
>> +static int qcom_ec_setup_soc_tj_zones(struct qcom_ec *ec)
>> +{
>> +	struct device *dev = &ec->client->dev;
>> +	struct device_node *np = dev->of_node;
>> +	int nproviders, total, i;
>> +
>> +	nproviders = of_property_count_elems_of_size(np, "qcom,tsens",
>> +						     2 * sizeof(u32));
>> +	if (nproviders <= 0)
>> +		return 0;
>> +
>> +	total = 0;
>> +	for (i = 0; i < nproviders; i++) {
>> +		struct of_phandle_args args;
>> +
>> +		if (of_parse_phandle_with_fixed_args(np, "qcom,tsens", 1, i, &args))
>> +			return -EINVAL;
>> +
>> +		of_node_put(args.np);
>> +		total += args.args[0];
> 
> [Severity: Medium]
> Is it possible for the total accumulation to overflow if the device tree
> provides large values for the qcom,tsens property?
> 
> If the sum surpasses the maximum integer value, devm_kcalloc() would allocate
> an undersized array, and the subsequent loop could cause a heap buffer
> overflow when writing to ec->soc_tj_zones.
>

I'm dropping qcom,tsens in favor of thermal-sensors, so this
accumulation code goes away entirely.

>> +	}
>> +
>> +	ec->soc_tj_zones = devm_kcalloc(dev, total, sizeof(*ec->soc_tj_zones),
>> +					GFP_KERNEL);
>> +	if (!ec->soc_tj_zones)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < nproviders; i++) {
>> +		struct of_phandle_args args;
>> +		struct platform_device *sensor_pdev;
>> +		u32 id;
>> +
>> +		if (of_parse_phandle_with_fixed_args(np, "qcom,tsens", 1, i, &args))
>> +			return -EINVAL;
>> +
>> +		sensor_pdev = of_find_device_by_node(args.np);
>> +		if (!sensor_pdev) {
>> +			of_node_put(args.np);
>> +			return -EPROBE_DEFER;
>> +		}
>> +
>> +		/* Ensure we unbind before the sensor frees its zones */
>> +		if (!device_link_add(dev, &sensor_pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER)) {
>> +			dev_err(dev, "Failed to link to sensor %pOF\n", args.np);
>> +			put_device(&sensor_pdev->dev);
>> +			of_node_put(args.np);
>> +			return -ENODEV;
> 
> [Severity: High]
> Should this return -EPROBE_DEFER instead of -ENODEV?
> 
> If the EC driver is probing and the sensor is not yet bound to a driver,
> device_link_add() refuses to create a managed link and returns NULL.
> Returning -ENODEV causes the probe to fail permanently, whereas returning
> -EPROBE_DEFER would allow it to try again once the sensor driver binds.
>

No, we already defer earlier if the sensor pdev isn't registered yet, so
that's not why this would fail. The only other case is a dependency
cycle, which won't fix itself on retry, so -ENODEV is right here.

>> +		}
>> +		put_device(&sensor_pdev->dev);
> 
> [ ... ]
> 
>> +static void qcom_ec_soc_tj_work_fn(struct work_struct *work)
>> +{
>> +	struct qcom_ec *ec = container_of(work, struct qcom_ec, soc_tj_work.work);
>> +
>> +	qcom_ec_send_soc_tj_temp(ec);
>> +
>> +	queue_delayed_work(system_percpu_wq, &ec->soc_tj_work, EC_SOC_TJ_TEMP_POLL_JIFFIES);
> 
> [Severity: Medium]
> Should the work item only be requeued if thermal zones were actually found?
> 
> If ec->num_soc_tj_zones is 0, qcom_ec_send_soc_tj_temp() evaluates valid as 0
> and does nothing, and then the work unconditionally requeues itself every
> 125ms, creating a permanent timer tick on systems without these sensors.
>

I'll fix this in the next series, bail out early if num_soc_tj_zones is
0 instead of requeuing unconditionally.

>> +}
>> +
>>  static int qcom_ec_resume(struct device *dev)
> 
> [ ... ]
> 
>>  static int qcom_ec_suspend(struct device *dev)
>>  {
>>  	struct i2c_client *client = to_i2c_client(dev);
>> +	struct qcom_ec *ec = i2c_get_clientdata(client);
>> +	int ret;
>>  
>> -	return i2c_smbus_write_byte_data(client, EC_MODERN_STANDBY_CMD,
>> -					 EC_MODERN_STANDBY_ENTER);
>> +	disable_delayed_work_sync(&ec->soc_tj_work);
>> +
>> +	mutex_lock(&ec->io_lock);
>> +	ret = i2c_smbus_write_byte_data(client, EC_MODERN_STANDBY_CMD,
>> +					EC_MODERN_STANDBY_ENTER);
>> +	mutex_unlock(&ec->io_lock);
>> +
>> +	return ret;
> 
> [Severity: Medium]
> If the I2C write for the modern standby command fails, does the error path
> leave the thermal polling permanently disabled?
> 
> The PM core aborts the system suspend on error but does not call the resume
> callback for the device that failed suspend, so enable_delayed_work() won't
> be called. Should the error path re-enable the delayed work before returning?
>

I'll fix that, re-enable and requeue soc_tj_work on the error path
before returning so polling doesn't stay stuck off.

>>  }
>>  
>>  static int qcom_ec_probe(struct i2c_client *client)
> 
> [ ... ]
> 
>> @@ -370,10 +587,24 @@ static int qcom_ec_probe(struct i2c_client *client)
>>  	if (ret < 0)
>>  		return dev_err_probe(dev, ret, "Failed to enable SCI events\n");
>>  
>> +	ret = devm_add_action_or_reset(dev, qcom_ec_sci_evt_disable, dev);
>> +	if (ret)
>> +		return ret;
>> +
>>  	ret = qcom_ec_thermal_capabilities(dev);
>>  	if (ret < 0)
>>  		return dev_err_probe(dev, ret, "Failed to read thermal capabilities\n");
>>  
>> +	ret = qcom_ec_setup_soc_tj_zones(ec);
> 
> [Severity: High]
> Can an SCI event trigger the IRQ handler before the probe finishes, causing
> a NULL pointer dereference?
> 
> The IRQ is requested and SCI events are enabled on the EC prior to calling
> qcom_ec_setup_soc_tj_zones(), which allocates ec->ec_cdev further down in
> the probe function.
> 
> If an event fires in this widened window, or if the setup fails and returns
> an error, qcom_ec_irq() can execute before devres disables the IRQ. The
> handler checks if the fan_id is less than ec->thermal_cap.fan_cnt, which
> would be true, and then dereferences the unallocated ec_cdev array.
>

I'll move the IRQ request and SCI-event enable to after the fan cooling
devices and ec_cdev array are set up, so the handler can't fire before
ec_cdev is allocated.

>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret, "Failed to setup SoC Tj thermal zones\n");
>> +
>> +	ret = devm_delayed_work_autocancel(dev, &ec->soc_tj_work, qcom_ec_soc_tj_work_fn);
>> +	if (ret)
>> +		return ret;
>> +
>> +	queue_delayed_work(system_percpu_wq, &ec->soc_tj_work, EC_SOC_TJ_TEMP_POLL_JIFFIES);
> 
> [Severity: Medium]
> Could we avoid queueing the work initially if ec->num_soc_tj_zones is 0?
> 
> Similar to the requeue path, this starts a background polling process even
> if no thermal zones were found during setup.
>

I'll make sure that only queues when num_soc_tj_zones is non-zero.

>> +
>>  	if (ec->thermal_cap.fan_cnt == 0) {
>>  		dev_warn(dev, FW_BUG "Failed to get fan count, firmware update required\n");
>>  		return 0;
> 

-- 
Best Regards,
Anvesh