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

[sashiko-bot@kernel.org]

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 <anvesh.p@oss.qualcomm.com>

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).

> +
> +	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.

> +
> +	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,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-ec_add_more_commands-v1-0-771abd65ee1a@oss.qualcomm.com?part=4