Re: [PATCH 4/7] Fix various bugs in the target code

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Mon, 2009-08-10 at 11:08 -0400, Alan Stern wrote:
> This patch (as1276) fixes some bugs in the SCSI target code:
> 
> 	In scsi_alloc_target(), a newly-created target was added to
> 	the host's list of targets before the host's target_alloc()
> 	method was called.

So this one is the design way the target code works:  allocate and add
first so the transport and target code have a fully usable device.

> 	In the same routine, if a match was found with an old target
> 	in the DEL state then that target's reap_ref was mistakenly
> 	incremented.  This is harmless, but it shouldn't happen.

It's required for the state != DEL case ... the reap_ref loses
significance after the state moves to DEL, so it's by design.

> 	If we have to wait for an old target to disappear, instead of
> 	calling flush_scheduled_work() the patch calls
> 	schedule_timeout_uninterruptible(1).  After all, whatever is
> 	pinning the old target might not have anything to do with a
> 	workqueue.  Besides, flush_scheduled_work() is prone to
> 	deadlocks and should never be used by drivers.

I don't really buy this; it's not (yet) a documented restriction of
flush_scheduled_work() and we have a few drivers using it.  It only
deadlocks if you call it from a running workqueue.

> 	scsi_target_reap() changed starget->state outside the
> 	protection of the host lock.

So does everything else that manipulates the target state.

> 	__scsi_add_device() called scsi_alloc_target() outside the
> 	protection of the host's scan_mutex, meaning that it might
> 	find an incompletely-initialized target or it might create
> 	a duplicate target.

scan mutex isn't used as a determinant for this.  There can be a slight
race in initialisation, but really, it isn't important.

> 	scsi_sysfs_add_sdev() would call transport_configure_device()
> 	for a target each time a new device was added under that
> 	target.  The call has been moved to scsi_target_add(), where
> 	it will be made only once.

That, unfortunately is an SPI required feature ... we can't actually
configure the target until we have a device because of the way SPI
parameters work.

James