PATCH: usb-storage-set-last-sector-bug-flag.patch

PATCH: usb-storage-set-last-sector-bug-flag.patch

[Hans de Goede]

[-- Attachment #1: Type: text/plain, Size: 491 bytes --]

Hi all,

This patch sets the last_sector_bug flag to 1 for all USB disks. This is
needed to makes the cardreader on various HP multifunction printers work.

Since the performance impact is negible we set this flag for all USB disks to
avoid an unusual_devs.h nightmare.

Note that this patch depends on:
PATCH: scsi-sd-last-sector-bug-flag.patch

Which actually adds this flag to the scsi subsystem.

Signed-off-by: Hans de Goede <j.w.r.degoede-fbo2DhPpy/Q@public.gmane.org>

Regards,

Hans

[-- Attachment #2: usb-storage-set-last-sector-bug-flag.patch --]
[-- Type: text/x-patch, Size: 1106 bytes --]

This patch sets the last_sector_bug flag to 1 for all USB disks. This is
needed to makes the cardreader on various HP multifunction printers work.

Since the performance impact is negible we set this flag for all USB disks to
avoid an unusual_devs.h nightmare.

Note that this patch depends on:
PATCH: scsi-sd-last-sector-bug-flag.patch

Which actually adds this flag to the scsi subsystem.

Signed-off-by: Hans de Goede <j.w.r.degoede-fbo2DhPpy/Q@public.gmane.org>
--- vanilla-2.6.24-rc7/drivers/usb/storage/scsiglue.c.psc1350	2008-01-11 19:40:31.000000000 +0100
+++ vanilla-2.6.24-rc7/drivers/usb/storage/scsiglue.c	2008-01-20 11:18:38.000000000 +0100
@@ -187,6 +187,10 @@ static int slave_configure(struct scsi_d
 		 * automatically, requiring a START-STOP UNIT command. */
 		sdev->allow_restart = 1;
 
+		/* Some USB cardreaders have trouble reading an sdcard's last
+		 * sector in a larger then 1 sector read, since the performance
+		 * impact is negible we set this flag for all USB disks */
+		sdev->last_sector_bug = 1;
 	} else {
 
 		/* Non-disk-type devices don't need to blacklist any pages

Re: PATCH: usb-storage-set-last-sector-bug-flag.patch

[Greg KH]

On Sun, Jan 20, 2008 at 11:27:29AM +0100, Hans de Goede wrote:
> Hi all,
>
> This patch sets the last_sector_bug flag to 1 for all USB disks. This is
> needed to makes the cardreader on various HP multifunction printers work.
>
> Since the performance impact is negible we set this flag for all USB disks 
> to avoid an unusual_devs.h nightmare.

Oh great, now my "working just fine" USB devices, which happen to have
data in the last sector, suddenly stop working.

That's not acceptable :(

greg k-h

Re: PATCH: usb-storage-set-last-sector-bug-flag.patch

[James Bottomley]

On Sun, 2008-01-20 at 12:56 -0800, Greg KH wrote:
> On Sun, Jan 20, 2008 at 11:27:29AM +0100, Hans de Goede wrote:
> > Hi all,
> >
> > This patch sets the last_sector_bug flag to 1 for all USB disks. This is
> > needed to makes the cardreader on various HP multifunction printers work.
> >
> > Since the performance impact is negible we set this flag for all USB disks 
> > to avoid an unusual_devs.h nightmare.
> 
> Oh great, now my "working just fine" USB devices, which happen to have
> data in the last sector, suddenly stop working.
> 
> That's not acceptable :(

I don't see how this will happen, might you not be confusing this change
(which allows access to the last sector, just insists that it be
accessed by a single sector read) with US_FL_FIX_CAPACITY which is for
devices that report having one more sectors than they actually have and
therefore adjusts the access limits down by one?

James

Re: PATCH: usb-storage-set-last-sector-bug-flag.patch

[Hans de Goede]

James Bottomley wrote:
> On Sun, 2008-01-20 at 12:56 -0800, Greg KH wrote:
>> On Sun, Jan 20, 2008 at 11:27:29AM +0100, Hans de Goede wrote:
>>> Hi all,
>>>
>>> This patch sets the last_sector_bug flag to 1 for all USB disks. This is
>>> needed to makes the cardreader on various HP multifunction printers work.
>>>
>>> Since the performance impact is negible we set this flag for all USB disks 
>>> to avoid an unusual_devs.h nightmare.
>> Oh great, now my "working just fine" USB devices, which happen to have
>> data in the last sector, suddenly stop working.
>>
>> That's not acceptable :(
> 
> I don't see how this will happen, might you not be confusing this change
> (which allows access to the last sector, just insists that it be
> accessed by a single sector read) with US_FL_FIX_CAPACITY which is for
> devices that report having one more sectors than they actually have and
> therefore adjusts the access limits down by one?
> 

Let me try to explain some more, the scsi-sd patch, which goes hand in hand 
with this adds a last_sector_bug flag, which is indeed a different flag form 
the fix_capacity flag. Both deal with with what are (in case of the 
last_sector_bug flag probably) off by one bugs.

The fix_capacity flag is for devices which report their last sector is N (with 
sectors numbered from 0 - N) but in reality  / they mean they have N sectors, 
so their last sector really is N - 1.

The last_sector_bug flag is for a bug (sofar only seen in HP multifunction 
printers with cardreader when using an sdcard) where the reader will cease to 
function (returns error codes in response to each and every command) after one 
has attempted to read the last sector in a read larger then 1 sector. To be 
clear an example lets say an example disk has 256 sectors, numbered 0 - 255.

Then the following reads will all cause the reader to go into borked mode:
16 sectors starting at 240
8 sectors starting at 248
2 sectors starting at 254

Yet the last sector can still be read without problems the following read:
1 sector starting at 255

So what the scsi-sd part of these 2 patches does it adds a last_sector_bug 
flag, which when set will cause the layer to split a read like this:
16 sectors starting at 240

Into:
15 sectors starting at 240
1 sector starting at 255


Since reading the last sector is a rare occurence (but one which does happen 
every time when determining the partition table, triggering the bug on every 
card insert). and since there are a lot of different HP printer models ( and 
cheap usb card readers are notoriously buggy so other cardreaders might be 
affected too), Matthew Dharm (the usb-storage maintainer) thought it best to 
enable this for all devices.

Regards,

Hans

Re: [usb-storage] PATCH: usb-storage-set-last-sector-bug-flag.patch

[Guillaume Bedot]

Le dimanche 20 janvier 2008 à 15:03 -0600, James Bottomley a écrit :
> On Sun, 2008-01-20 at 12:56 -0800, Greg KH wrote:
> > On Sun, Jan 20, 2008 at 11:27:29AM +0100, Hans de Goede wrote:
> > > Hi all,
> > >
> > > This patch sets the last_sector_bug flag to 1 for all USB disks. This is
> > > needed to makes the cardreader on various HP multifunction printers work.
> > >
> > > Since the performance impact is negible we set this flag for all USB disks 
> > > to avoid an unusual_devs.h nightmare.
> > 
> > Oh great, now my "working just fine" USB devices, which happen to have
> > data in the last sector, suddenly stop working.
> > 
> > That's not acceptable :(
> 
> I don't see how this will happen, might you not be confusing this change
> (which allows access to the last sector, just insists that it be
> accessed by a single sector read) with US_FL_FIX_CAPACITY which is for
> devices that report having one more sectors than they actually have and
> therefore adjusts the access limits down by one?

Well, i was the one suggesting more than 2 devices might be impacted
(i'm absolutely not sure about this, but it could be).
It seems caused by a common error when using 0 as a base index.

I don't like loosing performance for broken devices, but i'm not against
keeping that patch for all devices.

1) As the last sectors are read when the card is inserted, it results in
not working at all devices :
Users may think they are just not supported, and won't report any bug.
See how Hans worked hard to find 3 other cases in fedora, ubuntu, etc
forums !

2) It should not break in the other cases, IIUC, it just splits the read
in two.

3) It's just about the last sector, so any issue should only be greater
timing when using realtime perhaps ?

Whatever, this patch, or an other form of it, is needed (because of
1) ).
If it is a default, an option or dedicated to a limited set of devices
must be chosen.

I hope you will soon find this solution.

Best regards,

Guillaume B.

Re: PATCH: usb-storage-set-last-sector-bug-flag.patch

[Alan Stern]

On Sun, 20 Jan 2008, Greg KH wrote:

> On Sun, Jan 20, 2008 at 11:27:29AM +0100, Hans de Goede wrote:
> > Hi all,
> >
> > This patch sets the last_sector_bug flag to 1 for all USB disks. This is
> > needed to makes the cardreader on various HP multifunction printers work.
> >
> > Since the performance impact is negible we set this flag for all USB disks 
> > to avoid an unusual_devs.h nightmare.
> 
> Oh great, now my "working just fine" USB devices, which happen to have
> data in the last sector, suddenly stop working.
> 
> That's not acceptable :(

These patches really should not impact existing devices.  If they do
then something is definitely wrong.

Can you provide detailed logging information showing your problem?  For 
example, a usbmon trace would be good.  Better yet, a usbmon trace 
without the patches and a usbmon trace with the patches, for 
comparison.

Alan Stern

Re: PATCH: usb-storage-set-last-sector-bug-flag.patch

[Greg KH]

On Sun, Jan 20, 2008 at 04:45:36PM -0500, Alan Stern wrote:
> On Sun, 20 Jan 2008, Greg KH wrote:
> 
> > On Sun, Jan 20, 2008 at 11:27:29AM +0100, Hans de Goede wrote:
> > > Hi all,
> > >
> > > This patch sets the last_sector_bug flag to 1 for all USB disks. This is
> > > needed to makes the cardreader on various HP multifunction printers work.
> > >
> > > Since the performance impact is negible we set this flag for all USB disks 
> > > to avoid an unusual_devs.h nightmare.
> > 
> > Oh great, now my "working just fine" USB devices, which happen to have
> > data in the last sector, suddenly stop working.
> > 
> > That's not acceptable :(
> 
> These patches really should not impact existing devices.  If they do
> then something is definitely wrong.
> 
> Can you provide detailed logging information showing your problem?  For 
> example, a usbmon trace would be good.  Better yet, a usbmon trace 
> without the patches and a usbmon trace with the patches, for 
> comparison.

I don't have a device with such a problem, I just am worried that we are
now suddenly keeping access from the last sector for devices that
currently did work just fine.

If you all guarantee that this will not happen, hey, I'm happy, and I'll
gladly point all bug reports on to you all :)

thanks,

greg k-h

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: PATCH: usb-storage-set-last-sector-bug-flag.patch

[James Bottomley]

On Wed, 2008-01-23 at 10:12 -0800, Greg KH wrote:
> On Sun, Jan 20, 2008 at 04:45:36PM -0500, Alan Stern wrote:
> > On Sun, 20 Jan 2008, Greg KH wrote:
> > 
> > > On Sun, Jan 20, 2008 at 11:27:29AM +0100, Hans de Goede wrote:
> > > > Hi all,
> > > >
> > > > This patch sets the last_sector_bug flag to 1 for all USB disks. This is
> > > > needed to makes the cardreader on various HP multifunction printers work.
> > > >
> > > > Since the performance impact is negible we set this flag for all USB disks 
> > > > to avoid an unusual_devs.h nightmare.
> > > 
> > > Oh great, now my "working just fine" USB devices, which happen to have
> > > data in the last sector, suddenly stop working.
> > > 
> > > That's not acceptable :(
> > 
> > These patches really should not impact existing devices.  If they do
> > then something is definitely wrong.
> > 
> > Can you provide detailed logging information showing your problem?  For 
> > example, a usbmon trace would be good.  Better yet, a usbmon trace 
> > without the patches and a usbmon trace with the patches, for 
> > comparison.
> 
> I don't have a device with such a problem, I just am worried that we are
> now suddenly keeping access from the last sector for devices that
> currently did work just fine.
> 
> If you all guarantee that this will not happen, hey, I'm happy, and I'll
> gladly point all bug reports on to you all :)


Hey, you can trust me ... I'm a doctor ...

Would you like me to add your 'Responsibility-disclamed-by:' to the
patch ...?

James

Re: PATCH: usb-storage-set-last-sector-bug-flag.patch

[Greg KH]

On Wed, Jan 23, 2008 at 12:39:39PM -0600, James Bottomley wrote:
> 
> On Wed, 2008-01-23 at 10:12 -0800, Greg KH wrote:
> > On Sun, Jan 20, 2008 at 04:45:36PM -0500, Alan Stern wrote:
> > > On Sun, 20 Jan 2008, Greg KH wrote:
> > > 
> > > > On Sun, Jan 20, 2008 at 11:27:29AM +0100, Hans de Goede wrote:
> > > > > Hi all,
> > > > >
> > > > > This patch sets the last_sector_bug flag to 1 for all USB disks. This is
> > > > > needed to makes the cardreader on various HP multifunction printers work.
> > > > >
> > > > > Since the performance impact is negible we set this flag for all USB disks 
> > > > > to avoid an unusual_devs.h nightmare.
> > > > 
> > > > Oh great, now my "working just fine" USB devices, which happen to have
> > > > data in the last sector, suddenly stop working.
> > > > 
> > > > That's not acceptable :(
> > > 
> > > These patches really should not impact existing devices.  If they do
> > > then something is definitely wrong.
> > > 
> > > Can you provide detailed logging information showing your problem?  For 
> > > example, a usbmon trace would be good.  Better yet, a usbmon trace 
> > > without the patches and a usbmon trace with the patches, for 
> > > comparison.
> > 
> > I don't have a device with such a problem, I just am worried that we are
> > now suddenly keeping access from the last sector for devices that
> > currently did work just fine.
> > 
> > If you all guarantee that this will not happen, hey, I'm happy, and I'll
> > gladly point all bug reports on to you all :)
> 
> 
> Hey, you can trust me ... I'm a doctor ...
> 
> Would you like me to add your 'Responsibility-disclamed-by:' to the
> patch ...?

Heh.

As long as you take it through your trees, I inherently disclaim any
responsibility :)

greg k-h

Re: PATCH: usb-storage-set-last-sector-bug-flag.patch

[Stefan Richter]

Greg KH wrote:
> I just am worried that we are
> now suddenly keeping access from the last sector for devices that
> currently did work just fine.

This new workaround doesn't prevent access to the last sector.  It only
breaks up a multi-sector access which would also reach the last sector
into several (two? I'm too lazy to look back in the mail thread)
accesses, in order to access the last sector in a dedicated
single-sector access.

So that's very differently to the fix-capacity workaround.  The
fix-capacity workaround manipulates the READ CAPACITY parameter data.
Therefore the fix-capacity workaround is unsafe for non-buggy devices.

The last-sector-(access-)bug workaround _only_ modifies the command
stream which is sent to the device.  A dangerous command is replaced by
equivalent safe commands.  These commands are luckily safe for _all_
devices, buggy and non-buggy ones.  The only cost of this workaround is
(1.) the code, (2.) the runtime/ bandwidth/ latency overhead for
accesses which reach the last sector.

Somebody correct me if I got something wrong.
-- 
Stefan Richter
-=====-==--- ---= ==---
http://arcgraph.de/sr/

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: PATCH: usb-storage-set-last-sector-bug-flag.patch

[Greg KH]

On Thu, Jan 24, 2008 at 06:07:00PM +0100, Stefan Richter wrote:
> Greg KH wrote:
> > I just am worried that we are
> > now suddenly keeping access from the last sector for devices that
> > currently did work just fine.
> 
> This new workaround doesn't prevent access to the last sector.  It only
> breaks up a multi-sector access which would also reach the last sector
> into several (two? I'm too lazy to look back in the mail thread)
> accesses, in order to access the last sector in a dedicated
> single-sector access.
> 
> So that's very differently to the fix-capacity workaround.  The
> fix-capacity workaround manipulates the READ CAPACITY parameter data.
> Therefore the fix-capacity workaround is unsafe for non-buggy devices.
> 
> The last-sector-(access-)bug workaround _only_ modifies the command
> stream which is sent to the device.  A dangerous command is replaced by
> equivalent safe commands.  These commands are luckily safe for _all_
> devices, buggy and non-buggy ones.  The only cost of this workaround is
> (1.) the code, (2.) the runtime/ bandwidth/ latency overhead for
> accesses which reach the last sector.

Ok, thanks for explaining it better.  I have no objection to this change
anymore.

thanks,

greg k-h

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: PATCH: usb-storage-set-last-sector-bug-flag.patch

[James Bottomley]

On Thu, 2008-01-24 at 09:21 -0800, Greg KH wrote:
> On Thu, Jan 24, 2008 at 06:07:00PM +0100, Stefan Richter wrote:
> > Greg KH wrote:
> > > I just am worried that we are
> > > now suddenly keeping access from the last sector for devices that
> > > currently did work just fine.
> > 
> > This new workaround doesn't prevent access to the last sector.  It only
> > breaks up a multi-sector access which would also reach the last sector
> > into several (two? I'm too lazy to look back in the mail thread)
> > accesses, in order to access the last sector in a dedicated
> > single-sector access.
> > 
> > So that's very differently to the fix-capacity workaround.  The
> > fix-capacity workaround manipulates the READ CAPACITY parameter data.
> > Therefore the fix-capacity workaround is unsafe for non-buggy devices.
> > 
> > The last-sector-(access-)bug workaround _only_ modifies the command
> > stream which is sent to the device.  A dangerous command is replaced by
> > equivalent safe commands.  These commands are luckily safe for _all_
> > devices, buggy and non-buggy ones.  The only cost of this workaround is
> > (1.) the code, (2.) the runtime/ bandwidth/ latency overhead for
> > accesses which reach the last sector.
> 
> Ok, thanks for explaining it better.  I have no objection to this change
> anymore.

So, for forms sake to take this through the SCSI tree I need at least
one USB person to ack it ...

James



-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: PATCH: usb-storage-set-last-sector-bug-flag.patch

[Matthew Dharm]

[-- Attachment #1.1: Type: text/plain, Size: 1855 bytes --]

ACKed-by: Matthew Dharm <mdharm-usb@one-eyed-alien.net>

Matt

On Fri, Jan 25, 2008 at 11:12:21AM -0600, James Bottomley wrote:
> On Thu, 2008-01-24 at 09:21 -0800, Greg KH wrote:
> > On Thu, Jan 24, 2008 at 06:07:00PM +0100, Stefan Richter wrote:
> > > Greg KH wrote:
> > > > I just am worried that we are
> > > > now suddenly keeping access from the last sector for devices that
> > > > currently did work just fine.
> > > 
> > > This new workaround doesn't prevent access to the last sector.  It only
> > > breaks up a multi-sector access which would also reach the last sector
> > > into several (two? I'm too lazy to look back in the mail thread)
> > > accesses, in order to access the last sector in a dedicated
> > > single-sector access.
> > > 
> > > So that's very differently to the fix-capacity workaround.  The
> > > fix-capacity workaround manipulates the READ CAPACITY parameter data.
> > > Therefore the fix-capacity workaround is unsafe for non-buggy devices.
> > > 
> > > The last-sector-(access-)bug workaround _only_ modifies the command
> > > stream which is sent to the device.  A dangerous command is replaced by
> > > equivalent safe commands.  These commands are luckily safe for _all_
> > > devices, buggy and non-buggy ones.  The only cost of this workaround is
> > > (1.) the code, (2.) the runtime/ bandwidth/ latency overhead for
> > > accesses which reach the last sector.
> > 
> > Ok, thanks for explaining it better.  I have no objection to this change
> > anymore.
> 
> So, for forms sake to take this through the SCSI tree I need at least
> one USB person to ack it ...
> 
> James
> 

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

It was a new hope.
					-- Dust Puppy
User Friendly, 12/25/1998

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #3: Type: text/plain, Size: 191 bytes --]

_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: PATCH: usb-storage-set-last-sector-bug-flag.patch

[Stefan Richter]

> The last-sector-(access-)bug workaround _only_ modifies the command
> stream which is sent to the device.  A dangerous command is replaced by
> equivalent safe commands.

BTW, one thing about this patch set needs to be criticized:

"last_sector_bug" is a really bad name choice for the new flag.  No
wonder that it is confused with the (well named) "fix_capacity" flag.

How about "read_last_sector_separately" or something in the way?
-- 
Stefan Richter
-=====-==--- ---= ==---
http://arcgraph.de/sr/

Re: [usb-storage] PATCH: usb-storage-set-last-sector-bug-flag.patch

[Guillaume Bedot]

Hello,

Le dimanche 20 janvier 2008 à 11:27 +0100, Hans de Goede a écrit :
> Hi all,
> 
> This patch sets the last_sector_bug flag to 1 for all USB disks. This is
> needed to makes the cardreader on various HP multifunction printers work.
> 
> Since the performance impact is negible we set this flag for all USB disks to
> avoid an unusual_devs.h nightmare.
> 
> Note that this patch depends on:
> PATCH: scsi-sd-last-sector-bug-flag.patch
> 
> Which actually adds this flag to the scsi subsystem.
> 
> Signed-off-by: Hans de Goede <j.w.r.degoede-fbo2DhPpy/Q@public.gmane.org>
> 
> Regards,
> 
> Hans

Just a line to say these new patches work for me.

Best regards,

Guillaume B.