Re: [patch] Two problems in latest DRM
Am Montag, den 02.10.2006, 11:24 +0200 schrieb Michel Dänzer:
> Hi Felix,
>
> On Sun, 2006-10-01 at 17:44 -0400, Felix Kühling wrote:
> >
> > 1: drm_lock.c: drm_unlock uses a new tasklet_lock. That leads to hangs
> > on my system (for some reason the driver can't find an IRQ for my PCI
> > card). Fortunately the NMI watchdog detects the lockup and kills the
> > Xserver, so I can recover the system without rebooting.
> >
> > I can see that tasklet_lock is only initialized in drm_irq_install and
> > only if the DRIVER_IRQ_VBL is enabled. Thus you can't unconditionally
> > use that spinlock in drm_unlock. The patch adds a condition
> > dev->irq_enabled, but I'm not sure if this is the right condition. I
> > believe dev->irq_enabled can be set even without DRIVER_IRQ_VBL being
> > enabled. Michel, what's would be the right way to handle this?
>
> I think initializing the spinlock unconditionally in drm_fill_in_dev()
> is the simplest solution. Fixed in
> 3a16e615cabfed18b1891a732e7243ef41dc0ad0.
Thanks, this fixes the problem for me.
Regards,
Felix
>
>
> > 2: drm_drawable.c: make idx and id signed integers in drm_rmdraw. There
> > are some comparisons such as id < 0 and idx >= 0 in there that don't
> > make sense for unsigned integers. And I actually got kernel oopses when
> > running glxinfo or glxgears in a naked Xserver when it destroyed the
> > last drawable. It crapped out here:
> >
> > ...
> > /* Can we shrink the arrays? */
> > if (idx == bitfield_length - 1) {
> > while (idx >= 0 && !bitfield[idx])
> > --idx;
> > ...
> >
> > idx will always be >= 0 if it's unsigned. --idx will make the value wrap
> > around and the next access to bitfield[idx] oopses.
>
> Good catch! I must have been 'lucky' for never hitting this on my
> development machine, but I'm also slightly disappointed that gcc didn't
> warn about it. Anyway, I pushed your fix as
> d58389968124191a546a14b42ef84edc224be23d.
>
>
> Thanks for the detailed report,
>
>
--
| Felix Kühling <[EMAIL PROTECTED]> http://fxk.de.vu |
| PGP Fingerprint: 6A3C 9566 5B30 DDED 73C3 B152 151C 5CC1 D888 E595 |
-
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
--
___
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [patch] Two problems in latest DRM
Hi Felix,
On Sun, 2006-10-01 at 17:44 -0400, Felix Kühling wrote:
>
> 1: drm_lock.c: drm_unlock uses a new tasklet_lock. That leads to hangs
> on my system (for some reason the driver can't find an IRQ for my PCI
> card). Fortunately the NMI watchdog detects the lockup and kills the
> Xserver, so I can recover the system without rebooting.
>
> I can see that tasklet_lock is only initialized in drm_irq_install and
> only if the DRIVER_IRQ_VBL is enabled. Thus you can't unconditionally
> use that spinlock in drm_unlock. The patch adds a condition
> dev->irq_enabled, but I'm not sure if this is the right condition. I
> believe dev->irq_enabled can be set even without DRIVER_IRQ_VBL being
> enabled. Michel, what's would be the right way to handle this?
I think initializing the spinlock unconditionally in drm_fill_in_dev()
is the simplest solution. Fixed in
3a16e615cabfed18b1891a732e7243ef41dc0ad0.
> 2: drm_drawable.c: make idx and id signed integers in drm_rmdraw. There
> are some comparisons such as id < 0 and idx >= 0 in there that don't
> make sense for unsigned integers. And I actually got kernel oopses when
> running glxinfo or glxgears in a naked Xserver when it destroyed the
> last drawable. It crapped out here:
>
> ...
> /* Can we shrink the arrays? */
> if (idx == bitfield_length - 1) {
> while (idx >= 0 && !bitfield[idx])
> --idx;
> ...
>
> idx will always be >= 0 if it's unsigned. --idx will make the value wrap
> around and the next access to bitfield[idx] oopses.
Good catch! I must have been 'lucky' for never hitting this on my
development machine, but I'm also slightly disappointed that gcc didn't
warn about it. Anyway, I pushed your fix as
d58389968124191a546a14b42ef84edc224be23d.
Thanks for the detailed report,
--
Earthling Michel Dänzer | http://tungstengraphics.com
Libre software enthusiast | Debian, X and DRI developer
-
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
--
___
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
[patch] Two problems in latest DRM
Hi,
I found two problems in latest DRM while working on the mach64. I'm
attaching a patch for the two.
1: drm_lock.c: drm_unlock uses a new tasklet_lock. That leads to hangs
on my system (for some reason the driver can't find an IRQ for my PCI
card). Fortunately the NMI watchdog detects the lockup and kills the
Xserver, so I can recover the system without rebooting.
I can see that tasklet_lock is only initialized in drm_irq_install and
only if the DRIVER_IRQ_VBL is enabled. Thus you can't unconditionally
use that spinlock in drm_unlock. The patch adds a condition
dev->irq_enabled, but I'm not sure if this is the right condition. I
believe dev->irq_enabled can be set even without DRIVER_IRQ_VBL being
enabled. Michel, what's would be the right way to handle this?
2: drm_drawable.c: make idx and id signed integers in drm_rmdraw. There
are some comparisons such as id < 0 and idx >= 0 in there that don't
make sense for unsigned integers. And I actually got kernel oopses when
running glxinfo or glxgears in a naked Xserver when it destroyed the
last drawable. It crapped out here:
...
/* Can we shrink the arrays? */
if (idx == bitfield_length - 1) {
while (idx >= 0 && !bitfield[idx])
--idx;
...
idx will always be >= 0 if it's unsigned. --idx will make the value wrap
around and the next access to bitfield[idx] oopses.
Regards,
Felix
--
| Felix Kühling <[EMAIL PROTECTED]> http://fxk.de.vu |
| PGP Fingerprint: 6A3C 9566 5B30 DDED 73C3 B152 151C 5CC1 D888 E595 |
diff --git a/linux-core/drm_lock.c b/linux-core/drm_lock.c
index fa677ba..b25bf08 100644
--- a/linux-core/drm_lock.c
+++ b/linux-core/drm_lock.c
@@ -163,15 +163,17 @@ int drm_unlock(struct inode *inode, stru
return -EINVAL;
}
- spin_lock_irqsave(&dev->tasklet_lock, irqflags);
+ if (dev->irq_enabled) {
+ spin_lock_irqsave(&dev->tasklet_lock, irqflags);
- if (dev->locked_tasklet_func) {
- dev->locked_tasklet_func(dev);
+ if (dev->locked_tasklet_func) {
+ dev->locked_tasklet_func(dev);
- dev->locked_tasklet_func = NULL;
- }
+ dev->locked_tasklet_func = NULL;
+ }
- spin_unlock_irqrestore(&dev->tasklet_lock, irqflags);
+ spin_unlock_irqrestore(&dev->tasklet_lock, irqflags);
+ }
atomic_inc(&dev->counts[_DRM_STAT_UNLOCKS]);
diff --git a/shared-core/drm_drawable.c b/shared-core/drm_drawable.c
index 5e2fc86..d203b24 100644
--- a/shared-core/drm_drawable.c
+++ b/shared-core/drm_drawable.c
@@ -132,7 +132,8 @@ int drm_rmdraw(DRM_IOCTL_ARGS)
{
DRM_DEVICE;
drm_draw_t draw;
- unsigned int id, idx, shift;
+ int id, idx;
+ unsigned int shift;
unsigned int irqflags;
u32 *bitfield = dev->drw_bitfield;
unsigned int bitfield_length = dev->drw_bitfield_length;
-
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV--
___
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
