Re: [PATCH v2 02/17] iio: accel: kxcjk-1013: Discard pm_runtime_put_autosuspend() return values

[Brian Norris <briannorris@chromium.org>]

On Fri, Oct 23, 2026 at 04:36:37AM +0100, Jonathan Cameron wrote:
> On Wed, 21 Oct 2026 16:57:03 -0700
> Brian Norris <briannorris@chromium.org> wrote:
> 
> > This driver propagates the return value of pm_runtime_put_autosuspend(),
> > which is not particularly useful.
> > 
> > Returning an error code from pm_runtime_put_autosuspend() merely means
> > that it has not queued up a timer or work item to check whether or not
> > the device can be suspended and there are many perfectly valid
> > situations in which that can happen, like after writing "on" to the
> > device's runtime PM "control" attribute in sysfs for one example.
> > 
> > For this reason, modify this driver to discard the
> > pm_runtime_put_autosuspend() return value, and inline these pm_runtime
> > calls for simplicity.
> > 
> > While doing this, it becomes obvious that kxcjk1013_write_event_config()
> > and kxcjk1013_data_rdy_trigger_set_state() are intentionally mismanaging
> > runtime PM reference counts and instead are relying on (racy)
> > autosuspend delay for correct ordering. Undo this choice, and instead
> > properly drop the reference count only after we're done touching the
> > hardware
> .
> Sorry, I should have looked more closely at this in the earlier
> review.
> 
> I'm not sure this is actually true.

I'll admit I'm also not 100% sure either. But these comments I'm
deleting sound awfully suggestive:

	 * We will expect the enable and disable to do operation in
	 * reverse order. This will happen here anyway as our
	 * resume operation uses sync mode runtime pm calls, the
	 * suspend operation will be delayed by autosuspend delay
	 * So the disable operation will still happen in reverse of
	 * enable operation. When runtime pm is disabled the mode
	 * is always on so sequence doesn't matter

That sounds to me like, "the put_autosuspend() is in the wrong order,
but we're relying on autosuspend delay to flip that order." This text
has also been copied between several drivers that borrow that same
structure, IIUC. Overall, it seems like at least one author was
documenting a race condition, and was simply lazy about handling it.

> A device being in runtime suspend
> on an i2c bus doesn't in general mean you can't talk to it.
> Whether the mode switching in here is a problem or not isn't
> that obvious and will require a datasheet dive.  It is inelegant
> but maybe not a bug.

Agreed it's not guaranteed. But given the comment and the nature of
runtime PM, it seemed like an obvious transformation with little
downside and likely upside.

If we're not sure about these things, I can undo them.

But mainly, I don't want to spend much more time on this series if I can
help it, nor introduce more difficult driver-specific work items (see:
all of Andy's comments) or areas of potential regression. So I'll lean
toward the safe / less-change option if that helps make progress.

Brian

> Anyhow, out of time and energy for today. I'll take a closer look
> at the rest of the series in a day or two.
> 
> Thanks for looking at this.
> 
> Jonathan
> 
> > 
> > Use guard() for managing the mutex in kxcjk1013_write_event_config(),
> > because it helps simplify the above refactoring, making it less
> > error-prone.
> > 
> > This will facilitate a planned change of the
> > pm_runtime_put_autosuspend() return type to void in the future, similar
> > to commit 3afd8df02433 ("PM: runtime: Change pm_runtime_put() return
> > type to void").
> > 
> > Signed-off-by: Brian Norris <briannorris@chromium.org>