Re: [GIT PATCH] USB autosuspend fixes for 2.6.23-rc6

[Matthew Dharm <mdharm-kernel@one-eyed-alien.net>]

On Thu, Sep 13, 2007 at 03:13:46PM -0400, Alan Stern wrote:
> On Thu, 13 Sep 2007, Linus Torvalds wrote:
> 
> > In general, I think the USB blacklist/whitelists are generally a sign of 
> > some deeper bug.
> > 
> > We used to have a lot of those things due to simply incorrect SCSI 
> > probing, causing devices to lock up because Linux probed them with bad or 
> > unexpected modepages etc. I suspect we still have old blacklist entries 
> > from those days that just never got cleaned up, because nobody ever dared 
> > remove the blacklist entry.
> 
> I don't just suspect -- I know for a fact that we do.  Partly because
> of laziness and partly because of not being able to verify that an
> entry is no longer needed.

We do have some code that looks for unneeded entries.  When found by an
end-user (which confirms that the entry can be removed), it asks the user
to drop us an e-mail so we can remove it.

> > We should strive to make the default behaviour be so safe that we never 
> > need a black-list (or a whitelist), and basically consider blacklists to 
> > be not a way to "fix up a device", but a way to avoid some really serious 
> > AND *RARE* error.
> 
> In general I agree.  However there are some problems for which nobody 
> has been able to come up with another solution.  See below.

We generally do strive for such a thing.  Over the years, we've made
several changes to the way the SCSI core works (especially in the probing
department) to allow us to remove all sorts of special-case code and quirk
entries.

> > For example, why do we have that US_FL_MAX_SECTORS_64 at all? The fact 
> > that some USB device is broken with more than 64 sectors would seem to 
> > indicate that Windows *never* does more than 64 sectors, and that in turn 
> > means that pretty much *no* devices have ever been tested with anything 
> > bigger.
> > 
> > So why not make the 64 sector limit be the default? Get rid of the quirk: 
> > we already allow people to override it in /sys if they really want to, but 
> > realistically, it's probably not going to make any difference what-so-ever 
> > for *any* normal load. So we seem to have a quirk that really doesn't buy 
> > us anything but headache.
> 
> That's true now, but it wasn't always.  Until the last year or so, 
> cdrecord wouldn't work properly with USB CD drives having a 64-sector 
> limit unless the user added a particular command-line argument.
> 
> In fact, setting max_sectors down to 64 is probably overkill -- 120
> ought to be enough.  But there may have been one or two oddball devices
> that really did have a 32-KB limit, and better safe than sorry.  At one
> point an engineer from Genesys said their devices did, although they do
> seem to work perfectly well with 64-KB transfers (and that's what 
> Windows gives them).

It's worth pointing out that performance drops like a stone as this number
goes down.

> > Other quirks worth looking at (but likely unfixable) are:
> >  - US_FL_IGNORE_RESIDUE:
> > 	Does this really matter? Can we not just always do the 
> > 	US_FL_IGNORE_RESIDUE thing? Windows must not be doing what we're 
> > 	doing.
> 
> Windows does indeed ignore the residue field, as far as I can tell.
> 
> But this is a rather tricky thing.  The USB mass-storage spec
> specifically says that one way a device can signal a short transfer is
> to pad the data with 0s to the requested length and then set the
> residue to indicate how much of the data is valid.  If we ignore the
> residue then we run a risk of misinterpreting the 0s as valid data.
> 
> Now in practice this doesn't matter much because short transfers of
> block data (READ_10) generally involve other errors that would show up
> anyway, and for non-block data (MODE SENSE) the padding probably
> wouldn't matter.  Still it seems like a dangerous sort of thing to do,
> which is why I have resisted it.
> 
> (And by the way, there _definitely_ are devices which use this
> signalling method.  In fact, Linux contains a driver that does it.)

I think this last point is key.  I'm unwilling to sacrifice error detection
on properly working devices to enable error-prone use on clearly buggy
devices.

> >  - US_FL_FIX_CAPACITY: 
> > 	This is a generic SCSI issue, not a USB one, and maybe there are 
> > 	better solutions to it. Are we perhaps doing something wrong? Is 
> > 	there some patterns we haven't seen? Why do we need this, when 
> > 	presumably Windows does not?
> 
> This is another hard case.  No, we aren't doing anything wrong.  If
> there are any patterns we haven't seen, we aren't aware of them.  :-)  
> You might think that if a device claims to have an odd number of
> sectors then it must be wrong, but this turns out not to be true.
> 
> Why doesn't Windows need this?  For all we know, it does.  Has anybody
> ever tried forcing Windows to read the sector beyond the end of one of
> these buggy devices?

As far as I know, Windows doesn't need this because of the way FAT and NTFS
work.  They never use the end of the disk (by more than a few sectors, or
so I'm told).

> There's a straightforward solution: Never try to use the last sector --
> in effect, assume every device has the FIX_CAPACITY flag set.  Doing 
> this universally could cause data loss, however, so again I have been 
> opposed to it.

I agree here.

> >  - US_FL_SINGLE_LUN:
> > 	At least a few of these seem to indicate that the real problem 
> > 	could be detected dynamically ("device reports Sub=ff") rather 
> > 	than with a quirk. Quirks are unmaintainable (and change), but 
> > 	noticing when devices return impossible values and going into a 
> > 	"safe mode" is just defensive programming.
> 
> This is almost certainly a case where lots of the entries are no longer 
> needed.  But it isn't easy to tell which ones can safely be removed.

I've been meaning to start sending e-mails to see if we can get rid of
these.  Most of the devices which required it were UFI, which reports "LUN
not present" in a goofy way.  We fixed the code to detect it properly, but
there are still quite a few devices out there that don't implement the
correct (if goofy) method.

Most of those entries (which are for UFI devices) can go, if we get a
volunteer to take the e-mail addresses listed in unusual_devs.h and work
the list.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

Sir, for the hundreth time, we do NOT carry 600-round boxes of belt-fed 
suction darts!
					-- Salesperson to Greg
User Friendly, 12/30/1997

Attachment: pgp3txcKlfLjE.pgp
Description: PGP signature