Messages in this thread | | | From | Arnd Bergmann <> | Subject | Re: [PATCH 2/6] block: push down BKL into .open and .release | Date | Sun, 4 Jul 2010 23:09:09 +0200 |
| |
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
| |