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