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

[sashiko-bot@kernel.org]

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?

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

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

> +	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;
>  }

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