[PATCH v2 0/2] Do not read the IO hints VPD page from USB storage devices

[PATCH v2 0/2] Do not read the IO hints VPD page from USB storage devices

[Bart Van Assche]

Hi Martin,

Recently it was reported that reading the IO hints VPD page makes at least two
USB storage devices crash. Hence this patch series that disables reading the IO
hints VPD page from USB storage devices. Please consider this patch series for
your scsi-fixes branch.

Thanks,

Bart.

Changes compared to v1:
 - Also set the BLIST_SKIP_IO_HINTS for USB Attached SCSI devices.

Bart Van Assche (2):
  scsi: core: Introduce the BLIST_SKIP_IO_HINTS flag
  scsi: core: Do not query IO hints for USB devices

 drivers/scsi/sd.c              | 4 ++++
 drivers/usb/storage/scsiglue.c | 6 ++++++
 drivers/usb/storage/uas.c      | 7 +++++++
 include/scsi/scsi_devinfo.h    | 4 +++-
 4 files changed, 20 insertions(+), 1 deletion(-)

[PATCH v2] scsi: core: Fix an incorrect comment

[Bart Van Assche]

The comment that scsi_static_device_list would go away was added more
than 18 years ago. Today, that list is still there and a large number
of additional entries have been added. This shows that this comment is
incorrect. Hence fix that comment.

Cc: Christoph Hellwig <hch@infradead.org>
Cc: Avri Altman <Avri.Altman@wdc.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---

Changes compared to v1: addressed Christoph's review feedback.

 drivers/scsi/scsi_devinfo.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index a7071e71389e..90f1393a23f8 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -39,13 +39,12 @@ static LIST_HEAD(scsi_dev_info_list);
 static char scsi_dev_flags[256];
 
 /*
- * scsi_static_device_list: deprecated list of devices that require
- * settings that differ from the default, includes black-listed (broken)
- * devices. The entries here are added to the tail of scsi_dev_info_list
- * via scsi_dev_info_list_init.
+ * scsi_static_device_list: list of devices that require settings that differ
+ * from the default, includes black-listed (broken) devices. The entries here
+ * are added to the tail of scsi_dev_info_list via scsi_dev_info_list_init.
  *
- * Do not add to this list, use the command line or proc interface to add
- * to the scsi_dev_info_list. This table will eventually go away.
+ * If possible, set the BLIST_* flags from inside a SCSI LLD rather than
+ * adding an entry to this list.
  */
 static struct {
 	char *vendor;

[PATCH v2 1/2] scsi: core: Introduce the BLIST_SKIP_IO_HINTS flag

[Bart Van Assche]

Prepare for skipping reading the IO hints VPD page for USB storage devices.

Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Joao Machado <jocrismachado@gmail.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Christian Heusel <christian@heusel.eu>
Cc: stable@vger.kernel.org
Fixes: 4f53138fffc2 ("scsi: sd: Translate data lifetime information")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/sd.c           | 4 ++++
 include/scsi/scsi_devinfo.h | 4 +++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 3a43e2209751..fcf3d7730466 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -63,6 +63,7 @@
 #include <scsi/scsi_cmnd.h>
 #include <scsi/scsi_dbg.h>
 #include <scsi/scsi_device.h>
+#include <scsi/scsi_devinfo.h>
 #include <scsi/scsi_driver.h>
 #include <scsi/scsi_eh.h>
 #include <scsi/scsi_host.h>
@@ -3117,6 +3118,9 @@ static void sd_read_io_hints(struct scsi_disk *sdkp, unsigned char *buffer)
 	struct scsi_mode_data data;
 	int res;
 
+	if (sdp->sdev_bflags & BLIST_SKIP_IO_HINTS)
+		return;
+
 	res = scsi_mode_sense(sdp, /*dbd=*/0x8, /*modepage=*/0x0a,
 			      /*subpage=*/0x05, buffer, SD_BUF_SIZE, SD_TIMEOUT,
 			      sdkp->max_retries, &data, &sshdr);
diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
index 6b548dc2c496..fa8721e49dec 100644
--- a/include/scsi/scsi_devinfo.h
+++ b/include/scsi/scsi_devinfo.h
@@ -69,8 +69,10 @@
 #define BLIST_RETRY_ITF		((__force blist_flags_t)(1ULL << 32))
 /* Always retry ABORTED_COMMAND with ASC 0xc1 */
 #define BLIST_RETRY_ASC_C1	((__force blist_flags_t)(1ULL << 33))
+/* Do not read the I/O hints mode page */
+#define BLIST_SKIP_IO_HINTS	((__force blist_flags_t)(1ULL << 34))
 
-#define __BLIST_LAST_USED BLIST_RETRY_ASC_C1
+#define __BLIST_LAST_USED BLIST_SKIP_IO_HINTS
 
 #define __BLIST_HIGH_UNUSED (~(__BLIST_LAST_USED | \
 			       (__force blist_flags_t) \

[PATCH v2 2/2] scsi: core: Do not query IO hints for USB devices

[Bart Van Assche]

Recently it was reported that the following USB storage devices are unusable
with Linux kernel 6.9:
* Kingston DataTraveler G2
* Garmin FR35

This is because attempting to read the IO hint VPD page causes these devices
to reset. Hence do not read the IO hint VPD page from USB storage devices.

Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: linux-usb@vger.kernel.org
Cc: Joao Machado <jocrismachado@gmail.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Christian Heusel <christian@heusel.eu>
Cc: stable@vger.kernel.org
Fixes: 4f53138fffc2 ("scsi: sd: Translate data lifetime information")
Reported-by: Joao Machado <jocrismachado@gmail.com>
Closes: https://lore.kernel.org/linux-scsi/20240130214911.1863909-1-bvanassche@acm.org/T/#mf4e3410d8f210454d7e4c3d1fb5c0f41e651b85f
Tested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Bisected-by: Christian Heusel <christian@heusel.eu>
Reported-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Closes: https://lore.kernel.org/linux-scsi/CACLx9VdpUanftfPo2jVAqXdcWe8Y43MsDeZmMPooTzVaVJAh2w@mail.gmail.com/
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/usb/storage/scsiglue.c | 6 ++++++
 drivers/usb/storage/uas.c      | 7 +++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
index b31464740f6c..b4cf0349fd0d 100644
--- a/drivers/usb/storage/scsiglue.c
+++ b/drivers/usb/storage/scsiglue.c
@@ -79,6 +79,12 @@ static int slave_alloc (struct scsi_device *sdev)
 	if (us->protocol == USB_PR_BULK && us->max_lun > 0)
 		sdev->sdev_bflags |= BLIST_FORCELUN;
 
+	/*
+	 * Some USB storage devices reset if the IO hints VPD page is queried.
+	 * Hence skip that VPD page.
+	 */
+	sdev->sdev_bflags |= BLIST_SKIP_IO_HINTS;
+
 	return 0;
 }
 
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index a48870a87a29..77fdfb6a90c8 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -21,6 +21,7 @@
 #include <scsi/scsi.h>
 #include <scsi/scsi_eh.h>
 #include <scsi/scsi_dbg.h>
+#include <scsi/scsi_devinfo.h>
 #include <scsi/scsi_cmnd.h>
 #include <scsi/scsi_device.h>
 #include <scsi/scsi_host.h>
@@ -820,6 +821,12 @@ static int uas_slave_alloc(struct scsi_device *sdev)
 	struct uas_dev_info *devinfo =
 		(struct uas_dev_info *)sdev->host->hostdata;
 
+	/*
+	 * Some USB storage devices reset if the IO hints VPD page is queried.
+	 * Hence skip that VPD page.
+	 */
+	sdev->sdev_bflags |= BLIST_SKIP_IO_HINTS;
+
 	sdev->hostdata = devinfo;
 	return 0;
 }

Re: [PATCH v2] scsi: core: Fix an incorrect comment

[Bart Van Assche]

On 6/12/24 1:37 PM, Bart Van Assche wrote:
> The comment that scsi_static_device_list would go away was added more
> than 18 years ago. Today, that list is still there and a large number
> of additional entries have been added. This shows that this comment is
> incorrect. Hence fix that comment.

Please ignore this patch - this patch was already posted earlier today.

Thanks,

Bart.

Re: [PATCH v2 2/2] scsi: core: Do not query IO hints for USB devices

[Alan Stern]

On Wed, Jun 12, 2024 at 01:37:34PM -0700, Bart Van Assche wrote:
> Recently it was reported that the following USB storage devices are unusable
> with Linux kernel 6.9:
> * Kingston DataTraveler G2
> * Garmin FR35
> 
> This is because attempting to read the IO hint VPD page causes these devices
> to reset. Hence do not read the IO hint VPD page from USB storage devices.
> 
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: linux-usb@vger.kernel.org
> Cc: Joao Machado <jocrismachado@gmail.com>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Christian Heusel <christian@heusel.eu>
> Cc: stable@vger.kernel.org
> Fixes: 4f53138fffc2 ("scsi: sd: Translate data lifetime information")
> Reported-by: Joao Machado <jocrismachado@gmail.com>
> Closes: https://lore.kernel.org/linux-scsi/20240130214911.1863909-1-bvanassche@acm.org/T/#mf4e3410d8f210454d7e4c3d1fb5c0f41e651b85f
> Tested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Bisected-by: Christian Heusel <christian@heusel.eu>
> Reported-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Closes: https://lore.kernel.org/linux-scsi/CACLx9VdpUanftfPo2jVAqXdcWe8Y43MsDeZmMPooTzVaVJAh2w@mail.gmail.com/
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---

Acked-by: Alan Stern <stern@rowland.harvard.edu>

>  drivers/usb/storage/scsiglue.c | 6 ++++++
>  drivers/usb/storage/uas.c      | 7 +++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
> index b31464740f6c..b4cf0349fd0d 100644
> --- a/drivers/usb/storage/scsiglue.c
> +++ b/drivers/usb/storage/scsiglue.c
> @@ -79,6 +79,12 @@ static int slave_alloc (struct scsi_device *sdev)
>  	if (us->protocol == USB_PR_BULK && us->max_lun > 0)
>  		sdev->sdev_bflags |= BLIST_FORCELUN;
>  
> +	/*
> +	 * Some USB storage devices reset if the IO hints VPD page is queried.
> +	 * Hence skip that VPD page.
> +	 */
> +	sdev->sdev_bflags |= BLIST_SKIP_IO_HINTS;
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index a48870a87a29..77fdfb6a90c8 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -21,6 +21,7 @@
>  #include <scsi/scsi.h>
>  #include <scsi/scsi_eh.h>
>  #include <scsi/scsi_dbg.h>
> +#include <scsi/scsi_devinfo.h>
>  #include <scsi/scsi_cmnd.h>
>  #include <scsi/scsi_device.h>
>  #include <scsi/scsi_host.h>
> @@ -820,6 +821,12 @@ static int uas_slave_alloc(struct scsi_device *sdev)
>  	struct uas_dev_info *devinfo =
>  		(struct uas_dev_info *)sdev->host->hostdata;
>  
> +	/*
> +	 * Some USB storage devices reset if the IO hints VPD page is queried.
> +	 * Hence skip that VPD page.
> +	 */
> +	sdev->sdev_bflags |= BLIST_SKIP_IO_HINTS;
> +
>  	sdev->hostdata = devinfo;
>  	return 0;
>  }

Re: [PATCH v2 2/2] scsi: core: Do not query IO hints for USB devices

[Andy Shevchenko]

On Wed, Jun 12, 2024 at 10:37 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> Recently it was reported that the following USB storage devices are unusable
> with Linux kernel 6.9:
> * Kingston DataTraveler G2
> * Garmin FR35
>
> This is because attempting to read the IO hint VPD page causes these devices
> to reset. Hence do not read the IO hint VPD page from USB storage devices.

I have commented on v1, same applicable here. Not that it's a big deal
for this change, but in general can you follow the advice given there?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] scsi: core: Introduce the BLIST_SKIP_IO_HINTS flag

[Martin K. Petersen]

Hi Bart!

> Prepare for skipping reading the IO hints VPD page for USB storage
                                   ^^^^^^^^^^^^^^^^^
[...]
> @@ -3117,6 +3118,9 @@ static void sd_read_io_hints(struct scsi_disk *sdkp, unsigned char *buffer)
>  	struct scsi_mode_data data;
>  	int res;
>  
> +	if (sdp->sdev_bflags & BLIST_SKIP_IO_HINTS)
> +		return;
> +
>  	res = scsi_mode_sense(sdp, /*dbd=*/0x8, /*modepage=*/0x0a,
>  			      /*subpage=*/0x05, buffer, SD_BUF_SIZE, SD_TIMEOUT,
>  			      sdkp->max_retries, &data, &sshdr);
              ^^^^^^^^^^^^^^^

s/IO hints VPD page/IO Advice Hints Grouping mode page/g

?

PS. I'm not fussy about Cc. But I generally avoid listing anybody who
will be automatically copied by virtue of any *-by: tags.

I tend to use Cc as an indicator that this entity needs to act upon the
patch in question. Ack, review, test, respond to comments, or merge in
case of stable@.

If a person listed in Cc subsequently responds with a tag, their name
may be listed more than once in the commit description. But I view that
as documentation that the person whose feedback was requested actually
responded. That's useful information as far as I'm concerned...

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2 1/2] scsi: core: Introduce the BLIST_SKIP_IO_HINTS flag

[Andy Shevchenko]

On Thu, Jun 13, 2024 at 9:40 PM Martin K. Petersen
<martin.petersen@oracle.com> wrote:

...

> PS. I'm not fussy about Cc. But I generally avoid listing anybody who
> will be automatically copied by virtue of any *-by: tags.
>
> I tend to use Cc as an indicator that this entity needs to act upon the
> patch in question. Ack, review, test, respond to comments, or merge in
> case of stable@.

And nobody asks to remove them! The only outcome of what I see is
beneficial, i.e. cleaning up the commit message. Everything else will
work as expected, e.g., all listed people will be informed as you want
to.

> If a person listed in Cc subsequently responds with a tag, their name
> may be listed more than once in the commit description. But I view that
> as documentation that the person whose feedback was requested actually
> responded. That's useful information as far as I'm concerned...

Again, feel free to have a duplicate, but please do it after --- line.

-- 
With Best Regards,
Andy Shevchenko