Re: [RFC PATCH 3/3] arm64: dts: qcom: tiare: describe CW2017 fuel gauge

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] power: supply: cw2017: sample invalidation in cw2017_suspend() breaks resume
--

commit e63b82695c67205a9de8a30516bebe6f4181c2ad
Author: Kupreenko Danila <keckerz@yandex.ru>

arm64: dts: qcom: tiare: describe CW2017 fuel gauge

This commit enables the BLSP2 I2C1 controller and adds the CellWise CW2017
fuel gauge node at address 0x63 for the Xiaomi Redmi Go (msm8917-xiaomi-tiare).

> diff --git a/arch/arm64/boot/dts/qcom/msm8917-xiaomi-tiare.dts b/arch/arm64/boot/dts/qcom/msm8917-xiaomi-tiare.dts
> index fe844230030f2..6d72a4763ac48 100644
> --- a/arch/arm64/boot/dts/qcom/msm8917-xiaomi-tiare.dts
> +++ b/arch/arm64/boot/dts/qcom/msm8917-xiaomi-tiare.dts
> @@ -14,6 +14,15 @@ / {
>  	qcom,board-id = <0x1000b 1>;
>  };
>  
> +&blsp2_i2c1 {
> +	status = "okay";
> +
> +	fuel-gauge@63 {
> +		compatible = "cellwise,cw2017";
> +		reg = <0x63>;
> +	};
> +};

[Severity: Medium]
When binding the cellwise,cw2017 node added here to the cw2017-battery driver
from earlier in this series, does invalidating cw->sample.error in
cw2017_suspend() cause -ENODATA errors and spurious uevents across system
suspend and resume?

In drivers/power/supply/cw2017_battery.c, cw2017_suspend() overwrites the
cached sample state on every suspend:

drivers/power/supply/cw2017_battery.c:cw2017_suspend() {
	struct cw2017 *cw = dev_get_drvdata(dev);

	cancel_delayed_work_sync(&cw->work);
	mutex_lock(&cw->lock);
	cw->sample.error = -ENODATA;
	mutex_unlock(&cw->lock);

	return 0;
}

On resume, cw2017_resume() queues cw->work onto system_freezable_wq:

drivers/power/supply/cw2017_battery.c:cw2017_resume() {
	struct cw2017 *cw = dev_get_drvdata(dev);

	cw->poll_interval_ms = CW2017_POLL_INTERVAL_MS;
	queue_delayed_work(system_freezable_wq, &cw->work, 0);

	return 0;
}

Because cw2017_properties includes POWER_SUPPLY_PROP_TEMP and
cw->desc.no_thermal is false, devm_power_supply_register() registers a
thermal zone via psy_register_thermal() whose callback is
power_supply_read_temp().

During resume in suspend_devices_and_enter() -> dpm_resume_end(),
dpm_complete() calls thermal_pm_complete(), which queues
thermal_zone_device_resume() on the non-freezable thermal_wq before
suspend_finish() -> suspend_thaw_processes() -> thaw_workqueues() thaws
system_freezable_wq. When __thermal_zone_device_update() ->
power_supply_read_temp() -> cw2017_get_property() runs while cw->work is
still frozen or in flight, it sees cw->sample.error == -ENODATA, causing
thermal_zone_recheck() to log a temperature check failure (-61) and enter
error-recheck polling. Similarly, any PM_POST_SUSPEND notifier or thawed
userspace process reading battery attributes before cw2017_poll() finishes
its I2C transfers receives -ENODATA.

In addition, when cw2017_poll() completes after resume:

drivers/power/supply/cw2017_battery.c:cw2017_poll() {
	mutex_lock(&cw->lock);
	changed = !cw->sample.error != !sample.error ||
		  (!sample.error &&
		   (cw->sample.capacity != sample.capacity ||
		    cw->sample.voltage_uv != sample.voltage_uv ||
		    cw->sample.temp != sample.temp));
	cw->sample = sample;
	mutex_unlock(&cw->lock);

	if (changed)
		power_supply_changed(cw->psy);
}

Because cw2017_suspend() set cw->sample.error = -ENODATA, the expression
!cw->sample.error != !sample.error is always true when the post-resume poll
succeeds, triggering an unconditional power_supply_changed() notification on
every resume even if capacity, voltage_uv, and temp have not changed.

Should cw2017_suspend() keep the last sample intact until the post-resume
poll updates it, or refresh the sample synchronously during resume?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179329639684.489944.6322623410181934358.cw2017-first-public-rfc@yandex.ru?part=3