[PATCH 7/8] don't mangle INQUIRY if cmddt or evpd bits are set

[PATCH 7/8] don't mangle INQUIRY if cmddt or evpd bits are set

[Al Viro]

Date: 1139425740 -0500

sbp2.c mangles INQUIRY response in a way that only applies to standard
inquiry data (i.e. when both cmddt and evpd bits are 0).  Leave other cases
alone; e.g. when asking for VPD the length of reply is in byte 3, not 4
and byte 4 is the first byte of device serial number.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

---

 drivers/ieee1394/sbp2.c |   10 +---------
 1 files changed, 1 insertions(+), 9 deletions(-)

91d2006e216b5205a4fd076b73985a2f643c33e3
diff --git a/drivers/ieee1394/sbp2.c b/drivers/ieee1394/sbp2.c
index 18d7eda..c2c776f 100644
--- a/drivers/ieee1394/sbp2.c
+++ b/drivers/ieee1394/sbp2.c
@@ -2082,9 +2082,7 @@ static void sbp2_check_sbp2_response(str
 
 	SBP2_DEBUG("sbp2_check_sbp2_response");
 
-	switch (SCpnt->cmnd[0]) {
-
-	case INQUIRY:
+	if (SCpnt->cmnd[0] == INQUIRY && (SCpnt->cmnd[1] & 3) == 0) {
 		/*
 		 * Make sure data length is ok. Minimum length is 36 bytes
 		 */
@@ -2097,13 +2095,7 @@ static void sbp2_check_sbp2_response(str
 		 */
 		scsi_buf[2] |= 2;
 		scsi_buf[3] = (scsi_buf[3] & 0xf0) | 2;
-
-		break;
-
-	default:
-		break;
 	}
-	return;
 }
 
 /*
-- 
0.99.9.GIT

Re: [PATCH 7/8] don't mangle INQUIRY if cmddt or evpd bits are set

[Stefan Richter]

Al Viro wrote:
> Date: 1139425740 -0500
> 
> sbp2.c mangles INQUIRY response in a way that only applies to standard
> inquiry data (i.e. when both cmddt and evpd bits are 0).  Leave other cases
> alone; e.g. when asking for VPD the length of reply is in byte 3, not 4
> and byte 4 is the first byte of device serial number.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> 
> ---
> 

I tested the patch with 8 different SBP-2 bridges, based on 6 or 7 
different bridge chips. Works for me.

In fact, not a single one of these bridges is affected by the code 
change since the additional expression which was added always evaluates 
true.

...
> -	switch (SCpnt->cmnd[0]) {
> -
> -	case INQUIRY:
> +	if (SCpnt->cmnd[0] == INQUIRY && (SCpnt->cmnd[1] & 3) == 0) {
>  		/*
>  		 * Make sure data length is ok. Minimum length is 36 bytes
>  		 */
...

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

Re: [PATCH 7/8] don't mangle INQUIRY if cmddt or evpd bits are set

[Al Viro]

On Wed, Feb 08, 2006 at 11:35:18PM +0100, Stefan Richter wrote:
> I tested the patch with 8 different SBP-2 bridges, based on 6 or 7 
> different bridge chips. Works for me.
> 
> In fact, not a single one of these bridges is affected by the code 
> change since the additional expression which was added always evaluates 
> true.

The hell it does.  Try scsiinfo -s and you'll see.  All INQUIRY generated
by scsi midlayer have both flags set to 0.  Userland ones do not; example
I've mentioned (scsiinfo -s) will send an INQUIRY with EVPD=1 and page
code 0x80 (that's cmnd[2]), which results in response of form
	(periph_qualifier << 5) | device_type
	0x80
	<reserved>
	page length
	unit serial number (page length - 3 bytes)

Similar for page 0x83 (device identification descriptors), etc.  Userland
gets to those via SG_IO and yes, it's really used.

Re: [PATCH 7/8] don't mangle INQUIRY if cmddt or evpd bits are set

[Stefan Richter]

Al Viro wrote:
> On Wed, Feb 08, 2006 at 11:35:18PM +0100, Stefan Richter wrote:
>>In fact, not a single one of these bridges is affected by the code 
>>change since the additional expression which was added always evaluates 
>>true.
> 
> The hell it does.  Try scsiinfo -s and you'll see.  All INQUIRY generated
> by scsi midlayer have both flags set to 0.  Userland ones do not;

Ah, I see. Of course I didn't test that.
-- 
Stefan Richter
-=====-=-==- --=- -=--=
http://arcgraph.de/sr/

Re: [PATCH 7/8] don't mangle INQUIRY if cmddt or evpd bits are set

[Stefan Richter]

Al Viro wrote on 2006-02-09:
> On Wed, Feb 08, 2006 at 11:35:18PM +0100, Stefan Richter wrote:
...
>>not a single one of these bridges is affected by the code 
>>change since the additional expression which was added always evaluates 
>>true.
> 
> The hell it does.  Try scsiinfo -s and you'll see.  All INQUIRY generated
> by scsi midlayer have both flags set to 0.  Userland ones do not;
...

OK, tested scsiinfo now with 9 bridges (8 or 7 different chips).
The patch obviously works as expected.

Jody, are you going to channel the patch through your tree?

BTW, a Prolific based enclosure indeed seems to be unable to handle
CD-ROMs after scsiinfo treatment. An enclosure based on an old
revision of TI StorageLynx apparently causes mode_sense -> check
condition/ unit attention loops when scsiinfo tries to access some
mode page.
-- 
Stefan Richter
-=====-=-==- --=- -==-=
http://arcgraph.de/sr/

Re: [PATCH 7/8] don't mangle INQUIRY if cmddt or evpd bits are set

[Jody McIntyre]

On Mon, Feb 13, 2006 at 05:19:55PM +0100, Stefan Richter wrote:
> 
> Jody, are you going to channel the patch through your tree?

It's part of an 8-patch series, so generally this would go through
somewhere more general like linux-scsi that can take the whole series.

I can take it if necessary though - let me know if so.

Cheers,
Jody

> BTW, a Prolific based enclosure indeed seems to be unable to handle
> CD-ROMs after scsiinfo treatment. An enclosure based on an old
> revision of TI StorageLynx apparently causes mode_sense -> check
> condition/ unit attention loops when scsiinfo tries to access some
> mode page.
> -- 
> Stefan Richter
> -=====-=-==- --=- -==-=
> http://arcgraph.de/sr/

-- 

Re: [PATCH 7/8] don't mangle INQUIRY if cmddt or evpd bits are set

[Al Viro]

On Mon, Feb 13, 2006 at 05:19:55PM +0100, Stefan Richter wrote:
> OK, tested scsiinfo now with 9 bridges (8 or 7 different chips).
> The patch obviously works as expected.
> 
> Jody, are you going to channel the patch through your tree?
> 
> BTW, a Prolific based enclosure indeed seems to be unable to handle
> CD-ROMs after scsiinfo treatment. An enclosure based on an old
> revision of TI StorageLynx apparently causes mode_sense -> check
> condition/ unit attention loops when scsiinfo tries to access some
> mode page.

The former is best treated by using the hardware in question as a pissuary,
to make sure that its contents matches the quality of design.  The latter
might be more interesting - RBC devices are only required to implement
MODE SENSE/SELECT page 6; they shouldn't get messed by the rest, but at
least some of them blindly respond with page 6 to _every_ MODE SENSE.
So that might be a good reason to blacklist.

Re: [PATCH 7/8] don't mangle INQUIRY if cmddt or evpd bits are set

[Stefan Richter]

Al Viro wrote:
> On Mon, Feb 13, 2006 at 05:19:55PM +0100, Stefan Richter wrote:
>>BTW, a Prolific based enclosure indeed seems to be unable to handle
>>CD-ROMs after scsiinfo treatment. An enclosure based on an old
>>revision of TI StorageLynx apparently causes mode_sense -> check
>>condition/ unit attention loops when scsiinfo tries to access some
>>mode page.
> 
> The former is best treated by using the hardware in question as a pissuary,
> to make sure that its contents matches the quality of design.

I have got the impression that most of IEEE 1394a/ USB 2.0 combo bridges 
on the market are now based on Prolific chips.

> The latter
> might be more interesting - RBC devices are only required to implement
> MODE SENSE/SELECT page 6; they shouldn't get messed by the rest, but at
> least some of them blindly respond with page 6 to _every_ MODE SENSE.
> So that might be a good reason to blacklist.

Some more findings:
  - The TI StorageLynx based bridge reports device type 0 (TYPE_DISK).
    The problem occurs apparently with page 4 and page 8. Sbp2 has a
    fix since yesterday which sets the skip_ms_page_8 flag.
    http://marc.theaimsgroup.com/?l=linux1394-devel&m=113969287630893
  - Another bridge made by the same manufacturer but based on TI
    StorageLynx revision A features the same MODE SENSE bug. This bridge
    reports type 14 (TYPE_RBC).
  - I tested a tenth bridge now, based on Initio INIC-2430F. The bridge
    reports TYPE_DISK and seems to support all pages which scsiinfo is
    interested in. Sd_mod is a different story: After sd_mod accesses
    page 8, the kernel panics. (This is discussed in another thread. The
    mentioned sbp2 patch catches this bridge as a skip_ms_page_8
    candidate too, thus avoids the panic. I will eventually check what
    sd_mod is doing; the sbp2 patch is not the real fix.)

Of course sg does not care for any black list flags (like sd_mod and 
sr_mod do), but considering the nature of the bugs and anticipated usage 
of affected devices, there is hardly a reason for further safeguards in 
sbp2, let alone sg.
-- 
Stefan Richter
-=====-=-==- --=- -==-=
http://arcgraph.de/sr/

Re: [PATCH 7/8] don't mangle INQUIRY if cmddt or evpd bits are set

[Al Viro]

On Mon, Feb 13, 2006 at 09:28:17PM +0100, Stefan Richter wrote:
> Al Viro wrote:
> >On Mon, Feb 13, 2006 at 05:19:55PM +0100, Stefan Richter wrote:
> >>BTW, a Prolific based enclosure indeed seems to be unable to handle
> >>CD-ROMs after scsiinfo treatment. An enclosure based on an old
> >>revision of TI StorageLynx apparently causes mode_sense -> check
> >>condition/ unit attention loops when scsiinfo tries to access some
> >>mode page.
> >
> >The former is best treated by using the hardware in question as a pissuary,
> >to make sure that its contents matches the quality of design.
> 
> I have got the impression that most of IEEE 1394a/ USB 2.0 combo bridges 
> on the market are now based on Prolific chips.

Unfortunately.  Finding OXFW911-based ones for about the same price is still
not hard...

> Some more findings:
>  - The TI StorageLynx based bridge reports device type 0 (TYPE_DISK).
>    The problem occurs apparently with page 4 and page 8. Sbp2 has a
>    fix since yesterday which sets the skip_ms_page_8 flag.

That's going to cause fun problems on reboot if it actually has write-behind
cache...

>    http://marc.theaimsgroup.com/?l=linux1394-devel&m=113969287630893
>  - Another bridge made by the same manufacturer but based on TI
>    StorageLynx revision A features the same MODE SENSE bug. This bridge
>    reports type 14 (TYPE_RBC).

Pardon?  If it's type 14, we won't issue MODE SENSE for page 8 and will
go for page 6 instead...

>  - I tested a tenth bridge now, based on Initio INIC-2430F. The bridge
>    reports TYPE_DISK and seems to support all pages which scsiinfo is
>    interested in. Sd_mod is a different story: After sd_mod accesses
>    page 8, the kernel panics. (This is discussed in another thread. The
>    mentioned sbp2 patch catches this bridge as a skip_ms_page_8
>    candidate too, thus avoids the panic. I will eventually check what
>    sd_mod is doing; the sbp2 patch is not the real fix.)

sd_mod issues MODE SENSE from sd_read_cache_size() and does that twice -
once for minimal length to get the length device wants to give and then
for actual length.

> Of course sg does not care for any black list flags (like sd_mod and 
> sr_mod do), but considering the nature of the bugs and anticipated usage 
> of affected devices, there is hardly a reason for further safeguards in 
> sbp2, let alone sg.

Maybe, maybe not.  Note that e.g. aforementioned INQUIRY bug in pl3507 is
triggered by dmraid, which works via SG_IO, just as scsiinfo.  And unlike
scsiinfo it's run from /etc/rc.sysinit on current FC4...

Re: [PATCH 7/8] don't mangle INQUIRY if cmddt or evpd bits are set

[Stefan Richter]

Al Viro wrote:
> On Mon, Feb 13, 2006 at 09:28:17PM +0100, Stefan Richter wrote:
...
>> - The TI StorageLynx based bridge reports device type 0 (TYPE_DISK).
>>   The problem occurs apparently with page 4 and page 8. Sbp2 has a
>>   fix since yesterday which sets the skip_ms_page_8 flag.
> 
> That's going to cause fun problems on reboot if it actually has write-behind
> cache...

Not only on reboot but always when sd is told to shut down. I did not 
notice an actual problem so far but I will keep an eye on it.

But AFAIU, sd's cache syncing (of devices with WCE set) is ineffective 
anyway if devices are unplugged without manually shutting the driver 
down beforehand.

>>   http://marc.theaimsgroup.com/?l=linux1394-devel&m=113969287630893
>> - Another bridge made by the same manufacturer but based on TI
>>   StorageLynx revision A features the same MODE SENSE bug. This bridge
>>   reports type 14 (TYPE_RBC).
> 
> Pardon?  If it's type 14, we won't issue MODE SENSE for page 8 and will
> go for page 6 instead...

Correct. Which is why I did not notice the bug until testing with scsiinfo.

...
>>Of course sg does not care for any black list flags (like sd_mod and 
>>sr_mod do), but considering the nature of the bugs and anticipated usage 
>>of affected devices, there is hardly a reason for further safeguards in 
>>sbp2, let alone sg.
> 
> Maybe, maybe not.  Note that e.g. aforementioned INQUIRY bug in pl3507 is
> triggered by dmraid, which works via SG_IO, just as scsiinfo.  And unlike
> scsiinfo it's run from /etc/rc.sysinit on current FC4...

Are they probing all devices or only those which are part of a RAID set?
-- 
Stefan Richter
-=====-=-==- --=- -===-
http://arcgraph.de/sr/