| Message ID | 1308100165.2113.4.camel@Tux (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Wed, 15 Jun 2011 09:09:24 Wanlong Gao <wanlong.gao@gmail.com> wrote: > <snip> > Hi Francis: > can you test this patch? Do you have a deadlock trace which you are trying to fix? It's either the caller of unregister_framebuffer() which must be changed to not call unregister_framebuffer with info's lock held or the code reacting on the notification that must not try to acquire the lock again. The interesting par is if console semaphore has some relation to this deadlock as the order for taking both varies... It could be lock_fb_info(); console_lock() versus console_lock(); lock_fb_info() Bruno > Thanks > > From fe026c42af4cbdce053460a428a445e99071586a Mon Sep 17 00:00:00 2001 > From: Wanlong Gao <wanlong.gao@gmail.com> > Date: Wed, 15 Jun 2011 09:03:41 +0800 > Subject: [PATCH] test > > > > Signed-off-by: Wanlong Gao <wanlong.gao@gmail.com> > --- > drivers/video/fbmem.c | 3 --- > 1 files changed, 0 insertions(+), 3 deletions(-) > > diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c > index 5aac00e..6e6cef3 100644 > --- a/drivers/video/fbmem.c > +++ b/drivers/video/fbmem.c > @@ -1642,11 +1642,8 @@ static int do_unregister_framebuffer(struct > fb_info *fb_info) > if (i < 0 || i >= FB_MAX || registered_fb[i] != fb_info) > return -EINVAL; > > - if (!lock_fb_info(fb_info)) > - return -ENODEV; > event.info = fb_info; > ret = fb_notifier_call_chain(FB_EVENT_FB_UNBIND, &event); > - unlock_fb_info(fb_info); Not a good idea to stop taking fb_lock here. Pretty all calls of fb_notifier_call_chain are protected by info's lock, except the one for FB_EVENT_FB_UNREGISTERED a few lines further. IMHO it wou make sense to add the lock around that last one so all notifier chain calls are handled the same. > if (ret) > return -EINVAL; -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 15, 2011 at 1:58 PM, Bruno Prémont <bonbons@linux-vserver.org> wrote: > > Hi, > > On Wed, 15 Jun 2011 09:09:24 Wanlong Gao <wanlong.gao@gmail.com> wrote: > > <snip> > > Hi Francis: > > can you test this patch? > > Do you have a deadlock trace which you are trying to fix? No, I just look at the code and try to fix this but I'm not sure. Can you teach me how to have a deadlock trace here? Thanks > > It's either the caller of unregister_framebuffer() which must be > changed to not call unregister_framebuffer with info's lock held or > the code reacting on the notification that must not try to acquire the > lock again. > > The interesting par is if console semaphore has some relation to this > deadlock as the order for taking both varies... It could be > lock_fb_info(); console_lock() versus console_lock(); lock_fb_info() > I see, thanks > Bruno > > > > Thanks > > ><snip> > > Not a good idea to stop taking fb_lock here. > Pretty all calls of fb_notifier_call_chain are protected by info's > lock, except the one for FB_EVENT_FB_UNREGISTERED a few lines further. Yup, thanks > > IMHO it wou make sense to add the lock around that last one so all > notifier chain calls are handled the same. > > <snip> > -- Best regards Wanlong Gao -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 15, 2011 at 2:22 PM, Wanlong Gao <wanlong.gao@gmail.com> wrote: > On Wed, Jun 15, 2011 at 1:58 PM, Bruno Prémont > <bonbons@linux-vserver.org> wrote: >> >> Hi, >> >> On Wed, 15 Jun 2011 09:09:24 Wanlong Gao <wanlong.gao@gmail.com> wrote: >> > <snip> >> > Hi Francis: >> > can you test this patch? >> >> Do you have a deadlock trace which you are trying to fix? > No, I just look at the code and try to fix this but I'm not sure. > Can you teach me how to have a deadlock trace here? CONFIG_LOCKDEP=y -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 15, 2011 at 7:58 AM, Bruno Prémont <bonbons@linux-vserver.org> wrote: > Hi, > > On Wed, 15 Jun 2011 09:09:24 Wanlong Gao <wanlong.gao@gmail.com> wrote: >> <snip> >> Hi Francis: >> can you test this patch? > > Do you have a deadlock trace which you are trying to fix? > > It's either the caller of unregister_framebuffer() which must be > changed to not call unregister_framebuffer with info's lock held or > the code reacting on the notification that must not try to acquire the > lock again. > > The interesting par is if console semaphore has some relation to this > deadlock as the order for taking both varies... It could be > lock_fb_info(); console_lock() versus console_lock(); lock_fb_info() > > Bruno > > >> Thanks >> >> From fe026c42af4cbdce053460a428a445e99071586a Mon Sep 17 00:00:00 2001 >> From: Wanlong Gao <wanlong.gao@gmail.com> >> Date: Wed, 15 Jun 2011 09:03:41 +0800 >> Subject: [PATCH] test >> >> >> >> Signed-off-by: Wanlong Gao <wanlong.gao@gmail.com> >> --- >> drivers/video/fbmem.c | 3 --- >> 1 files changed, 0 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c >> index 5aac00e..6e6cef3 100644 >> --- a/drivers/video/fbmem.c >> +++ b/drivers/video/fbmem.c >> @@ -1642,11 +1642,8 @@ static int do_unregister_framebuffer(struct >> fb_info *fb_info) >> if (i < 0 || i >= FB_MAX || registered_fb[i] != fb_info) >> return -EINVAL; >> >> - if (!lock_fb_info(fb_info)) >> - return -ENODEV; >> event.info = fb_info; >> ret = fb_notifier_call_chain(FB_EVENT_FB_UNBIND, &event); >> - unlock_fb_info(fb_info); > > Not a good idea to stop taking fb_lock here. > Pretty all calls of fb_notifier_call_chain are protected by info's > lock, except the one for FB_EVENT_FB_UNREGISTERED a few lines further. > > IMHO it wou make sense to add the lock around that last one so all > notifier chain calls are handled the same. > >> if (ret) >> return -EINVAL; > > Well, sorry for the dumb question but the fb/fbcon code is pretty hard to follow for me. Why does store_fbstate() and any fb driver's suspsend methods acquire the console lock at all ? Thanks
On Wed, 15 Jun 2011 09:12:46 Francis Moreau wrote: > On Wed, Jun 15, 2011 at 7:58 AM, Bruno Prémont wrote: > Well, sorry for the dumb question but the fb/fbcon code is pretty hard > to follow for me. Certainly not just for you > Why does store_fbstate() and any fb driver's suspsend methods acquire > the console lock at all ? From my understanding, fbcon currently has very loose binding with framebuffers in general instead of just with those few framebuffers it is effectively mapped to (and active on!). James Simmons started a complete rework of fbcon/tty code which is expected to get things more fine-grained, until then console semaphore (console_lock) remains a kind of big kernel lock in the console/framebuffer area. As such any state change of framebuffer may influence/race against fbcon. e.g. for the suspend state you want to avoid fbcon to fiddle with your framebuffer while it is suspending (and have fbcon know about the suspended state). This way fbcon can unsuspend framebuffer if needed but also stop accessing it when it should not. Bruno -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From fe026c42af4cbdce053460a428a445e99071586a Mon Sep 17 00:00:00 2001 From: Wanlong Gao <wanlong.gao@gmail.com> Date: Wed, 15 Jun 2011 09:03:41 +0800 Subject: [PATCH] test test Signed-off-by: Wanlong Gao <wanlong.gao@gmail.com> --- drivers/video/fbmem.c | 3 --- 1 files changed, 0 insertions(+), 3 deletions(-) diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c index 5aac00e..6e6cef3 100644 --- a/drivers/video/fbmem.c +++ b/drivers/video/fbmem.c @@ -1642,11 +1642,8 @@ static int do_unregister_framebuffer(struct fb_info *fb_info) if (i < 0 || i >= FB_MAX || registered_fb[i] != fb_info) return -EINVAL; - if (!lock_fb_info(fb_info)) - return -ENODEV; event.info = fb_info; ret = fb_notifier_call_chain(FB_EVENT_FB_UNBIND, &event); - unlock_fb_info(fb_info); if (ret) return -EINVAL; -- 1.7.4.1