Discussion:
PATCH: usb-storage-set-last-sector-bug-flag.patch
Hans de Goede
2008-01-20 10:27:29 UTC
Permalink
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/***@public.gmane.org>

Regards,

Hans
Greg KH
2008-01-20 20:56:25 UTC
Permalink
Post by 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
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
James Bottomley
2008-01-20 21:03:28 UTC
Permalink
Post by Greg KH
Post by 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 :(
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
Hans de Goede
2008-01-20 21:25:38 UTC
Permalink
Post by James Bottomley
Post by Greg KH
Post by 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 :(
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:19 UTC
Permalink
Le dimanche 20 janvier 2008 =C3=A0 15:03 -0600, James Bottomley a =C3=A9=
Post by Hans de Goede
Hi all,
This patch sets the last_sector_bug flag to 1 for all USB disks. =
This is
Post by Hans de Goede
needed to makes the cardreader on various HP multifunction printe=
rs work.
Post by Hans de Goede
Since the performance impact is negible we set this flag for all =
USB disks=20
Post by Hans de Goede
to avoid an unusual_devs.h nightmare.
=20
Oh great, now my "working just fine" USB devices, which happen to h=
ave
data in the last sector, suddenly stop working.
=20
That's not acceptable :(
=20
I don't see how this will happen, might you not be confusing this cha=
nge
(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 fo=
r
devices that report having one more sectors than they actually have a=
nd
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 agains=
t
keeping that patch for all devices.

1) As the last sectors are read when the card is inserted, it results i=
n
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 rea=
d
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:36 UTC
Permalink
Post by Greg KH
Post by 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 :(
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:39 UTC
Permalink
Post by Alan Stern
Post by Greg KH
Post by 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 :(
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-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
James Bottomley
2008-01-23 18:39:39 UTC
Permalink
Post by Greg KH
Post by Alan Stern
Post by Greg KH
Post by 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 :(
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
Greg KH
2008-01-23 18:50:28 UTC
Permalink
Post by James Bottomley
Post by Greg KH
Post by Alan Stern
Post by Greg KH
Post by 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 :(
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
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Dave Higton
2008-01-24 07:37:17 UTC
Permalink
-----Original Message-----
Sent: 2008 January 23 18:13
To: Alan Stern
Cc: Hans de Goede; Matthew Dharm; James Bottomley; Boaz
Harrosh; USB Storage list; USB development list; David Brown;
Subject: Re: PATCH: usb-storage-set-last-sector-bug-flag.patch
Post by Alan Stern
Post by Greg KH
Post by Hans de Goede
Hi all,
This patch sets the last_sector_bug flag to 1 for all
USB disks. This is
Post by Alan Stern
Post by Greg KH
Post by Hans de Goede
needed to makes the cardreader on various HP
multifunction printers work.
Post by Alan Stern
Post by Greg KH
Post by Hans de Goede
Since the performance impact is negible we set this
flag for all USB disks
Post by Alan Stern
Post by Greg KH
Post by Hans de Goede
to avoid an unusual_devs.h nightmare.
Oh great, now my "working just fine" USB devices, which
happen to have
Post by Alan Stern
Post by Greg KH
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
Post by Alan Stern
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.
I thought that this flag permitted access to the last sector,
it caused requests for multiple sectors including the last to
be broken into two requests. Did I misunderstand?

Dave


*************************************************************************************************************************************************************************************************************************************************

NICE CTI Systems UK Limited ("NICE") is registered in England under company number, 3403044. The registered office of NICE is at Tollbar Way, Hedge End, Southampton, Hampshire SO30 2ZP.

Confidentiality: This communication and any attachments are intended for the above-named persons only and may be confidential and/or legally privileged. Any opinions expressed in this communication are not necessarily those of NICE. If this communication has come to you in error you must take no action based on it, nor must you copy or show it to anyone; please delete/destroy and inform the sender by e-mail immediately.

Monitoring: NICE may monitor incoming and outgoing e-mails.

Viruses: Although we have taken steps toward ensuring that this e-mail and attachments are free from any virus, we advise that in keeping with good computing practice the recipient should ensure they are actually virus free.

****************************************************************************************************************************************************************************************************************************************************




-------------------------------------------------------------------------
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-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Stefan Richter
2008-01-24 17:07:00 UTC
Permalink
Post by 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.
--
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-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Stefan Richter
2008-01-24 17:22:57 UTC
Permalink
Post by 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/
Greg KH
2008-01-24 17:21:58 UTC
Permalink
Post by Stefan Richter
Post by 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.
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-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
James Bottomley
2008-01-25 17:12:21 UTC
Permalink
Post by Greg KH
Post by Stefan Richter
Post by 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.
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-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
Matthew Dharm
2008-01-25 19:23:23 UTC
Permalink
ACKed-by: Matthew Dharm <mdharm-***@one-eyed-alien.net>

Matt
Post by James Bottomley
Post by Greg KH
Post by Stefan Richter
Post by 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.
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-***@one-eyed-alien.net
Maintainer, Linux USB Mass Storage Driver

It was a new hope.
-- Dust Puppy
User Friendly, 12/25/1998
Guillaume Bedot
2008-01-22 08:26:16 UTC
Permalink
Hello,

Le dimanche 20 janvier 2008 =C3=A0 11:27 +0100, Hans de Goede a =C3=A9c=
Post by Hans de Goede
Hi all,
=20
This patch sets the last_sector_bug flag to 1 for all USB disks. This=
is
Post by Hans de Goede
needed to makes the cardreader on various HP multifunction printers w=
ork.
Post by Hans de Goede
=20
Since the performance impact is negible we set this flag for all USB =
disks to
Post by Hans de Goede
avoid an unusual_devs.h nightmare.
=20
PATCH: scsi-sd-last-sector-bug-flag.patch
=20
Which actually adds this flag to the scsi subsystem.
=20
=20
Regards,
=20
Hans
Just a line to say these new patches work for me.

Best regards,

Guillaume B.
Loading...