[PATCH] ASoC: soc-pcm: use GFP_ATOMIC in dpcm_create_debugfs_state()

[PATCH] ASoC: soc-pcm: use GFP_ATOMIC in dpcm_create_debugfs_state()

[Christophe JAILLET]

The commit below states that dpcm_be_connect() may be called from atomic
context. It changes a GFP_KERNEL into a GFP_ATOMIC to deal with it.

Another memory allocation is done in dpcm_create_debugfs_state() which is
called by dpcm_be_connect(). Also use GFP_ATOMIC there to be consistent
and be compliant with atomic context.

Fixes: d8a9c6e1f676 ("ASoC: soc-pcm: use GFP_ATOMIC for dpcm structure")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Not clear to me how dpcm_be_connect() can be called from an atomic context,
though. But better safe than sorry.
---
 sound/soc/soc-pcm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 7abfc48b26ca..1a536a2b9dc3 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -212,7 +212,7 @@ static void dpcm_create_debugfs_state(struct snd_soc_dpcm *dpcm, int stream)
 {
 	char *name;
 
-	name = kasprintf(GFP_KERNEL, "%s:%s", dpcm->be->dai_link->name,
+	name = kasprintf(GFP_ATOMIC, "%s:%s", dpcm->be->dai_link->name,
 			 stream ? "capture" : "playback");
 	if (name) {
 		dpcm->debugfs_state = debugfs_create_dir(
-- 
2.32.0

[PATCH] ASoC: soc-pcm: use GFP_ATOMIC in dpcm_create_debugfs_state()

[Christophe JAILLET]

The commit below states that dpcm_be_connect() may be called from atomic
context. It changes a GFP_KERNEL into a GFP_ATOMIC to deal with it.

Another memory allocation is done in dpcm_create_debugfs_state() which is
called by dpcm_be_connect(). Also use GFP_ATOMIC there to be consistent
and be compliant with atomic context.

Fixes: d8a9c6e1f676 ("ASoC: soc-pcm: use GFP_ATOMIC for dpcm structure")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Not clear to me how dpcm_be_connect() can be called from an atomic context,
though. But better safe than sorry.
---
 sound/soc/soc-pcm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 7abfc48b26ca..1a536a2b9dc3 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -212,7 +212,7 @@ static void dpcm_create_debugfs_state(struct snd_soc_dpcm *dpcm, int stream)
 {
 	char *name;
 
-	name = kasprintf(GFP_KERNEL, "%s:%s", dpcm->be->dai_link->name,
+	name = kasprintf(GFP_ATOMIC, "%s:%s", dpcm->be->dai_link->name,
 			 stream ? "capture" : "playback");
 	if (name) {
 		dpcm->debugfs_state = debugfs_create_dir(
-- 
2.32.0

Re: [PATCH] ASoC: soc-pcm: use GFP_ATOMIC in dpcm_create_debugfs_state()

[Takashi Iwai]

On Sun, 16 Jan 2022 12:18:17 +0100,
Christophe JAILLET wrote:
> 
> The commit below states that dpcm_be_connect() may be called from atomic
> context. It changes a GFP_KERNEL into a GFP_ATOMIC to deal with it.
> 
> Another memory allocation is done in dpcm_create_debugfs_state() which is
> called by dpcm_be_connect(). Also use GFP_ATOMIC there to be consistent
> and be compliant with atomic context.
> 
> Fixes: d8a9c6e1f676 ("ASoC: soc-pcm: use GFP_ATOMIC for dpcm structure")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Not clear to me how dpcm_be_connect() can be called from an atomic context,
> though. But better safe than sorry.

I don't think this no longer valid for the very latest code.
The commit b7898396f4bb dropped the spurious dpcm_lock spinlock, so
the code path you touched must be always sleepable.

Similarly, the commit d8a9c6e1f676 can be reverted now.


thanks,

Takashi

> ---
>  sound/soc/soc-pcm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 7abfc48b26ca..1a536a2b9dc3 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -212,7 +212,7 @@ static void dpcm_create_debugfs_state(struct snd_soc_dpcm *dpcm, int stream)
>  {
>  	char *name;
>  
> -	name = kasprintf(GFP_KERNEL, "%s:%s", dpcm->be->dai_link->name,
> +	name = kasprintf(GFP_ATOMIC, "%s:%s", dpcm->be->dai_link->name,
>  			 stream ? "capture" : "playback");
>  	if (name) {
>  		dpcm->debugfs_state = debugfs_create_dir(
> -- 
> 2.32.0
> 

Re: [PATCH] ASoC: soc-pcm: use GFP_ATOMIC in dpcm_create_debugfs_state()

[Takashi Iwai]

On Sun, 16 Jan 2022 12:18:17 +0100,
Christophe JAILLET wrote:
> 
> The commit below states that dpcm_be_connect() may be called from atomic
> context. It changes a GFP_KERNEL into a GFP_ATOMIC to deal with it.
> 
> Another memory allocation is done in dpcm_create_debugfs_state() which is
> called by dpcm_be_connect(). Also use GFP_ATOMIC there to be consistent
> and be compliant with atomic context.
> 
> Fixes: d8a9c6e1f676 ("ASoC: soc-pcm: use GFP_ATOMIC for dpcm structure")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Not clear to me how dpcm_be_connect() can be called from an atomic context,
> though. But better safe than sorry.

I don't think this no longer valid for the very latest code.
The commit b7898396f4bb dropped the spurious dpcm_lock spinlock, so
the code path you touched must be always sleepable.

Similarly, the commit d8a9c6e1f676 can be reverted now.


thanks,

Takashi

> ---
>  sound/soc/soc-pcm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 7abfc48b26ca..1a536a2b9dc3 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -212,7 +212,7 @@ static void dpcm_create_debugfs_state(struct snd_soc_dpcm *dpcm, int stream)
>  {
>  	char *name;
>  
> -	name = kasprintf(GFP_KERNEL, "%s:%s", dpcm->be->dai_link->name,
> +	name = kasprintf(GFP_ATOMIC, "%s:%s", dpcm->be->dai_link->name,
>  			 stream ? "capture" : "playback");
>  	if (name) {
>  		dpcm->debugfs_state = debugfs_create_dir(
> -- 
> 2.32.0
> 

Re: [PATCH] ASoC: soc-pcm: use GFP_ATOMIC in dpcm_create_debugfs_state()

[Pierre-Louis Bossart]

On 1/17/22 2:49 AM, Takashi Iwai wrote:
> On Sun, 16 Jan 2022 12:18:17 +0100,
> Christophe JAILLET wrote:
>>
>> The commit below states that dpcm_be_connect() may be called from atomic
>> context. It changes a GFP_KERNEL into a GFP_ATOMIC to deal with it.
>>
>> Another memory allocation is done in dpcm_create_debugfs_state() which is
>> called by dpcm_be_connect(). Also use GFP_ATOMIC there to be consistent
>> and be compliant with atomic context.
>>
>> Fixes: d8a9c6e1f676 ("ASoC: soc-pcm: use GFP_ATOMIC for dpcm structure")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> Not clear to me how dpcm_be_connect() can be called from an atomic context,
>> though. But better safe than sorry.
> 
> I don't think this no longer valid for the very latest code.
> The commit b7898396f4bb dropped the spurious dpcm_lock spinlock, so
> the code path you touched must be always sleepable.
> 
> Similarly, the commit d8a9c6e1f676 can be reverted now.

Can we really revert d8a9c6e1f676?

We did propagate the non-atomic FE property to the BE, but if both FE
and BE are both atomic that constraint would be required, no?


> 
> thanks,
> 
> Takashi
> 
>> ---
>>  sound/soc/soc-pcm.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
>> index 7abfc48b26ca..1a536a2b9dc3 100644
>> --- a/sound/soc/soc-pcm.c
>> +++ b/sound/soc/soc-pcm.c
>> @@ -212,7 +212,7 @@ static void dpcm_create_debugfs_state(struct snd_soc_dpcm *dpcm, int stream)
>>  {
>>  	char *name;
>>  
>> -	name = kasprintf(GFP_KERNEL, "%s:%s", dpcm->be->dai_link->name,
>> +	name = kasprintf(GFP_ATOMIC, "%s:%s", dpcm->be->dai_link->name,
>>  			 stream ? "capture" : "playback");
>>  	if (name) {
>>  		dpcm->debugfs_state = debugfs_create_dir(
>> -- 
>> 2.32.0
>>

Re: [PATCH] ASoC: soc-pcm: use GFP_ATOMIC in dpcm_create_debugfs_state()

[Pierre-Louis Bossart]

On 1/17/22 2:49 AM, Takashi Iwai wrote:
> On Sun, 16 Jan 2022 12:18:17 +0100,
> Christophe JAILLET wrote:
>>
>> The commit below states that dpcm_be_connect() may be called from atomic
>> context. It changes a GFP_KERNEL into a GFP_ATOMIC to deal with it.
>>
>> Another memory allocation is done in dpcm_create_debugfs_state() which is
>> called by dpcm_be_connect(). Also use GFP_ATOMIC there to be consistent
>> and be compliant with atomic context.
>>
>> Fixes: d8a9c6e1f676 ("ASoC: soc-pcm: use GFP_ATOMIC for dpcm structure")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> Not clear to me how dpcm_be_connect() can be called from an atomic context,
>> though. But better safe than sorry.
> 
> I don't think this no longer valid for the very latest code.
> The commit b7898396f4bb dropped the spurious dpcm_lock spinlock, so
> the code path you touched must be always sleepable.
> 
> Similarly, the commit d8a9c6e1f676 can be reverted now.

Can we really revert d8a9c6e1f676?

We did propagate the non-atomic FE property to the BE, but if both FE
and BE are both atomic that constraint would be required, no?


> 
> thanks,
> 
> Takashi
> 
>> ---
>>  sound/soc/soc-pcm.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
>> index 7abfc48b26ca..1a536a2b9dc3 100644
>> --- a/sound/soc/soc-pcm.c
>> +++ b/sound/soc/soc-pcm.c
>> @@ -212,7 +212,7 @@ static void dpcm_create_debugfs_state(struct snd_soc_dpcm *dpcm, int stream)
>>  {
>>  	char *name;
>>  
>> -	name = kasprintf(GFP_KERNEL, "%s:%s", dpcm->be->dai_link->name,
>> +	name = kasprintf(GFP_ATOMIC, "%s:%s", dpcm->be->dai_link->name,
>>  			 stream ? "capture" : "playback");
>>  	if (name) {
>>  		dpcm->debugfs_state = debugfs_create_dir(
>> -- 
>> 2.32.0
>>

Re: [PATCH] ASoC: soc-pcm: use GFP_ATOMIC in dpcm_create_debugfs_state()

[Takashi Iwai]

On Mon, 17 Jan 2022 18:11:42 +0100,
Pierre-Louis Bossart wrote:
> 
> 
> 
> On 1/17/22 2:49 AM, Takashi Iwai wrote:
> > On Sun, 16 Jan 2022 12:18:17 +0100,
> > Christophe JAILLET wrote:
> >>
> >> The commit below states that dpcm_be_connect() may be called from atomic
> >> context. It changes a GFP_KERNEL into a GFP_ATOMIC to deal with it.
> >>
> >> Another memory allocation is done in dpcm_create_debugfs_state() which is
> >> called by dpcm_be_connect(). Also use GFP_ATOMIC there to be consistent
> >> and be compliant with atomic context.
> >>
> >> Fixes: d8a9c6e1f676 ("ASoC: soc-pcm: use GFP_ATOMIC for dpcm structure")
> >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> >> ---
> >> Not clear to me how dpcm_be_connect() can be called from an atomic context,
> >> though. But better safe than sorry.
> > 
> > I don't think this no longer valid for the very latest code.
> > The commit b7898396f4bb dropped the spurious dpcm_lock spinlock, so
> > the code path you touched must be always sleepable.
> > 
> > Similarly, the commit d8a9c6e1f676 can be reverted now.
> 
> Can we really revert d8a9c6e1f676?
> 
> We did propagate the non-atomic FE property to the BE, but if both FE
> and BE are both atomic that constraint would be required, no?

At the kzalloc() call in dpcm_be_connect, there is no spin lock
involved.  It's merely protected by card->pcm_mutex, instead.  The
spinlock is applied at the later call with
snd_soc_pcm_stream_lock_irq() only for the list manipulations.  (See
it's *_irq(), not *_irqsave(); that means the context being sleepable
at that point.)  So, we can use GFP_KERNEL safely there.

GFP_ATOMIC was needed in the past where dpcm_be_connect() itself is
called in dpcm_lock spinlock.  It was removed recently.


Takashi

> 
> 
> > 
> > thanks,
> > 
> > Takashi
> > 
> >> ---
> >>  sound/soc/soc-pcm.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> >> index 7abfc48b26ca..1a536a2b9dc3 100644
> >> --- a/sound/soc/soc-pcm.c
> >> +++ b/sound/soc/soc-pcm.c
> >> @@ -212,7 +212,7 @@ static void dpcm_create_debugfs_state(struct snd_soc_dpcm *dpcm, int stream)
> >>  {
> >>  	char *name;
> >>  
> >> -	name = kasprintf(GFP_KERNEL, "%s:%s", dpcm->be->dai_link->name,
> >> +	name = kasprintf(GFP_ATOMIC, "%s:%s", dpcm->be->dai_link->name,
> >>  			 stream ? "capture" : "playback");
> >>  	if (name) {
> >>  		dpcm->debugfs_state = debugfs_create_dir(
> >> -- 
> >> 2.32.0
> >>
> 

Re: [PATCH] ASoC: soc-pcm: use GFP_ATOMIC in dpcm_create_debugfs_state()

[Takashi Iwai]

On Mon, 17 Jan 2022 18:11:42 +0100,
Pierre-Louis Bossart wrote:
> 
> 
> 
> On 1/17/22 2:49 AM, Takashi Iwai wrote:
> > On Sun, 16 Jan 2022 12:18:17 +0100,
> > Christophe JAILLET wrote:
> >>
> >> The commit below states that dpcm_be_connect() may be called from atomic
> >> context. It changes a GFP_KERNEL into a GFP_ATOMIC to deal with it.
> >>
> >> Another memory allocation is done in dpcm_create_debugfs_state() which is
> >> called by dpcm_be_connect(). Also use GFP_ATOMIC there to be consistent
> >> and be compliant with atomic context.
> >>
> >> Fixes: d8a9c6e1f676 ("ASoC: soc-pcm: use GFP_ATOMIC for dpcm structure")
> >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> >> ---
> >> Not clear to me how dpcm_be_connect() can be called from an atomic context,
> >> though. But better safe than sorry.
> > 
> > I don't think this no longer valid for the very latest code.
> > The commit b7898396f4bb dropped the spurious dpcm_lock spinlock, so
> > the code path you touched must be always sleepable.
> > 
> > Similarly, the commit d8a9c6e1f676 can be reverted now.
> 
> Can we really revert d8a9c6e1f676?
> 
> We did propagate the non-atomic FE property to the BE, but if both FE
> and BE are both atomic that constraint would be required, no?

At the kzalloc() call in dpcm_be_connect, there is no spin lock
involved.  It's merely protected by card->pcm_mutex, instead.  The
spinlock is applied at the later call with
snd_soc_pcm_stream_lock_irq() only for the list manipulations.  (See
it's *_irq(), not *_irqsave(); that means the context being sleepable
at that point.)  So, we can use GFP_KERNEL safely there.

GFP_ATOMIC was needed in the past where dpcm_be_connect() itself is
called in dpcm_lock spinlock.  It was removed recently.


Takashi

> 
> 
> > 
> > thanks,
> > 
> > Takashi
> > 
> >> ---
> >>  sound/soc/soc-pcm.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> >> index 7abfc48b26ca..1a536a2b9dc3 100644
> >> --- a/sound/soc/soc-pcm.c
> >> +++ b/sound/soc/soc-pcm.c
> >> @@ -212,7 +212,7 @@ static void dpcm_create_debugfs_state(struct snd_soc_dpcm *dpcm, int stream)
> >>  {
> >>  	char *name;
> >>  
> >> -	name = kasprintf(GFP_KERNEL, "%s:%s", dpcm->be->dai_link->name,
> >> +	name = kasprintf(GFP_ATOMIC, "%s:%s", dpcm->be->dai_link->name,
> >>  			 stream ? "capture" : "playback");
> >>  	if (name) {
> >>  		dpcm->debugfs_state = debugfs_create_dir(
> >> -- 
> >> 2.32.0
> >>
> 

Re: [PATCH] ASoC: soc-pcm: use GFP_ATOMIC in dpcm_create_debugfs_state()

[Dan Carpenter]

On Mon, Jan 17, 2022 at 11:11:42AM -0600, Pierre-Louis Bossart wrote:
> 
> 
> On 1/17/22 2:49 AM, Takashi Iwai wrote:
> > On Sun, 16 Jan 2022 12:18:17 +0100,
> > Christophe JAILLET wrote:
> >>
> >> The commit below states that dpcm_be_connect() may be called from atomic
> >> context. It changes a GFP_KERNEL into a GFP_ATOMIC to deal with it.
> >>
> >> Another memory allocation is done in dpcm_create_debugfs_state() which is
> >> called by dpcm_be_connect(). Also use GFP_ATOMIC there to be consistent
> >> and be compliant with atomic context.
> >>
> >> Fixes: d8a9c6e1f676 ("ASoC: soc-pcm: use GFP_ATOMIC for dpcm structure")
> >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> >> ---
> >> Not clear to me how dpcm_be_connect() can be called from an atomic context,
> >> though. But better safe than sorry.
> > 
> > I don't think this no longer valid for the very latest code.
> > The commit b7898396f4bb dropped the spurious dpcm_lock spinlock, so
> > the code path you touched must be always sleepable.
> > 
> > Similarly, the commit d8a9c6e1f676 can be reverted now.
> 
> Can we really revert d8a9c6e1f676?
> 
> We did propagate the non-atomic FE property to the BE, but if both FE
> and BE are both atomic that constraint would be required, no?

I have a Smatch check for these now, so I reviewed this and came to the
same conclusions as Takashi.  If there is really a bug, let me know so
I can figure out where Smatch went wrong.

Unfortunately, I still have 64 sleeping in atomic warnings that I have
yet to triage.  It kind of takes a long time to look through the
warnings because of the long call trees involved.  Also Smatch the check
needs to be updated to warn about sleeping in IRQ context.

regards,
dan carpenter

Re: [PATCH] ASoC: soc-pcm: use GFP_ATOMIC in dpcm_create_debugfs_state()

[Dan Carpenter]

On Mon, Jan 17, 2022 at 11:11:42AM -0600, Pierre-Louis Bossart wrote:
> 
> 
> On 1/17/22 2:49 AM, Takashi Iwai wrote:
> > On Sun, 16 Jan 2022 12:18:17 +0100,
> > Christophe JAILLET wrote:
> >>
> >> The commit below states that dpcm_be_connect() may be called from atomic
> >> context. It changes a GFP_KERNEL into a GFP_ATOMIC to deal with it.
> >>
> >> Another memory allocation is done in dpcm_create_debugfs_state() which is
> >> called by dpcm_be_connect(). Also use GFP_ATOMIC there to be consistent
> >> and be compliant with atomic context.
> >>
> >> Fixes: d8a9c6e1f676 ("ASoC: soc-pcm: use GFP_ATOMIC for dpcm structure")
> >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> >> ---
> >> Not clear to me how dpcm_be_connect() can be called from an atomic context,
> >> though. But better safe than sorry.
> > 
> > I don't think this no longer valid for the very latest code.
> > The commit b7898396f4bb dropped the spurious dpcm_lock spinlock, so
> > the code path you touched must be always sleepable.
> > 
> > Similarly, the commit d8a9c6e1f676 can be reverted now.
> 
> Can we really revert d8a9c6e1f676?
> 
> We did propagate the non-atomic FE property to the BE, but if both FE
> and BE are both atomic that constraint would be required, no?

I have a Smatch check for these now, so I reviewed this and came to the
same conclusions as Takashi.  If there is really a bug, let me know so
I can figure out where Smatch went wrong.

Unfortunately, I still have 64 sleeping in atomic warnings that I have
yet to triage.  It kind of takes a long time to look through the
warnings because of the long call trees involved.  Also Smatch the check
needs to be updated to warn about sleeping in IRQ context.

regards,
dan carpenter