diff mbox series

scsi: sd: usb_storage: uas: Access media prior to querying device properties

Message ID 20240213143306.2194237-1-martin.petersen@oracle.com (mailing list archive)
State Accepted
Headers show
Series scsi: sd: usb_storage: uas: Access media prior to querying device properties | expand

Commit Message

Martin K. Petersen Feb. 13, 2024, 2:33 p.m. UTC
It has been observed that some USB/UAS devices return generic
properties hardcoded in firmware for mode pages and vital product data
for a period of time after a device has been discovered. The reported
properties are either garbage or they do not accurately reflect the
properties of the physical storage device attached in the case of a
bridge.

Prior to commit 1e029397d12f ("scsi: sd: Reorganize DIF/DIX code to
avoid calling revalidate twice") we would call revalidate several
times during device discovery. As a result, incorrect values would
eventually get replaced with ones accurately describing the attached
storage. When we did away with the redundant revalidate pass, several
cases were reported where devices reported nonsensical values or would
end up in write-protected state.

An initial attempt at addressing this issue involved introducing a
delayed second revalidate invocation. However, this approach still
left some devices reporting incorrect characteristics.

Tasos Sahanidis debugged the problem further and identified that
introducing a READ operation prior to MODE SENSE fixed the problem and
that it wasn't a timing issue. Issuing a READ appears to cause the
devices to update their SCSI pages to reflect the actual properties of
the storage media. Device properties like vendor, model, and storage
capacity appear to be correctly reported from the get-go. It is
unclear why these device defer populating the remaining
characteristics.

Match the behavior of a well known commercial operating system and
trigger a READ operation prior to querying device characteristics to
force the device to populate mode pages and VPDs.

The additional READ is triggered by a flag set in the USB storage and
UAS drivers. We avoid issuing the READ for other transport classes
since some storage devices identify Linux through our particular
discovery command sequence.

Cc: <stable@vger.kernel.org>
Fixes: 1e029397d12f ("scsi: sd: Reorganize DIF/DIX code to avoid calling revalidate twice")
Reported-by: Tasos Sahanidis <tasos@tasossah.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/sd.c              | 27 ++++++++++++++++++++++++++-
 drivers/usb/storage/scsiglue.c |  7 +++++++
 drivers/usb/storage/uas.c      |  7 +++++++
 include/scsi/scsi_device.h     |  1 +
 4 files changed, 41 insertions(+), 1 deletion(-)

Comments

Ewan Milne Feb. 13, 2024, 4:18 p.m. UTC | #1
On Tue, Feb 13, 2024 at 9:33 AM Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>
> It has been observed that some USB/UAS devices return generic
> properties hardcoded in firmware for mode pages and vital product data
> for a period of time after a device has been discovered. The reported
> properties are either garbage or they do not accurately reflect the
> properties of the physical storage device attached in the case of a
> bridge.
>
> Prior to commit 1e029397d12f ("scsi: sd: Reorganize DIF/DIX code to
> avoid calling revalidate twice") we would call revalidate several
> times during device discovery. As a result, incorrect values would
> eventually get replaced with ones accurately describing the attached
> storage. When we did away with the redundant revalidate pass, several
> cases were reported where devices reported nonsensical values or would
> end up in write-protected state.
>
> An initial attempt at addressing this issue involved introducing a
> delayed second revalidate invocation. However, this approach still
> left some devices reporting incorrect characteristics.
>
> Tasos Sahanidis debugged the problem further and identified that
> introducing a READ operation prior to MODE SENSE fixed the problem and
> that it wasn't a timing issue. Issuing a READ appears to cause the
> devices to update their SCSI pages to reflect the actual properties of
> the storage media. Device properties like vendor, model, and storage
> capacity appear to be correctly reported from the get-go. It is
> unclear why these device defer populating the remaining
> characteristics.
>
> Match the behavior of a well known commercial operating system and
> trigger a READ operation prior to querying device characteristics to
> force the device to populate mode pages and VPDs.
>
> The additional READ is triggered by a flag set in the USB storage and
> UAS drivers. We avoid issuing the READ for other transport classes
> since some storage devices identify Linux through our particular
> discovery command sequence.
>
> Cc: <stable@vger.kernel.org>
> Fixes: 1e029397d12f ("scsi: sd: Reorganize DIF/DIX code to avoid calling revalidate twice")
> Reported-by: Tasos Sahanidis <tasos@tasossah.com>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  drivers/scsi/sd.c              | 27 ++++++++++++++++++++++++++-
>  drivers/usb/storage/scsiglue.c |  7 +++++++
>  drivers/usb/storage/uas.c      |  7 +++++++
>  include/scsi/scsi_device.h     |  1 +
>  4 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 530918cbfce2..c284628f702c 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3405,6 +3405,24 @@ static bool sd_validate_opt_xfer_size(struct scsi_disk *sdkp,
>         return true;
>  }
>
> +static void sd_read_block_zero(struct scsi_disk *sdkp)
> +{
> +       unsigned int buf_len = sdkp->device->sector_size;
> +       char *buffer, cmd[10] = { };
> +
> +       buffer = kmalloc(buf_len, GFP_KERNEL);
> +       if (!buffer)
> +               return;
> +
> +       cmd[0] = READ_10;
> +       put_unaligned_be32(0, &cmd[2]); /* Logical block address 0 */
> +       put_unaligned_be16(1, &cmd[7]); /* Transfer 1 logical block */
> +
> +       scsi_execute_cmd(sdkp->device, cmd, REQ_OP_DRV_IN, buffer, buf_len,
> +                        SD_TIMEOUT, sdkp->max_retries, NULL);
> +       kfree(buffer);
> +}
> +
>  /**
>   *     sd_revalidate_disk - called the first time a new disk is seen,
>   *     performs disk spin up, read_capacity, etc.
> @@ -3444,7 +3462,14 @@ static int sd_revalidate_disk(struct gendisk *disk)
>          */
>         if (sdkp->media_present) {
>                 sd_read_capacity(sdkp, buffer);
> -
> +               /*
> +                * Some USB/UAS devices return generic values for mode pages
> +                * and VPDs until the media has been accessed. Trigger a READ
> +                * operation to force the device to populate mode pages and
> +                * VPDs.
> +                */
> +               if (sdp->read_before_ms)
> +                       sd_read_block_zero(sdkp);
>                 /*
>                  * set the default to rotational.  All non-rotational devices
>                  * support the block characteristics VPD page, which will
> diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
> index c54e9805da53..12cf9940e5b6 100644
> --- a/drivers/usb/storage/scsiglue.c
> +++ b/drivers/usb/storage/scsiglue.c
> @@ -179,6 +179,13 @@ static int slave_configure(struct scsi_device *sdev)
>                  */
>                 sdev->use_192_bytes_for_3f = 1;
>
> +               /*
> +                * Some devices report generic values until the media has been
> +                * accessed. Force a READ(10) prior to querying device
> +                * characteristics.
> +                */
> +               sdev->read_before_ms = 1;
> +
>                 /*
>                  * Some devices don't like MODE SENSE with page=0x3f,
>                  * which is the command used for checking if a device
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 696bb0b23599..299a6767b7b3 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -878,6 +878,13 @@ static int uas_slave_configure(struct scsi_device *sdev)
>         if (devinfo->flags & US_FL_CAPACITY_HEURISTICS)
>                 sdev->guess_capacity = 1;
>
> +       /*
> +        * Some devices report generic values until the media has been
> +        * accessed. Force a READ(10) prior to querying device
> +        * characteristics.
> +        */
> +       sdev->read_before_ms = 1;
> +
>         /*
>          * Some devices don't like MODE SENSE with page=0x3f,
>          * which is the command used for checking if a device
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 10480eb582b2..cb019c80763b 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -202,6 +202,7 @@ struct scsi_device {
>         unsigned use_10_for_rw:1; /* first try 10-byte read / write */
>         unsigned use_10_for_ms:1; /* first try 10-byte mode sense/select */
>         unsigned set_dbd_for_ms:1; /* Set "DBD" field in mode sense */
> +       unsigned read_before_ms:1;      /* perform a READ before MODE SENSE */
>         unsigned no_report_opcodes:1;   /* no REPORT SUPPORTED OPERATION CODES */
>         unsigned no_write_same:1;       /* no WRITE SAME command */
>         unsigned use_16_for_rw:1; /* Use read/write(16) over read/write(10) */
> --
> 2.42.1
>
>

So the READ CAPACITY is correct but the VPD pages etc are not?  OK.

Reviewed-by: Ewan D. Milne <emilne@redhat.com>
Alan Stern Feb. 13, 2024, 4:33 p.m. UTC | #2
On Tue, Feb 13, 2024 at 09:33:06AM -0500, Martin K. Petersen wrote:
> It has been observed that some USB/UAS devices return generic
> properties hardcoded in firmware for mode pages and vital product data
> for a period of time after a device has been discovered. The reported
> properties are either garbage or they do not accurately reflect the
> properties of the physical storage device attached in the case of a
> bridge.

...

> +static void sd_read_block_zero(struct scsi_disk *sdkp)
> +{
> +	unsigned int buf_len = sdkp->device->sector_size;
> +	char *buffer, cmd[10] = { };
> +
> +	buffer = kmalloc(buf_len, GFP_KERNEL);
> +	if (!buffer)
> +		return;
> +
> +	cmd[0] = READ_10;
> +	put_unaligned_be32(0, &cmd[2]); /* Logical block address 0 */
> +	put_unaligned_be16(1, &cmd[7]);	/* Transfer 1 logical block */
> +
> +	scsi_execute_cmd(sdkp->device, cmd, REQ_OP_DRV_IN, buffer, buf_len,
> +			 SD_TIMEOUT, sdkp->max_retries, NULL);
> +	kfree(buffer);
> +}

Do we still claim to support devices that implement only the 6-byte
commands, not the 10-byte forms?  Or is that now a non-issue?

Alan Stern
Martin K. Petersen Feb. 13, 2024, 5:09 p.m. UTC | #3
Hi Alan!

> Do we still claim to support devices that implement only the 6-byte
> commands, not the 10-byte forms?  Or is that now a non-issue?

We have defaulted to 10-byte commands for a long time and the 6-byte
variants are no longer to be found in SCSI SBC-4 and SBC-5. In addition,
most devices now have capacities which exceed what 6-byte commands can
address.

I will not object to adding a fallback to a READ(6) if I receive a bug
report as a result of this change. But I prefer not to perpetuate the
6-byte command special-casing if we can avoid it. Especially if we know
that $OTHER_OS is issuing a READ(10) during discovery.
Bart Van Assche Feb. 13, 2024, 6:13 p.m. UTC | #4
On 2/13/24 06:33, Martin K. Petersen wrote:
> Match the behavior of a well known commercial operating system and
> trigger a READ operation prior to querying device characteristics to
> force the device to populate mode pages and VPDs.
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Tasos Sahanidis Feb. 14, 2024, 12:21 p.m. UTC | #5
On 2024-02-13 16:33, Martin K. Petersen wrote:
> Match the behavior of a well known commercial operating system and
> trigger a READ operation prior to querying device characteristics to
> force the device to populate mode pages and VPDs.

Applied the patch, reverted the Kingston quirk, and:

[  447.655130] usb-storage 3-2:1.0: USB Mass Storage device detected
[  447.664875] scsi host10: usb-storage 3-2:1.0
[  448.690959] scsi 10:0:0:0: Direct-Access     Kingston DT Ultimate G3   PMAP PQ: 0 ANSI: 6
[  448.709502] sd 10:0:0:0: Attached scsi generic sg1 type 0
[  448.837376] sd 10:0:0:0: [sdb] 61472768 512-byte logical blocks: (31.5 GB/29.3 GiB)
[  448.847489] sd 10:0:0:0: [sdb] Write Protect is off
[  448.852593] sd 10:0:0:0: [sdb] Mode Sense: 2b 00 00 08
[  448.854221] sd 10:0:0:0: [sdb] Write cache: disabled, read cache: enabled, doesn't support DPO or FUA

Thanks!

Tested-by: Tasos Sahanidis <tasos@tasossah.com>

--
Tasos
diff mbox series

Patch

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 530918cbfce2..c284628f702c 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3405,6 +3405,24 @@  static bool sd_validate_opt_xfer_size(struct scsi_disk *sdkp,
 	return true;
 }
 
+static void sd_read_block_zero(struct scsi_disk *sdkp)
+{
+	unsigned int buf_len = sdkp->device->sector_size;
+	char *buffer, cmd[10] = { };
+
+	buffer = kmalloc(buf_len, GFP_KERNEL);
+	if (!buffer)
+		return;
+
+	cmd[0] = READ_10;
+	put_unaligned_be32(0, &cmd[2]); /* Logical block address 0 */
+	put_unaligned_be16(1, &cmd[7]);	/* Transfer 1 logical block */
+
+	scsi_execute_cmd(sdkp->device, cmd, REQ_OP_DRV_IN, buffer, buf_len,
+			 SD_TIMEOUT, sdkp->max_retries, NULL);
+	kfree(buffer);
+}
+
 /**
  *	sd_revalidate_disk - called the first time a new disk is seen,
  *	performs disk spin up, read_capacity, etc.
@@ -3444,7 +3462,14 @@  static int sd_revalidate_disk(struct gendisk *disk)
 	 */
 	if (sdkp->media_present) {
 		sd_read_capacity(sdkp, buffer);
-
+		/*
+		 * Some USB/UAS devices return generic values for mode pages
+		 * and VPDs until the media has been accessed. Trigger a READ
+		 * operation to force the device to populate mode pages and
+		 * VPDs.
+		 */
+		if (sdp->read_before_ms)
+			sd_read_block_zero(sdkp);
 		/*
 		 * set the default to rotational.  All non-rotational devices
 		 * support the block characteristics VPD page, which will
diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
index c54e9805da53..12cf9940e5b6 100644
--- a/drivers/usb/storage/scsiglue.c
+++ b/drivers/usb/storage/scsiglue.c
@@ -179,6 +179,13 @@  static int slave_configure(struct scsi_device *sdev)
 		 */
 		sdev->use_192_bytes_for_3f = 1;
 
+		/*
+		 * Some devices report generic values until the media has been
+		 * accessed. Force a READ(10) prior to querying device
+		 * characteristics.
+		 */
+		sdev->read_before_ms = 1;
+
 		/*
 		 * Some devices don't like MODE SENSE with page=0x3f,
 		 * which is the command used for checking if a device
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 696bb0b23599..299a6767b7b3 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -878,6 +878,13 @@  static int uas_slave_configure(struct scsi_device *sdev)
 	if (devinfo->flags & US_FL_CAPACITY_HEURISTICS)
 		sdev->guess_capacity = 1;
 
+	/*
+	 * Some devices report generic values until the media has been
+	 * accessed. Force a READ(10) prior to querying device
+	 * characteristics.
+	 */
+	sdev->read_before_ms = 1;
+
 	/*
 	 * Some devices don't like MODE SENSE with page=0x3f,
 	 * which is the command used for checking if a device
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 10480eb582b2..cb019c80763b 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -202,6 +202,7 @@  struct scsi_device {
 	unsigned use_10_for_rw:1; /* first try 10-byte read / write */
 	unsigned use_10_for_ms:1; /* first try 10-byte mode sense/select */
 	unsigned set_dbd_for_ms:1; /* Set "DBD" field in mode sense */
+	unsigned read_before_ms:1;   	/* perform a READ before MODE SENSE */
 	unsigned no_report_opcodes:1;	/* no REPORT SUPPORTED OPERATION CODES */
 	unsigned no_write_same:1;	/* no WRITE SAME command */
 	unsigned use_16_for_rw:1; /* Use read/write(16) over read/write(10) */