Re: usb-storage, error reading the last 8 sectors, regression in 2.6.25-rc7

[Hans de Goede <j.w.r.degoede@hhs.nl>]

Alan Stern wrote:
> On Tue, 1 Apr 2008, Hans de Goede wrote:
> 
>> Reverting the patch is easy, edit drivers/usb/storage/scsiglue.c and remove the 
>> following line:
>> "                sdev->last_sector_bug = 1;"
>>
>> Which should be close to line 193 (it is 193 in my source tree, but thats a bit 
>> stale).
>>
>>> The old way was not necessarily correct for this type of device bug. Only
>>> that it had a very high chance of not appearing.
>>>
>>> When discussing the last bug, it was said to enable it by default for USB
>>> instead of using blacklists. It looks like this bug, or the other, needs a
>>> blacklist.
>>>
>> If the splitting of the request is the cause, yes then it looks like that.
>>
>>> But to me it looks like this is a 4k thing. I think Windows will always
>>> use 4k for FAT, though never triggering either of the bugs.
>>>
>> I'm pretty sure the last sector must only be read by itself bug (for lack of a 
>> better name) is present under windows too, but won't be triggered as windows 
>> normally doesn't access the last sector, where as various pieces of Linux 
>> routinely probe the end of the disk, for detection of exotic partition types/ 
>> disklabes.etc.
>>
>>> The one submitting the last sector patch was, I think, Hans de Goede (CCed)
>>> Hans ?
>> Correct I wrote the split up requests which touch the last sector changes to 
>> the scsi disk handling, and a seperate patch to always set the flag which 
>> enables the splitting for usb disks.
>>
>>>   If I read last 8 sectors (4k) on a device that exhibits the "last sector bug"
>>>   Does it work? (Is 8 a magic number here)
>>>
>> I just tried and I'm afraid not, an 8 sector read which includes at the last 
>> sector completely kills the device, no other transfers to / from the device 
>> will work until the sdcard is removed and reinserted (the troublesome device is 
>> a card reader build into a multi function printer, one gets what one pays for).
> 
> It sounds like the last_sector_bug setting should be conditional on a 
> blacklist entry.  As far as I know it affects only a small proportion 
> of devices; most are fine with multi-sector reads at the end.
> 

Maybe we should first determine that the regression is _really_ caused by this?

When testing yesterday I did a printk of any (unmodified) reads which would 
touch the last sector and quite a few where not 8 sector reads, so if this 
devices goes barf on any last sector including read of a different size then 8, 
it will have troubles without the work around too.

First we need to determine what _exactly_ causes this device to become unhappy 
(which should be trivial to test through dd), if I understand things sofar, the 
device doesn't like if a read is done which stops one sector short of the last 
sector. Such  a read being done could happen without my split requests code 
too, so making that conditional then wouldn't do any good, it would only make 
the bug (much) less likely to get triggered, but it could still happen en 
probably be reproduced by a trivial dd command.

Regards,

Hans