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

PATCH Hans de Goede 9 participants 15 messages Jan 20, 2008 – Jan 25, 2008 5 messages not in the archive

Subsystems (3)

SCSI USB USB mass storage driver

lore

Share & tools

lore

Hans de Goede

author

2008-01-20 10:27 UTC #0 lore raw

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/[email protected]>

Regards,

Hans

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/[email protected]>

drivers/usb/storage/scsiglue.c renamed from drivers/usb/storage/scsiglue.c.psc1350 +4 -0

@@ -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

Greg KH

2008-01-20 20:55 UTC #1 lore raw

Quoted 7 lines · Hans de Goede

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

James Bottomley

2008-01-20 21:03 UTC #3 lore raw

Quoted 6 lines · Greg KH

Quoted 5 lines · Hans de Goede
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.

Quoted 7 lines

Quoted 2 lines
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

A message that is not in the archive (sent off-list or not yet received): 1200863008.3105.30.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/[email protected]

Hans de Goede

author

2008-01-20 21:25 UTC #5 lore raw

Quoted 11 lines · James Bottomley

Quoted 10 lines · Greg KH
Quoted 6 lines · Hans de Goede
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.

Quoted 8 lines

Quoted 1 line
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

Guillaume Bedot

2008-01-20 21:39 UTC #6 lore raw

In reply to a message not in the archive

Quoted 13 lines · James Bottomley

Quoted 12 lines · Greg KH
Quoted 7 lines · Hans de Goede
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.

Quoted 7 lines

Quoted 1 line
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.

Alan Stern

2008-01-20 21:45 UTC #7 lore raw

In reply to a message not in the archive

Quoted 6 lines · Greg KH

Quoted 5 lines · Hans de Goede
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.

Quoted 7 lines

Quoted 2 lines
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

Greg KH

2008-01-23 18:12 UTC #8 lore raw

Quoted 16 lines · Alan Stern

Quoted 6 lines · Greg KH
Quoted 5 lines · Hans de Goede
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.
Quoted 7 lines
Quoted 2 lines
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 :(

Quoted 7 lines

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/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

A message that is not in the archive (sent off-list or not yet received): [email protected]

James Bottomley

2008-01-23 18:39 UTC #10 lore raw

Quoted 25 lines · Greg KH

Quoted 16 lines · Alan Stern
Quoted 6 lines · Greg KH
Quoted 5 lines · Hans de Goede
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.
Quoted 7 lines
Quoted 2 lines
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 :(
Quoted 7 lines
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.

Quoted 6 lines

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

Greg KH

2008-01-23 18:49 UTC #11 lore raw

Quoted 31 lines · James Bottomley

Quoted 29 lines · Greg KH
Quoted 16 lines · Alan Stern
Quoted 6 lines · Greg KH
Quoted 5 lines · Hans de Goede
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.
Quoted 7 lines
Quoted 2 lines
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 :(
Quoted 7 lines
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.

Quoted 8 lines

Quoted 2 lines
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

Stefan Richter

2008-01-24 17:07 UTC #12 lore raw

In reply to Greg KH

Quoted 3 lines · Greg KH

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.

Signature · 12 lines

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/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Greg KH

2008-01-24 17:21 UTC #13 lore raw

Quoted 15 lines · Stefan Richter

Quoted 3 lines · Greg KH
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.

Quoted 6 lines

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

James Bottomley

maintainer

2008-01-25 17:12 UTC #14 lore raw

Quoted 23 lines · Greg KH

Quoted 15 lines · Stefan Richter
Quoted 3 lines · Greg KH
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.
Quoted 6 lines
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.

Quoted 2 lines

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

Acked-by

2008-01-25 19:23 UTC #15 lore raw

Acked-by Matthew Dharm <[email protected]>

Matt

Quoted 32 lines · James Bottomley

Quoted 23 lines · Greg KH
Quoted 15 lines · Stefan Richter
Quoted 3 lines · Greg KH
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.
Quoted 6 lines
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.
Quoted 2 lines
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

Signature · 6 lines

Matthew Dharm                              Home: [email protected] 
Maintainer, Linux USB Mass Storage Driver

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

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

A message that is not in the archive (sent off-list or not yet received): 4798C5B4.1000208-MtYdepGKPcBMYopoZt5u/[email protected]

Stefan Richter

2008-01-24 17:22 UTC #17 lore raw

Quoted 3 lines

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?

Signature · 3 lines

Stefan Richter
-=====-==--- ---= ==---
http://arcgraph.de/sr/

A message that is not in the archive (sent off-list or not yet received): 47932211.90105-fbo2DhPpy/[email protected]

Guillaume Bedot

2008-01-22 08:26 UTC #19 lore raw

Hello,

Quoted 11 lines · Hans de Goede

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

Quoted 7 lines

Which actually adds this flag to the scsi subsystem.
Signed-off-by Hans de Goede <j.w.r.degoede-fbo2DhPpy/[email protected]>
Regards,
Hans

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

Best regards,

Guillaume B.