Re: panic after removing usb flash drive

Bernd Walter wrote:

> On Wed, Aug 31, 2005 at 05:02:38PM -0600, Scott Long wrote:
> 
>>Bernd Walter wrote:
>>
>>>On Wed, Aug 31, 2005 at 12:05:17PM -0600, Scott Long wrote:
>>>What bugs are you refering?
>>
>>The CAM detach code should be returning EBUSY errors since the 'da'
>>periph has open references to it.  I'm pretty sure that thse errors
>>are being ignored by umass.c, leading to the SIM going away unexpectedly
>>to CAM.
> 
> 
> OK.
> 
> 
>>>Sample code on how to correctly detach a sim a sparse.
>>>The umass code doesn't know that devices on a scbus are in use, or
>>>should it?
>>
>>Well, it doesn't need to right now because you only have one target
>>per SIM.  It should though; most normal SCSI drivers keep a table of
>>known devices in order to remember negotiated transfer settings and
>>handle reconnections.
> 
> 
> OK.
> 
> 
>>>In the USB-world we have the problem that a blocking detach blocks
>>>other port attachment/detachments on the same bus as well.
>>>This is because we currently have a single thread per bus processing
>>>these events.
>>>We already see this problem with ucom devices where an open tty on a
>>>detached device blocks.
>>>But it is better than the panic that we have right now.
>>>In the tty case we have a timeout, don't know what we can do with CAM.
>>>
>>
>>In the sim-per-target model, you need to completely drain the simq and
>>devq for the SIM before allowing detach to complete.  This means 
>>freezing the simq then waiting until the camisr can run and process any
>>pending CCBs on the completion queue.  The camisr is an SWI, so you'll
>>need to sleep so that it can run.
> 
> 
> Sounds doable.
> 

You could probably get around the problem of sleeping in the USB event
thread by doing the sleep parts of the detach in a taskqueue.  But that
just adds more complexity and more need for synchronization.  If the
only choice was to retain the SIM-per-device model then this is what I
would do, but it's not a choice that I like.

> 
>>>With the single-sim design we have had the following problems:
>>>- probing of LUNs filed on attach and required a manual rescan.
>>> undoubly fixable by someone with good CAM knowledge.
>>
>>I'm not parsing this.  Are you referring to the need to do a rescan
>>after plugging in a device?  This is easy to solve.  When a umass
> 
> 
> Yes.
> 
> 
>>target is attached, you either send an AC_FOUND_DEVICE async event to
>>announce the target, or you request a bus rescan from within the driver.
>>I think that the ciss driver has an example of this.
> 
> 
> Don't know ciss, but I never had doubts that this is was a fixable
> thing.
> 
> 
>>>- CAM needs a max target value, but how many target do we really have?
>>> Each USB has up to 127 devices (pratically 100 useable as we need
>>> some hubs)
>>
>>The max target value is really only important for bus rescans.  The SIM 
>>can just track what targets it currently knows about and reject CCBs
>>for ones that don't exist (it somewhat does this now, though with only
>>one target per SIM it's kinda silly).  Setting the max target value to
>>127 and rejecting targets that don't exist won't slow down a bus rescan
>>but much at all.
> 
> 
> But you can have more than 127 umass instances per bus.
> 
> 
>>> Each device can have multiple functions, which means multiple umass
>>> instances.
>>
>>I have a umass device that is a CF+SD card reader.  It shows up in CAM
>>as a single target with 2 LUNs.  Is this the kind of thing that you are
>>talking about?  If so, then there is no reason not to continue to use
>>the model of a single target with multiple LUNs.
> 
> 
> No, that is multiple LUN with a single umass instance.
> What I mean is a single USB device with multiple umass instances.
> umass ist just a logical interface in the USB world, normaly ment
> to allow e.g. an ulpt and umass on the same USB device, but also
> possible to have multiple umass interfaces on the same USB device.
> Since an umass interface needs at least two bulk endpoints a single
> USB-channel can have up to 16 * 127 umass instances.
> 

CAM right now is really geared for parallel SCSI with only 16 targets,
but it should work fine with 2048 targets.  A project for CAMng is to
properly support FC fabrics properly as well as iSCSI; in both cases
the max-target-per-bus concept has little meaning and would need 
fundamental changes.  That's future work, though.

For a physical device with multiple umass logical devices, each logical
device would again just be a target.  The actual target and bus numbers
don't really matter in practical use because they aren't exposed to
/dev.  To make it truly transparent we would need to have an automounter
like other OSes that mounts based on filesystem label, not based on
device node name.  But that's mostly a detail at this point.

> 
>>> Previously we had a small hardcoded number, too big numbers slow
>>> down bus rescans, too small restrict the number of possible devices.
>>> We should have a dynamic way.
>>>Don't remember if ther were others.
>>>
>>
>>>From the technical standpoint - no matter what we do, there are
>>
>>>problems to solve.
>>>
>>>
>>>
>>>>Your analogy of a PCI bus is correct for the USB-SCSI adapters, where 
>>>>the adapter is doing a full conversion and bridge from one bus type to
>>>>another.  It's not true for a umass device where it's merely using the
>>>>USB bus as a SCSI transport.
>>>
>>>
>>>So what is an USB-IDE converter?
>>
>>I assumed that you were talking about devices that bridge from USB to
>>IDE/SCSI and did not conform to the umass standard.  I have a USB2-IDE
>>converter in an external exclosure that speaks umass and is probably
>>closer to what you are talking about here.  But again, umass is really
>>about using the USB bus to transport SCSI/ATA protocol, not about
>>providing full access to a SCSI/IDE bus via a USB tunnel.  That is a
>>significant difference, IMHO.  The USB controller is acting in a direct
>>role as the SCSI/ATA initiator, vs. just tunnelling to a smart
>>initiator.
> 
> 
> OK - I got it now.
> Nevertheless I could imagine a pseudo USB-SCSI converter that just
> enhances umass transactions with a target-ID.
> This won't invalidate what you wrote above, since you still don't have
> full access on the SCSI-bus, but also requires a sim per (enhanced)umass.
> 

I assume that this is really just a mythical device, right?  If it were
really just a very simple extension of the umass protocol then I'd 
probably elect to treat it as multiple targets with the same single
SIM.

> 
>>>OK - that won't help for a practical solution.
>>>In the practical way it sounded easier to go the multiple sim way.
>>>sim detach needs to be fixed either way.
>>
>>Yes.  It somewhat works now as long as the system is completely idle.
>>It breaks down horribly if I/O or error recovery is in progress and
>>a periph driver is left with CCBs in flight and/or a dangling
>>reference to a SIM.  The only way to deal with this is to allow
>>blocking while CAM drains itself.
> 
> 
> Have to think about it.
> 
> 
>>>Are there any other technical reasons for doing single sim?
>>>You've mentioned rescource arbitration and error recovery.
>>>Is there anything that can CAM do for us that it won't with multiple
>>>sim?
>>>
>>
>>It means that you'll be able to detach umass targets without doing the
>>complicated dance of sleeping for CAM to drain itself.  It also will
>>mean that it's less fragile to edge cases that are hard to identify and
>>deal with.  Fixing CAM detach so that this works reliably is definitely
>>something that must be done, but you can't avoid sleeping in order for
>>it to work.
> 
> 
> Mmm.
> 

So was the motivation to change from a single SIM to SIM-per-device 
based solely on the problem of doing manual bus rescans when a device
got inserted?  If not, what were the other problems that got solved?
Btw, I envision my changes resulting in a SIM per USB controller.  So
on a system with 3-4 controllers (as seem to be common these days), 
you'd have that many SIMs.  umass targets would be paired with the SIM
that was associated with the physical USB controller/bus of the target.

Scott