Re: [PATCH 4/8] platform: arm64: qcom-hamoa-ec: Add fan RPM query and LUT calibration

On 7/28/2026 11:45 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Race condition between suspend/remove paths and `fan_calib_work_fn` can cause a 10 to 20-second hang.
> - [High] `psy_work` can be queued and executed while the device is in suspend, leading to I2C transfer failures.
> --
> 
> --- Patch [4]: [PATCH 4/8] platform: arm64: qcom-hamoa-ec: Add fan RPM query and LUT calibration ---
> commit e49733d2c63b28585a159eb3f465f9613c0aec7d
> Author: Anvesh Jain P <[email protected]>
> 
> platform: arm64: qcom-hamoa-ec: Add fan RPM query and LUT calibration
> 
> This commit adds EC command definitions and handler functions for querying
> fan RPM and programming per-fan LUTs mapping SoC temperature to target RPM.
> It implements a runtime calibration work item to determine the maximum RPM,
> and programs the LUT once calibration finishes and upon power-supply changes.
> 
>> diff --git a/drivers/platform/arm64/qcom-hamoa-ec.c b/drivers/platform/arm64/qcom-hamoa-ec.c
>> index f6ff77d4e8f69..d0c45d9970eee 100644
>> --- a/drivers/platform/arm64/qcom-hamoa-ec.c
>> +++ b/drivers/platform/arm64/qcom-hamoa-ec.c
> [ ... ]
>> @@ -665,6 +1097,49 @@ static void qcom_ec_soc_tj_work_fn(struct work_struct *work)
>>  	queue_delayed_work(system_percpu_wq, &ec->soc_tj_work, EC_SOC_TJ_TEMP_POLL_JIFFIES);
>>  }
>>  
>> +static void qcom_ec_fan_calib_work_fn(struct work_struct *work)
>> +{
>> +	struct qcom_ec *ec = container_of(work, struct qcom_ec, fan_calib_work);
>> +	struct device *dev = &ec->client->dev;
>> +	int i, ret;
>> +
>> +	reinit_completion(&ec->calib_abort);
> 
> [Severity: High]
> Can this race with the suspend or remove paths and cause a long delay?
> 
> If qcom_ec_fan_calib_work_fn() begins executing concurrently with
> qcom_ec_suspend() or qcom_ec_remove(), the suspend/remove thread calls
> complete_all(&ec->calib_abort) to signal the worker to abort.
> 
> However, if the worker executes reinit_completion() immediately after the
> suspend thread's complete_all(), the abort signal is erased. The suspend
> thread then blocks in cancel_work_sync(), waiting for the worker to finish,
> and the worker will block in wait_for_completion_interruptible_timeout()
> for the full timeout per fan (up to 10-20 seconds total).
>

I'll guard against this, probably by checking completion state before
reinit or using a separate abort flag instead of relying on
reinit_completion() alone.

>> +
>> +	mutex_lock(&ec->io_lock);
>> +	ec->calibrating = true;
>> +	mutex_unlock(&ec->io_lock);
>> +
> [ ... ]
>> @@ -690,7 +1169,16 @@ static int qcom_ec_suspend(struct device *dev)
>>  	struct qcom_ec *ec = i2c_get_clientdata(client);
>>  	int ret;
>>  
>> -	disable_delayed_work_sync(&ec->soc_tj_work);
>> +	if (ec->lut_cfg && ec->thermal_cap.fan_cnt) {
>> +		complete_all(&ec->calib_abort);
>> +		cancel_work_sync(&ec->fan_calib_work);
>> +	}
>> +
>> +	if (ec->lut_cfg)
>> +		cancel_work_sync(&ec->psy_work);
> 
> [Severity: High]
> Does this allow psy_work to be re-queued and executed during suspend?
> 
> While cancel_work_sync() stops currently pending work, the system-wide
> power supply notifier qcom_ec_psy_notifier() is not unregistered during
> suspend. 
> 
> If a power supply event occurs during the suspend process after
> cancel_work_sync() has run, the notifier can immediately queue psy_work
> back to the non-freezable system_long_wq.
> 
> This could cause the work to run and attempt I2C transfers while the
> I2C controller is suspended or the EC is in Modern Standby, potentially
> leading to bus lockups or suspend failures.
>

I'll switch that to disable_work_sync() so the notifier can't requeue it
mid-suspend.

>> +
>> +	if (ec->lut_cfg && ec->lut_cfg->send_soc_tj)
>> +		disable_delayed_work_sync(&ec->soc_tj_work);
>>  
>>  	mutex_lock(&ec->io_lock);
>>  	ret = i2c_smbus_write_byte_data(client, EC_MODERN_STANDBY_CMD,
> 

-- 
Best Regards,
Anvesh