On Sat, 2005-09-10 at 23:16 +0200, Stefan Richter wrote: > I was just trying to adapt that fix and stumbled upon potential problems > of my code. Two questions: > > 1. Is the following sequence safe? > [connect to device] > sdev = __scsi_add_device(); > scsi_device_put(sdev); > store sdev in sbp2's private data for later use > [sbp2 .remove hook] > scsi_remove_device(sdev); not really ... you have an undeclared reference to sdev. If someone removed it outside of the driver (using one of the remove APIs) then you'd be left with a stale pointer. > 2. Is this safe? > [connect to device] > sdev = __scsi_add_device(); > scsi_device_put(sdev); > [sbp2 .remove hook] > scsi_remove_host(shost); > I ask because scsi_remove_host will implicitly remove the device too. > > BTW, the following won't work: > [connect to device] > sdev = __scsi_add_device(); > store sdev for later use > [sbp2 .remove hook] > scsi_remove_device(sdev); > scsi_device_put(sdev); > That way, "modprobe -r sbp2" would fail because sbp2 is "in use" until > scsi_device_put. well, you could do this [connect to device] sdev = __scsi_add_device(); get_device(&sdev->sdev_gendev); scsi_device_put(sdev); store sdev for later use [sbp2 .remove hook] scsi_remove_device(sdev); put_device(&sdev->sdev_gendev); But that would hold the device for the entire lifetime of your module, which I think, isn't really what you want. Any user requested removal would remove the visibility of the device but wouldn't actually destroy it (so the next add would get into difficulty). I suspect what you want to do is something like this: [connect to device] scsi_add_device(); store sdev parameters (id and lun) for later use [sbp2 .remove hook] spin_lock_irqsave(host_lock); sdev = __scsi_lookup_device(shost, c, id, lun); spin_unlock_irqsave(host_lock); if (sdev) scsi_remove_device(sdev); which will behave correctly if the user removes the device; It's a bit inelegant, but it should be the minimum code change. A better method would be to use the slave_alloc/slave_destroy hooks to attach your data to that of the device. You know that if slave_destroy hasn't been called on the sdev, then it must be valid ... and you also know that the user has requested an ejection if you get a slave_destroy () call on it. James - : 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
