Re: [PATCH] media: add GFP flag to media_*() that could get called in atomic context

[Mauro Carvalho Chehab]

Em Wed, 16 Mar 2016 10:28:35 +0200
Sakari Ailus  escreveu:

> Hi Mauro,
> 
> On Tue, Mar 15, 2016 at 12:55:35PM -0300, Mauro Carvalho Chehab wrote:
> > Em Mon, 14 Mar 2016 14:09:09 +0200
> > Sakari Ailus  escreveu:
> >   
> > > Hi Mauro,
> > > 

...

> > > Notify callbacks, perhaps not, but the list is still protected by the
> > > spinlock. It perhaps is not likely that another process would change it 
> > > but
> > > I don't think we can rely on that.  
> > 
> > I can see only 2 risks protected by the lock:
> > 
> > 1) mdev gets freed while an entity is being created. This is a problem
> >with the current memory protection schema we're using. I guess the
> >only way to fix it is to use kref for 
> > mdev/entities/interfaces/links/pads.
> >This change doesn't make it better or worse.
> >Also, I don't think we have such risk with the current devices.
> > 
> > 2) a notifier may be inserted or removed by another driver, while the
> >loop is running.
> > 
> > To avoid (2), I see 3 alternatives:
> > 
> > a) keep the loop as proposed on this patch. As the list is navigated using 
> > list_for_each_entry_safe(), I guess[1] it should be safe to remove/add
> > new notify callbacks there while the loop is running by some other process. 
> >   
> 
> list_for_each_entry_safe() does not protect against concurrent access, only
> against adding and removing list entries by the same user. List access
> serialisation is still needed, whether you use _safe() functions or not.
> 
> > 
> > [1] It *is* safe if the change were done inside the loop - but I'm not
> > 100% sure that it is safe if some other CPU touches the notify list.  
> 
> Indeed.
> 
> > 
> > b) Unlock/relock the spinlock every time:
> > 
> > /* previous code that locks mdev->lock spinlock */
> > 
> > /* invoke entity_notify callbacks */
> > list_for_each_entry_safe(notify, next, >entity_notify, list) {
> > spin_unlock(>lock);
> > (notify)->notify(entity, notify->notify_data);
> > spin_lock(>lock);
> > }
> >  
> > spin_unlock(>lock);
> > 
> > c) use a separate lock for the notify list -this seems to be an overkill.
> > 
> > d) Protect it with the graph traversal mutex. That sounds the worse idea,
> >IMHO, as we'll be abusing the lock.  
> 
> I'd simply replace the spinlock with a mutex here. As we want to get rid of
> the graph mutex anyway in the long run, let's not mix the two as they're
> well separated now. As long as the mutex users do not sleep (i.e. the
> notify() callback) the mutex is about as fast to use as the spinlock.

It could work. I added such patch on an experimental branch, where
I'm addressing a few troubles with au0828 unbind logic:

https://git.linuxtv.org/mchehab/experimental.git/log/?h=au0828-unbind-fixes

The patch itself is at:

https://git.linuxtv.org/mchehab/experimental.git/commit/?h=au0828-unbind-fixes=dba4d41bdfa6bb8dc51cb0f692102919b2b7c8b4

At least for au0828, it seems to work fine.

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] media: add GFP flag to media_*() that could get called in atomic context

[Sakari Ailus]

Hi Mauro,

On Tue, Mar 15, 2016 at 12:55:35PM -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 14 Mar 2016 14:09:09 +0200
> Sakari Ailus  escreveu:
> 
> > Hi Mauro,
> > 
> > On Mon, Mar 14, 2016 at 08:46:33AM -0300, Mauro Carvalho Chehab wrote:
> > > Em Mon, 14 Mar 2016 12:52:54 +0200
> > > Sakari Ailus  escreveu:
> > >   
> > > > Hi Mauro,
> > > > 
> > > > On Mon, Mar 14, 2016 at 07:13:58AM -0300, Mauro Carvalho Chehab wrote:  
> > > > > Em Mon, 14 Mar 2016 09:22:37 +0200
> > > > > Sakari Ailus  escreveu:
> > > > > 
> > > > > > Hi Shuah,
> > > > > > 
> > > > > > On Sat, Mar 12, 2016 at 06:48:09PM -0700, Shuah Khan wrote:
> > > > > > > Add GFP flags to media_create_pad_link(), 
> > > > > > > media_create_intf_link(),
> > > > > > > media_devnode_create(), and media_add_link() that could get called
> > > > > > > in atomic context to allow callers to pass in the right flags for
> > > > > > > memory allocation.
> > > > > > > 
> > > > > > > tree-wide driver changes for media_*() GFP flags change:
> > > > > > > Change drivers to add gfpflags to interffaces, 
> > > > > > > media_create_pad_link(),
> > > > > > > media_create_intf_link() and media_devnode_create().
> > > > > > > 
> > > > > > > Signed-off-by: Shuah Khan 
> > > > > > > Suggested-by: Mauro Carvalho Chehab  
> > > > > > >  
> > > > > > 
> > > > > > What's the use case for calling the above functions in an atomic 
> > > > > > context?
> > > > > > 
> > > > > 
> > > > > ALSA code seems to do a lot of stuff at atomic context. That's what
> > > > > happens on my test machine when au0828 gets probed before
> > > > > snd-usb-audio:
> > > > >   http://pastebin.com/LEX5LD5K
> > > > > 
> > > > > It seems that ALSA USB probe routine (usb_audio_probe) happens in
> > > > > atomic context.
> > > > 
> > > > usb_audio_probe() grabs a mutex (register_mutex) on its own. It 
> > > > certainly
> > > > cannot be called in atomic context.
> > > > 
> > > > In the above log, what I did notice, though, was that because *we* grab
> > > > mdev->lock spinlock in media_device_register_entity(), we may not sleep
> > > > which is what the notify() callback implementation in au0828 driver does
> > > > (for memory allocation).  
> > > 
> > > True. After looking into the code, the problem is that the notify
> > > callbacks are called with the spinlock hold. I don't see any reason
> > > to do that.  
> > 
> > Notify callbacks, perhaps not, but the list is still protected by the
> > spinlock. It perhaps is not likely that another process would change it but
> > I don't think we can rely on that.
> 
> I can see only 2 risks protected by the lock:
> 
> 1) mdev gets freed while an entity is being created. This is a problem
>with the current memory protection schema we're using. I guess the
>only way to fix it is to use kref for mdev/entities/interfaces/links/pads.
>This change doesn't make it better or worse.
>Also, I don't think we have such risk with the current devices.
> 
> 2) a notifier may be inserted or removed by another driver, while the
>loop is running.
> 
> To avoid (2), I see 3 alternatives:
> 
> a) keep the loop as proposed on this patch. As the list is navigated using 
> list_for_each_entry_safe(), I guess[1] it should be safe to remove/add
> new notify callbacks there while the loop is running by some other process. 

list_for_each_entry_safe() does not protect against concurrent access, only
against adding and removing list entries by the same user. List access
serialisation is still needed, whether you use _safe() functions or not.

> 
> [1] It *is* safe if the change were done inside the loop - but I'm not
> 100% sure that it is safe if some other CPU touches the notify list.

Indeed.

> 
> b) Unlock/relock the spinlock every time:
> 
>   /* previous code that locks mdev->lock spinlock */
> 
>   /* invoke entity_notify callbacks */
>   list_for_each_entry_safe(notify, next, >entity_notify, list) {
>   spin_unlock(>lock);
>   (notify)->notify(entity, notify->notify_data);
>   spin_lock(>lock);
>   }
>  
>   spin_unlock(>lock);
> 
> c) use a separate lock for the notify list -this seems to be an overkill.
> 
> d) Protect it with the graph traversal mutex. That sounds the worse idea,
>IMHO, as we'll be abusing the lock.

I'd simply replace the spinlock with a mutex here. As we want to get rid of
the graph mutex anyway in the long run, let's not mix the two as they're
well separated now. As long as the mutex users do not sleep (i.e. the
notify() callback) the mutex is about as fast to use as the spinlock.

> 
> > 
> > >   
> > > > Could we instead replace mdev->lock by a mutex?  
> > > 
> > > We changed the code to use a spinlock for a reason: this fixed some
> > > troubles in the past with the code locking (can't remember the problem,
> > > but this was documented 

Re: [PATCH] media: add GFP flag to media_*() that could get called in atomic context

[Mauro Carvalho Chehab]

Em Mon, 14 Mar 2016 14:09:09 +0200
Sakari Ailus  escreveu:

> Hi Mauro,
> 
> On Mon, Mar 14, 2016 at 08:46:33AM -0300, Mauro Carvalho Chehab wrote:
> > Em Mon, 14 Mar 2016 12:52:54 +0200
> > Sakari Ailus  escreveu:
> >   
> > > Hi Mauro,
> > > 
> > > On Mon, Mar 14, 2016 at 07:13:58AM -0300, Mauro Carvalho Chehab wrote:  
> > > > Em Mon, 14 Mar 2016 09:22:37 +0200
> > > > Sakari Ailus  escreveu:
> > > > 
> > > > > Hi Shuah,
> > > > > 
> > > > > On Sat, Mar 12, 2016 at 06:48:09PM -0700, Shuah Khan wrote:
> > > > > > Add GFP flags to media_create_pad_link(), media_create_intf_link(),
> > > > > > media_devnode_create(), and media_add_link() that could get called
> > > > > > in atomic context to allow callers to pass in the right flags for
> > > > > > memory allocation.
> > > > > > 
> > > > > > tree-wide driver changes for media_*() GFP flags change:
> > > > > > Change drivers to add gfpflags to interffaces, 
> > > > > > media_create_pad_link(),
> > > > > > media_create_intf_link() and media_devnode_create().
> > > > > > 
> > > > > > Signed-off-by: Shuah Khan 
> > > > > > Suggested-by: Mauro Carvalho Chehab   
> > > > > 
> > > > > What's the use case for calling the above functions in an atomic 
> > > > > context?
> > > > > 
> > > > 
> > > > ALSA code seems to do a lot of stuff at atomic context. That's what
> > > > happens on my test machine when au0828 gets probed before
> > > > snd-usb-audio:
> > > > http://pastebin.com/LEX5LD5K
> > > > 
> > > > It seems that ALSA USB probe routine (usb_audio_probe) happens in
> > > > atomic context.
> > > 
> > > usb_audio_probe() grabs a mutex (register_mutex) on its own. It certainly
> > > cannot be called in atomic context.
> > > 
> > > In the above log, what I did notice, though, was that because *we* grab
> > > mdev->lock spinlock in media_device_register_entity(), we may not sleep
> > > which is what the notify() callback implementation in au0828 driver does
> > > (for memory allocation).  
> > 
> > True. After looking into the code, the problem is that the notify
> > callbacks are called with the spinlock hold. I don't see any reason
> > to do that.  
> 
> Notify callbacks, perhaps not, but the list is still protected by the
> spinlock. It perhaps is not likely that another process would change it but
> I don't think we can rely on that.

I can see only 2 risks protected by the lock:

1) mdev gets freed while an entity is being created. This is a problem
   with the current memory protection schema we're using. I guess the
   only way to fix it is to use kref for mdev/entities/interfaces/links/pads.
   This change doesn't make it better or worse.
   Also, I don't think we have such risk with the current devices.

2) a notifier may be inserted or removed by another driver, while the
   loop is running.

To avoid (2), I see 3 alternatives:

a) keep the loop as proposed on this patch. As the list is navigated using 
list_for_each_entry_safe(), I guess[1] it should be safe to remove/add
new notify callbacks there while the loop is running by some other process. 

[1] It *is* safe if the change were done inside the loop - but I'm not
100% sure that it is safe if some other CPU touches the notify list.

b) Unlock/relock the spinlock every time:

/* previous code that locks mdev->lock spinlock */

/* invoke entity_notify callbacks */
list_for_each_entry_safe(notify, next, >entity_notify, list) {
spin_unlock(>lock);
(notify)->notify(entity, notify->notify_data);
spin_lock(>lock);
}
 
spin_unlock(>lock);

c) use a separate lock for the notify list -this seems to be an overkill.

d) Protect it with the graph traversal mutex. That sounds the worse idea,
   IMHO, as we'll be abusing the lock.

> 
> >   
> > > Could we instead replace mdev->lock by a mutex?  
> > 
> > We changed the code to use a spinlock for a reason: this fixed some
> > troubles in the past with the code locking (can't remember the problem,
> > but this was documented at the kernel logs and at the ML). Yet, the code
> > under the spinlock never sleeps, so this is fine.  
> 
> struct media_device.lock was added by this patch:
> 
> commit 53e269c102fbaf77e7dc526b1606ad4a48e57200
> Author: Laurent Pinchart 
> Date:   Wed Dec 9 08:40:00 2009 -0300
> 
> [media] media: Entities, pads and links
> 
> As video hardware pipelines become increasingly complex and
> configurable, the current hardware description through v4l2 subdevices
> reaches its limits. In addition to enumerating and configuring
> subdevices, video camera drivers need a way to discover and modify at
> runtime how those subdevices are connected. This is done through new
> elements called entities, pads and links.
> 
> ...
> 
> Signed-off-by: Laurent Pinchart 

Re: [PATCH] media: add GFP flag to media_*() that could get called in atomic context

[Sakari Ailus]

Hi Mauro,

On Mon, Mar 14, 2016 at 08:46:33AM -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 14 Mar 2016 12:52:54 +0200
> Sakari Ailus  escreveu:
> 
> > Hi Mauro,
> > 
> > On Mon, Mar 14, 2016 at 07:13:58AM -0300, Mauro Carvalho Chehab wrote:
> > > Em Mon, 14 Mar 2016 09:22:37 +0200
> > > Sakari Ailus  escreveu:
> > >   
> > > > Hi Shuah,
> > > > 
> > > > On Sat, Mar 12, 2016 at 06:48:09PM -0700, Shuah Khan wrote:  
> > > > > Add GFP flags to media_create_pad_link(), media_create_intf_link(),
> > > > > media_devnode_create(), and media_add_link() that could get called
> > > > > in atomic context to allow callers to pass in the right flags for
> > > > > memory allocation.
> > > > > 
> > > > > tree-wide driver changes for media_*() GFP flags change:
> > > > > Change drivers to add gfpflags to interffaces, 
> > > > > media_create_pad_link(),
> > > > > media_create_intf_link() and media_devnode_create().
> > > > > 
> > > > > Signed-off-by: Shuah Khan 
> > > > > Suggested-by: Mauro Carvalho Chehab 
> > > > 
> > > > What's the use case for calling the above functions in an atomic 
> > > > context?
> > > >   
> > > 
> > > ALSA code seems to do a lot of stuff at atomic context. That's what
> > > happens on my test machine when au0828 gets probed before
> > > snd-usb-audio:
> > >   http://pastebin.com/LEX5LD5K
> > > 
> > > It seems that ALSA USB probe routine (usb_audio_probe) happens in
> > > atomic context.  
> > 
> > usb_audio_probe() grabs a mutex (register_mutex) on its own. It certainly
> > cannot be called in atomic context.
> > 
> > In the above log, what I did notice, though, was that because *we* grab
> > mdev->lock spinlock in media_device_register_entity(), we may not sleep
> > which is what the notify() callback implementation in au0828 driver does
> > (for memory allocation).
> 
> True. After looking into the code, the problem is that the notify
> callbacks are called with the spinlock hold. I don't see any reason
> to do that.

Notify callbacks, perhaps not, but the list is still protected by the
spinlock. It perhaps is not likely that another process would change it but
I don't think we can rely on that.

> 
> > Could we instead replace mdev->lock by a mutex?
> 
> We changed the code to use a spinlock for a reason: this fixed some
> troubles in the past with the code locking (can't remember the problem,
> but this was documented at the kernel logs and at the ML). Yet, the code
> under the spinlock never sleeps, so this is fine.

struct media_device.lock was added by this patch:

commit 53e269c102fbaf77e7dc526b1606ad4a48e57200
Author: Laurent Pinchart 
Date:   Wed Dec 9 08:40:00 2009 -0300

[media] media: Entities, pads and links

As video hardware pipelines become increasingly complex and
configurable, the current hardware description through v4l2 subdevices
reaches its limits. In addition to enumerating and configuring
subdevices, video camera drivers need a way to discover and modify at
runtime how those subdevices are connected. This is done through new
elements called entities, pads and links.

...

Signed-off-by: Laurent Pinchart 
Signed-off-by: Sakari Ailus 
Acked-by: Hans Verkuil 
Signed-off-by: Mauro Carvalho Chehab 

I think it was always a spinlock, for the reason you stated above as well:
it did not need to sleep. But if there is a need to sleep, I think we should
consider changing that.

> 
> Yet, in the future, we'll need to do a review of all the locking schema,
> in order to better support dynamic graph changes.

Agreed. I think more fine grained locking should be considered. The media
graph mutex will become a bottleneck at some point, especially if we make
the media devices system wide at some point.

> 
> > If there is no pressing need to implement atomic memory allocation I simply
> > wouldn't do it, especially in device initialisation where an allocation
> > failure will lead to probe failure as well.
> 
> The fix for this issue should be simple. See the enclosed code. Btw.
> it probably makes sense to add some code here to avoid starving the
> stack, as a notify callback could try to create an entity, with,
> in turn, would call the notify callback again.
> 
> I'll run some tests here to double check if it fixes the issue.
> 
> ---
> 
> [media] media-device: Don't call notify callbacks holding a spinlock
> 
> The notify routines may sleep. So, they can't be called in spinlock
> context. Also, they may want to call other media routines that might
> be spinning the spinlock, like creating a link.
> 
> This fixes the following bug:
> 
> [ 1839.510587] BUG: sleeping function called from invalid context at 
> mm/slub.c:1289
> [ 1839.510881] in_atomic(): 1, irqs_disabled(): 0, pid: 3479, name: modprobe
> [ 1839.511157] 

Re: [PATCH] media: add GFP flag to media_*() that could get called in atomic context

[Mauro Carvalho Chehab]

Em Mon, 14 Mar 2016 12:52:54 +0200
Sakari Ailus  escreveu:

> Hi Mauro,
> 
> On Mon, Mar 14, 2016 at 07:13:58AM -0300, Mauro Carvalho Chehab wrote:
> > Em Mon, 14 Mar 2016 09:22:37 +0200
> > Sakari Ailus  escreveu:
> >   
> > > Hi Shuah,
> > > 
> > > On Sat, Mar 12, 2016 at 06:48:09PM -0700, Shuah Khan wrote:  
> > > > Add GFP flags to media_create_pad_link(), media_create_intf_link(),
> > > > media_devnode_create(), and media_add_link() that could get called
> > > > in atomic context to allow callers to pass in the right flags for
> > > > memory allocation.
> > > > 
> > > > tree-wide driver changes for media_*() GFP flags change:
> > > > Change drivers to add gfpflags to interffaces, media_create_pad_link(),
> > > > media_create_intf_link() and media_devnode_create().
> > > > 
> > > > Signed-off-by: Shuah Khan 
> > > > Suggested-by: Mauro Carvalho Chehab 
> > > 
> > > What's the use case for calling the above functions in an atomic context?
> > >   
> > 
> > ALSA code seems to do a lot of stuff at atomic context. That's what
> > happens on my test machine when au0828 gets probed before
> > snd-usb-audio:
> > http://pastebin.com/LEX5LD5K
> > 
> > It seems that ALSA USB probe routine (usb_audio_probe) happens in
> > atomic context.  
> 
> usb_audio_probe() grabs a mutex (register_mutex) on its own. It certainly
> cannot be called in atomic context.
> 
> In the above log, what I did notice, though, was that because *we* grab
> mdev->lock spinlock in media_device_register_entity(), we may not sleep
> which is what the notify() callback implementation in au0828 driver does
> (for memory allocation).

True. After looking into the code, the problem is that the notify
callbacks are called with the spinlock hold. I don't see any reason
to do that.

> Could we instead replace mdev->lock by a mutex?

We changed the code to use a spinlock for a reason: this fixed some
troubles in the past with the code locking (can't remember the problem,
but this was documented at the kernel logs and at the ML). Yet, the code
under the spinlock never sleeps, so this is fine.

Yet, in the future, we'll need to do a review of all the locking schema,
in order to better support dynamic graph changes.

> If there is no pressing need to implement atomic memory allocation I simply
> wouldn't do it, especially in device initialisation where an allocation
> failure will lead to probe failure as well.

The fix for this issue should be simple. See the enclosed code. Btw.
it probably makes sense to add some code here to avoid starving the
stack, as a notify callback could try to create an entity, with,
in turn, would call the notify callback again.

I'll run some tests here to double check if it fixes the issue.

---

[media] media-device: Don't call notify callbacks holding a spinlock

The notify routines may sleep. So, they can't be called in spinlock
context. Also, they may want to call other media routines that might
be spinning the spinlock, like creating a link.

This fixes the following bug:

[ 1839.510587] BUG: sleeping function called from invalid context at 
mm/slub.c:1289
[ 1839.510881] in_atomic(): 1, irqs_disabled(): 0, pid: 3479, name: modprobe
[ 1839.511157] 4 locks held by modprobe/3479:
[ 1839.511415]  #0:  (>mutex){..}, at: [] 
__driver_attach+0xa3/0x160
[ 1839.512381]  #1:  (>mutex){..}, at: [] 
__driver_attach+0xb1/0x160
[ 1839.512388]  #2:  (register_mutex#5){+.+.+.}, at: [] 
usb_audio_probe+0x257/0x1c90 [snd_usb_audio]
[ 1839.512401]  #3:  (&(>lock)->rlock){+.+.+.}, at: [] 
media_device_register_entity+0x1cb/0x700 [media]
[ 1839.512412] CPU: 2 PID: 3479 Comm: modprobe Not tainted 4.5.0-rc3+ #49
[ 1839.512415] Hardware name:  /NUC5i7RYB, BIOS 
RYBDWi35.86A.0350.2015.0812.1722 08/12/2015
[ 1839.512417]   8803b3f6f288 81933901 
8803c4bae000
[ 1839.512422]  8803c4bae5c8 8803b3f6f2b0 811c6af5 
8803c4bae000
[ 1839.512427]  8285d7f6 0509 8803b3f6f2f0 
811c6ce5
[ 1839.512432] Call Trace:
[ 1839.512436]  [] dump_stack+0x85/0xc4
[ 1839.512440]  [] ___might_sleep+0x245/0x3a0
[ 1839.512443]  [] __might_sleep+0x95/0x1a0
[ 1839.512446]  [] kmem_cache_alloc_trace+0x20e/0x300
[ 1839.512451]  [] ? media_add_link+0x4d/0x140 [media]
[ 1839.512455]  [] media_add_link+0x4d/0x140 [media]
[ 1839.512459]  [] media_create_pad_link+0xa1/0x600 [media]
[ 1839.512463]  [] au0828_media_graph_notify+0x173/0x360 
[au0828]
[ 1839.512467]  [] ? media_gobj_create+0x1ba/0x480 [media]
[ 1839.512471]  [] media_device_register_entity+0x3ab/0x700 
[media]

(untested)

Signed-off-by: Mauro Carvalho Chehab 

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 6ba6e8f982fc..fc3c199e5500 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -587,14 +587,15 @@ int __must_check 

Re: [PATCH] media: add GFP flag to media_*() that could get called in atomic context

[Sakari Ailus]

Hi Mauro,

On Mon, Mar 14, 2016 at 07:13:58AM -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 14 Mar 2016 09:22:37 +0200
> Sakari Ailus  escreveu:
> 
> > Hi Shuah,
> > 
> > On Sat, Mar 12, 2016 at 06:48:09PM -0700, Shuah Khan wrote:
> > > Add GFP flags to media_create_pad_link(), media_create_intf_link(),
> > > media_devnode_create(), and media_add_link() that could get called
> > > in atomic context to allow callers to pass in the right flags for
> > > memory allocation.
> > > 
> > > tree-wide driver changes for media_*() GFP flags change:
> > > Change drivers to add gfpflags to interffaces, media_create_pad_link(),
> > > media_create_intf_link() and media_devnode_create().
> > > 
> > > Signed-off-by: Shuah Khan 
> > > Suggested-by: Mauro Carvalho Chehab   
> > 
> > What's the use case for calling the above functions in an atomic context?
> > 
> 
> ALSA code seems to do a lot of stuff at atomic context. That's what
> happens on my test machine when au0828 gets probed before
> snd-usb-audio:
>   http://pastebin.com/LEX5LD5K
> 
> It seems that ALSA USB probe routine (usb_audio_probe) happens in
> atomic context.

usb_audio_probe() grabs a mutex (register_mutex) on its own. It certainly
cannot be called in atomic context.

In the above log, what I did notice, though, was that because *we* grab
mdev->lock spinlock in media_device_register_entity(), we may not sleep
which is what the notify() callback implementation in au0828 driver does
(for memory allocation).

Could we instead replace mdev->lock by a mutex?

If there is no pressing need to implement atomic memory allocation I simply
wouldn't do it, especially in device initialisation where an allocation
failure will lead to probe failure as well.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] media: add GFP flag to media_*() that could get called in atomic context

[Takashi Iwai]

On Mon, 14 Mar 2016 11:13:58 +0100,
Mauro Carvalho Chehab wrote:
> 
> Em Mon, 14 Mar 2016 09:22:37 +0200
> Sakari Ailus  escreveu:
> 
> > Hi Shuah,
> > 
> > On Sat, Mar 12, 2016 at 06:48:09PM -0700, Shuah Khan wrote:
> > > Add GFP flags to media_create_pad_link(), media_create_intf_link(),
> > > media_devnode_create(), and media_add_link() that could get called
> > > in atomic context to allow callers to pass in the right flags for
> > > memory allocation.
> > > 
> > > tree-wide driver changes for media_*() GFP flags change:
> > > Change drivers to add gfpflags to interffaces, media_create_pad_link(),
> > > media_create_intf_link() and media_devnode_create().
> > > 
> > > Signed-off-by: Shuah Khan 
> > > Suggested-by: Mauro Carvalho Chehab   
> > 
> > What's the use case for calling the above functions in an atomic context?
> > 
> 
> ALSA code seems to do a lot of stuff at atomic context. That's what
> happens on my test machine when au0828 gets probed before
> snd-usb-audio:
>   http://pastebin.com/LEX5LD5K
> 
> It seems that ALSA USB probe routine (usb_audio_probe) happens in
> atomic context.

No, usb_audio_probe() doesn't take a lock.  But
media_device_register_entity() seems taking a spinlock, instead.


Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] media: add GFP flag to media_*() that could get called in atomic context

[Mauro Carvalho Chehab]

Em Mon, 14 Mar 2016 09:22:37 +0200
Sakari Ailus  escreveu:

> Hi Shuah,
> 
> On Sat, Mar 12, 2016 at 06:48:09PM -0700, Shuah Khan wrote:
> > Add GFP flags to media_create_pad_link(), media_create_intf_link(),
> > media_devnode_create(), and media_add_link() that could get called
> > in atomic context to allow callers to pass in the right flags for
> > memory allocation.
> > 
> > tree-wide driver changes for media_*() GFP flags change:
> > Change drivers to add gfpflags to interffaces, media_create_pad_link(),
> > media_create_intf_link() and media_devnode_create().
> > 
> > Signed-off-by: Shuah Khan 
> > Suggested-by: Mauro Carvalho Chehab   
> 
> What's the use case for calling the above functions in an atomic context?
> 

ALSA code seems to do a lot of stuff at atomic context. That's what
happens on my test machine when au0828 gets probed before
snd-usb-audio:
http://pastebin.com/LEX5LD5K

It seems that ALSA USB probe routine (usb_audio_probe) happens in
atomic context.

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] media: add GFP flag to media_*() that could get called in atomic context

[Sakari Ailus]

Hi Shuah,

On Sat, Mar 12, 2016 at 06:48:09PM -0700, Shuah Khan wrote:
> Add GFP flags to media_create_pad_link(), media_create_intf_link(),
> media_devnode_create(), and media_add_link() that could get called
> in atomic context to allow callers to pass in the right flags for
> memory allocation.
> 
> tree-wide driver changes for media_*() GFP flags change:
> Change drivers to add gfpflags to interffaces, media_create_pad_link(),
> media_create_intf_link() and media_devnode_create().
> 
> Signed-off-by: Shuah Khan 
> Suggested-by: Mauro Carvalho Chehab 

What's the use case for calling the above functions in an atomic context?

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] media: add GFP flag to media_*() that could get called in atomic context

[Mauro Carvalho Chehab]

Em Sat, 12 Mar 2016 18:48:09 -0700
Shuah Khan  escreveu:

> Add GFP flags to media_create_pad_link(), media_create_intf_link(),
> media_devnode_create(), and media_add_link() that could get called
> in atomic context to allow callers to pass in the right flags for
> memory allocation.
> 
> tree-wide driver changes for media_*() GFP flags change:
> Change drivers to add gfpflags to interffaces, media_create_pad_link(),
> media_create_intf_link() and media_devnode_create().
> 
> Signed-off-by: Shuah Khan 
> Suggested-by: Mauro Carvalho Chehab 

Patches look OK to me[1], provided that a followup patch is
changes some of the callers to use GFP_ATOMIC.

I'll wait for such patch before merging this one.

Regards,
Mauro

[1] but see Nicholas comments about indentation.

> ---
> Ran through kbuild-all compile testing.
> Tested the changes in Win-TV HVR-950Q device
> 
>  drivers/media/dvb-core/dvbdev.c| 26 +++-
>  drivers/media/i2c/s5c73m3/s5c73m3-core.c   |  6 ++-
>  drivers/media/i2c/s5k5baf.c|  3 +-
>  drivers/media/i2c/smiapp/smiapp-core.c |  3 +-
>  drivers/media/i2c/tvp5150.c|  3 +-
>  drivers/media/media-entity.c   | 30 --
>  drivers/media/platform/exynos4-is/media-dev.c  | 19 +
>  drivers/media/platform/omap3isp/isp.c  | 47 
> ++
>  drivers/media/platform/s3c-camif/camif-core.c  |  4 +-
>  drivers/media/platform/vsp1/vsp1_drm.c |  6 +--
>  drivers/media/platform/vsp1/vsp1_drv.c |  9 +++--
>  drivers/media/platform/xilinx/xilinx-vipp.c|  4 +-
>  drivers/media/usb/au0828/au0828-core.c |  3 +-
>  drivers/media/usb/uvc/uvc_entity.c |  2 +-
>  drivers/media/v4l2-core/v4l2-dev.c |  5 ++-
>  drivers/media/v4l2-core/v4l2-device.c  |  3 +-
>  drivers/media/v4l2-core/v4l2-mc.c  | 25 +++-
>  drivers/staging/media/davinci_vpfe/dm365_ipipeif.c |  3 +-
>  drivers/staging/media/davinci_vpfe/dm365_isif.c|  2 +-
>  drivers/staging/media/davinci_vpfe/dm365_resizer.c | 10 +++--
>  .../staging/media/davinci_vpfe/vpfe_mc_capture.c   | 10 ++---
>  drivers/staging/media/omap4iss/iss.c   | 17 +---
>  drivers/staging/media/omap4iss/iss_csi2.c  |  6 ++-
>  drivers/staging/media/omap4iss/iss_ipipeif.c   |  3 +-
>  drivers/staging/media/omap4iss/iss_resizer.c   |  3 +-
>  include/media/media-entity.h   |  9 +++--
>  sound/usb/media.c  | 15 ---
>  27 files changed, 170 insertions(+), 106 deletions(-)
> 
> diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
> index e1684c5..57f3e1e 100644
> --- a/drivers/media/dvb-core/dvbdev.c
> +++ b/drivers/media/dvb-core/dvbdev.c
> @@ -399,7 +399,8 @@ static int dvb_register_media_device(struct dvb_device 
> *dvbdev,
>  
>   dvbdev->intf_devnode = media_devnode_create(dvbdev->adapter->mdev,
>   intf_type, 0,
> - DVB_MAJOR, minor);
> + DVB_MAJOR, minor,
> + GFP_KERNEL);
>  
>   if (!dvbdev->intf_devnode)
>   return -ENOMEM;
> @@ -416,7 +417,7 @@ static int dvb_register_media_device(struct dvb_device 
> *dvbdev,
>   return 0;
>  
>   link = media_create_intf_link(dvbdev->entity, 
> >intf_devnode->intf,
> -   MEDIA_LNK_FL_ENABLED);
> +   MEDIA_LNK_FL_ENABLED, GFP_KERNEL);
>   if (!link)
>   return -ENOMEM;
>  #endif
> @@ -558,7 +559,8 @@ static int dvb_create_io_intf_links(struct dvb_adapter 
> *adap,
>   if (strncmp(entity->name, name, strlen(name)))
>   continue;
>   link = media_create_intf_link(entity, intf,
> -   MEDIA_LNK_FL_ENABLED);
> +   MEDIA_LNK_FL_ENABLED,
> +   GFP_KERNEL);
>   if (!link)
>   return -ENOMEM;
>   }
> @@ -680,7 +682,8 @@ int dvb_create_media_graph(struct dvb_adapter *adap,
>   }
>   if (demux && ca) {
>   ret = media_create_pad_link(demux, 1, ca,
> - 0, MEDIA_LNK_FL_ENABLED);
> + 0, MEDIA_LNK_FL_ENABLED,
> + GFP_KERNEL);
>   if (ret)
>   return -ENOMEM;
>   }
> @@ -693,7 +696,8 @@ int dvb_create_media_graph(struct dvb_adapter *adap,
>   

Re: [PATCH] media: add GFP flag to media_*() that could get called in atomic context

[Nicholas Mc Guire]

On Sat, Mar 12, 2016 at 06:48:09PM -0700, Shuah Khan wrote:
> Add GFP flags to media_create_pad_link(), media_create_intf_link(),
> media_devnode_create(), and media_add_link() that could get called
> in atomic context to allow callers to pass in the right flags for
> memory allocation.
> 
> tree-wide driver changes for media_*() GFP flags change:
> Change drivers to add gfpflags to interffaces, media_create_pad_link(),
> media_create_intf_link() and media_devnode_create().
>

in two cases (media_add_link,media_devnode_create) it is passing in 
gfpflags but then checking for in_atomic() and in case it is, dumping
stack - so does it make sense in those cases to allow GFP_ATOMIC to be
passed in ? could not figure out why that would be needed (current 
callers in media-entity.c do not seem to be under a spin_lock).

and in a few cases there seems to be a little glitch with indentation 
  dvb_create_media_graph
  __fimc_md_create_fimc_sink_links
  __fimc_md_create_fimc_is_links
  vsp1_create_sink_links
  v4l2_mc_create_media_graph
  vpfe_ipipeif_register_entities

thx!
hofrat
 
> Signed-off-by: Shuah Khan 
> Suggested-by: Mauro Carvalho Chehab 
> ---
> Ran through kbuild-all compile testing.
> Tested the changes in Win-TV HVR-950Q device
> 
>  drivers/media/dvb-core/dvbdev.c| 26 +++-
>  drivers/media/i2c/s5c73m3/s5c73m3-core.c   |  6 ++-
>  drivers/media/i2c/s5k5baf.c|  3 +-
>  drivers/media/i2c/smiapp/smiapp-core.c |  3 +-
>  drivers/media/i2c/tvp5150.c|  3 +-
>  drivers/media/media-entity.c   | 30 --
>  drivers/media/platform/exynos4-is/media-dev.c  | 19 +
>  drivers/media/platform/omap3isp/isp.c  | 47 
> ++
>  drivers/media/platform/s3c-camif/camif-core.c  |  4 +-
>  drivers/media/platform/vsp1/vsp1_drm.c |  6 +--
>  drivers/media/platform/vsp1/vsp1_drv.c |  9 +++--
>  drivers/media/platform/xilinx/xilinx-vipp.c|  4 +-
>  drivers/media/usb/au0828/au0828-core.c |  3 +-
>  drivers/media/usb/uvc/uvc_entity.c |  2 +-
>  drivers/media/v4l2-core/v4l2-dev.c |  5 ++-
>  drivers/media/v4l2-core/v4l2-device.c  |  3 +-
>  drivers/media/v4l2-core/v4l2-mc.c  | 25 +++-
>  drivers/staging/media/davinci_vpfe/dm365_ipipeif.c |  3 +-
>  drivers/staging/media/davinci_vpfe/dm365_isif.c|  2 +-
>  drivers/staging/media/davinci_vpfe/dm365_resizer.c | 10 +++--
>  .../staging/media/davinci_vpfe/vpfe_mc_capture.c   | 10 ++---
>  drivers/staging/media/omap4iss/iss.c   | 17 +---
>  drivers/staging/media/omap4iss/iss_csi2.c  |  6 ++-
>  drivers/staging/media/omap4iss/iss_ipipeif.c   |  3 +-
>  drivers/staging/media/omap4iss/iss_resizer.c   |  3 +-
>  include/media/media-entity.h   |  9 +++--
>  sound/usb/media.c  | 15 ---
>  27 files changed, 170 insertions(+), 106 deletions(-)
> 
> diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
> index e1684c5..57f3e1e 100644
> --- a/drivers/media/dvb-core/dvbdev.c
> +++ b/drivers/media/dvb-core/dvbdev.c
> @@ -399,7 +399,8 @@ static int dvb_register_media_device(struct dvb_device 
> *dvbdev,
>  
>   dvbdev->intf_devnode = media_devnode_create(dvbdev->adapter->mdev,
>   intf_type, 0,
> - DVB_MAJOR, minor);
> + DVB_MAJOR, minor,
> + GFP_KERNEL);
>  
>   if (!dvbdev->intf_devnode)
>   return -ENOMEM;
> @@ -416,7 +417,7 @@ static int dvb_register_media_device(struct dvb_device 
> *dvbdev,
>   return 0;
>  
>   link = media_create_intf_link(dvbdev->entity, 
> >intf_devnode->intf,
> -   MEDIA_LNK_FL_ENABLED);
> +   MEDIA_LNK_FL_ENABLED, GFP_KERNEL);
>   if (!link)
>   return -ENOMEM;
>  #endif
> @@ -558,7 +559,8 @@ static int dvb_create_io_intf_links(struct dvb_adapter 
> *adap,
>   if (strncmp(entity->name, name, strlen(name)))
>   continue;
>   link = media_create_intf_link(entity, intf,
> -   MEDIA_LNK_FL_ENABLED);
> +   MEDIA_LNK_FL_ENABLED,
> +   GFP_KERNEL);
>   if (!link)
>   return -ENOMEM;
>   }
> @@ -680,7 +682,8 @@ int dvb_create_media_graph(struct dvb_adapter *adap,
>   }
>   if (demux && ca) {
>   ret = media_create_pad_link(demux, 1, ca,
> -   

[PATCH] media: add GFP flag to media_*() that could get called in atomic context

[Shuah Khan]

Add GFP flags to media_create_pad_link(), media_create_intf_link(),
media_devnode_create(), and media_add_link() that could get called
in atomic context to allow callers to pass in the right flags for
memory allocation.

tree-wide driver changes for media_*() GFP flags change:
Change drivers to add gfpflags to interffaces, media_create_pad_link(),
media_create_intf_link() and media_devnode_create().

Signed-off-by: Shuah Khan 
Suggested-by: Mauro Carvalho Chehab 
---
Ran through kbuild-all compile testing.
Tested the changes in Win-TV HVR-950Q device

 drivers/media/dvb-core/dvbdev.c| 26 +++-
 drivers/media/i2c/s5c73m3/s5c73m3-core.c   |  6 ++-
 drivers/media/i2c/s5k5baf.c|  3 +-
 drivers/media/i2c/smiapp/smiapp-core.c |  3 +-
 drivers/media/i2c/tvp5150.c|  3 +-
 drivers/media/media-entity.c   | 30 --
 drivers/media/platform/exynos4-is/media-dev.c  | 19 +
 drivers/media/platform/omap3isp/isp.c  | 47 ++
 drivers/media/platform/s3c-camif/camif-core.c  |  4 +-
 drivers/media/platform/vsp1/vsp1_drm.c |  6 +--
 drivers/media/platform/vsp1/vsp1_drv.c |  9 +++--
 drivers/media/platform/xilinx/xilinx-vipp.c|  4 +-
 drivers/media/usb/au0828/au0828-core.c |  3 +-
 drivers/media/usb/uvc/uvc_entity.c |  2 +-
 drivers/media/v4l2-core/v4l2-dev.c |  5 ++-
 drivers/media/v4l2-core/v4l2-device.c  |  3 +-
 drivers/media/v4l2-core/v4l2-mc.c  | 25 +++-
 drivers/staging/media/davinci_vpfe/dm365_ipipeif.c |  3 +-
 drivers/staging/media/davinci_vpfe/dm365_isif.c|  2 +-
 drivers/staging/media/davinci_vpfe/dm365_resizer.c | 10 +++--
 .../staging/media/davinci_vpfe/vpfe_mc_capture.c   | 10 ++---
 drivers/staging/media/omap4iss/iss.c   | 17 +---
 drivers/staging/media/omap4iss/iss_csi2.c  |  6 ++-
 drivers/staging/media/omap4iss/iss_ipipeif.c   |  3 +-
 drivers/staging/media/omap4iss/iss_resizer.c   |  3 +-
 include/media/media-entity.h   |  9 +++--
 sound/usb/media.c  | 15 ---
 27 files changed, 170 insertions(+), 106 deletions(-)

diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
index e1684c5..57f3e1e 100644
--- a/drivers/media/dvb-core/dvbdev.c
+++ b/drivers/media/dvb-core/dvbdev.c
@@ -399,7 +399,8 @@ static int dvb_register_media_device(struct dvb_device 
*dvbdev,
 
dvbdev->intf_devnode = media_devnode_create(dvbdev->adapter->mdev,
intf_type, 0,
-   DVB_MAJOR, minor);
+   DVB_MAJOR, minor,
+   GFP_KERNEL);
 
if (!dvbdev->intf_devnode)
return -ENOMEM;
@@ -416,7 +417,7 @@ static int dvb_register_media_device(struct dvb_device 
*dvbdev,
return 0;
 
link = media_create_intf_link(dvbdev->entity, 
>intf_devnode->intf,
- MEDIA_LNK_FL_ENABLED);
+ MEDIA_LNK_FL_ENABLED, GFP_KERNEL);
if (!link)
return -ENOMEM;
 #endif
@@ -558,7 +559,8 @@ static int dvb_create_io_intf_links(struct dvb_adapter 
*adap,
if (strncmp(entity->name, name, strlen(name)))
continue;
link = media_create_intf_link(entity, intf,
- MEDIA_LNK_FL_ENABLED);
+ MEDIA_LNK_FL_ENABLED,
+ GFP_KERNEL);
if (!link)
return -ENOMEM;
}
@@ -680,7 +682,8 @@ int dvb_create_media_graph(struct dvb_adapter *adap,
}
if (demux && ca) {
ret = media_create_pad_link(demux, 1, ca,
-   0, MEDIA_LNK_FL_ENABLED);
+   0, MEDIA_LNK_FL_ENABLED,
+   GFP_KERNEL);
if (ret)
return -ENOMEM;
}
@@ -693,7 +696,8 @@ int dvb_create_media_graph(struct dvb_adapter *adap,
strlen(DVR_TSOUT))) {
ret = media_create_pad_link(demux,
++dvr_pad,
-   entity, 0, 0);
+   entity, 0, 0,
+   GFP_KERNEL);