Hi, all
I just tested the sis315 driver in the latest master branch of
linux-mips git repository on fuloong2f(loongson-based machine, loongson
is mips comptable):
Device Drivers --->
Graphics support --->
<*> Support for frame buffer devices --->
<*> SiS/XGI display support
[*] SiS 315/330/340 series and XGI support
The kernel stopped at(output via serial port):
...
sisfb: Detected SiS301C video bridge
sisfb: CRT1 DDC probing failed
sisfb: Default mode is 800x600x8 (60HZ)
no more output, but the Num,Caps light of keyboard works.
and because sis 315 of the latest linux-2.6.30-stable works well on
fuloong2f, so, I tried to check the latest commits who modified sis
relative source code via "git whatchanged", at last I found this:
commit 537a1bf059fa312355696fa6db80726e655e7f17
Author: Krzysztof Helt <[email protected]>
Date: Tue Jun 30 11:41:29 2009 -0700
fbdev: add mutex for fb_mmap locking
Add a mutex to avoid a circular locking problem between the mm layer
semaphore and fbdev ioctl mutex through the fb_mmap() call.
Also, add mutex to all places where smem_start and smem_len fields
change
so the mutex inside the fb_mmap() is actually used. Changing of
these
fields before calling the framebuffer_register() are not mutexed.
This is 2.6.31 material. It removes one lockdep (fb_mmap() and
register_framebuffer()) but there is still another one (fb_release()
and
register_framebuffer()). It also cleans up handling of the
smem_start and
smem_len fields used by mutexed section of the fb_mmap().
Signed-off-by: Krzysztof Helt <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
Signed-off-by: Linus Torvalds <[email protected]>
and then, I tried to revert the modification in
drivers/video/sis/sis_main.c:
diff --git a/drivers/video/sis/sis_main.c b/drivers/video/sis/sis_main.c
index fd33455..7072d19 100644
--- a/drivers/video/sis/sis_main.c
+++ b/drivers/video/sis/sis_main.c
@@ -1847,10 +1847,8 @@ sisfb_get_fix(struct fb_fix_screeninfo *fix, int
con, struct fb_info *info)
strcpy(fix->id, ivideo->myid);
- mutex_lock(&info->mm_lock);
fix->smem_start = ivideo->video_base + ivideo->video_offset;
fix->smem_len = ivideo->sisfb_mem;
- mutex_unlock(&info->mm_lock);
fix->type = FB_TYPE_PACKED_PIXELS;
fix->type_aux = 0;
fix->visual = (ivideo->video_bpp == 8) ?
FB_VISUAL_PSEUDOCOLOR : FB_VISUAL_TRUECOLOR;
then it works! so, I guess there is a deadlock introduced by the above
commit.
I attached my config file, hope it can give you more information about
this bug.
Regards,
Wu Zhangjin
On Sun, Jul 05, 2009 at 07:19:33AM -0700, Linus Torvalds wrote:
>
>
> On Sun, 5 Jul 2009, Wu Zhangjin wrote:
> >
> > then it works! so, I guess there is a deadlock introduced by the above
> > commit.
>
> Hmm. Perhaps more likely, the 'mm_lock' mutex hasn't even been initialized
> yet. We appear to have had that problem with matroxfb and sm501fb, and it
> may be more common than that. See commit f50bf2b2.
>
> That said, I do agree that the mm_lock seems to be causing more problems
> than it actually fixes, and maybe we should revert it. Krzysztof?
>
Looking at this a bit closer, just moving the mutex initialization in to
framebuffer_alloc() should basically fix most of these, at least it
certainly does for sm501fb and for this sis case as well. So, here's a
patch to do that.
As an aside note, matroxfb is the odd one out, as it doesn't use
framebuffer_alloc() directly for whatever reason. This actually raises an
additional issue, in that framebuffer_alloc() is already where other
mutexes are initialized, which will simply never happen on matroxfb
(suggesting that at least the FB_BACKLIGHT and matroxfb combination will
blow up, although perhaps that's not a valid combination).
Signed-off-by: Paul Mundt <[email protected]>
---
drivers/video/fbmem.c | 1 -
drivers/video/fbsysfs.c | 2 ++
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 53ea056..27a5271 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1514,7 +1514,6 @@ register_framebuffer(struct fb_info *fb_info)
break;
fb_info->node = i;
mutex_init(&fb_info->lock);
- mutex_init(&fb_info->mm_lock);
fb_info->dev = device_create(fb_class, fb_info->device,
MKDEV(FB_MAJOR, i), NULL, "fb%d", i);
diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
index d4a2c11..dd413ad 100644
--- a/drivers/video/fbsysfs.c
+++ b/drivers/video/fbsysfs.c
@@ -62,6 +62,8 @@ struct fb_info *framebuffer_alloc(size_t size, struct device *dev)
mutex_init(&info->bl_curve_mutex);
#endif
+ mutex_init(&info->mm_lock);
+
return info;
#undef PADDING
#undef BYTES_PER_LONG
On Sun, 5 Jul 2009 07:19:33 -0700 (PDT)
Linus Torvalds <[email protected]> wrote:
>
>
> On Sun, 5 Jul 2009, Wu Zhangjin wrote:
> >
> > then it works! so, I guess there is a deadlock introduced by the above
> > commit.
>
> Hmm. Perhaps more likely, the 'mm_lock' mutex hasn't even been initialized
> yet. We appear to have had that problem with matroxfb and sm501fb, and it
> may be more common than that. See commit f50bf2b2.
>
> That said, I do agree that the mm_lock seems to be causing more problems
> than it actually fixes, and maybe we should revert it. Krzysztof?
>
I vote for fixing these drivers after my change. I will send a patch for the sis driver soon. I am building new kernel now.
Regards,
Krzysztof
----------------------------------------------------------------------
Rozwiaz krzyzowke i wygraj nagrody!
Sprawdz >> http://link.interia.pl/f2232
On Sun, 5 Jul 2009, Paul Mundt wrote:
> break;
> fb_info->node = i;
> mutex_init(&fb_info->lock);
> - mutex_init(&fb_info->mm_lock);
Why not "lock" as well?
Linus
On Sun, Jul 05, 2009 at 07:56:56AM -0700, Linus Torvalds wrote:
>
>
> On Sun, 5 Jul 2009, Paul Mundt wrote:
> > break;
> > fb_info->node = i;
> > mutex_init(&fb_info->lock);
> > - mutex_init(&fb_info->mm_lock);
>
> Why not "lock" as well?
>
I had that initially, but matroxfb will break if we do that, and
presently nothing cares about trying to take ->lock that early on.
->mm_lock was a special case as the lock/unlock pairs were sprinkled
around well before initialization, while in the ->lock case all of the
lock/unlock pairs are handled internally by the fbmem code (at least a
quick grep does not show any drivers using it on their own).
On Mon, 6 Jul 2009, Paul Mundt wrote:
> >
> > Why not "lock" as well?
>
> I had that initially, but matroxfb will break if we do that, and
> presently nothing cares about trying to take ->lock that early on.
I really would rather have consistency than some odd rules like that.
In particular - if matroxfb is different and needs its own lock
initialization because it doesn't use the common allocation routine, then
please make _that_ consistent too. Rather than have it special-case just
one lock that it needs to initialize separately, make it clear that since
it does its own allocations it needs to initialize _everything_
separately.
Otherwise anybody grepping for things will always be confused, since
depending on random factors they'll notice the initializations in one
place or the other, and then do the wrong thing - exactly like mm_lock was
not correctly initialized this time around.
In other words: it's actually _better_ to make the matroxfb pain _more_
obvious rather than less.
And maybe we can deprecate the driver, throw it out entirely, or have
somebody actually fix it?
Linus
On Sun, Jul 05, 2009 at 08:19:40AM -0700, Linus Torvalds wrote:
>
>
> On Mon, 6 Jul 2009, Paul Mundt wrote:
> > >
> > > Why not "lock" as well?
> >
> > I had that initially, but matroxfb will break if we do that, and
> > presently nothing cares about trying to take ->lock that early on.
>
> I really would rather have consistency than some odd rules like that.
>
> In particular - if matroxfb is different and needs its own lock
> initialization because it doesn't use the common allocation routine, then
> please make _that_ consistent too. Rather than have it special-case just
> one lock that it needs to initialize separately, make it clear that since
> it does its own allocations it needs to initialize _everything_
> separately.
>
Ok, here is an updated version with an updated matroxfb and the sm501fb
change reverted.
Signed-off-by: Paul Mundt <[email protected]>
---
drivers/video/fbmem.c | 2 --
drivers/video/fbsysfs.c | 3 +++
drivers/video/matrox/matroxfb_base.c | 1 +
drivers/video/sm501fb.c | 2 --
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 53ea056..53eb396 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1513,8 +1513,6 @@ register_framebuffer(struct fb_info *fb_info)
if (!registered_fb[i])
break;
fb_info->node = i;
- mutex_init(&fb_info->lock);
- mutex_init(&fb_info->mm_lock);
fb_info->dev = device_create(fb_class, fb_info->device,
MKDEV(FB_MAJOR, i), NULL, "fb%d", i);
diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
index d4a2c11..afc04df 100644
--- a/drivers/video/fbsysfs.c
+++ b/drivers/video/fbsysfs.c
@@ -62,6 +62,9 @@ struct fb_info *framebuffer_alloc(size_t size, struct device *dev)
mutex_init(&info->bl_curve_mutex);
#endif
+ mutex_init(&info->lock);
+ mutex_init(&info->mm_lock);
+
return info;
#undef PADDING
#undef BYTES_PER_LONG
diff --git a/drivers/video/matrox/matroxfb_base.c b/drivers/video/matrox/matroxfb_base.c
index 59c3a2e..76bc51b 100644
--- a/drivers/video/matrox/matroxfb_base.c
+++ b/drivers/video/matrox/matroxfb_base.c
@@ -2083,6 +2083,7 @@ static int matroxfb_probe(struct pci_dev* pdev, const struct pci_device_id* dumm
spin_lock_init(&ACCESS_FBINFO(lock.accel));
init_rwsem(&ACCESS_FBINFO(crtc2.lock));
init_rwsem(&ACCESS_FBINFO(altout.lock));
+ mutex_init(&ACCESS_FBINFO(fbcon).lock);
mutex_init(&ACCESS_FBINFO(fbcon).mm_lock);
ACCESS_FBINFO(irq_flags) = 0;
init_waitqueue_head(&ACCESS_FBINFO(crtc1.vsync.wait));
diff --git a/drivers/video/sm501fb.c b/drivers/video/sm501fb.c
index 16d4f4c..98f24f0 100644
--- a/drivers/video/sm501fb.c
+++ b/drivers/video/sm501fb.c
@@ -1624,8 +1624,6 @@ static int __devinit sm501fb_start_one(struct sm501fb_info *info,
if (!fbi)
return 0;
- mutex_init(&info->fb[head]->mm_lock);
-
ret = sm501fb_init_fb(info->fb[head], head, drvname);
if (ret) {
dev_err(info->dev, "cannot initialise fb %s\n", drvname);
On Sun, 5 Jul 2009, Wu Zhangjin wrote:
>
> then it works! so, I guess there is a deadlock introduced by the above
> commit.
Hmm. Perhaps more likely, the 'mm_lock' mutex hasn't even been initialized
yet. We appear to have had that problem with matroxfb and sm501fb, and it
may be more common than that. See commit f50bf2b2.
That said, I do agree that the mm_lock seems to be causing more problems
than it actually fixes, and maybe we should revert it. Krzysztof?
Linus
On Mon, 6 Jul 2009, Paul Mundt wrote:
>
> Ok, here is an updated version with an updated matroxfb and the sm501fb
> change reverted.
Wu Zhangjin, can you also confirm that this works for you (without your
patch)?
Linus
On Mon, 6 Jul 2009 00:25:57 +0900
Paul Mundt <[email protected]> wrote:
> On Sun, Jul 05, 2009 at 08:19:40AM -0700, Linus Torvalds wrote:
> >
> >
> > On Mon, 6 Jul 2009, Paul Mundt wrote:
> > > >
> > > > Why not "lock" as well?
> > >
> > > I had that initially, but matroxfb will break if we do that, and
> > > presently nothing cares about trying to take ->lock that early on.
> >
> > I really would rather have consistency than some odd rules like that.
> >
> > In particular - if matroxfb is different and needs its own lock
> > initialization because it doesn't use the common allocation routine, then
> > please make _that_ consistent too. Rather than have it special-case just
> > one lock that it needs to initialize separately, make it clear that since
> > it does its own allocations it needs to initialize _everything_
> > separately.
> >
> Ok, here is an updated version with an updated matroxfb and the sm501fb
> change reverted.
>
> Signed-off-by: Paul Mundt <[email protected]>
>
> ---
>
This is incorrect way to fix this as some drivers do not use the framebuffer_alloc()
at all. They use global (for a file) fb_info structure. I have done some cleanups to
the fbdev layer before the 2.6.31 and there should no drivers which uses kmalloc or
kzalloc to allocate the fb_info (your patch would break these drivers too).
A root of the whole mm_lock issue is that the fb_mmap() BKL protected two fb_info
fields which were never protected when set. I changed this by add the mm_lock
around these fields but only in drivers which modified this fields AFTER call
to the register_framebuffer(). Some drivers set these fields using the same
function before and after the register_framebuffer(). I strongly believe that
setting these fields before the register_framebuffer() is wrong or redundant for
these drivers. See my fix for the sisfb driver below.
I have tested the patch below. Wu Zhangjin, can you also confirm that this
works for you (without your patch)?
I will look into the matroxfb and sm501fb drivers now. The same problem is
already fixed for the mx3fb driver and the patch is sent to Andrew Morton.
Regards,
Krzysztof
From: Krzysztof Helt <[email protected]>
Remove redundant call to the sisfb_get_fix() before sis frambuffer is registered.
This fixes a problem with uninitialized the fb_info->mm_lock mutex.
Signed-off-by: Krzysztof Helt <[email protected]>
---
diff -urp linux-ref/drivers/video/sis/sis_main.c linux-next/drivers/video/sis/sis_main.c
--- linux-ref/drivers/video/sis/sis_main.c 2009-07-01 18:07:05.000000000 +0200
+++ linux-next/drivers/video/sis/sis_main.c 2009-07-05 17:20:33.000000000 +0200
@@ -6367,7 +6367,6 @@ error_3: vfree(ivideo->bios_abase);
sis_fb_info->fix = ivideo->sisfb_fix;
sis_fb_info->screen_base = ivideo->video_vbase + ivideo->video_offset;
sis_fb_info->fbops = &sisfb_ops;
- sisfb_get_fix(&sis_fb_info->fix, -1, sis_fb_info);
sis_fb_info->pseudo_palette = ivideo->pseudo_palette;
fb_alloc_cmap(&sis_fb_info->cmap, 256 , 0);
----------------------------------------------------------------------
Najlepsze OC i AC tylko w Ergo Hestia
http://link.interia.pl/f222
On Mon, 6 Jul 2009 00:25:57 +0900
Paul Mundt <[email protected]> wrote:
> Ok, here is an updated version with an updated matroxfb and the sm501fb
> change reverted.
>
> Signed-off-by: Paul Mundt <[email protected]>
>
Here is a patch which should fix problem with sm501fb driver:
diff --git a/drivers/video/sm501fb.c b/drivers/video/sm501fb.c
index 16d4f4c..924d794 100644
--- a/drivers/video/sm501fb.c
+++ b/drivers/video/sm501fb.c
@@ -1540,9 +1540,6 @@ static int sm501fb_init_fb(struct fb_info *fb,
if (ret)
dev_err(info->dev, "check_var() failed on initial setup?\n");
- /* ensure we've activated our new configuration */
- (fb->fbops->fb_set_par)(fb);
-
return 0;
}
Paul, please test it (without additional initialization of the mm_lock mutext). I will post the patch
if it works for you.
An issue here is that these drivers calls fb_set_par() function (or part of it as the sisfb driver)
but the register_framebuffer() calls the fb_set_par() also after all structures are set up. There
should be no need to call the fb_set_par() just before the register_framebuffer().
The matroxfb driver is quite far from standard driver framework by now. I vote for fixing it
by adding this early initialization of the mm_mutex for now.
Kind regards,
Krzysztof
----------------------------------------------------------------------
Promocja ubezpieczen komunikacyjnych Ergo Hestia. Sprawdz!
http://link.interia.pl/f222f
Hi,
On Sun, 2009-07-05 at 18:18 +0200, Krzysztof Helt wrote:
> On Mon, 6 Jul 2009 00:25:57 +0900
> Paul Mundt <[email protected]> wrote:
>
> > On Sun, Jul 05, 2009 at 08:19:40AM -0700, Linus Torvalds wrote:
> > >
> > >
> > > On Mon, 6 Jul 2009, Paul Mundt wrote:
> > > > >
> > > > > Why not "lock" as well?
> > > >
> > > > I had that initially, but matroxfb will break if we do that, and
> > > > presently nothing cares about trying to take ->lock that early on.
> > >
> > > I really would rather have consistency than some odd rules like that.
> > >
> > > In particular - if matroxfb is different and needs its own lock
> > > initialization because it doesn't use the common allocation routine, then
> > > please make _that_ consistent too. Rather than have it special-case just
> > > one lock that it needs to initialize separately, make it clear that since
> > > it does its own allocations it needs to initialize _everything_
> > > separately.
> > >
> > Ok, here is an updated version with an updated matroxfb and the sm501fb
> > change reverted.
> >
> > Signed-off-by: Paul Mundt <[email protected]>
> >
> > ---
> >
>
> This is incorrect way to fix this as some drivers do not use the framebuffer_alloc()
> at all. They use global (for a file) fb_info structure. I have done some cleanups to
> the fbdev layer before the 2.6.31 and there should no drivers which uses kmalloc or
> kzalloc to allocate the fb_info (your patch would break these drivers too).
>
> A root of the whole mm_lock issue is that the fb_mmap() BKL protected two fb_info
> fields which were never protected when set. I changed this by add the mm_lock
> around these fields but only in drivers which modified this fields AFTER call
> to the register_framebuffer(). Some drivers set these fields using the same
> function before and after the register_framebuffer(). I strongly believe that
> setting these fields before the register_framebuffer() is wrong or redundant for
> these drivers. See my fix for the sisfb driver below.
>
> I have tested the patch below. Wu Zhangjin, can you also confirm that this
> works for you (without your patch)?
>
This patch also works for me, thanks!
Regards,
Wu Zhangjin
> I will look into the matroxfb and sm501fb drivers now. The same problem is
> already fixed for the mx3fb driver and the patch is sent to Andrew Morton.
>
> Regards,
> Krzysztof
>
>
> From: Krzysztof Helt <[email protected]>
>
> Remove redundant call to the sisfb_get_fix() before sis frambuffer is registered.
>
> This fixes a problem with uninitialized the fb_info->mm_lock mutex.
>
> Signed-off-by: Krzysztof Helt <[email protected]>
> ---
>
> diff -urp linux-ref/drivers/video/sis/sis_main.c linux-next/drivers/video/sis/sis_main.c
> --- linux-ref/drivers/video/sis/sis_main.c 2009-07-01 18:07:05.000000000 +0200
> +++ linux-next/drivers/video/sis/sis_main.c 2009-07-05 17:20:33.000000000 +0200
> @@ -6367,7 +6367,6 @@ error_3: vfree(ivideo->bios_abase);
> sis_fb_info->fix = ivideo->sisfb_fix;
> sis_fb_info->screen_base = ivideo->video_vbase + ivideo->video_offset;
> sis_fb_info->fbops = &sisfb_ops;
> - sisfb_get_fix(&sis_fb_info->fix, -1, sis_fb_info);
> sis_fb_info->pseudo_palette = ivideo->pseudo_palette;
>
> fb_alloc_cmap(&sis_fb_info->cmap, 256 , 0);
>
>
>
> ----------------------------------------------------------------------
> Najlepsze OC i AC tylko w Ergo Hestia
> http://link.interia.pl/f222
>
On Mon, 06 Jul 2009 09:13:11 +0800
Wu Zhangjin <[email protected]> wrote:
> Hi,
>
>
> This patch also works for me, thanks!
>
> Regards,
> Wu Zhangjin
>
Who should I send this patch to be included as a 2.6.31 regression fix?
Regards,
Krzysztof
> >
> > From: Krzysztof Helt <[email protected]>
> >
> > Remove redundant call to the sisfb_get_fix() before sis frambuffer is registered.
> >
> > This fixes a problem with uninitialized the fb_info->mm_lock mutex.
> >
> > Signed-off-by: Krzysztof Helt <[email protected]>
> > ---
> >
> > diff -urp linux-ref/drivers/video/sis/sis_main.c linux-next/drivers/video/sis/sis_main.c
> > --- linux-ref/drivers/video/sis/sis_main.c 2009-07-01 18:07:05.000000000 +0200
> > +++ linux-next/drivers/video/sis/sis_main.c 2009-07-05 17:20:33.000000000 +0200
> > @@ -6367,7 +6367,6 @@ error_3: vfree(ivideo->bios_abase);
> > sis_fb_info->fix = ivideo->sisfb_fix;
> > sis_fb_info->screen_base = ivideo->video_vbase + ivideo->video_offset;
> > sis_fb_info->fbops = &sisfb_ops;
> > - sisfb_get_fix(&sis_fb_info->fix, -1, sis_fb_info);
> > sis_fb_info->pseudo_palette = ivideo->pseudo_palette;
> >
> > fb_alloc_cmap(&sis_fb_info->cmap, 256 , 0);
> >
> >
> >
> > ----------------------------------------------------------------------
> > Najlepsze OC i AC tylko w Ergo Hestia
> > http://link.interia.pl/f222
> >
>
>
----------------------------------------------------------------------
Najlepsze OC i AC tylko w Ergo Hestia
http://link.interia.pl/f222
On Mon, 6 Jul 2009, Krzysztof Helt wrote:
>
> Who should I send this patch to be included as a 2.6.31 regression fix?
Just send the patches to me. I've got the patch, but no changelog, and so
I'd like to see them again, with changelogs and the proper "Acked-by:" or
"Tested-by" lines etc.
Linus