Re: [PATCH] ASoC: cs42l43: Move shutter IRQ handling into a worker thread

[Takashi Iwai <tiwai@xxxxxxx>]

On Tue, 30 Jul 2024 16:13:33 +0200,
Jaroslav Kysela wrote:
> 
> On 30. 07. 24 14:33, Takashi Iwai wrote:
> > On Tue, 30 Jul 2024 10:43:01 +0200,
> > Charles Keepax wrote:
> >> 
> >> On Mon, Jul 29, 2024 at 09:30:00PM +0200, Takashi Iwai wrote:
> >>> On Mon, 29 Jul 2024 18:44:59 +0200,
> >>> Charles Keepax wrote:
> >>>> 
> >>>> On Mon, Jul 29, 2024 at 06:18:50PM +0200, Takashi Iwai wrote:
> >>>>> On Mon, 29 Jul 2024 17:59:32 +0200,
> >>>>> Charles Keepax wrote:
> >>>>>> 
> >>>>>> The microphone/speaker privacy shutter ALSA control handlers need to
> >>>>>> call pm_runtime_resume, since the hardware needs to be powered up to
> >>>>>> check the hardware state of the shutter. The IRQ handler for the
> >>>>>> shutters also needs to notify the ALSA control to inform user-space
> >>>>>> the shutters updated. However this leads to a mutex inversion, between
> >>>>>> the sdw_dev_lock and the controls_rwsem.
> >>>>> 
> >>>>> That's bad, how does the mutex inversion look like?  Generally
> >>>>> speaking, a call of snd_ctl_notify() from an irq handler is a very
> >>>>> standard procedure, and it should work without too much workaround.
> >>>>> 
> >>>> 
> >>>> SoundWire IRQs are called under the sdw_dev_lock, and then in the
> >>>> IRQ handler we call snd_ctl_notify which takes controls_rwsem.
> >>> 
> >>> snd_ctl_notify() doesn't take rwsem but ctl_files_rwlock.
> >>> And rwlock isn't taken except for two cases, at a list traverse at
> >>> enumerating from user-space and at the device disconnection, so it
> >>> shouldn't race.  Anything missing there...?
> >>> 
> >> 
> >> The trouble here isn't the snd_ctl_notify, the handler uses
> >> snd_soc_component_notify_control which also does a
> >> snd_soc_card_get_kcontrol, which eventually calls snd_ctl_find_id
> >> which does take the controls_rwsem.
> > 
> > OK, now it's clearer.  I think we can change snd_ctl_find_*() rather
> > to take rwlock instead of rwsem.  It's only a quick look-up, hence
> > rwlock can work well.
> > 
> > Totally untested patch set is below.
> 
> But why the interrupt routine does not use the cached kcontrol
> pointer? It looks like a bad driver design when you need to do such
> name (string based) lookups from the interrupt routine.

Yeah, it'd be even better :)

Though, the conversion to rwlock might be still worth; it's a good
cleanup to drop the *_unlocked() versions that can be messy.


Takashi