| Message ID | 20240729155932.3054632-1-ckeepax@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: cs42l43: Move shutter IRQ handling into a worker thread | expand |
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. thanks, Takashi > > To avoid this mutex inversion add a work item for handling the shutter > IRQ and move the notifies into that. > > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> > --- > sound/soc/codecs/cs42l43.c | 43 +++++++++++++++++++++++++++----------- > sound/soc/codecs/cs42l43.h | 3 +++ > 2 files changed, 34 insertions(+), 12 deletions(-) > > diff --git a/sound/soc/codecs/cs42l43.c b/sound/soc/codecs/cs42l43.c > index 92674314227c4..74cb396ec0576 100644 > --- a/sound/soc/codecs/cs42l43.c > +++ b/sound/soc/codecs/cs42l43.c > @@ -249,9 +249,10 @@ CS42L43_IRQ_COMPLETE(spkr_startup) > CS42L43_IRQ_COMPLETE(spkl_startup) > CS42L43_IRQ_COMPLETE(load_detect) > > -static irqreturn_t cs42l43_mic_shutter(int irq, void *data) > +static void cs42l43_mic_shutter_work(struct work_struct *work) > { > - struct cs42l43_codec *priv = data; > + struct cs42l43_codec *priv = container_of(work, struct cs42l43_codec, > + mic_shutter_work); > static const char * const controls[] = { > "Decimator 1 Switch", > "Decimator 2 Switch", > @@ -263,32 +264,47 @@ static irqreturn_t cs42l43_mic_shutter(int irq, void *data) > dev_dbg(priv->dev, "Microphone shutter changed\n"); > > if (!priv->component) > - return IRQ_NONE; > + return; > > for (i = 0; i < ARRAY_SIZE(controls); i++) { > - ret = snd_soc_component_notify_control(priv->component, > - controls[i]); > + ret = snd_soc_component_notify_control(priv->component, controls[i]); > if (ret) > - return IRQ_NONE; > + dev_err(priv->dev, "Failed to notify control %s: %d\n", > + controls[i], ret); > } > +} > + > +static irqreturn_t cs42l43_mic_shutter(int irq, void *data) > +{ > + struct cs42l43_codec *priv = data; > + > + queue_work(system_wq, &priv->mic_shutter_work); > > return IRQ_HANDLED; > } > > -static irqreturn_t cs42l43_spk_shutter(int irq, void *data) > +static void cs42l43_spk_shutter_work(struct work_struct *work) > { > - struct cs42l43_codec *priv = data; > + struct cs42l43_codec *priv = container_of(work, struct cs42l43_codec, > + mic_shutter_work); > + const char * const control = "Speaker Digital Switch"; > int ret; > > dev_dbg(priv->dev, "Speaker shutter changed\n"); > > if (!priv->component) > - return IRQ_NONE; > + return; > > - ret = snd_soc_component_notify_control(priv->component, > - "Speaker Digital Switch"); > + ret = snd_soc_component_notify_control(priv->component, control); > if (ret) > - return IRQ_NONE; > + dev_err(priv->dev, "Failed to notify control %s: %d\n", control, ret); > +} > + > +static irqreturn_t cs42l43_spk_shutter(int irq, void *data) > +{ > + struct cs42l43_codec *priv = data; > + > + queue_work(system_wq, &priv->spk_shutter_work); > > return IRQ_HANDLED; > } > @@ -2280,6 +2296,9 @@ static int cs42l43_codec_probe(struct platform_device *pdev) > INIT_WORK(&priv->button_release_work, cs42l43_button_release_work); > INIT_WORK(&priv->hp_ilimit_work, cs42l43_hp_ilimit_work); > > + INIT_WORK(&priv->mic_shutter_work, cs42l43_mic_shutter_work); > + INIT_WORK(&priv->spk_shutter_work, cs42l43_spk_shutter_work); > + > pm_runtime_set_autosuspend_delay(priv->dev, 100); > pm_runtime_use_autosuspend(priv->dev); > pm_runtime_set_active(priv->dev); > diff --git a/sound/soc/codecs/cs42l43.h b/sound/soc/codecs/cs42l43.h > index 9924c13e1eb53..5592aab5c8042 100644 > --- a/sound/soc/codecs/cs42l43.h > +++ b/sound/soc/codecs/cs42l43.h > @@ -100,6 +100,9 @@ struct cs42l43_codec { > struct delayed_work hp_ilimit_clear_work; > bool hp_ilimited; > int hp_ilimit_count; > + > + struct work_struct mic_shutter_work; > + struct work_struct spk_shutter_work; > }; > > #if IS_REACHABLE(CONFIG_SND_SOC_CS42L43_SDW) > -- > 2.39.2 > >
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. On the other side, in the ALSA control handler which is obviously called under the controls_rwsem, we do a pm_runtime_resume, which causes SoundWire stuff to happen that takes the sdw_dev_lock. Thanks, Charles
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...? thanks, Takashi > On > the other side, in the ALSA control handler which is obviously > called under the controls_rwsem, we do a pm_runtime_resume, which > causes SoundWire stuff to happen that takes the sdw_dev_lock. > > Thanks, > Charles
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. Thanks, Charles
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. Takashi
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. Jaroslav
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
On Tue, Jul 30, 2024 at 04:13:33PM +0200, Jaroslav Kysela wrote: > On 30. 07. 24 14:33, Takashi Iwai wrote: > > 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. With some of these slower buses it's not immediately obvious that it's worth the bother of caching - the overhead of doing the lookup is negligable in the overall context of handling the interrupt.
On 30. 07. 24 16:44, Mark Brown wrote: > On Tue, Jul 30, 2024 at 04:13:33PM +0200, Jaroslav Kysela wrote: >> On 30. 07. 24 14:33, Takashi Iwai wrote: > >>> 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. > > With some of these slower buses it's not immediately obvious that it's > worth the bother of caching - the overhead of doing the lookup is > negligable in the overall context of handling the interrupt. I don't buy that argument. We should always try to write an optimal code. Every CPU tick counts. Jaroslav
On Tue, Jul 30, 2024 at 11:47:48PM +0200, Jaroslav Kysela wrote: > On 30. 07. 24 16:44, Mark Brown wrote: > > With some of these slower buses it's not immediately obvious that it's > > worth the bother of caching - the overhead of doing the lookup is > > negligable in the overall context of handling the interrupt. > I don't buy that argument. We should always try to write an optimal code. > Every CPU tick counts. So do the bytes of memory you'd use caching the pointers!
On Wed, 31 Jul 2024 01:20:40 +0200, Mark Brown wrote: > > On Tue, Jul 30, 2024 at 11:47:48PM +0200, Jaroslav Kysela wrote: > > On 30. 07. 24 16:44, Mark Brown wrote: > > > > With some of these slower buses it's not immediately obvious that it's > > > worth the bother of caching - the overhead of doing the lookup is > > > negligable in the overall context of handling the interrupt. > > > I don't buy that argument. We should always try to write an optimal code. > > Every CPU tick counts. > > So do the bytes of memory you'd use caching the pointers! Adding two more works would be even more wastes, that's the beginning of the discussion :) The problem is the order of ASoC component initialization, though; AFAIUC, the kcontrols aren't instantiated at the point of component probe, hence you can't get kcontrol objects there. Takashi
On Wed, Jul 31, 2024 at 08:30:15AM +0200, Takashi Iwai wrote: > On Wed, 31 Jul 2024 01:20:40 +0200, > Mark Brown wrote: > > > > On Tue, Jul 30, 2024 at 11:47:48PM +0200, Jaroslav Kysela wrote: > > > On 30. 07. 24 16:44, Mark Brown wrote: > > > > > > With some of these slower buses it's not immediately obvious that it's > > > > worth the bother of caching - the overhead of doing the lookup is > > > > negligable in the overall context of handling the interrupt. > > > > > I don't buy that argument. We should always try to write an optimal code. > > > Every CPU tick counts. > > > > So do the bytes of memory you'd use caching the pointers! > > Adding two more works would be even more wastes, that's the beginning > of the discussion :) > > The problem is the order of ASoC component initialization, though; > AFAIUC, the kcontrols aren't instantiated at the point of component > probe, hence you can't get kcontrol objects there. > I think there probably should be some way to cache the kcontrols, I will have a look at implementing that. In general I chose the delayed work as caching didn't obviously seem worth it as Mark notes, and also the sdw_dev_lock tends to cause a lot of these inversions and the delayed work is a very safe solution, whereas caching makes me slightly nervous we will find another inversion at some point. In general I think that is the actual problem here, the sdw_dev_lock wraps too much stuff and frequently causes these inversions. Although fixing that may not be possible and certainly requires a lot of thinking. Thanks, Charles
diff --git a/sound/soc/codecs/cs42l43.c b/sound/soc/codecs/cs42l43.c index 92674314227c4..74cb396ec0576 100644 --- a/sound/soc/codecs/cs42l43.c +++ b/sound/soc/codecs/cs42l43.c @@ -249,9 +249,10 @@ CS42L43_IRQ_COMPLETE(spkr_startup) CS42L43_IRQ_COMPLETE(spkl_startup) CS42L43_IRQ_COMPLETE(load_detect) -static irqreturn_t cs42l43_mic_shutter(int irq, void *data) +static void cs42l43_mic_shutter_work(struct work_struct *work) { - struct cs42l43_codec *priv = data; + struct cs42l43_codec *priv = container_of(work, struct cs42l43_codec, + mic_shutter_work); static const char * const controls[] = { "Decimator 1 Switch", "Decimator 2 Switch", @@ -263,32 +264,47 @@ static irqreturn_t cs42l43_mic_shutter(int irq, void *data) dev_dbg(priv->dev, "Microphone shutter changed\n"); if (!priv->component) - return IRQ_NONE; + return; for (i = 0; i < ARRAY_SIZE(controls); i++) { - ret = snd_soc_component_notify_control(priv->component, - controls[i]); + ret = snd_soc_component_notify_control(priv->component, controls[i]); if (ret) - return IRQ_NONE; + dev_err(priv->dev, "Failed to notify control %s: %d\n", + controls[i], ret); } +} + +static irqreturn_t cs42l43_mic_shutter(int irq, void *data) +{ + struct cs42l43_codec *priv = data; + + queue_work(system_wq, &priv->mic_shutter_work); return IRQ_HANDLED; } -static irqreturn_t cs42l43_spk_shutter(int irq, void *data) +static void cs42l43_spk_shutter_work(struct work_struct *work) { - struct cs42l43_codec *priv = data; + struct cs42l43_codec *priv = container_of(work, struct cs42l43_codec, + mic_shutter_work); + const char * const control = "Speaker Digital Switch"; int ret; dev_dbg(priv->dev, "Speaker shutter changed\n"); if (!priv->component) - return IRQ_NONE; + return; - ret = snd_soc_component_notify_control(priv->component, - "Speaker Digital Switch"); + ret = snd_soc_component_notify_control(priv->component, control); if (ret) - return IRQ_NONE; + dev_err(priv->dev, "Failed to notify control %s: %d\n", control, ret); +} + +static irqreturn_t cs42l43_spk_shutter(int irq, void *data) +{ + struct cs42l43_codec *priv = data; + + queue_work(system_wq, &priv->spk_shutter_work); return IRQ_HANDLED; } @@ -2280,6 +2296,9 @@ static int cs42l43_codec_probe(struct platform_device *pdev) INIT_WORK(&priv->button_release_work, cs42l43_button_release_work); INIT_WORK(&priv->hp_ilimit_work, cs42l43_hp_ilimit_work); + INIT_WORK(&priv->mic_shutter_work, cs42l43_mic_shutter_work); + INIT_WORK(&priv->spk_shutter_work, cs42l43_spk_shutter_work); + pm_runtime_set_autosuspend_delay(priv->dev, 100); pm_runtime_use_autosuspend(priv->dev); pm_runtime_set_active(priv->dev); diff --git a/sound/soc/codecs/cs42l43.h b/sound/soc/codecs/cs42l43.h index 9924c13e1eb53..5592aab5c8042 100644 --- a/sound/soc/codecs/cs42l43.h +++ b/sound/soc/codecs/cs42l43.h @@ -100,6 +100,9 @@ struct cs42l43_codec { struct delayed_work hp_ilimit_clear_work; bool hp_ilimited; int hp_ilimit_count; + + struct work_struct mic_shutter_work; + struct work_struct spk_shutter_work; }; #if IS_REACHABLE(CONFIG_SND_SOC_CS42L43_SDW)
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. To avoid this mutex inversion add a work item for handling the shutter IRQ and move the notifies into that. Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> --- sound/soc/codecs/cs42l43.c | 43 +++++++++++++++++++++++++++----------- sound/soc/codecs/cs42l43.h | 3 +++ 2 files changed, 34 insertions(+), 12 deletions(-)