Re: [PATCH 3/8] platform: arm64: qcom-hamoa-ec: Switch fan profile based on power supply state

[Anvesh Jain P <anvesh.p@oss.qualcomm.com>]

On 7/28/2026 11:35 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Missing synchronization for `psy_work` during suspend causes I2C transfers in standby state.
> - [Low] Missing Kconfig dependency on POWER_SUPPLY causes build failures.
> - [Low] Unused static function `qcom_ec_get_fan_profile` triggers compiler warnings.
> --
> 
> --- Patch [3]: [PATCH 3/8] platform: arm64: qcom-hamoa-ec: Switch fan profile based on power supply state ---
> commit 14490e0de249214f9c1b00a4aad8f89c2d05843e
> Author: Anvesh Jain P <anvesh.p@oss.qualcomm.com>
> 
> platform: arm64: qcom-hamoa-ec: Switch fan profile based on power supply state
> 
> This commit adds EC command definitions and handlers to switch the EC fan
> profile. It registers a power supply notifier to track AC/battery state
> transitions and queues a work item to apply the corresponding fan profile.
> 
>> diff --git a/drivers/platform/arm64/qcom-hamoa-ec.c b/drivers/platform/arm64/qcom-hamoa-ec.c
>> index 4c745b78322c4..f6ff77d4e8f69 100644
>> --- a/drivers/platform/arm64/qcom-hamoa-ec.c
>> +++ b/drivers/platform/arm64/qcom-hamoa-ec.c
> 
> [ ... ]
> 
>> @@ -351,6 +359,140 @@ static int qcom_ec_send_soc_tj_temp(struct qcom_ec *ec)
>>  	return ret;
>>  }
>>  
> 
> [ ... ]
> 
>> +static int qcom_ec_get_fan_profile(struct qcom_ec *ec, u8 *profile_id)
>> +{
>> +	int ret;
>> +
>> +	ret = i2c_smbus_read_byte_data(ec->client, EC_FAN_PROFILE_CMD);
> 
> [Severity: Low]
> Is this function called anywhere in the driver? It appears to be added
> but never used. 
> 
> Because it is static and lacks the __maybe_unused attribute, will this
> trigger a -Wunused-function compiler warning?
>

It's used in the very next patch of the series, looks like this was
flagged without factoring in the rest of the series.
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	*profile_id = (u8)ret & GENMASK(3, 0);
>> +
>> +	return 0;
>> +}
> 
> [ ... ]
> 
>> +static int qcom_ec_psy_notifier(struct notifier_block *nb,
>> +				unsigned long event, void *ptr)
>> +{
>> +	struct qcom_ec *ec = container_of(nb, struct qcom_ec, psy_nb);
>> +
>> +	if (event != PSY_EVENT_PROP_CHANGED)
>> +		return NOTIFY_DONE;
>> +
>> +	queue_work(system_long_wq, &ec->psy_work);
> 
> [Severity: High]
> Is it safe to leave this work item un-synchronized during suspend?
> 
> Looking at the suspend path, soc_tj_work is properly disabled before the EC
> enters modern standby, but psy_work is not:
> 
> qcom_ec_suspend() {
>     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);
>     ...
> }
> 
> Since psy_work is queued on system_long_wq (which is not freezable), if a
> power supply event like an AC unplug occurs right before or during suspend,
> psy_work could execute concurrently with or after the EC is put into standby.
> 
> If the work runs, it sends the EC_FAN_PROFILE_CMD over I2C. Will this wake
> or corrupt the EC state, or potentially deadlock if the I2C controller has
> already suspended? 
> 
> Should psy_work be disabled during suspend as well?
>

I'll make sure psy_work is disabled around the standby-enter write too,
same as soc_tj_work.

>> +
>> +	return NOTIFY_OK;
>> +}
> 
> [ ... ]
> 
>> @@ -630,6 +777,17 @@ static int qcom_ec_probe(struct i2c_client *client)
>>  		}
>>  	}
>>  
>> +	ec->psy_nb.notifier_call = qcom_ec_psy_notifier;
>> +	ret = power_supply_reg_notifier(&ec->psy_nb);
> 
> [Severity: Low]
> Building without CONFIG_POWER_SUPPLY will lead to link errors here because
> power_supply_reg_notifier() and power_supply_unreg_notifier() are called
> unconditionally, but have no stub implementations in the power_supply.h
> header.
> 
> Does this driver need a Kconfig dependency on POWER_SUPPLY to prevent build
> failures?
>

I'll add a depends on POWER_SUPPLY line to the Kconfig entry.

>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to register power supply notifier\n");
>> +
>> +	ret = devm_add_action_or_reset(dev, qcom_ec_unreg_psy_notifier, &ec->psy_nb);
>> +	if (ret)
>> +		return ret;
>> +
>> +	qcom_ec_update_profile_from_power_supply(ec);
>> +
>>  	return 0;
>>  }
> 

-- 
Best Regards,
Anvesh