Re: [RFC PATCH 2/3] power: supply: add read-only CW2017 battery monitor

[sashiko-bot@kernel.org]

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

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

power: supply: add read-only CW2017 battery monitor

This patch adds a read-only power_supply driver for firmware-initialized
CellWise CW2017 battery fuel gauges over I2C, polling voltage, state of
charge, and temperature with exponential backoff on read errors.

> diff --git a/drivers/power/supply/cw2017_battery.c b/drivers/power/supply/cw2017_battery.c
> new file mode 100644
> index 0000000000000..86323849a813a
> --- /dev/null
> +++ b/drivers/power/supply/cw2017_battery.c
> @@ -0,0 +1,370 @@

[ ... ]

> +static void cw2017_poll(struct work_struct *work)
> +{
> +	struct cw2017 *cw = container_of(to_delayed_work(work), struct cw2017, work);
> +	struct cw2017_sample sample = { };
> +	bool changed;
> +
> +	sample.error = cw2017_read_sample(cw, &sample);
> +	if (sample.error)
> +		dev_dbg_ratelimited(cw->dev, "Measurements unavailable (%d)\n",
> +				    sample.error);
> +
> +	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);
> +
> +	if (sample.error)
> +		cw->poll_interval_ms = min(cw->poll_interval_ms * 2,
> +					   CW2017_MAX_POLL_INTERVAL_MS);
> +	else
> +		cw->poll_interval_ms = CW2017_POLL_INTERVAL_MS;
> +
> +	queue_delayed_work(system_freezable_wq, &cw->work,
> +			   round_jiffies_relative(msecs_to_jiffies(cw->poll_interval_ms)));
> +}

[ ... ]

> +static int cw2017_suspend(struct device *dev)
> +{
> +	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;
> +}
> +
> +static int cw2017_resume(struct device *dev)
> +{
> +	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;
> +}

[Severity: Medium]
Does setting cw->sample.error to -ENODATA in cw2017_suspend() without
emitting a notification break the validity-transition tracking in
cw2017_poll() across suspend and resume?

In cw2017_poll(), validity transitions are detected by comparing the cached
error state against the new sample error state:

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

Because cw2017_suspend() overwrites cw->sample.error with -ENODATA without
calling power_supply_changed(cw->psy), two notification issues arise when
cw2017_poll() runs after resume:

1. If measurements were valid before suspend (cw->sample.error == 0) and
   cw2017_read_sample() fails after resume (for example, if the gauge lost
   its profile or readiness across suspend or an I2C error occurs), both
   cw->sample.error and sample.error are non-zero. As a result, changed
   evaluates to false and power_supply_changed(cw->psy) is never called,
   leaving consumers with stale pre-suspend values.

2. If measurements were valid before suspend and remain valid and unchanged
   after resume (sample.error == 0), !cw->sample.error != !sample.error
   evaluates to true solely because cw2017_suspend() forced cw->sample.error
   to -ENODATA, triggering an unconditional power_supply_changed(cw->psy)
   uevent on every resume.

In addition, cw2017_resume() queues cw->work on system_freezable_wq, which
remains frozen until suspend_thaw_processes() runs after dpm_complete().
When dpm_complete() -> thermal_pm_complete() schedules
thermal_zone_device_resume() on the non-freezable thermal_wq (registered
for POWER_SUPPLY_PROP_TEMP via __power_supply_register() ->
psy_register_thermal()), cw2017_get_property() will see cw->sample.error ==
-ENODATA and fail thermal zone resume before the post-resume poll work has a
chance to run.

Would it be better to preserve cw->sample across suspend so cw2017_poll()
can compare the post-resume reading against the last reported state?

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