Prev: [RFC][PATCH 1/3] Bcache: Version 6
Next: [PATCH] gpiolib: Implement set_debounce for WM831x GPIOs
From: Sam Ravnborg on 4 Jul 2010 04:10
>
> static int DAC960_getgeo(struct block_device *bdev, struct hd_geometry *geo)
> diff --git a/drivers/block/amiflop.c b/drivers/block/amiflop.c
> index 7a51535..8af7af7 100644
> --- a/drivers/block/amiflop.c
> +++ b/drivers/block/amiflop.c
> @@ -1555,10 +1555,13 @@ static int floppy_open(struct block_device *bdev, fmode_t mode)
> int old_dev;
> unsigned long flags;
>
> + lock_kernel();
> old_dev = fd_device[drive];
>
> - if (fd_ref[drive] && old_dev != system)
> + if (fd_ref[drive] && old_dev != system) {
> + unlock_kernel();
> return -EBUSY;
> + }
>
> if (mode & (FMODE_READ|FMODE_WRITE)) {
> check_disk_change(bdev);
> @@ -1571,8 +1574,10 @@ static int floppy_open(struct block_device *bdev, fmode_t mode)
> fd_deselect (drive);
> rel_fdc();
>
> - if (wrprot)
> + if (wrprot) {
> + unlock_kernel();
> return -EROFS;
> + }
> }
> }
>
> @@ -1589,6 +1594,7 @@ static int floppy_open(struct block_device *bdev, fmode_t mode)
> printk(KERN_INFO "fd%d: accessing %s-disk with %s-layout\n",drive,
> unit[drive].type->name, data_types[system].name);
>
> + unlock_kernel();
> return 0;
> }

Using goto for errorhandling here would have been nicer.
Also did you forget to include smp_locks.h?



>
> @@ -1597,6 +1603,7 @@ static int floppy_release(struct gendisk *disk, fmode_t mode)
> struct amiga_floppy_struct *p = disk->private_data;
> int drive = p - unit;
>
> + lock_kernel();
> if (unit[drive].dirty == 1) {
> del_timer (flush_track_timer + drive);
> non_int_flush_track (drive);
> @@ -1610,6 +1617,7 @@ static int floppy_release(struct gendisk *disk, fmode_t mode)
> /* the mod_use counter is handled this way */
> floppy_off (drive | 0x40000000);
> #endif
> + unlock_kernel();
> return 0;
> }

Cooked up the following replacement patch:
[Not build tested]

diff --git a/drivers/block/amiflop.c b/drivers/block/amiflop.c
index 832798a..25d63cf 100644
--- a/drivers/block/amiflop.c
+++ b/drivers/block/amiflop.c
@@ -67,6 +67,7 @@
#include <linux/elevator.h>
#include <linux/interrupt.h>
#include <linux/platform_device.h>
+#include <linux/smp_lock.h>

#include <asm/setup.h>
#include <asm/uaccess.h>
@@ -1541,11 +1542,16 @@ static int floppy_open(struct block_device *bdev, fmode_t mode)
int system = (MINOR(bdev->bd_dev) & 4) >> 2;
int old_dev;
unsigned long flags;
+ int err;

+ err = 0;
+ lock_kernel();
old_dev = fd_device[drive];

- if (fd_ref[drive] && old_dev != system)
- return -EBUSY;
+ if (fd_ref[drive] && old_dev != system) {
+ err = -EBUSY;
+ goto out;
+ }

if (mode & (FMODE_READ|FMODE_WRITE)) {
check_disk_change(bdev);
@@ -1558,8 +1564,10 @@ static int floppy_open(struct block_device *bdev, fmode_t mode)
fd_deselect (drive);
rel_fdc();

- if (wrprot)
- return -EROFS;
+ if (wrprot) {
+ err = -EROFS;
+ goto out;
+ }
}
}

@@ -1576,7 +1584,9 @@ static int floppy_open(struct block_device *bdev, fmode_t mode)
printk(KERN_INFO "fd%d: accessing %s-disk with %s-layout\n",drive,
unit[drive].type->name, data_types[system].name);

- return 0;
+out:
+ unlock_kernel();
+ return err;
}

static int floppy_release(struct gendisk *disk, fmode_t mode)
@@ -1584,6 +1594,7 @@ static int floppy_release(struct gendisk *disk, fmode_t mode)
struct amiga_floppy_struct *p = disk->private_data;
int drive = p - unit;

+ lock_kernel();
if (unit[drive].dirty == 1) {
del_timer (flush_track_timer + drive);
non_int_flush_track (drive);
@@ -1597,6 +1608,7 @@ static int floppy_release(struct gendisk *disk, fmode_t mode)
/* the mod_use counter is handled this way */
floppy_off (drive | 0x40000000);
#endif
+ unlock_kernel();
return 0;
}


Feel free to use this as is.


> +++ b/drivers/block/aoe/aoeblk.c
> @@ -12,6 +12,7 @@
> #include <linux/slab.h>
> #include <linux/genhd.h>
> #include <linux/netdevice.h>
> +#include <linux/smp_lock.h>
> #include "aoe.h"
>
> static struct kmem_cache *buf_pool_cache;
> @@ -124,13 +125,16 @@ aoeblk_open(struct block_device *bdev, fmode_t mode)
> struct aoedev *d = bdev->bd_disk->private_data;
> ulong flags;
>
> + lock_kernel();
> spin_lock_irqsave(&d->lock, flags);
> if (d->flags & DEVFL_UP) {
> d->nopen++;
> spin_unlock_irqrestore(&d->lock, flags);
> + unlock_kernel();
> return 0;
> }
> spin_unlock_irqrestore(&d->lock, flags);
> + unlock_kernel();
> return -ENODEV;
> }

"goto out" would be cleaner.

> diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
> index 1f70aec..4f2e12f 100644
> --- a/drivers/block/pktcdvd.c
> +++ b/drivers/block/pktcdvd.c
> @@ -2383,6 +2383,7 @@ static int pkt_open(struct block_device *bdev, fmode_t mode)
>
> VPRINTK(DRIVER_NAME": entering open\n");
>
> + lock_kernel();
> mutex_lock(&ctl_mutex);
> pd = pkt_find_dev_from_minor(MINOR(bdev->bd_dev));
> if (!pd) {
> @@ -2410,6 +2411,7 @@ static int pkt_open(struct block_device *bdev, fmode_t mode)
> }
>
> mutex_unlock(&ctl_mutex);
> + unlock_kernel();
> return 0;
>
> out_dec:
> @@ -2417,6 +2419,7 @@ out_dec:
> out:
> VPRINTK(DRIVER_NAME": failed open (%d)\n", ret);
> mutex_unlock(&ctl_mutex);
> + unlock_kernel();
> return ret;
> }
>
> @@ -2425,6 +2428,7 @@ static int pkt_close(struct gendisk *disk, fmode_t mode)
> struct pktcdvd_device *pd = disk->private_data;
> int ret = 0;
>
> + lock_kernel();
> mutex_lock(&ctl_mutex);
> pd->refcnt--;
> BUG_ON(pd->refcnt < 0);
> @@ -2433,6 +2437,7 @@ static int pkt_close(struct gendisk *disk, fmode_t mode)
> pkt_release_dev(pd, flush);
> }
> mutex_unlock(&ctl_mutex);
> + unlock_kernel();
> return ret;
> }

Improved "goto" errorhandling would be nicer.
Is smp_lock.h included by other files?

Replacement patch (not build tested...):

diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index 8a549db..30d585d 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -59,6 +59,8 @@
#include <linux/freezer.h>
#include <linux/mutex.h>
#include <linux/slab.h>
+#include <linux/smp_lock.h>
+
#include <scsi/scsi_cmnd.h>
#include <scsi/scsi_ioctl.h>
#include <scsi/scsi.h>
@@ -2382,11 +2384,12 @@ static int pkt_open(struct block_device *bdev, fmode_t mode)

VPRINTK(DRIVER_NAME": entering open\n");

+ lock_kernel();
mutex_lock(&ctl_mutex);
pd = pkt_find_dev_from_minor(MINOR(bdev->bd_dev));
if (!pd) {
ret = -ENODEV;
- goto out;
+ goto out_fail;
}
BUG_ON(pd->refcnt < 0);

@@ -2408,14 +2411,16 @@ static int pkt_open(struct block_device *bdev, fmode_t mode)
set_blocksize(bdev, CD_FRAMESIZE);
}

- mutex_unlock(&ctl_mutex);
- return 0;
+ ret = 0;
+ goto out;

out_dec:
pd->refcnt--;
-out:
+out_fail:
VPRINTK(DRIVER_NAME": failed open (%d)\n", ret);
+out:
mutex_unlock(&ctl_mutex);
+ unlock_kernel();
return ret;
}

Lot's of other places could benefit from improved goto error handling.
The driver maintainers should do this (if there is a maintainer).
I stopped with the small replacement patches.

I did not find anything else going through this patch.

Sam
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Arjan van de Ven on 4 Jul 2010 14:00
On Sun, 4 Jul 2010 10:01:46 +0200
Sam Ravnborg <sam(a)ravnborg.org> wrote:
> Lot's of other places could benefit from improved goto error handling.
> The driver maintainers should do this (if there is a maintainer).
> I stopped with the small replacement patches.

it's the floppy driver for amigas
not sure if any of those are still alive at all running linux.....

I think time can be more usefully spend on other drivers...

this guy probably should move to staging instead


--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Geert Uytterhoeven on 4 Jul 2010 16:30
On Sun, Jul 4, 2010 at 19:33, Arjan van de Ven <arjan(a)infradead.org> wrote:
> On Sun, 4 Jul 2010 10:01:46 +0200
> Sam Ravnborg <sam(a)ravnborg.org> wrote:
>> Lot's of other places could benefit from improved goto error handling.
>> The driver maintainers should do this (if there is a maintainer).
>> I stopped with the small replacement patches.
>
> it's the floppy driver for amigas
> not sure if any of those are still alive at all running linux.....

Sure there are. Whether the floppy drives in them are still actively used is
another question. SSH and NFS over Ethernet are much more convenient ;-)

Last time I tried (hmm, this or previous century?), my floppy drive had problems
reading the floppy, probably my drive is dying. Oh well...

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert(a)linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Arnd Bergmann on 4 Jul 2010 17:10
On Sunday 04 July 2010 10:01:46 Sam Ravnborg wrote:
> > --- a/drivers/block/amiflop.c
> > +++ b/drivers/block/amiflop.c
> > @@ -1555,10 +1555,13 @@ static int floppy_open(struct block_device *bdev, fmode_t mode)
> > int old_dev;
> > unsigned long flags;
> >
> > + lock_kernel();
> > old_dev = fd_device[drive];
> >
> > - if (fd_ref[drive] && old_dev != system)
> > + if (fd_ref[drive] && old_dev != system) {
> > + unlock_kernel();
> > return -EBUSY;
> > + }
> >
> > if (mode & (FMODE_READ|FMODE_WRITE)) {
> > check_disk_change(bdev);
> > @@ -1571,8 +1574,10 @@ static int floppy_open(struct block_device *bdev, fmode_t mode)
> > fd_deselect (drive);
> > rel_fdc();
> >
> > - if (wrprot)
> > + if (wrprot) {
> > + unlock_kernel();
> > return -EROFS;
> > + }
> > }
> > }
> >
> > @@ -1589,6 +1594,7 @@ static int floppy_open(struct block_device *bdev, fmode_t mode)
> > printk(KERN_INFO "fd%d: accessing %s-disk with %s-layout\n",drive,
> > unit[drive].type->name, data_types[system].name);
> >
> > + unlock_kernel();
> > return 0;
> > }
>
> Using goto for errorhandling here would have been nicer.

I tried to minimize the chance for breaking stuff in code I cannot easily
test build. As shown by the bug you found in my pktcdvd patch, changing the
control flow of a function has a higher potential of introducing bugs,
so I didn't do it for drivers that people don't care much about any more.

> Also did you forget to include smp_locks.h?

No, that was already there from the first patch.

> Lot's of other places could benefit from improved goto error handling.
> The driver maintainers should do this (if there is a maintainer).

If that makes Jens accept my patches, I'd gladly change them this way.
I agree that it's cleaner and I always write my own code like that,
but when modifying (mostly) unmaintained code, my preference is on
trying to ensure the patch is correct.


> I did not find anything else going through this patch.

Ok, thanks a lot for looking through this!

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Christoph Hellwig on 6 Jul 2010 22:00
I might be missing something, but this doesn't actually push the
BKL anyway. It adds duplicate BKL locking inside the ->open and
->release methods without actually removing it in the callers.

That's a pretty pointless thing to do. If you're not feeling
confident enough about touching block_dev.c I can look into that
part of the pushdown, but this patch on it's own is not actually
useful.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: [RFC][PATCH 1/3] Bcache: Version 6
Next: [PATCH] gpiolib: Implement set_debounce for WM831x GPIOs