| Message ID | 20220302053559.32147-4-martin.petersen@oracle.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [01/14] scsi: mpt3sas: Use cached ATA Information VPD page | expand |
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
On 3/1/22 21:35, Martin K. Petersen wrote: > Low-level device drivers have had the ability to limit the size of an > INQUIRY for many years. This made sense for a wide variety of legacy > devices. However, we are unnecessarily truncating the INQUIRY response > for many modern devices. This prevents us from consulting fields > beyond the first 36 bytes. > > If a device reports that it supports a larger INQUIRY response, and > the device also reports that it implements SPC-4 or newer, allow the > larger INQUIRY to proceed. > > Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com> > --- > drivers/scsi/scsi_scan.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c > index f4e6c68ac99e..95bf9a1f35ce 100644 > --- a/drivers/scsi/scsi_scan.c > +++ b/drivers/scsi/scsi_scan.c > @@ -728,7 +728,17 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result, > if (pass == 1) { > if (BLIST_INQUIRY_36 & *bflags) > next_inquiry_len = 36; > - else if (sdev->inquiry_len) > + /* > + * LLD specified a maximum sdev->inquiry_len > + * but device claims it has more data. Capping > + * the length only makes sense for legacy > + * devices. If a device supports SPC-4 (2014) > + * or newer, assume that it is safe to ask for > + * as much as the device says it supports. > + */ > + else if (sdev->inquiry_len && > + response_len > sdev->inquiry_len && > + (inq_result[2] & 0x7) < 6) /* SPC-4 */ > next_inquiry_len = sdev->inquiry_len; > else > next_inquiry_len = response_len; Hi Martin, Do the benefits of this change outweigh the additional complexity introduced by this code and the risk of breaking support for certain devices? I'm asking this because the number of LLDs that sets inquiry_len is small: $ git grep -nH '>inquiry_len[[:blank:]]*=[[:blank:]]'|grep -v scsi_scan drivers/firewire/sbp2.c:1508: sdev->inquiry_len = 36; drivers/staging/rts5208/rtsx.c:67: sdev->inquiry_len = 36; drivers/usb/image/microtek.c:323: s->inquiry_len = 0x24; drivers/usb/storage/scsiglue.c:77: sdev->inquiry_len = 36; Does any of these LLDs support SPC-4 devices? Can this change e.g. break support for certain USB sticks? Thanks, Bart.
Bart, > Do the benefits of this change outweigh the additional complexity > introduced by this code and the risk of breaking support for certain > devices? I'm asking this because the number of LLDs that sets > inquiry_len is small: > > $ git grep -nH '>inquiry_len[[:blank:]]*=[[:blank:]]'|grep -v scsi_scan > drivers/firewire/sbp2.c:1508: sdev->inquiry_len = 36; > drivers/staging/rts5208/rtsx.c:67: sdev->inquiry_len = 36; > drivers/usb/image/microtek.c:323: s->inquiry_len = 0x24; > drivers/usb/storage/scsiglue.c:77: sdev->inquiry_len = 36; > > Does any of these LLDs support SPC-4 devices? Can this change > e.g. break support for certain USB sticks? The intent is to enable more modern protocol features on devices attached using USB. I was concerned about blindly increasingly the inquiry length and risk breaking older devices that we know are likely to have problems in this department. At the same time the conservative clamp on the inquiry length prevents several useful features from being enabled on modern devices. There is a risk associated with any change. This patch tries to strike a reasonable balance between avoiding regressions and accommodating modern USB SSDs.
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index f4e6c68ac99e..95bf9a1f35ce 100644 --- a/drivers/scsi/scsi_scan.c +++ b/drivers/scsi/scsi_scan.c @@ -728,7 +728,17 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result, if (pass == 1) { if (BLIST_INQUIRY_36 & *bflags) next_inquiry_len = 36; - else if (sdev->inquiry_len) + /* + * LLD specified a maximum sdev->inquiry_len + * but device claims it has more data. Capping + * the length only makes sense for legacy + * devices. If a device supports SPC-4 (2014) + * or newer, assume that it is safe to ask for + * as much as the device says it supports. + */ + else if (sdev->inquiry_len && + response_len > sdev->inquiry_len && + (inq_result[2] & 0x7) < 6) /* SPC-4 */ next_inquiry_len = sdev->inquiry_len; else next_inquiry_len = response_len;
Low-level device drivers have had the ability to limit the size of an INQUIRY for many years. This made sense for a wide variety of legacy devices. However, we are unnecessarily truncating the INQUIRY response for many modern devices. This prevents us from consulting fields beyond the first 36 bytes. If a device reports that it supports a larger INQUIRY response, and the device also reports that it implements SPC-4 or newer, allow the larger INQUIRY to proceed. Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com> --- drivers/scsi/scsi_scan.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)