linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] staging: greybus: Add missing rwsem around snd_ctl_remove() calls
@ 2021-11-16  7:20 Takashi Iwai
  2021-11-17 19:56 ` [greybus-dev] " Alex Elder
  0 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2021-11-16  7:20 UTC (permalink / raw)
  To: greybus-dev; +Cc: Johan Hovold, Alex Elder, Greg Kroah-Hartman, linux-kernel

snd_ctl_remove() has to be called with card->controls_rwsem held (when
called after the card instantiation).  This patch adds the missing
rwsem calls around it.

Fixes: 510e340efe0c ("staging: greybus: audio: Add helper APIs for dynamic audio modules")
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/staging/greybus/audio_helper.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/greybus/audio_helper.c b/drivers/staging/greybus/audio_helper.c
index 1ed4772d2771..843760675876 100644
--- a/drivers/staging/greybus/audio_helper.c
+++ b/drivers/staging/greybus/audio_helper.c
@@ -192,7 +192,11 @@ int gbaudio_remove_component_controls(struct snd_soc_component *component,
 				      unsigned int num_controls)
 {
 	struct snd_card *card = component->card->snd_card;
+	int err;
 
-	return gbaudio_remove_controls(card, component->dev, controls,
-				       num_controls, component->name_prefix);
+	down_write(&card->controls_rwsem);
+	err = gbaudio_remove_controls(card, component->dev, controls,
+				      num_controls, component->name_prefix);
+	up_write(&card->controls_rwsem);
+	return err;
 }
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [greybus-dev] [PATCH] staging: greybus: Add missing rwsem around snd_ctl_remove() calls
  2021-11-16  7:20 [PATCH] staging: greybus: Add missing rwsem around snd_ctl_remove() calls Takashi Iwai
@ 2021-11-17 19:56 ` Alex Elder
  2021-11-17 21:02   ` Takashi Iwai
  0 siblings, 1 reply; 6+ messages in thread
From: Alex Elder @ 2021-11-17 19:56 UTC (permalink / raw)
  To: Takashi Iwai, greybus-dev; +Cc: Alex Elder, Johan Hovold, linux-kernel

On 11/16/21 1:20 AM, Takashi Iwai wrote:
> snd_ctl_remove() has to be called with card->controls_rwsem held (when
> called after the card instantiation).  This patch adds the missing
> rwsem calls around it.

I see the comment above snd_ctl_remove() that says you must hold
the write lock.  And given that, this seems correct to me.

I understand why you want to take the lock just once, rather
than each time snd_ctl_remove() is called.

However I believe the acquisition and release of the lock
belongs inside gbaudio_remove_controls(), not in its caller.

If you disagree, can you please explain why?

Otherwise, will you please submit version two, taking the
lock inside gbaudio_remove_controls()?

Thanks.

					-Alex

> Fixes: 510e340efe0c ("staging: greybus: audio: Add helper APIs for dynamic audio modules")
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>   drivers/staging/greybus/audio_helper.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/greybus/audio_helper.c b/drivers/staging/greybus/audio_helper.c
> index 1ed4772d2771..843760675876 100644
> --- a/drivers/staging/greybus/audio_helper.c
> +++ b/drivers/staging/greybus/audio_helper.c
> @@ -192,7 +192,11 @@ int gbaudio_remove_component_controls(struct snd_soc_component *component,
>   				      unsigned int num_controls)
>   {
>   	struct snd_card *card = component->card->snd_card;
> +	int err;
>   
> -	return gbaudio_remove_controls(card, component->dev, controls,
> -				       num_controls, component->name_prefix);
> +	down_write(&card->controls_rwsem);
> +	err = gbaudio_remove_controls(card, component->dev, controls,
> +				      num_controls, component->name_prefix);
> +	up_write(&card->controls_rwsem);
> +	return err;
>   }
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [greybus-dev] [PATCH] staging: greybus: Add missing rwsem around snd_ctl_remove() calls
  2021-11-17 19:56 ` [greybus-dev] " Alex Elder
@ 2021-11-17 21:02   ` Takashi Iwai
  2021-11-17 21:55     ` Alex Elder
  0 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2021-11-17 21:02 UTC (permalink / raw)
  To: Alex Elder
  Cc: Takashi Iwai, greybus-dev, Alex Elder, Johan Hovold, linux-kernel

On Wed, 17 Nov 2021 20:56:14 +0100,
Alex Elder wrote:
> 
> On 11/16/21 1:20 AM, Takashi Iwai wrote:
> > snd_ctl_remove() has to be called with card->controls_rwsem held (when
> > called after the card instantiation).  This patch adds the missing
> > rwsem calls around it.
> 
> I see the comment above snd_ctl_remove() that says you must hold
> the write lock.  And given that, this seems correct to me.
> 
> I understand why you want to take the lock just once, rather
> than each time snd_ctl_remove() is called.
> 
> However I believe the acquisition and release of the lock
> belongs inside gbaudio_remove_controls(), not in its caller.
> 
> If you disagree, can you please explain why?

In general if the function returns an error and has a loop inside,
taking a lock in the caller side avoids the forgotten unlock.


Takashi


> Otherwise, will you please submit version two, taking the
> lock inside gbaudio_remove_controls()?
> 
> Thanks.
> 
> 					-Alex
> 
> > Fixes: 510e340efe0c ("staging: greybus: audio: Add helper APIs for dynamic audio modules")
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >   drivers/staging/greybus/audio_helper.c | 8 ++++++--
> >   1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/greybus/audio_helper.c b/drivers/staging/greybus/audio_helper.c
> > index 1ed4772d2771..843760675876 100644
> > --- a/drivers/staging/greybus/audio_helper.c
> > +++ b/drivers/staging/greybus/audio_helper.c
> > @@ -192,7 +192,11 @@ int gbaudio_remove_component_controls(struct snd_soc_component *component,
> >   				      unsigned int num_controls)
> >   {
> >   	struct snd_card *card = component->card->snd_card;
> > +	int err;
> >   -	return gbaudio_remove_controls(card, component->dev, controls,
> > -				       num_controls, component->name_prefix);
> > +	down_write(&card->controls_rwsem);
> > +	err = gbaudio_remove_controls(card, component->dev, controls,
> > +				      num_controls, component->name_prefix);
> > +	up_write(&card->controls_rwsem);
> > +	return err;
> >   }
> >
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [greybus-dev] [PATCH] staging: greybus: Add missing rwsem around snd_ctl_remove() calls
  2021-11-17 21:02   ` Takashi Iwai
@ 2021-11-17 21:55     ` Alex Elder
  2021-11-18  3:32       ` Vaibhav Agarwal
  0 siblings, 1 reply; 6+ messages in thread
From: Alex Elder @ 2021-11-17 21:55 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: greybus-dev, Alex Elder, Johan Hovold, linux-kernel

On 11/17/21 3:02 PM, Takashi Iwai wrote:
> On Wed, 17 Nov 2021 20:56:14 +0100,
> Alex Elder wrote:
>>
>> On 11/16/21 1:20 AM, Takashi Iwai wrote:
>>> snd_ctl_remove() has to be called with card->controls_rwsem held (when
>>> called after the card instantiation).  This patch adds the missing
>>> rwsem calls around it.
>>
>> I see the comment above snd_ctl_remove() that says you must hold
>> the write lock.  And given that, this seems correct to me.
>>
>> I understand why you want to take the lock just once, rather
>> than each time snd_ctl_remove() is called.
>>
>> However I believe the acquisition and release of the lock
>> belongs inside gbaudio_remove_controls(), not in its caller.
>>
>> If you disagree, can you please explain why?
> 
> In general if the function returns an error and has a loop inside,
> taking a lock in the caller side avoids the forgotten unlock.

But taking the lock in the called function makes the
caller not need to take the lock (which would be even
more valuable if there were more than one caller).

I prefer having the lock acquisition in the called
function.  Please send version 2, as I suggested.

					-Alex

> Takashi
> 
> 
>> Otherwise, will you please submit version two, taking the
>> lock inside gbaudio_remove_controls()?
>>
>> Thanks.
>>
>> 					-Alex
>>
>>> Fixes: 510e340efe0c ("staging: greybus: audio: Add helper APIs for dynamic audio modules")
>>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>>> ---
>>>    drivers/staging/greybus/audio_helper.c | 8 ++++++--
>>>    1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/staging/greybus/audio_helper.c b/drivers/staging/greybus/audio_helper.c
>>> index 1ed4772d2771..843760675876 100644
>>> --- a/drivers/staging/greybus/audio_helper.c
>>> +++ b/drivers/staging/greybus/audio_helper.c
>>> @@ -192,7 +192,11 @@ int gbaudio_remove_component_controls(struct snd_soc_component *component,
>>>    				      unsigned int num_controls)
>>>    {
>>>    	struct snd_card *card = component->card->snd_card;
>>> +	int err;
>>>    -	return gbaudio_remove_controls(card, component->dev, controls,
>>> -				       num_controls, component->name_prefix);
>>> +	down_write(&card->controls_rwsem);
>>> +	err = gbaudio_remove_controls(card, component->dev, controls,
>>> +				      num_controls, component->name_prefix);
>>> +	up_write(&card->controls_rwsem);
>>> +	return err;
>>>    }
>>>
>>


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [greybus-dev] [PATCH] staging: greybus: Add missing rwsem around snd_ctl_remove() calls
  2021-11-17 21:55     ` Alex Elder
@ 2021-11-18  3:32       ` Vaibhav Agarwal
  2021-11-18  6:55         ` Takashi Iwai
  0 siblings, 1 reply; 6+ messages in thread
From: Vaibhav Agarwal @ 2021-11-18  3:32 UTC (permalink / raw)
  To: Alex Elder
  Cc: Takashi Iwai, moderated list:GREYBUS SUBSYSTEM, Alex Elder,
	Johan Hovold, open list

On Thu, Nov 18, 2021 at 3:25 AM Alex Elder <elder@linaro.org> wrote:
>
> On 11/17/21 3:02 PM, Takashi Iwai wrote:
> > On Wed, 17 Nov 2021 20:56:14 +0100,
> > Alex Elder wrote:
> >>
> >> On 11/16/21 1:20 AM, Takashi Iwai wrote:
> >>> snd_ctl_remove() has to be called with card->controls_rwsem held (when
> >>> called after the card instantiation).  This patch adds the missing
> >>> rwsem calls around it.
> >>
> >> I see the comment above snd_ctl_remove() that says you must hold
> >> the write lock.  And given that, this seems correct to me.
> >>
> >> I understand why you want to take the lock just once, rather
> >> than each time snd_ctl_remove() is called.
> >>
> >> However I believe the acquisition and release of the lock
> >> belongs inside gbaudio_remove_controls(), not in its caller.
> >>
> >> If you disagree, can you please explain why?
> >
> > In general if the function returns an error and has a loop inside,
> > taking a lock in the caller side avoids the forgotten unlock.
>
> But taking the lock in the called function makes the
> caller not need to take the lock (which would be even
> more valuable if there were more than one caller).
>
> I prefer having the lock acquisition in the called
> function.  Please send version 2, as I suggested.


Hi Takashi,

Thanks for sharing this patch. In reference to the suggestion from Alex,
do you think replacing snd_ctl_find_id(), snd_ctl_remove() with
snd_ctl_remove_id() inside gbaudio_remove_controls() would be an even
better choice without worrying about locks?

--
vaibhav

 >
>
>                                         -Alex
>
> > Takashi
> >
> >
> >> Otherwise, will you please submit version two, taking the
> >> lock inside gbaudio_remove_controls()?
> >>
> >> Thanks.
> >>
> >>                                      -Alex
> >>
> >>> Fixes: 510e340efe0c ("staging: greybus: audio: Add helper APIs for dynamic audio modules")
> >>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> >>> ---
> >>>    drivers/staging/greybus/audio_helper.c | 8 ++++++--
> >>>    1 file changed, 6 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/staging/greybus/audio_helper.c b/drivers/staging/greybus/audio_helper.c
> >>> index 1ed4772d2771..843760675876 100644
> >>> --- a/drivers/staging/greybus/audio_helper.c
> >>> +++ b/drivers/staging/greybus/audio_helper.c
> >>> @@ -192,7 +192,11 @@ int gbaudio_remove_component_controls(struct snd_soc_component *component,
> >>>                                   unsigned int num_controls)
> >>>    {
> >>>     struct snd_card *card = component->card->snd_card;
> >>> +   int err;
> >>>    -        return gbaudio_remove_controls(card, component->dev, controls,
> >>> -                                  num_controls, component->name_prefix);
> >>> +   down_write(&card->controls_rwsem);
> >>> +   err = gbaudio_remove_controls(card, component->dev, controls,
> >>> +                                 num_controls, component->name_prefix);
> >>> +   up_write(&card->controls_rwsem);
> >>> +   return err;
> >>>    }
> >>>
> >>
>
> _______________________________________________
> greybus-dev mailing list
> greybus-dev@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/greybus-dev

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [greybus-dev] [PATCH] staging: greybus: Add missing rwsem around snd_ctl_remove() calls
  2021-11-18  3:32       ` Vaibhav Agarwal
@ 2021-11-18  6:55         ` Takashi Iwai
  0 siblings, 0 replies; 6+ messages in thread
From: Takashi Iwai @ 2021-11-18  6:55 UTC (permalink / raw)
  To: Vaibhav Agarwal
  Cc: Alex Elder, Takashi Iwai, moderated list:GREYBUS SUBSYSTEM,
	Alex Elder, Johan Hovold, open list

On Thu, 18 Nov 2021 04:32:40 +0100,
Vaibhav Agarwal wrote:
> 
> On Thu, Nov 18, 2021 at 3:25 AM Alex Elder <elder@linaro.org> wrote:
> >
> > On 11/17/21 3:02 PM, Takashi Iwai wrote:
> > > On Wed, 17 Nov 2021 20:56:14 +0100,
> > > Alex Elder wrote:
> > >>
> > >> On 11/16/21 1:20 AM, Takashi Iwai wrote:
> > >>> snd_ctl_remove() has to be called with card->controls_rwsem held (when
> > >>> called after the card instantiation).  This patch adds the missing
> > >>> rwsem calls around it.
> > >>
> > >> I see the comment above snd_ctl_remove() that says you must hold
> > >> the write lock.  And given that, this seems correct to me.
> > >>
> > >> I understand why you want to take the lock just once, rather
> > >> than each time snd_ctl_remove() is called.
> > >>
> > >> However I believe the acquisition and release of the lock
> > >> belongs inside gbaudio_remove_controls(), not in its caller.
> > >>
> > >> If you disagree, can you please explain why?
> > >
> > > In general if the function returns an error and has a loop inside,
> > > taking a lock in the caller side avoids the forgotten unlock.
> >
> > But taking the lock in the called function makes the
> > caller not need to take the lock (which would be even
> > more valuable if there were more than one caller).
> >
> > I prefer having the lock acquisition in the called
> > function.  Please send version 2, as I suggested.
> 
> 
> Hi Takashi,
> 
> Thanks for sharing this patch. In reference to the suggestion from Alex,
> do you think replacing snd_ctl_find_id(), snd_ctl_remove() with
> snd_ctl_remove_id() inside gbaudio_remove_controls() would be an even
> better choice without worrying about locks?

Yeah, that sounds like a better plan, indeed.


Takashi

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-18  6:56 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-16  7:20 [PATCH] staging: greybus: Add missing rwsem around snd_ctl_remove() calls Takashi Iwai
2021-11-17 19:56 ` [greybus-dev] " Alex Elder
2021-11-17 21:02   ` Takashi Iwai
2021-11-17 21:55     ` Alex Elder
2021-11-18  3:32       ` Vaibhav Agarwal
2021-11-18  6:55         ` Takashi Iwai

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).