Re: [PATCH] ASoC: ak4458: Check reset status after deassert

[Alvin Šipraga]

Hi Shengjiu,

On Mon, Oct 12, 2026 at 02:49:03PM +0800, Shengjiu Wang wrote:
> Hi Alvin, Philipp Zabel
> 
> On Sun, Oct 11, 2026 at 3:05 AM Alvin Šipraga <alvin.sipraga@xxxxxxxxxx> wrote:
> >
> > On Sat, Oct 10, 2026 at 04:40:58PM +0800, shengjiu.wang@xxxxxxxxxxx wrote:
> > > From: Shengjiu Wang <shengjiu.wang@xxxxxxx>
> > >
> > > The reset line of the ak4458 can be shared, for example when two ak4458
> > > devices use the same reset GPIO. In that case the two codecs may call
> > > reset_control_deassert() concurrently from their runtime resume paths.
> > >
> > > Only the first caller performs the actual hardware deassert, which may
> > > sleep (for example a GPIO-expander based reset controller). A second
> > > caller merely increments the shared deassert count and returns
> > > immediately, so it can continue while the first caller's .deassert() has
> > > not finished releasing the line yet, leaving the device still in reset.
> >
> > This seems like something that should be fixed in the reset controller
> > framework. In fact the issue you describe seems to contradict the
> > documentation of reset_control_deassert():
> >
> > /**
> >  * reset_control_deassert - deasserts the reset line
> >  * @rstc: reset controller
> >  *
> >  * After calling this function, the reset is guaranteed to be deasserted.
> >
> > ^ (1) seems not to be the case as you have found
> 
> Agreed. Looking at the documentation again, the guarantee
> 
> * After calling this function, the reset is guaranteed to be deasserted.
> 
> does not hold for a shared reset control today, so the issue is really in
> the reset core. For a shared reset only the first caller runs the
> hardware operation; later callers just bump the refcount and return
> immediately, e.g. in reset_control_deassert():
> 
> if (atomic_inc_return(&rstc->deassert_count) != 1)
> return 0;
> 
> So when two consumers of the same shared reset (here two ak4458 sharing
> one reset GPIO) call concurrently, the second caller can return before
> the first caller's .deassert() - which may sleep, e.g. a GPIO-expander
> based controller - has actually released the line. assert() and reset()
> have the same pattern.
> 
> Rather than work around it in the codec, I'd like to propose a reset-core
> fix for your review. The idea is to take the existing per-control
> rstc->lock across the shared refcount update and the hardware
> .assert()/.deassert()/.reset(), so a second concurrent caller waits for
> the first caller's hardware operation to complete. The lock is taken
> before the SRCU section, matching reset_control_acquire(), and these
> paths do not take rcdev->lock, so no new lock ordering is introduced:
> 
> if (reset_control_is_array(rstc))
> return reset_control_array_deassert(rstc_to_array(rstc));
> 
> guard(mutex)(&rstc->lock);
> guard(srcu)(&rstc->srcu);
> ...
> 
> Does this approach look reasonable to you? If so I will send it as a
> proper patch and drop the ASoC: ak4458 change.

Yeah, I think that is the correct fix!

> 
> >
> >  * Consumers must not use reset_control_reset on shared reset lines when
> >  * reset_control_(de)assert has been used.
> >
> > ^ (2) isn't this rule also being broken in the suspend/resume path of
> >       this driver?
> 
> I don't think so: the ak4458 suspend/resume path only uses
> reset_control_assert()/reset_control_deassert(); it never calls
> reset_control_reset(), so this rule isn't violated here. Please let me
> know if you had a different concern in mind.

Ahh, sorry! I conflated reset_control_reset() with
reset_control_deassert()... Please ignore this part :-)

Kind regards,
Alvin