[PATCH v3 0/4] scsi: add runtime PM workaround for SD cardreaders

[PATCH v3 0/4] scsi: add runtime PM workaround for SD cardreaders

[Martin Kepplinger]

hi,

In short: there are SD cardreaders that send MEDIA_CHANGED on
(runtime) resume. We cannot use runtime PM with these devices as
I/O always fails. I'd like to discuss a way to fix this
or at least allow us to work around this problem:

For the full background, the discussion started in June 2020 here:
https://lore.kernel.org/linux-scsi/20200623111018.31954-1-martin.kepplinger@puri.sm/

I'd appreciate any feedback.

Especially: Any naming-preferences for the flags? And is the specific
device that I need this workaround for (Generic Ultra HS-SD/MMC, connected
via USB) too "generic" maybe? Not sure about what possibilities I'd have here...


revision history
----------------
v3: (thank you Bart)
 * create a new BLIST entry to mark affected devices instead of the
   sysfs module parameter for sd only. still, only sd implements handling
   the flag for now.
 * cc linux-pm list

v2:
https://lore.kernel.org/linux-scsi/20210112093329.3639-1-martin.kepplinger@puri.sm/
 * move module parameter to sd
 * add Documentation
v1:
https://lore.kernel.org/linux-scsi/20210111152029.28426-1-martin.kepplinger@puri.sm/T/


Martin Kepplinger (4):
  scsi: add expecting_media_change flag to error path
  scsi: devinfo: add new flag BLIST_MEDIA_CHANGE
  scsi: sd: use expecting_media_change for BLIST_MEDIA_CHANGE devices
  scsi: devinfo: add BLIST_MEDIA_CHANGE for Ultra HS-SD/MMC usb
    cardreaders

 drivers/scsi/scsi_devinfo.c |  1 +
 drivers/scsi/scsi_error.c   | 36 +++++++++++++++++++++++++++++++-----
 drivers/scsi/sd.c           | 23 ++++++++++++++++++++++-
 include/scsi/scsi_device.h  |  1 +
 include/scsi/scsi_devinfo.h |  6 +++---
 5 files changed, 58 insertions(+), 9 deletions(-)

-- 
2.30.2

[PATCH v3 1/4] scsi: add expecting_media_change flag to error path

[Martin Kepplinger]

SD Cardreaders (especially) sometimes lose the state during suspend
and deliver a "media changed" unit attention when really only a
(runtime) suspend/resume cycle has been done.

For such devices, I/O fails when runtime PM is enabled, see below.
That's the motivation to add this flag. If set by a driver the
one following MEDIA CHANGE unit attention will be ignored.

Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
---
 drivers/scsi/scsi_error.c  | 36 +++++++++++++++++++++++++++++++-----
 include/scsi/scsi_device.h |  1 +
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 08c06c56331c..c62915d34ba4 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -585,6 +585,18 @@ int scsi_check_sense(struct scsi_cmnd *scmd)
 				return NEEDS_RETRY;
 			}
 		}
+		if (scmd->device->expecting_media_change) {
+			if (sshdr.asc == 0x28 && sshdr.ascq == 0x00) {
+				/*
+				 * clear the expecting_media_change in
+				 * scsi_decide_disposition() because we
+				 * need to catch possible "fail fast" overrides
+				 * that block readahead can cause.
+				 */
+				return NEEDS_RETRY;
+			}
+		}
+
 		/*
 		 * we might also expect a cc/ua if another LUN on the target
 		 * reported a UA with an ASC/ASCQ of 3F 0E -
@@ -1977,14 +1989,28 @@ int scsi_decide_disposition(struct scsi_cmnd *scmd)
 	 * the request was not marked fast fail.  Note that above,
 	 * even if the request is marked fast fail, we still requeue
 	 * for queue congestion conditions (QUEUE_FULL or BUSY) */
-	if (scsi_cmd_retry_allowed(scmd) && !scsi_noretry_cmd(scmd)) {
-		return NEEDS_RETRY;
-	} else {
-		/*
-		 * no more retries - report this one back to upper level.
+	if (scsi_cmd_retry_allowed(scmd)) {
+		/* but scsi_noretry_cmd() cannot override the
+		 * expecting_media_change flag.
 		 */
+		if (!scsi_noretry_cmd(scmd) ||
+		    scmd->device->expecting_media_change) {
+			scmd->device->expecting_media_change = 0;
+			return NEEDS_RETRY;
+		}
+
+		/* Not marked fail fast, or marked but not expected.
+		 * Clear the flag too because it's meant for the
+		 * next UA only.
+		 */
+		scmd->device->expecting_media_change = 0;
 		return SUCCESS;
 	}
+
+	/*
+	 * no more retries - report this one back to upper level.
+	 */
+	return SUCCESS;
 }
 
 static void eh_lock_door_done(struct request *req, blk_status_t status)
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 05c7c320ef32..926b42ce1dc4 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -171,6 +171,7 @@ struct scsi_device {
 				 * this device */
 	unsigned expecting_cc_ua:1; /* Expecting a CHECK_CONDITION/UNIT_ATTN
 				     * because we did a bus reset. */
+	unsigned expecting_media_change:1; /* Expecting "media changed" UA */
 	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 */
-- 
2.30.2

Re: [PATCH v3 1/4] scsi: add expecting_media_change flag to error path

[Bart Van Assche]

On 3/28/21 3:25 AM, Martin Kepplinger wrote:
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 08c06c56331c..c62915d34ba4 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -585,6 +585,18 @@ int scsi_check_sense(struct scsi_cmnd *scmd)
>  				return NEEDS_RETRY;
>  			}
>  		}
> +		if (scmd->device->expecting_media_change) {
> +			if (sshdr.asc == 0x28 && sshdr.ascq == 0x00) {
> +				/*
> +				 * clear the expecting_media_change in
> +				 * scsi_decide_disposition() because we
> +				 * need to catch possible "fail fast" overrides
> +				 * that block readahead can cause.
> +				 */
> +				return NEEDS_RETRY;
> +			}
> +		}

Introducing a new state variable carries some risk, namely that a path
that should set or clear the state variable is overlooked. Is there an
approach that does not require to introduce a new state variable, e.g.
to send a REQUEST SENSE command after a resume?

Thanks,

Bart.

Re: [PATCH v3 1/4] scsi: add expecting_media_change flag to error path

[Martin Kepplinger]

Am Sonntag, dem 28.03.2021 um 09:53 -0700 schrieb Bart Van Assche:
> On 3/28/21 3:25 AM, Martin Kepplinger wrote:
> > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> > index 08c06c56331c..c62915d34ba4 100644
> > --- a/drivers/scsi/scsi_error.c
> > +++ b/drivers/scsi/scsi_error.c
> > @@ -585,6 +585,18 @@ int scsi_check_sense(struct scsi_cmnd *scmd)
> >                                 return NEEDS_RETRY;
> >                         }
> >                 }
> > +               if (scmd->device->expecting_media_change) {
> > +                       if (sshdr.asc == 0x28 && sshdr.ascq ==
> > 0x00) {
> > +                               /*
> > +                                * clear the expecting_media_change
> > in
> > +                                * scsi_decide_disposition()
> > because we
> > +                                * need to catch possible "fail
> > fast" overrides
> > +                                * that block readahead can cause.
> > +                                */
> > +                               return NEEDS_RETRY;
> > +                       }
> > +               }
> 
> Introducing a new state variable carries some risk, namely that a
> path
> that should set or clear the state variable is overlooked. Is there
> an
> approach that does not require to introduce a new state variable,
> e.g.
> to send a REQUEST SENSE command after a resume?
> 
> Thanks,
> 
> Bart.

wow, thanks for that. Indeed my first tests succeed with the below
change, that doesn't use the error-path additions at all (not setting
expecting_media_change), and sends a request sense instead.

I'm just too little of a scsi developer that I know whether the below
change correctly does what you had in mind. Does it?


--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3707,6 +3707,10 @@ static int sd_resume_runtime(struct device *dev)
 {
        struct scsi_disk *sdkp = dev_get_drvdata(dev);
        struct scsi_device *sdp = sdkp->device;
+       const int timeout = sdp->request_queue->rq_timeout
+               * SD_FLUSH_TIMEOUT_MULTIPLIER;
+       int retries, res;
+       struct scsi_sense_hdr my_sshdr;
        int ret;
 
        if (!sdkp)      /* E.g.: runtime resume at the start of
sd_probe() */
@@ -3714,10 +3718,25 @@ static int sd_resume_runtime(struct device
*dev)
 
        /*
         * This devices issues a MEDIA CHANGE unit attention when
-        * resuming from suspend. Ignore the next one now.
+        * resuming from suspend.
         */
-       if (sdp->sdev_bflags & BLIST_MEDIA_CHANGE)
-               sdkp->device->expecting_media_change = 1;
+       if (sdp->sdev_bflags & BLIST_MEDIA_CHANGE) {
+               for (retries = 3; retries > 0; --retries) {
+                       unsigned char cmd[10] = { 0 };
+
+                       cmd[0] = REQUEST_SENSE;
+                       /*
+                        * Leave the rest of the command zero to
indicate
+                        * flush everything.
+                        */
+                       res = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0,
NULL, &my_sshdr,
+                                       timeout, sdkp->max_retries, 0,
RQF_PM, NULL);
+                       if (res == 0)
+                               break;
+               }
+       }
 
        return sd_resume(dev);

[PATCH v3 2/4] scsi: devinfo: add new flag BLIST_MEDIA_CHANGE

[Martin Kepplinger]

add a new flag for devices that issue MEDIA CHANGE unit attentions
when actually no medium changed. Drivers can for example set the
expecting_media_change device flag in order to ignore the next
following MEDIA CHANGE unit attention.

Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
---
 include/scsi/scsi_devinfo.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
index 3fdb322d4c4b..dee9dce14887 100644
--- a/include/scsi/scsi_devinfo.h
+++ b/include/scsi/scsi_devinfo.h
@@ -28,7 +28,8 @@
 #define BLIST_LARGELUN		((__force blist_flags_t)(1ULL << 9))
 /* override additional length field */
 #define BLIST_INQUIRY_36	((__force blist_flags_t)(1ULL << 10))
-#define __BLIST_UNUSED_11	((__force blist_flags_t)(1ULL << 11))
+/* Ignore the next media change event */
+#define BLIST_MEDIA_CHANGE	((__force blist_flags_t)(1ULL << 11))
 /* do not do automatic start on add */
 #define BLIST_NOSTARTONADD	((__force blist_flags_t)(1ULL << 12))
 #define __BLIST_UNUSED_13	((__force blist_flags_t)(1ULL << 13))
@@ -73,8 +74,7 @@
 #define __BLIST_HIGH_UNUSED (~(__BLIST_LAST_USED | \
 			       (__force blist_flags_t) \
 			       ((__force __u64)__BLIST_LAST_USED - 1ULL)))
-#define __BLIST_UNUSED_MASK (__BLIST_UNUSED_11 | \
-			     __BLIST_UNUSED_13 | \
+#define __BLIST_UNUSED_MASK (__BLIST_UNUSED_13 | \
 			     __BLIST_UNUSED_14 | \
 			     __BLIST_UNUSED_15 | \
 			     __BLIST_UNUSED_16 | \
-- 
2.30.2

Re: [PATCH v3 2/4] scsi: devinfo: add new flag BLIST_MEDIA_CHANGE

[Bart Van Assche]

On 3/28/21 3:25 AM, Martin Kepplinger wrote:
> +/* Ignore the next media change event */
> +#define BLIST_MEDIA_CHANGE	((__force blist_flags_t)(1ULL << 11))

That comment is not descriptive enough. Consider to change it into
something like the following: "ignore one MEDIA CHANGE unit attention
after resuming from runtime suspend".

Thanks,

Bart.

[PATCH v3 3/4] scsi: sd: use expecting_media_change for BLIST_MEDIA_CHANGE devices

[Martin Kepplinger]

For SD cardreader devices that have the BLIST_MEDIA_CHANGE flag set,
ignore one MEDIA CHANGE unit attention after resuming from runtime
suspend. These devices issue said unit attention when resuming
even when no medium changed.

expecting_media_change is the device flag that is being clearing in
the error path.

The "downside" is that for these devices we now rely on users not to
really change the medium (SD card) *during* a runtime suspend/resume
cycle, i.e. when not unmounting.

Since these devices don't distinguish between resume and medium changed
there's no better solution.

To enable runtime PM for an SD cardreader (device number 0:0:0:0), do:

echo 0 > /sys/module/block/parameters/events_dfl_poll_msecs
echo 1000 > /sys/bus/scsi/devices/0:0:0:0/power/autosuspend_delay_ms
echo auto > /sys/bus/scsi/devices/0:0:0:0/power/control

Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
---
 drivers/scsi/sd.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 91c34ee972c7..0a6413a4c629 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>
@@ -114,6 +115,7 @@ static void sd_shutdown(struct device *);
 static int sd_suspend_system(struct device *);
 static int sd_suspend_runtime(struct device *);
 static int sd_resume(struct device *);
+static int sd_resume_runtime(struct device *);
 static void sd_rescan(struct device *);
 static blk_status_t sd_init_command(struct scsi_cmnd *SCpnt);
 static void sd_uninit_command(struct scsi_cmnd *SCpnt);
@@ -608,7 +610,7 @@ static const struct dev_pm_ops sd_pm_ops = {
 	.poweroff		= sd_suspend_system,
 	.restore		= sd_resume,
 	.runtime_suspend	= sd_suspend_runtime,
-	.runtime_resume		= sd_resume,
+	.runtime_resume		= sd_resume_runtime,
 };
 
 static struct scsi_driver sd_template = {
@@ -3701,6 +3703,25 @@ static int sd_resume(struct device *dev)
 	return ret;
 }
 
+static int sd_resume_runtime(struct device *dev)
+{
+	struct scsi_disk *sdkp = dev_get_drvdata(dev);
+	struct scsi_device *sdp = sdkp->device;
+	int ret;
+
+	if (!sdkp)	/* E.g.: runtime resume at the start of sd_probe() */
+		return 0;
+
+	/*
+	 * This device issues a MEDIA CHANGE unit attention when
+	 * resuming from suspend. Ignore the next one from now.
+	 */
+	if (sdp->sdev_bflags & BLIST_MEDIA_CHANGE)
+		sdkp->device->expecting_media_change = 1;
+
+	return sd_resume(dev);
+}
+
 /**
  *	init_sd - entry point for this driver (both when built in or when
  *	a module).
-- 
2.30.2

Re: [PATCH v3 3/4] scsi: sd: use expecting_media_change for BLIST_MEDIA_CHANGE devices

[Bart Van Assche]

On 3/28/21 3:25 AM, Martin Kepplinger wrote:
> Since these devices don't distinguish between resume and medium changed
> there's no better solution.

Is there any information in the SCSI VPD pages that could be used to
determine whether or not the medium has been changed, e.g. in VPD page 0x83?

Thanks,

Bart.

[PATCH v3 4/4] scsi: devinfo: add BLIST_MEDIA_CHANGE for Ultra HS-SD/MMC usb cardreaders

[Martin Kepplinger]

This cardreader device issues a MEDIA CHANGE unit attention not only
when actually a medium changed but also simply when resuming from suspend.
(probably because the device can't know what happens during suspend and
want to say "medium could have changed").

Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
---
 drivers/scsi/scsi_devinfo.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index d92cec12454c..7d6446f81908 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -171,6 +171,7 @@ static struct {
 	{"FUJITSU", "ETERNUS_DXM", "*", BLIST_RETRY_ASC_C1},
 	{"Generic", "USB SD Reader", "1.00", BLIST_FORCELUN | BLIST_INQUIRY_36},
 	{"Generic", "USB Storage-SMC", NULL, BLIST_FORCELUN | BLIST_INQUIRY_36}, /* FW: 0180 and 0207 */
+	{"Generic", "Ultra HS-SD/MMC", "2.09", BLIST_MEDIA_CHANGE | BLIST_INQUIRY_36},
 	{"HITACHI", "DF400", "*", BLIST_REPORTLUN2},
 	{"HITACHI", "DF500", "*", BLIST_REPORTLUN2},
 	{"HITACHI", "DISK-SUBSYSTEM", "*", BLIST_REPORTLUN2},
-- 
2.30.2

Re: [PATCH v3 0/4] scsi: add runtime PM workaround for SD cardreaders

[Alan Stern]

On Sun, Mar 28, 2021 at 12:25:27PM +0200, Martin Kepplinger wrote:
> hi,
> 
> In short: there are SD cardreaders that send MEDIA_CHANGED on
> (runtime) resume. We cannot use runtime PM with these devices as
> I/O always fails. I'd like to discuss a way to fix this
> or at least allow us to work around this problem:

In fact, as far as I know _all_ USB SD card readers send Media Changed 
notifications on resume.  Maybe there are some that don't, but I haven't 
heard of any.

Alan Stern

Re: [PATCH v3 0/4] scsi: add runtime PM workaround for SD cardreaders

[Martin Kepplinger]

Am Sonntag, dem 28.03.2021 um 10:58 -0400 schrieb Alan Stern:
> On Sun, Mar 28, 2021 at 12:25:27PM +0200, Martin Kepplinger wrote:
> > hi,
> > 
> > In short: there are SD cardreaders that send MEDIA_CHANGED on
> > (runtime) resume. We cannot use runtime PM with these devices as
> > I/O always fails. I'd like to discuss a way to fix this
> > or at least allow us to work around this problem:
> 
> In fact, as far as I know _all_ USB SD card readers send Media
> Changed 
> notifications on resume.  Maybe there are some that don't, but I
> haven't 
> heard of any.
> 
> Alan Stern

that makes me worry less about enabling this for "Generic", "Ultra HS-
SD/MMC" then. thanks.

it also makes me think about whether sd should implement this even for
system-resume (not only runtime resume), but I guess that's a minor
issue we could add at any time later.

                               martin