[PATCH][SCS] sd: Read Capacity if (16) fails

[PATCH][SCS] sd: Read Capacity if (16) fails

[Hugh Dickins]

Commit e0597d70012c82e16ee152270a55d89d8bf66693 (sd: Identify DIF protection
type and application tag ownership) says that if a disk is formatted with
Inquiry bit PROTECT=1, it is required to support Read Capacity(16).  But my
SD cards, accessed by builtin cardreader and generic USB storage, disagree.

Therefore fall back to the familiar Read Capacity if Read Capacity(16) fails:
without even showing the "failed" message since I expect this will be common.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---
Or is USB storage missing something to support Read Capacity(16)?

 drivers/scsi/sd.c |    6 ++++++
 1 file changed, 6 insertions(+)

--- 2.6.27-rc2/drivers/scsi/sd.c	2008-07-29 04:24:38.000000000 +0100
+++ linux/drivers/scsi/sd.c	2008-08-06 08:53:13.000000000 +0100
@@ -1287,6 +1287,7 @@ sd_read_capacity(struct scsi_disk *sdkp,
 	int sector_size = 0;
 	/* Force READ CAPACITY(16) when PROTECT=1 */
 	int longrc = scsi_device_protection(sdkp->device) ? 1 : 0;
+	int tried_both = 0;
 	struct scsi_sense_hdr sshdr;
 	int sense_valid = 0;
 	struct scsi_device *sdp = sdkp->device;
@@ -1341,6 +1342,10 @@ repeat:
 		return;
 	} else if (the_result && longrc) {
 		/* READ CAPACITY(16) has been failed */
+		if (!tried_both++) {
+			longrc = 0;
+			goto repeat;
+		}
 		sd_printk(KERN_NOTICE, sdkp, "READ CAPACITY(16) failed\n");
 		sd_print_result(sdkp, the_result);
 		sd_printk(KERN_NOTICE, sdkp, "Use 0xffffffff as device size\n");
@@ -1357,6 +1362,7 @@ repeat:
 			if(sizeof(sdkp->capacity) > 4) {
 				sd_printk(KERN_NOTICE, sdkp, "Very big device. "
 					  "Trying to use READ CAPACITY(16).\n");
+				tried_both++;
 				longrc = 1;
 				goto repeat;
 			}

Re: [PATCH][SCS] sd: Read Capacity if (16) fails

[Matthew Wilcox]

On Wed, Aug 06, 2008 at 03:06:21PM +0100, Hugh Dickins wrote:
> Commit e0597d70012c82e16ee152270a55d89d8bf66693 (sd: Identify DIF protection
> type and application tag ownership) says that if a disk is formatted with
> Inquiry bit PROTECT=1, it is required to support Read Capacity(16).  But my
> SD cards, accessed by builtin cardreader and generic USB storage, disagree.
> 
> Therefore fall back to the familiar Read Capacity if Read Capacity(16) fails:
> without even showing the "failed" message since I expect this will be common.

How about we flip it around?  Unconditionally try READ CAPACITY 16 first,
then if that fails, try READ CAPACITY?  I suppose there's always the
possibility that a drive will go tits-up if it receives the RC16
command, so maybe we'll need a blacklist.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH][SCS] sd: Read Capacity if (16) fails

[Martin K. Petersen]

>>>>> "Hugh" == Hugh Dickins <hugh@veritas.com> writes:

Hugh> Commit e0597d70012c82e16ee152270a55d89d8bf66693 (sd: Identify
Hugh> DIF protection type and application tag ownership) says that if
Hugh> a disk is formatted with Inquiry bit PROTECT=1, it is required
Hugh> to support Read Capacity(16).  But my SD cards, accessed by
Hugh> builtin cardreader and generic USB storage, disagree.

Argh.  That's really broken.  USB storage has no business setting
PROTECT.

Can you send me the sg_inq output so I can see what else they are
returning?

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH][SCS] sd: Read Capacity if (16) fails

[Hugh Dickins]

On Wed, 6 Aug 2008, Martin K. Petersen wrote:
> >>>>> "Hugh" == Hugh Dickins <hugh@veritas.com> writes:
> 
> Hugh> Commit e0597d70012c82e16ee152270a55d89d8bf66693 (sd: Identify
> Hugh> DIF protection type and application tag ownership) says that if
> Hugh> a disk is formatted with Inquiry bit PROTECT=1, it is required
> Hugh> to support Read Capacity(16).  But my SD cards, accessed by
> Hugh> builtin cardreader and generic USB storage, disagree.
> 
> Argh.  That's really broken.  USB storage has no business setting
> PROTECT.
> 
> Can you send me the sg_inq output so I can see what else they are
> returning?

sg_inq?  I assume that's the same as the inquiry_len bytes at inquiry:

00000000: 00 8d 00 01 1f 01 00 00 47 65 6e 65 72 69 63 2d  ........Generic-
00000010: 4d 75 6c 74 69 2d 43 61 72 64 20 20 20 20 20 20  Multi-Card      
00000020: 31 2e 30 30                                      1.00

Ah, sg_inq is a command which spells that out, here you go:

standard INQUIRY:
  PQual=0  Device_type=0  RMB=1  version=0x00  [no conformance claimed]
  [AERC=0]  [TrmTsk=0]  NormACA=0  HiSUP=0  Resp_data_format=1
  SCCS=0  ACC=0  TGPS=0  3PC=0  Protect=1  BQue=0
  EncServ=0  MultiP=0  [MChngr=0]  [ACKREQQ=0]  Addr16=0
  [RelAdr=0]  WBus16=0  Sync=0  Linked=0  [TranDis=0]  CmdQue=0
    length=36 (0x24)   Peripheral device type: disk
 Vendor identification: Generic-
 Product identification: Multi-Card      
 Product revision level: 1.00

Hugh

Re: [PATCH][SCS] sd: Read Capacity if (16) fails

[James Bottomley]

On Wed, 2008-08-06 at 08:25 -0600, Matthew Wilcox wrote:
> On Wed, Aug 06, 2008 at 03:06:21PM +0100, Hugh Dickins wrote:
> > Commit e0597d70012c82e16ee152270a55d89d8bf66693 (sd: Identify DIF protection
> > type and application tag ownership) says that if a disk is formatted with
> > Inquiry bit PROTECT=1, it is required to support Read Capacity(16).  But my
> > SD cards, accessed by builtin cardreader and generic USB storage, disagree.
> > 
> > Therefore fall back to the familiar Read Capacity if Read Capacity(16) fails:
> > without even showing the "failed" message since I expect this will be common.
> 
> How about we flip it around?  Unconditionally try READ CAPACITY 16 first,
> then if that fails, try READ CAPACITY?  I suppose there's always the
> possibility that a drive will go tits-up if it receives the RC16
> command, so maybe we'll need a blacklist.

I don't think so ... the read capacity logic looks the way it does
because we had a bit of trouble with USB devices simply going out to
lunch on READ_CAPACITY(16) ... otherwise we'd have done the 16 then
fallback to 10 ages ago.

The best way is probably a blacklist for protect ... I assume there's no
plans in the near future for USB to support it, so we could just turn it
off globally in USB slave configure.

James

Re: [PATCH][SCS] sd: Read Capacity if (16) fails

[James Bottomley]

On Wed, 2008-08-06 at 16:34 +0100, Hugh Dickins wrote:
>   PQual=0  Device_type=0  RMB=1  version=0x00  [no conformance
> claimed]

We could try using this as the trigger.  I assume all DIF supporting
devices must also claim conformance to SPC3/SBC2 or higher?

James

Re: [PATCH][SCS] sd: Read Capacity if (16) fails

[Douglas Gilbert]

Hugh Dickins wrote:
> On Wed, 6 Aug 2008, Martin K. Petersen wrote:
>>>>>>> "Hugh" == Hugh Dickins <hugh@veritas.com> writes:
>> Hugh> Commit e0597d70012c82e16ee152270a55d89d8bf66693 (sd: Identify
>> Hugh> DIF protection type and application tag ownership) says that if
>> Hugh> a disk is formatted with Inquiry bit PROTECT=1, it is required
>> Hugh> to support Read Capacity(16).  But my SD cards, accessed by
>> Hugh> builtin cardreader and generic USB storage, disagree.
>>
>> Argh.  That's really broken.  USB storage has no business setting
>> PROTECT.
>>
>> Can you send me the sg_inq output so I can see what else they are
>> returning?
> 
> sg_inq?  I assume that's the same as the inquiry_len bytes at inquiry:
> 
> 00000000: 00 8d 00 01 1f 01 00 00 47 65 6e 65 72 69 63 2d  ........Generic-
> 00000010: 4d 75 6c 74 69 2d 43 61 72 64 20 20 20 20 20 20  Multi-Card      
> 00000020: 31 2e 30 30                                      1.00
> 
> Ah, sg_inq is a command which spells that out, here you go:
> 
> standard INQUIRY:
>   PQual=0  Device_type=0  RMB=1  version=0x00  [no conformance claimed]
>   [AERC=0]  [TrmTsk=0]  NormACA=0  HiSUP=0  Resp_data_format=1
>   SCCS=0  ACC=0  TGPS=0  3PC=0  Protect=1  BQue=0
>   EncServ=0  MultiP=0  [MChngr=0]  [ACKREQQ=0]  Addr16=0
>   [RelAdr=0]  WBus16=0  Sync=0  Linked=0  [TranDis=0]  CmdQue=0
>     length=36 (0x24)   Peripheral device type: disk
>  Vendor identification: Generic-
>  Product identification: Multi-Card      
>  Product revision level: 1.00

Classic, USB strikes again! One needs to go back to SCSI-1
(1985 ?) to find when byte 5 bit 0 of an INQUIRY response
could be used for anything other than PROTECT.
So that bit is reserved in SCSI-2, SPC and SPC-2 then
has its current meaning in SPC-3 and SPC-4.

Doug Gilbert

Re: [PATCH][SCS] sd: Read Capacity if (16) fails

[Martin K. Petersen]

>>>>> "James" == James Bottomley <James.Bottomley@HansenPartnership.com> writes:

James> On Wed, 2008-08-06 at 16:34 +0100, Hugh Dickins wrote:
>> PQual=0 Device_type=0 RMB=1 version=0x00 [no conformance claimed]

James> We could try using this as the trigger.  I assume all DIF
James> supporting devices must also claim conformance to SPC3/SBC2 or
James> higher?

You'd think, wouldn't you?

Originally I only checked for PROTECT if version was SBC2 or better.
But as it turns out I have drives that only report SBC and yet they do
DIF.  This old version is apparently reported to prevent problems in
other operating systems.

*sigh*

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH][SCS] sd: Read Capacity if (16) fails

[James Bottomley]

On Wed, 2008-08-06 at 09:36 -0700, James Bottomley wrote:
> On Wed, 2008-08-06 at 16:34 +0100, Hugh Dickins wrote:
> >   PQual=0  Device_type=0  RMB=1  version=0x00  [no conformance
> > claimed]
> 
> We could try using this as the trigger.  I assume all DIF supporting
> devices must also claim conformance to SPC3/SBC2 or higher?

Just to formalise it, that would be this patch.  Hugh, could you try it?

Thanks,

James

---

diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 291d56a..ffbc84a 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -426,7 +426,7 @@ static inline int scsi_device_enclosure(struct scsi_device *sdev)
 
 static inline int scsi_device_protection(struct scsi_device *sdev)
 {
-	return sdev->inquiry[5] & (1<<0);
+	return sdev->scsi_level > SCSI_2 && sdev->inquiry[5] & (1<<0);
 }
 
 #define MODULE_ALIAS_SCSI_DEVICE(type) \

Re: [PATCH][SCS] sd: Read Capacity if (16) fails

[Hugh Dickins]

On Wed, 6 Aug 2008, James Bottomley wrote:
> On Wed, 2008-08-06 at 09:36 -0700, James Bottomley wrote:
> > On Wed, 2008-08-06 at 16:34 +0100, Hugh Dickins wrote:
> > >   PQual=0  Device_type=0  RMB=1  version=0x00  [no conformance
> > > claimed]
> > 
> > We could try using this as the trigger.  I assume all DIF supporting
> > devices must also claim conformance to SPC3/SBC2 or higher?
> 
> Just to formalise it, that would be this patch.  Hugh, could you try it?

Almost: I had to #include <scsi/scsi.h> to get SCSI_2 for the build
somewhere (and a SCSI_2 seems to be worth 1 more than anyone else's 2),
then it worked just fine for me thanks.

Hugh

p.s. sorry for losing an I from the "[SCSI]" Subject

--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -6,6 +6,7 @@
 #include <linux/spinlock.h>
 #include <linux/workqueue.h>
 #include <linux/blkdev.h>
+#include <scsi/scsi.h>
 #include <asm/atomic.h>
 
 struct request_queue;
@@ -426,7 +427,7 @@ static inline int scsi_device_enclosure(
 
 static inline int scsi_device_protection(struct scsi_device *sdev)
 {
-	return sdev->inquiry[5] & (1<<0);
+	return sdev->scsi_level > SCSI_2 && sdev->inquiry[5] & (1<<0);
 }
 
 #define MODULE_ALIAS_SCSI_DEVICE(type) \

Re: [PATCH][SCS] sd: Read Capacity if (16) fails

[Martin K. Petersen]

>>>>> "Hugh" == Hugh Dickins <hugh@veritas.com> writes:

Hugh> Almost: I had to #include <scsi/scsi.h> to get SCSI_2 for the
Hugh> build somewhere (and a SCSI_2 seems to be worth 1 more than
Hugh> anyone else's 2), then it worked just fine for me thanks.

The DIF drives in question claim rev. 3 and are configured correctly
with James' patch in place.

Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH][SCS] sd: Read Capacity if (16) fails

[James Bottomley]

On Wed, 2008-08-06 at 13:32 -0400, Martin K. Petersen wrote:
> >>>>> "Hugh" == Hugh Dickins <hugh@veritas.com> writes:
> 
> Hugh> Almost: I had to #include <scsi/scsi.h> to get SCSI_2 for the
> Hugh> build somewhere (and a SCSI_2 seems to be worth 1 more than
> Hugh> anyone else's 2), then it worked just fine for me thanks.
> 
> The DIF drives in question claim rev. 3 and are configured correctly
> with James' patch in place.
> 
> Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

OK, so this is the solution until we get USB devices claiming SCSI-3 and
mucking with the protection bit ...

James