On 10/11/2009 3:45 PM, John Kacur wrote: > Locking in ch_open is covered by the spin_lock, it serializes the calls > to idr_find and scsi_device_get. The BKL appears redundant to me here. Superficially this looks OK (i.e. no race with driver init in general). But I do wonder if there isn't already a race condition possible with the current code, between ch_probe and the file operations. ch_probe makes a scsi_changer instance available in the IDR and creates the corresponding character device file _before_ the scsi_changer instance is fully initialized. (Full quote follows for lsml) > From b385c85bb5c2579e542cfe55475b729325eb65e1 Mon Sep 17 00:00:00 2001 > From: John Kacur <jkacur@xxxxxxxxxx> > Date: Sun, 11 Oct 2009 13:06:54 +0200 > Subject: [PATCH] drivers/scsi/ch.c: Remove BKL in ch_open > > Everything in ch_open is covered by a spin_lock, so the lock_kernel is redundant > Remove it. > > Signed-off-by: John Kacur <jkacur@xxxxxxxxxx> > --- > drivers/scsi/ch.c | 5 +---- > 1 files changed, 1 insertions(+), 4 deletions(-) > > diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c > index fe11c1d..4ba8b67 100644 > --- a/drivers/scsi/ch.c > +++ b/drivers/scsi/ch.c > @@ -579,19 +579,16 @@ ch_open(struct inode *inode, struct file *file) > scsi_changer *ch; > int minor = iminor(inode); > > - lock_kernel(); > spin_lock(&ch_index_lock); > ch = idr_find(&ch_index_idr, minor); > > if (NULL == ch || scsi_device_get(ch->device)) { > spin_unlock(&ch_index_lock); > - unlock_kernel(); > return -ENXIO; > } > + file->private_data = ch; > spin_unlock(&ch_index_lock); > > - file->private_data = ch; > - unlock_kernel(); > return 0; > } > -- Stefan Richter -=====-==--= =-=- -==-- http://arcgraph.de/sr/ -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html