[PATCH resend 1/4] scsi/sr: add no_read_disc_info scsi_device flag

[PATCH resend 1/4] scsi/sr: add no_read_disc_info scsi_device flag

[Hans de Goede]

Some USB devices emulate a usb-mass-storage attached (scsi) cdrom device,
usually this fake cdrom contains the windows software for the device.
While working on supporting Appotech ax3003 based photoframes, which do
this I discovered that they will go of into lala land when ever they
see a READ_DISC_INFO scsi command.

Thus this patch adds a scsi_device flag (which can then be set by the
usb-storage driver through an unsual-devs entry), to indicate this, and
makes the sr driver honor this flag.

I know this sucks, but as discussed on linux-scsi list there is no other
way to make this device work properly.

Looking at usb traces made under windows, windows never sends a
READ_DISC_INFO during normal interactions with a usb cdrom device. So as
this cdrom emulation thingie becomes more common we might see more of
this problem.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/scsi/sr.c          |    8 +++++++-
 include/scsi/scsi_device.h |    1 +
 2 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 0a90abc..18077b5 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -849,10 +849,16 @@ static void get_capabilities(struct scsi_cd *cd)
 static int sr_packet(struct cdrom_device_info *cdi,
 		struct packet_command *cgc)
 {
+	struct scsi_cd *cd = cdi->handle;
+	struct scsi_device *sdev = cd->device;
+
+	if (cgc->cmd[0] == GPCMD_READ_DISC_INFO && sdev->no_read_disc_info)
+		return -EDRIVE_CANT_DO_THIS;
+
 	if (cgc->timeout <= 0)
 		cgc->timeout = IOCTL_TIMEOUT;
 
-	sr_do_ioctl(cdi->handle, cgc);
+	sr_do_ioctl(cd, cgc);
 
 	return cgc->stat;
 }
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index d80b6db..e1b2db8 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -148,6 +148,7 @@ struct scsi_device {
 	unsigned retry_hwerror:1;	/* Retry HARDWARE_ERROR */
 	unsigned last_sector_bug:1;	/* do not use multisector accesses on
 					   SD_LAST_BUGGY_SECTORS */
+	unsigned no_read_disc_info:1;	/* Avoid READ_DISC_INFO cmds */
 	unsigned is_visible:1;	/* is the device visible in sysfs */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
-- 
1.7.0.1

[PATCH resend 3/4] scsi/sd: Add a no_read_capacity_16 scsi_device flag

[Hans de Goede]

I know I know, I seem to have a nack for digging up buggy usb devices
which don't work with Linux, and I'm crazy enough to try to make them
work. So this time a friend of mine asked me to get an mp4 player (an
mp3 player which can play videos on a small screen) to work with Linux.

It is based on the well known rockbox chipset for which we already have
an unusual devs entries to work around some of its bugs. But this model
comes with an additional twist.

This model chokes on read_capacity_16 calls. Now normally we don't make
those calls, but this model comes with an sdcard slot and when there
is no card in there (and shipped from the factory there is none), it
reports a size of 0. However this time the programmers actually
got the read_capacity_10 response right! So they substract one from
the size as stored internally in the mp3 player before reporting it back,
resulting in an answer of ... 0xffffffff sectors, causing sd.c to
try a read_capacity_16, on which the device crashes.

This patch adds a flag to scsi_device to indicate that a a device cannot
handle read_capacity_16, and when this flag is set if a device reports
an lba of 0xffffffff as answer to a read_capacity_10, assumes it tries
to report a size of 0.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/scsi/sd.c          |   12 ++++++++++++
 include/scsi/scsi_device.h |    1 +
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 8802e48..1ad339b 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1450,6 +1450,9 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
 	unsigned long long lba;
 	unsigned sector_size;
 
+	if (sdp->no_read_capacity_16)
+		return -EINVAL;
+
 	do {
 		memset(cmd, 0, 16);
 		cmd[0] = SERVICE_ACTION_IN;
@@ -1578,6 +1581,15 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
 	sector_size = get_unaligned_be32(&buffer[4]);
 	lba = get_unaligned_be32(&buffer[0]);
 
+	if (sdp->no_read_capacity_16 && (lba == 0xffffffff)) {
+		/* Some buggy (usb cardreader) devices return an lba of
+		   0xffffffff when the want to report a size of 0 (with
+		   which they really mean no media is present) */
+		sdkp->capacity = 0;
+		sdkp->hw_sector_size = sector_size;
+		return sector_size;
+	}
+
 	if ((sizeof(sdkp->capacity) == 4) && (lba == 0xffffffff)) {
 		sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
 			"kernel compiled with support for large block "
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index e1b2db8..02bbbe5 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -149,6 +149,7 @@ struct scsi_device {
 	unsigned last_sector_bug:1;	/* do not use multisector accesses on
 					   SD_LAST_BUGGY_SECTORS */
 	unsigned no_read_disc_info:1;	/* Avoid READ_DISC_INFO cmds */
+	unsigned no_read_capacity_16:1; /* Avoid READ_CAPACITY_16 cmds */
 	unsigned is_visible:1;	/* is the device visible in sysfs */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
-- 
1.7.0.1

[PATCH resend 4/4] usb-storage: Add new no_read_capacity_16 quirk

[Hans de Goede]

Some Rockbox based mp4 players will crash when ever they see a
read_capacity_16 scsi command. So add a new US_FL which tells the
scsi sd driver to not issue any read_capacity_16 scsi commands.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/storage/scsiglue.c     |    4 ++++
 drivers/usb/storage/unusual_devs.h |    3 ++-
 include/linux/usb_usual.h          |    4 +++-
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
index 4a30fbf..4d0c437 100644
--- a/drivers/usb/storage/scsiglue.c
+++ b/drivers/usb/storage/scsiglue.c
@@ -209,6 +209,10 @@ static int slave_configure(struct scsi_device *sdev)
 		if (us->fflags & US_FL_CAPACITY_HEURISTICS)
 			sdev->guess_capacity = 1;
 
+		/* Some devices cannot handle READ_CAPACITY_16 */
+		if (us->fflags & US_FL_NO_READ_CAPACITY_16)
+			sdev->no_read_capacity_16 = 1;
+
 		/* assume SPC3 or latter devices support sense size > 18 */
 		if (sdev->scsi_level > SCSI_SPC_2)
 			us->fflags |= US_FL_SANE_SENSE;
diff --git a/drivers/usb/storage/unusual_devs.h b/drivers/usb/storage/unusual_devs.h
index 42a83c4..262ba6d 100644
--- a/drivers/usb/storage/unusual_devs.h
+++ b/drivers/usb/storage/unusual_devs.h
@@ -877,7 +877,8 @@ UNUSUAL_DEV(  0x071b, 0x3203, 0x0000, 0x0000,
 		"RockChip",
 		"MP3",
 		US_SC_DEVICE, US_PR_DEVICE, NULL,
-		US_FL_NO_WP_DETECT | US_FL_MAX_SECTORS_64),
+		US_FL_NO_WP_DETECT | US_FL_MAX_SECTORS_64 |
+		US_FL_NO_READ_CAPACITY_16),
 
 /* Reported by Jean-Baptiste Onofre <jb@nanthrax.net>
  * Support the following product :
diff --git a/include/linux/usb_usual.h b/include/linux/usb_usual.h
index bff51d0..d4ee5b8 100644
--- a/include/linux/usb_usual.h
+++ b/include/linux/usb_usual.h
@@ -60,7 +60,9 @@
 	US_FLAG(BAD_SENSE,	0x00020000)			\
 		/* Bad Sense (never more than 18 bytes) */	\
 	US_FLAG(NO_READ_DISC_INFO,	0x00040000)		\
-		/* cannot handle READ_DISC_INFO */
+		/* cannot handle READ_DISC_INFO */		\
+	US_FLAG(NO_READ_CAPACITY_16,	0x00080000)		\
+		/* cannot handle READ_CAPACITY_16 */
 
 #define US_FLAG(name, value)	US_FL_##name = value ,
 enum { US_DO_ALL_FLAGS };
-- 
1.7.0.1

[PATCH resend 2/4] usb-storage: Add new no_read_disc_info quirk

[Hans de Goede]

Appotech ax3003 (the larger brother of the ax203) based devices are even
more buggy then the ax203. They will go of into lala land when ever they
see a READ_DISC_INFO scsi command. So add a new US_FL which tells the
scsi sr driver to not issue any READ_DISC_INFO scsi commands.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/usb/storage/scsiglue.c     |    4 ++++
 drivers/usb/storage/unusual_devs.h |    5 +++++
 include/linux/usb_usual.h          |    4 +++-
 3 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
index d8d98cf..4a30fbf 100644
--- a/drivers/usb/storage/scsiglue.c
+++ b/drivers/usb/storage/scsiglue.c
@@ -253,6 +253,10 @@ static int slave_configure(struct scsi_device *sdev)
 		 * or to force 192-byte transfer lengths for MODE SENSE.
 		 * But they do need to use MODE SENSE(10). */
 		sdev->use_10_for_ms = 1;
+
+		/* Some (fake) usb cdrom devices don't like READ_DISC_INFO */
+		if (us->fflags & US_FLAG_NO_READ_DISC_INFO)
+			sdev->no_read_disc_info = 1;
 	}
 
 	/* The CB and CBI transports have no way to pass LUN values
diff --git a/drivers/usb/storage/unusual_devs.h b/drivers/usb/storage/unusual_devs.h
index 2c897ee..42a83c4 100644
--- a/drivers/usb/storage/unusual_devs.h
+++ b/drivers/usb/storage/unusual_devs.h
@@ -1858,6 +1858,11 @@ UNUSUAL_DEV( 0x1908, 0x1320, 0x0000, 0x0000,
 		"Photo Frame",
 		US_SC_DEVICE, US_PR_DEVICE, NULL,
 		US_FL_BAD_SENSE ),
+UNUSUAL_DEV( 0x1908, 0x3335, 0x0200, 0x0200,
+		"BUILDWIN",
+		"Photo Frame",
+		US_SC_DEVICE, US_PR_DEVICE, NULL,
+		US_FL_NO_READ_DISC_INFO ),
 
 UNUSUAL_DEV( 0x2116, 0x0320, 0x0001, 0x0001,
 		"ST",
diff --git a/include/linux/usb_usual.h b/include/linux/usb_usual.h
index a4b947e..bff51d0 100644
--- a/include/linux/usb_usual.h
+++ b/include/linux/usb_usual.h
@@ -58,7 +58,9 @@
 	US_FLAG(CAPACITY_OK,	0x00010000)			\
 		/* READ CAPACITY response is correct */		\
 	US_FLAG(BAD_SENSE,	0x00020000)			\
-		/* Bad Sense (never more than 18 bytes) */
+		/* Bad Sense (never more than 18 bytes) */	\
+	US_FLAG(NO_READ_DISC_INFO,	0x00040000)		\
+		/* cannot handle READ_DISC_INFO */
 
 #define US_FLAG(name, value)	US_FL_##name = value ,
 enum { US_DO_ALL_FLAGS };
-- 
1.7.0.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH resend 1/4] scsi/sr: add no_read_disc_info scsi_device flag

[James Bottomley]

On Fri, 2010-07-23 at 10:52 +0200, Hans de Goede wrote:
> Some USB devices emulate a usb-mass-storage attached (scsi) cdrom device,
> usually this fake cdrom contains the windows software for the device.
> While working on supporting Appotech ax3003 based photoframes, which do
> this I discovered that they will go of into lala land when ever they
> see a READ_DISC_INFO scsi command.
> 
> Thus this patch adds a scsi_device flag (which can then be set by the
> usb-storage driver through an unsual-devs entry), to indicate this, and
> makes the sr driver honor this flag.
> 
> I know this sucks, but as discussed on linux-scsi list there is no other
> way to make this device work properly.
> 
> Looking at usb traces made under windows, windows never sends a
> READ_DISC_INFO during normal interactions with a usb cdrom device. So as
> this cdrom emulation thingie becomes more common we might see more of
> this problem.

So, I suppose we can do this.  I dislike threading device bugs like this
up and down the stack.  The usb stor_control_thread already does some
command filtering; if it just rejected this command and READ
CAPACITY(16) on the flags, I think it would be far less code and the
SCSI subsystem would just do the right thing.

I'd actually like to hear from USB what they'd prefer to do.

James


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH resend 1/4] scsi/sr: add no_read_disc_info scsi_device flag

[Hans de Goede]

Hi,

On 07/28/2010 03:19 PM, James Bottomley wrote:
> On Fri, 2010-07-23 at 10:52 +0200, Hans de Goede wrote:
>> Some USB devices emulate a usb-mass-storage attached (scsi) cdrom device,
>> usually this fake cdrom contains the windows software for the device.
>> While working on supporting Appotech ax3003 based photoframes, which do
>> this I discovered that they will go of into lala land when ever they
>> see a READ_DISC_INFO scsi command.
>>
>> Thus this patch adds a scsi_device flag (which can then be set by the
>> usb-storage driver through an unsual-devs entry), to indicate this, and
>> makes the sr driver honor this flag.
>>
>> I know this sucks, but as discussed on linux-scsi list there is no other
>> way to make this device work properly.
>>
>> Looking at usb traces made under windows, windows never sends a
>> READ_DISC_INFO during normal interactions with a usb cdrom device. So as
>> this cdrom emulation thingie becomes more common we might see more of
>> this problem.
>
> So, I suppose we can do this.  I dislike threading device bugs like this
> up and down the stack.  The usb stor_control_thread already does some
> command filtering; if it just rejected this command and READ
> CAPACITY(16) on the flags, I think it would be far less code and the
> SCSI subsystem would just do the right thing.
>
> I'd actually like to hear from USB what they'd prefer to do.
>

I was sort of hoping that Alan Stern would reply to this, but maybe he is
on vacation?

The reason I've done this patchset this way, is that I've added several
other usb mass storage specific quirks in the past and each time the USB
guys said that they don't want to do any (extra) command filtering and
asked me to split the patch in a scsi / sd flag adding patch and a
patch for the usb mass storage driver to use that flag.

Regards,

Hans

Re: [PATCH resend 1/4] scsi/sr: add no_read_disc_info scsi_device flag

[Alan Stern]

On Mon, 2 Aug 2010, Hans de Goede wrote:

> Hi,
> 
> On 07/28/2010 03:19 PM, James Bottomley wrote:
> > On Fri, 2010-07-23 at 10:52 +0200, Hans de Goede wrote:
> >> Some USB devices emulate a usb-mass-storage attached (scsi) cdrom device,
> >> usually this fake cdrom contains the windows software for the device.
> >> While working on supporting Appotech ax3003 based photoframes, which do
> >> this I discovered that they will go of into lala land when ever they
> >> see a READ_DISC_INFO scsi command.
> >>
> >> Thus this patch adds a scsi_device flag (which can then be set by the
> >> usb-storage driver through an unsual-devs entry), to indicate this, and
> >> makes the sr driver honor this flag.
> >>
> >> I know this sucks, but as discussed on linux-scsi list there is no other
> >> way to make this device work properly.
> >>
> >> Looking at usb traces made under windows, windows never sends a
> >> READ_DISC_INFO during normal interactions with a usb cdrom device. So as
> >> this cdrom emulation thingie becomes more common we might see more of
> >> this problem.
> >
> > So, I suppose we can do this.  I dislike threading device bugs like this
> > up and down the stack.  The usb stor_control_thread already does some
> > command filtering; if it just rejected this command and READ
> > CAPACITY(16) on the flags, I think it would be far less code and the
> > SCSI subsystem would just do the right thing.
> >
> > I'd actually like to hear from USB what they'd prefer to do.
> >
> 
> I was sort of hoping that Alan Stern would reply to this, but maybe he is
> on vacation?

No, I'm here.  I've been waiting to hear from Matt Dharm.  He's the 
usb-storage maintainer, so it's his call.

> The reason I've done this patchset this way, is that I've added several
> other usb mass storage specific quirks in the past and each time the USB
> guys said that they don't want to do any (extra) command filtering and
> asked me to split the patch in a scsi / sd flag adding patch and a
> patch for the usb mass storage driver to use that flag.

In general it's hard to know how command filtering at the usb-storage 
level will interact with the higher SCSI layers.  In this case we have 
James's assurance that it will be okay.

Alan Stern

Re: [PATCH resend 1/4] scsi/sr: add no_read_disc_info scsi_device flag

[Matthew Dharm]

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

On Tue, Aug 03, 2010 at 10:14:48AM -0400, Alan Stern wrote:
> On Mon, 2 Aug 2010, Hans de Goede wrote:
> 
> > Hi,
> > 
> > On 07/28/2010 03:19 PM, James Bottomley wrote:
> > > On Fri, 2010-07-23 at 10:52 +0200, Hans de Goede wrote:
> > >> Some USB devices emulate a usb-mass-storage attached (scsi) cdrom device,
> > >> usually this fake cdrom contains the windows software for the device.
> > >> While working on supporting Appotech ax3003 based photoframes, which do
> > >> this I discovered that they will go of into lala land when ever they
> > >> see a READ_DISC_INFO scsi command.
> > >>
> > >> Thus this patch adds a scsi_device flag (which can then be set by the
> > >> usb-storage driver through an unsual-devs entry), to indicate this, and
> > >> makes the sr driver honor this flag.
> > >>
> > >> I know this sucks, but as discussed on linux-scsi list there is no other
> > >> way to make this device work properly.
> > >>
> > >> Looking at usb traces made under windows, windows never sends a
> > >> READ_DISC_INFO during normal interactions with a usb cdrom device. So as
> > >> this cdrom emulation thingie becomes more common we might see more of
> > >> this problem.
> > >
> > > So, I suppose we can do this.  I dislike threading device bugs like this
> > > up and down the stack.  The usb stor_control_thread already does some
> > > command filtering; if it just rejected this command and READ
> > > CAPACITY(16) on the flags, I think it would be far less code and the
> > > SCSI subsystem would just do the right thing.
> > >
> > > I'd actually like to hear from USB what they'd prefer to do.
> > >
> > 
> > I was sort of hoping that Alan Stern would reply to this, but maybe he is
> > on vacation?
> 
> No, I'm here.  I've been waiting to hear from Matt Dharm.  He's the 
> usb-storage maintainer, so it's his call.

Sorry, I've been travelling quite a lot lately.

> In general it's hard to know how command filtering at the usb-storage 
> level will interact with the higher SCSI layers.  In this case we have 
> James's assurance that it will be okay.

usb-storage is really just a command/data transport mechanism.  I've tried
to keep the command filtering to an absolute minimum.  In the past, we've
engaged in command filtering and it was a pretty big disaster in terms of
maintainance.

Basically, my theory for pushing this into the SCSI layers comes down to
these reasons:

1) A lot of these sorts of things only applies to one type of device or
another.  Either a specific type (CD-ROM, Random-Access, etc) or a specific
vendor.  It is much easier for SCSI layers to identify these devices than
usb-storage to do so.  In this specific case, we need to make a change only
to how CD-ROM devices work; usb-storage doesn't track the device type.

2) A lot of these bugs will re-appear in either sbp2 or UAS, both of which
use SCSI, so let's put all this in a common place rather than replicate it
multiple times.

3) With many of these things, what we are really trying to do is implement
some sort of sea-change in the command stream.  Consider devices which only
accept 6-byte commands instead of 10-byte commands.  Usb-storage could
intercept the 10-byte commands and either re-write all of them
(problematic, we've tried this before) or reject them in such a way as to
try to get the SCSI layers to generate the 6-byte variants (problematic, as
the SCSI folks could change their logic at any time, we've tried this
before).  It makes *way* more sense to just *tell* the SCSI layers what we
want, rather than do various forms of trickery to make it happen.

In short, after years of running around trying to make devices work, the
only reliable and maintainable way for doing this seems to be to make
commands originate correctly, rather than try to intercept the commands
in-flight and do some sort of fixup.

Matt

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

P:  Nine more messages in admin.policy.
M: I know, I'm typing as fast as I can!
					-- Pitr and Mike
User Friendly, 11/27/97

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

Re: [PATCH resend 1/4] scsi/sr: add no_read_disc_info scsi_device flag

[James Bottomley]

On Tue, 2010-08-03 at 09:37 -0700, Matthew Dharm wrote:
> On Tue, Aug 03, 2010 at 10:14:48AM -0400, Alan Stern wrote:
> > In general it's hard to know how command filtering at the usb-storage 
> > level will interact with the higher SCSI layers.  In this case we have 
> > James's assurance that it will be okay.
> 
> usb-storage is really just a command/data transport mechanism.  I've tried
> to keep the command filtering to an absolute minimum.  In the past, we've
> engaged in command filtering and it was a pretty big disaster in terms of
> maintainance.
> 
> Basically, my theory for pushing this into the SCSI layers comes down to
> these reasons:
> 
> 1) A lot of these sorts of things only applies to one type of device or
> another.  Either a specific type (CD-ROM, Random-Access, etc) or a specific
> vendor.  It is much easier for SCSI layers to identify these devices than
> usb-storage to do so.  In this specific case, we need to make a change only
> to how CD-ROM devices work; usb-storage doesn't track the device type.
> 
> 2) A lot of these bugs will re-appear in either sbp2 or UAS, both of which
> use SCSI, so let's put all this in a common place rather than replicate it
> multiple times.
> 
> 3) With many of these things, what we are really trying to do is implement
> some sort of sea-change in the command stream.  Consider devices which only
> accept 6-byte commands instead of 10-byte commands.  Usb-storage could
> intercept the 10-byte commands and either re-write all of them
> (problematic, we've tried this before) or reject them in such a way as to
> try to get the SCSI layers to generate the 6-byte variants (problematic, as
> the SCSI folks could change their logic at any time, we've tried this
> before).  It makes *way* more sense to just *tell* the SCSI layers what we
> want, rather than do various forms of trickery to make it happen.
> 
> In short, after years of running around trying to make devices work, the
> only reliable and maintainable way for doing this seems to be to make
> commands originate correctly, rather than try to intercept the commands
> in-flight and do some sort of fixup.

This is two devices, both identified by their usb (not SCSI) strings.
The problem is they crash rather than returning illegal command on two
specific SCSI commands.  I don't see the problem in having the USB
command filter in usb_stor_control_thread returning this on their
behalf.  SCSI will do the right thing and it's a lot less fragile.

It will also prevent the user space tools ... which no amount of SCSI
init changes can fix ... from crashing the devices.

James

Re: [PATCH resend 1/4] scsi/sr: add no_read_disc_info scsi_device flag

[Matthew Dharm]

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

On Tue, Aug 03, 2010 at 11:48:33AM -0500, James Bottomley wrote:
> This is two devices, both identified by their usb (not SCSI) strings.
> The problem is they crash rather than returning illegal command on two
> specific SCSI commands.  I don't see the problem in having the USB
> command filter in usb_stor_control_thread returning this on their
> behalf.  SCSI will do the right thing and it's a lot less fragile.

I'm willing to bet there are more devices out there like this.  Experience
has shown that the more we make the stack issue commands to the device like
one of the "popular" OSes, the fewer problems we have.  Thus, I prefer to
fix these where commands originate.

Also, every time I've tried to take the "filtering" approach in the past,
I've been burned 6-10 months later by a change in the SCSI stack which
brings these sorts of bugs back.  I really prefer these things to stay
fixed.

You have to admit, there is something ill-conceived about a driver which is
basically an HBA trying to second-guess code which actually generates the
commands in the first place.  If we can generate the command stream
"better" (i.e. more compatible with a wider variety of devices), then
everyone benefits.

> It will also prevent the user space tools ... which no amount of SCSI
> init changes can fix ... from crashing the devices.

Granted, this is true.  That said, user-space crashes have very rarely been
an issue in the past.  It does happen, but almost never due to this sort of
thing -- in those cases, the device chokes on something relatively subtle
about the command, which usb-storage would have a very very difficult time
filtering on.

Matt

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

DP: And judging from the scores, Stef has the sma...  
T:  LET'S NOT GO THERE!
					-- Dust Puppy and Tanya
User Friendly, 12/11/1997

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

Re: [PATCH resend 1/4] scsi/sr: add no_read_disc_info scsi_device flag

[Alan Stern]

On Tue, 3 Aug 2010, Matthew Dharm wrote:

> I'm willing to bet there are more devices out there like this.  Experience
> has shown that the more we make the stack issue commands to the device like
> one of the "popular" OSes, the fewer problems we have.  Thus, I prefer to
> fix these where commands originate.

This is a good point.  Since Windows apparently never sends 
READ_DISC_INFO commands, we court trouble by using those commands in 
the cdrom driver.  Is there any way to avoid using them?

As far as the READ-CAPACITY(16) bug, there may be a simpler fix.  In
sd_read_capacity(), change

	if (sdp->fix_capacity ||

to

	if ((sdp->fix_capacity && sdkp->capacity > 0) ||

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH resend 1/4] scsi/sr: add no_read_disc_info scsi_device flag

[Hans de Goede]

Hi,

On 08/04/2010 04:41 PM, Alan Stern wrote:
> On Tue, 3 Aug 2010, Matthew Dharm wrote:
>
>> I'm willing to bet there are more devices out there like this.  Experience
>> has shown that the more we make the stack issue commands to the device like
>> one of the "popular" OSes, the fewer problems we have.  Thus, I prefer to
>> fix these where commands originate.
>
> This is a good point.  Since Windows apparently never sends
> READ_DISC_INFO commands, we court trouble by using those commands in
> the cdrom driver.  Is there any way to avoid using them?
>
> As far as the READ-CAPACITY(16) bug, there may be a simpler fix.  In
> sd_read_capacity(), change
>
> 	if (sdp->fix_capacity ||
>
> to
>
> 	if ((sdp->fix_capacity&&  sdkp->capacity>  0) ||

That won't work, the trouble happens before fix_capacity comes into play. The
overflow is happening inside the mp3 player. When there is no card in the slot
the mp3 player tries to report a size of 0. But as scsi get_capacity uses
the last sector number, the mp3 player returns its internal size variable -1,
resulting in it returning 0xffffffff. This then gets increased by 1 by
read_capacity_10 to 0x100000000, which triggers the following bit:

                 if ((sizeof(sdkp->capacity) > 4) &&
                     (sdkp->capacity > 0xffffffffULL)) {
                         int old_sector_size = sector_size;
                         sd_printk(KERN_NOTICE, sdkp, "Very big device. "
                                         "Trying to use READ CAPACITY(16).\n");
                         sector_size = read_capacity_16(sdkp, sdp, buffer);

This issues a READ CAPACITY(16) and the mp3 player dies (until reset). Also
notice that my sd.c patch for this does more then just stop sd.c from
sending READ CAPACITY(16), it also turns a READ CAPACITY(10) answer
of 0xffffffff into 0 instead of 0x100000000 when the quirk flag is set.


Regards,

Hans

Re: [PATCH resend 1/4] scsi/sr: add no_read_disc_info scsi_device flag

[Alan Stern]

On Wed, 4 Aug 2010, Hans de Goede wrote:

> Hi,
> 
> On 08/04/2010 04:41 PM, Alan Stern wrote:
> > On Tue, 3 Aug 2010, Matthew Dharm wrote:
> >
> >> I'm willing to bet there are more devices out there like this.  Experience
> >> has shown that the more we make the stack issue commands to the device like
> >> one of the "popular" OSes, the fewer problems we have.  Thus, I prefer to
> >> fix these where commands originate.
> >
> > This is a good point.  Since Windows apparently never sends
> > READ_DISC_INFO commands, we court trouble by using those commands in
> > the cdrom driver.  Is there any way to avoid using them?
> >
> > As far as the READ-CAPACITY(16) bug, there may be a simpler fix.  In
> > sd_read_capacity(), change
> >
> > 	if (sdp->fix_capacity ||
> >
> > to
> >
> > 	if ((sdp->fix_capacity&&  sdkp->capacity>  0) ||
> 
> That won't work, the trouble happens before fix_capacity comes into play. The
> overflow is happening inside the mp3 player. When there is no card in the slot
> the mp3 player tries to report a size of 0. But as scsi get_capacity uses
> the last sector number, the mp3 player returns its internal size variable -1,
> resulting in it returning 0xffffffff. This then gets increased by 1 by
> read_capacity_10 to 0x100000000, which triggers the following bit:
> 
>                  if ((sizeof(sdkp->capacity) > 4) &&
>                      (sdkp->capacity > 0xffffffffULL)) {
>                          int old_sector_size = sector_size;
>                          sd_printk(KERN_NOTICE, sdkp, "Very big device. "
>                                          "Trying to use READ CAPACITY(16).\n");
>                          sector_size = read_capacity_16(sdkp, sdp, buffer);
> 
> This issues a READ CAPACITY(16) and the mp3 player dies (until reset). Also
> notice that my sd.c patch for this does more then just stop sd.c from
> sending READ CAPACITY(16), it also turns a READ CAPACITY(10) answer
> of 0xffffffff into 0 instead of 0x100000000 when the quirk flag is set.

Okay, so much for that bright idea.

On the other hand, it's clear that the sd driver doesn't even try to
issue a READ CAPACITY command if there's no medium present.  How come
this is happening at all?  Does the device report that a card is
present even when the slot is empty?  Or does it claim not to have
removable media in the first place?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH resend 1/4] scsi/sr: add no_read_disc_info scsi_device flag

[Hans de Goede]

Hi,

On 08/04/2010 05:25 PM, Alan Stern wrote:
> On Wed, 4 Aug 2010, Hans de Goede wrote:
>
>> Hi,
>>
>> On 08/04/2010 04:41 PM, Alan Stern wrote:
>>> On Tue, 3 Aug 2010, Matthew Dharm wrote:
>>>
>>>> I'm willing to bet there are more devices out there like this.  Experience
>>>> has shown that the more we make the stack issue commands to the device like
>>>> one of the "popular" OSes, the fewer problems we have.  Thus, I prefer to
>>>> fix these where commands originate.
>>>
>>> This is a good point.  Since Windows apparently never sends
>>> READ_DISC_INFO commands, we court trouble by using those commands in
>>> the cdrom driver.  Is there any way to avoid using them?
>>>
>>> As far as the READ-CAPACITY(16) bug, there may be a simpler fix.  In
>>> sd_read_capacity(), change
>>>
>>> 	if (sdp->fix_capacity ||
>>>
>>> to
>>>
>>> 	if ((sdp->fix_capacity&&   sdkp->capacity>   0) ||
>>
>> That won't work, the trouble happens before fix_capacity comes into play. The
>> overflow is happening inside the mp3 player. When there is no card in the slot
>> the mp3 player tries to report a size of 0. But as scsi get_capacity uses
>> the last sector number, the mp3 player returns its internal size variable -1,
>> resulting in it returning 0xffffffff. This then gets increased by 1 by
>> read_capacity_10 to 0x100000000, which triggers the following bit:
>>
>>                   if ((sizeof(sdkp->capacity)>  4)&&
>>                       (sdkp->capacity>  0xffffffffULL)) {
>>                           int old_sector_size = sector_size;
>>                           sd_printk(KERN_NOTICE, sdkp, "Very big device. "
>>                                           "Trying to use READ CAPACITY(16).\n");
>>                           sector_size = read_capacity_16(sdkp, sdp, buffer);
>>
>> This issues a READ CAPACITY(16) and the mp3 player dies (until reset). Also
>> notice that my sd.c patch for this does more then just stop sd.c from
>> sending READ CAPACITY(16), it also turns a READ CAPACITY(10) answer
>> of 0xffffffff into 0 instead of 0x100000000 when the quirk flag is set.
>
> Okay, so much for that bright idea.
>
> On the other hand, it's clear that the sd driver doesn't even try to
> issue a READ CAPACITY command if there's no medium present.  How come
> this is happening at all?  Does the device report that a card is
> present even when the slot is empty?  Or does it claim not to have
> removable media in the first place?
>

I have not investigated, but this is not something which we can easily fix
with a quirk AFAIK, as some of the LUN's of the device are not removable
and others are, and we don't know if there is a card present or not.

More over I guess (but have not verified) the answer is:
It claims not to have removable media in the first place

Regards,

Hans

[PATCH resend 1/4] scsi/sr: add no_read_disc_info scsi_device flag

[Hans de Goede]

Some USB devices emulate a usb-mass-storage attached (scsi) cdrom device,
usually this fake cdrom contains the windows software for the device.
While working on supporting Appotech ax3003 based photoframes, which do
this I discovered that they will go of into lala land when ever they
see a READ_DISC_INFO scsi command.

Thus this patch adds a scsi_device flag (which can then be set by the
usb-storage driver through an unsual-devs entry), to indicate this, and
makes the sr driver honor this flag.

I know this sucks, but as discussed on linux-scsi list there is no other
way to make this device work properly.

Looking at usb traces made under windows, windows never sends a
READ_DISC_INFO during normal interactions with a usb cdrom device. So as
this cdrom emulation thingie becomes more common we might see more of
this problem.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/scsi/sr.c          |    8 +++++++-
 include/scsi/scsi_device.h |    1 +
 2 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 0a90abc..18077b5 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -849,10 +849,16 @@ static void get_capabilities(struct scsi_cd *cd)
 static int sr_packet(struct cdrom_device_info *cdi,
 		struct packet_command *cgc)
 {
+	struct scsi_cd *cd = cdi->handle;
+	struct scsi_device *sdev = cd->device;
+
+	if (cgc->cmd[0] == GPCMD_READ_DISC_INFO && sdev->no_read_disc_info)
+		return -EDRIVE_CANT_DO_THIS;
+
 	if (cgc->timeout <= 0)
 		cgc->timeout = IOCTL_TIMEOUT;
 
-	sr_do_ioctl(cdi->handle, cgc);
+	sr_do_ioctl(cd, cgc);
 
 	return cgc->stat;
 }
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index d80b6db..e1b2db8 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -148,6 +148,7 @@ struct scsi_device {
 	unsigned retry_hwerror:1;	/* Retry HARDWARE_ERROR */
 	unsigned last_sector_bug:1;	/* do not use multisector accesses on
 					   SD_LAST_BUGGY_SECTORS */
+	unsigned no_read_disc_info:1;	/* Avoid READ_DISC_INFO cmds */
 	unsigned is_visible:1;	/* is the device visible in sysfs */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
-- 
1.7.0.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html