[PATCH v2 0/3] scsi: add runtime PM workaround for SD cardreaders

[PATCH v2 0/3] scsi: add runtime PM workaround for SD cardreaders

[Martin Kepplinger]

revision history
----------------
v2:
 * move module parameter to sd
 * add Documentation

v1:
https://lore.kernel.org/linux-scsi/20210111152029.28426-1-martin.kepplinger@puri.sm/T/#t



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 basically always fails. I'd like to discuss a way to fix this
or at least allow users 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/

and I sent the first of these patches in August, as a reference:
https://lore.kernel.org/linux-scsi/20200824190400.12339-1-martin.kepplinger@puri.sm/
so this is where I'm following up on.

I'm not sure whether maintaining an in-kernel quirk for specific devices
makes sense so here I suggest adding a userspace knob. This way there's at
least a chance to use runtime PM for sd cardreaders that send MEDIA_CHANGED.

I'd appreciate any feedback.

Martin Kepplinger (3):
  scsi: add expecting_media_change flag to error path
  scsi: sd: add ignore_resume_medium_changed disk setting
  scsi: sd: Documentation: describe ignore_resume_medium_changed

 Documentation/scsi/sd-parameters.rst | 14 ++++++++
 drivers/scsi/scsi_error.c            | 36 +++++++++++++++++---
 drivers/scsi/sd.c                    | 50 +++++++++++++++++++++++++++-
 drivers/scsi/sd.h                    |  1 +
 include/scsi/scsi_device.h           |  1 +
 5 files changed, 96 insertions(+), 6 deletions(-)

-- 
2.20.1

[PATCH v2 1/3] 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.

Add a flag for drivers to use when this is expected. It's handled in the
scsi core error path and allows to use (runtime) PM when it has
not been possible before on said hardware.

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

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

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/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 f11f51e2465f..f3b34c142088 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -573,6 +573,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 -
@@ -1959,14 +1971,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 1a5c9a3df6d6..ca2c3eb5830f 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -170,6 +170,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.20.1

[PATCH v2 2/3] scsi: sd: add ignore_resume_medium_changed disk setting

[Martin Kepplinger]

Add a userspace knob for scsi disks that deliver a MEDIA CHANGED
unit attention when the device actually only resumes from (runtime) suspend.
Those devices need the new ignore_resume_medium_changed knob set to 1
in order to be able to use runtime PM.

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

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

Set ignore_resume_medium_changed to 1 if you experience this problem.
Otherwise the unit attention would trigger I/O failure like the following
when using the mounted disk:

[  167.603864] sd 0:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x00
driverbyte=0x08 cmd_age=0s
[  167.603892] sd 0:0:0:0: [sda] tag#0 Sense Key : 0x6 [current]
[  167.603899] sd 0:0:0:0: [sda] tag#0 ASC=0x28 ASCQ=0x0
[  167.603909] sd 0:0:0:0: [sda] tag#0 CDB: opcode=0x2a 2a 00 01 c4 08 98 00 00 10 00
[  167.603915] blk_update_request: I/O error, dev sda, sector 29624472 op
0x1:(WRITE) flags 0x800 phys_seg 2 prio class 0
[  167.614750] Aborting journal on device sda1-8.
[  167.619460] sd 0:0:0:0: [sda] tag#0 device offline or changed
[  167.625342] blk_update_request: I/O error, dev sda, sector 29624320 op
0x1:(WRITE) flags 0x800 phys_seg 1 prio class 0
[  167.636161] Buffer I/O error on dev sda1, logical block 3702784, lost sync page write
[  167.644132] JBD2: Error -5 detected when updating journal superblock for sda1-8.

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

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index a3d2d4bc4a3d..14b850d2af59 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -114,6 +114,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);
@@ -375,6 +376,33 @@ thin_provisioning_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(thin_provisioning);
 
+static ssize_t
+ignore_resume_medium_changed_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct scsi_disk *sdkp = to_scsi_disk(dev);
+
+	return sprintf(buf, "%u\n", sdkp->ignore_resume_medium_changed);
+}
+
+static ssize_t
+ignore_resume_medium_changed_store(struct device *dev, struct device_attribute *attr,
+		    const char *buf, size_t count)
+{
+	bool v;
+	struct scsi_disk *sdkp = to_scsi_disk(dev);
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EACCES;
+
+	if (kstrtobool(buf, &v))
+		return -EINVAL;
+
+	sdkp->ignore_resume_medium_changed = v;
+
+	return count;
+}
+static DEVICE_ATTR_RW(ignore_resume_medium_changed);
+
 /* sysfs_match_string() requires dense arrays */
 static const char *lbp_mode[] = {
 	[SD_LBP_FULL]		= "full",
@@ -591,6 +619,7 @@ static struct attribute *sd_disk_attrs[] = {
 	&dev_attr_max_medium_access_timeouts.attr,
 	&dev_attr_zoned_cap.attr,
 	&dev_attr_max_retries.attr,
+	&dev_attr_ignore_resume_medium_changed.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(sd_disk);
@@ -608,7 +637,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 = {
@@ -3699,6 +3728,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);
+	int ret;
+
+	if (!sdkp)	/* E.g.: runtime resume at the start of sd_probe() */
+		return 0;
+
+	/*
+	 * ignore_resume_media_change is the userspace setting and
+	 * expecting_media_change is what is checked and cleared in the
+	 * error path if we set it here.
+	 */
+	if (sdkp->ignore_resume_medium_changed)
+		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).
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index b59136c4125b..1b041331356c 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -125,6 +125,7 @@ struct scsi_disk {
 	unsigned	urswrz : 1;
 	unsigned	security : 1;
 	unsigned	ignore_medium_access_errors : 1;
+	unsigned	ignore_resume_medium_changed : 1;
 };
 #define to_scsi_disk(obj) container_of(obj,struct scsi_disk,dev)
 
-- 
2.20.1

[PATCH v2 3/3] scsi: sd: Documentation: describe ignore_resume_medium_changed

[Martin Kepplinger]

Add notes about the new sd sysfs knob that works around problems
with runtime PM for certain types of SD cardreaders.

Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
---
 Documentation/scsi/sd-parameters.rst | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/scsi/sd-parameters.rst b/Documentation/scsi/sd-parameters.rst
index 87d554008bfb..a77b9fdffddf 100644
--- a/Documentation/scsi/sd-parameters.rst
+++ b/Documentation/scsi/sd-parameters.rst
@@ -25,3 +25,17 @@ To modify the caching mode without making the change persistent, prepend
 "temporary " to the cache type string. E.g.::
 
   # echo "temporary write back" > cache_type
+
+ignore_resume_medium_changed (RW)
+---------------------------------
+Some SD cardreaders deliver a "media changed" unit attention (that results
+in I/O error) when they are resumed from suspend. This prevents users
+to use runtime PM with these devices. To enable runtime PM for an SD
+cardreader (here, device number 0:0:0:0), do something like:
+
+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
+
+And if using the mounted disk filesystem causes trouble, try setting
+ignore_resume_medium_changed to 1.
-- 
2.20.1

Re: [PATCH v2 0/3] scsi: add runtime PM workaround for SD cardreaders

[Martin Kepplinger]

Am Dienstag, dem 12.01.2021 um 10:33 +0100 schrieb Martin Kepplinger:
> revision history
> ----------------
> v2:
>  * move module parameter to sd
>  * add Documentation
> 
> v1:
> https://lore.kernel.org/linux-scsi/20210111152029.28426-1-martin.kepplinger@puri.sm/T/#t
> 
> 
> 
> 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 basically always fails. I'd like to discuss a way to fix this
> or at least allow users 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/
> 
> and I sent the first of these patches in August, as a reference:
> https://lore.kernel.org/linux-scsi/20200824190400.12339-1-martin.kepplinger@puri.sm/
> so this is where I'm following up on.
> 
> I'm not sure whether maintaining an in-kernel quirk for specific
> devices
> makes sense so here I suggest adding a userspace knob. This way
> there's at
> least a chance to use runtime PM for sd cardreaders that send
> MEDIA_CHANGED.
> 
> I'd appreciate any feedback.
> 
> Martin Kepplinger (3):
>   scsi: add expecting_media_change flag to error path
>   scsi: sd: add ignore_resume_medium_changed disk setting
>   scsi: sd: Documentation: describe ignore_resume_medium_changed
> 
>  Documentation/scsi/sd-parameters.rst | 14 ++++++++
>  drivers/scsi/scsi_error.c            | 36 +++++++++++++++++---
>  drivers/scsi/sd.c                    | 50
> +++++++++++++++++++++++++++-
>  drivers/scsi/sd.h                    |  1 +
>  include/scsi/scsi_device.h           |  1 +
>  5 files changed, 96 insertions(+), 6 deletions(-)
> 

hi James, Bart and all,

since this is absolutely needed for runtime pm with the SD device we
use I assume there are others that would benefit from this too. Do you
have any concerns or thoughts about this (logic and interface)?

the patches still apply.

thanks a lot,

                                     martin

Re: [PATCH v2 0/3] scsi: add runtime PM workaround for SD cardreaders

[Bart Van Assche]

On 3/27/21 3:48 AM, Martin Kepplinger wrote:
> since this is absolutely needed for runtime pm with the SD device we
> use I assume there are others that would benefit from this too. Do you
> have any concerns or thoughts about this (logic and interface)?

Hi Martin,

Since the issue addressed by this patch series concerns a controller
bug, please use the SCSI core BLIST mechanism instead of introducing a
new sysfs attribute. A sysfs attribute could be misconfigured. BLIST
attributes however are set from inside the kernel and hence do not need
help from userspace. See e.g. c360652006bb ("scsi: devinfo:
BLIST_RETRY_ASC_C1 for Fujitsu ETERNUS").

Thanks,

Bart.