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

[PATCH 0/3] 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 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 setting. Of course we should
document it properly if this makes sense to you. But this way there's at least
a chance to use runtime PM for sd cardreaders that send MEDIA_CHANGED.

Questions would be:
* Do you like the approach of a user+internal flag?
* what to do in scsi drivers that ignore the flag? Is documentation enough?
* review the *clearing* of the (internal) flag once again. the first
  occurrence of MEDIA_CHANGED should do that only. (note to myself)

I'd appreciate any feedback.


Martin Kepplinger (3):
  scsi: add expecting_media_change flag to error path
  scsi: add expect_media_change_suspend sysfs device setting
  scsi: sd: add support for expect_media_change_suspend flag

 drivers/scsi/scsi_error.c  | 36 +++++++++++++++++++++++++++++++-----
 drivers/scsi/scsi_sysfs.c  | 38 ++++++++++++++++++++++++++++++++++++++
 drivers/scsi/sd.c          | 21 ++++++++++++++++++++-
 include/scsi/scsi_device.h |  3 +++
 4 files changed, 92 insertions(+), 6 deletions(-)

-- 
2.20.1

[PATCH 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 2/3] scsi: add expect_media_change_suspend sysfs device setting

[Martin Kepplinger]

Add a user-facing flag that sets expecting_media_change on runtime
resume. That works around devices that send MEDIA_CHANGED when it
actually is just resumed from suspend and the media can be expected
not to have changed.

Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
---
 drivers/scsi/scsi_sysfs.c  | 38 ++++++++++++++++++++++++++++++++++++++
 include/scsi/scsi_device.h |  2 ++
 2 files changed, 40 insertions(+)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index b6378c8ca783..a049290addff 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1189,6 +1189,43 @@ static DEVICE_ATTR(queue_ramp_up_period, S_IRUGO | S_IWUSR,
 		   sdev_show_queue_ramp_up_period,
 		   sdev_store_queue_ramp_up_period);
 
+static ssize_t
+sdev_show_expect_media_change_suspend(struct device *dev,
+				      struct device_attribute *attr, char *buf)
+{
+	struct scsi_device *sdev;
+	sdev = to_scsi_device(dev);
+
+	if (sdev->expect_media_change_suspend)
+		return sprintf(buf, "1\n");
+	else
+		return sprintf(buf, "0\n");
+}
+
+static ssize_t
+sdev_store_expect_media_change_suspend(struct device *dev,
+				       struct device_attribute *attr,
+				       const char *buf, size_t count)
+{
+	struct scsi_device *sdev;
+	unsigned int flag;
+	int err;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EACCES;
+
+	sdev = to_scsi_device(dev);
+	err = kstrtouint(buf, 10, &flag);
+	if (err)
+		return err;
+	sdev->expect_media_change_suspend = !!flag;
+
+	return count;
+}
+static DEVICE_ATTR(expect_media_change_suspend, S_IRUGO | S_IWUSR,
+		   sdev_show_expect_media_change_suspend,
+		   sdev_store_expect_media_change_suspend);
+
 static umode_t scsi_sdev_attr_is_visible(struct kobject *kobj,
 					 struct attribute *attr, int i)
 {
@@ -1260,6 +1297,7 @@ static struct attribute *scsi_sdev_attrs[] = {
 	&dev_attr_queue_type.attr,
 	&dev_attr_wwid.attr,
 	&dev_attr_blacklist.attr,
+	&dev_attr_expect_media_change_suspend.attr,
 #ifdef CONFIG_SCSI_DH
 	&dev_attr_dh_state.attr,
 	&dev_attr_access_state.attr,
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index ca2c3eb5830f..fafb8e6ea4d0 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -171,6 +171,8 @@ struct scsi_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 expect_media_change_suspend:1; /* User facing flag to enable
+						 * the above flag on runtime resume */
 	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 3/3] scsi: sd: add support for expect_media_change_suspend flag

[Martin Kepplinger]

Make the sd driver act appropriately when the user has set
expect_media_change_suspend for a device.

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

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index a3d2d4bc4a3d..ad89f8c76a27 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -608,7 +608,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 +3699,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;
+
+	/*
+	 * expect_media_change_suspend 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->device->expect_media_change_suspend)
+		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.20.1

Re: [PATCH 3/3] scsi: sd: add support for expect_media_change_suspend flag

[Martin Kepplinger]

On 11.01.21 16:20, Martin Kepplinger wrote:
> Make the sd driver act appropriately when the user has set
> expect_media_change_suspend for a device.
> 
> Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> ---
>   drivers/scsi/sd.c | 21 ++++++++++++++++++++-
>   1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index a3d2d4bc4a3d..ad89f8c76a27 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -608,7 +608,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 +3699,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;
> +
> +	/*
> +	 * expect_media_change_suspend 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->device->expect_media_change_suspend)
> +		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).
> 

oops, I'm very sorry, but the following is missing in order for this to 
build:

--- 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);

Re: [PATCH 3/3] scsi: sd: add support for expect_media_change_suspend flag

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2835 bytes --]

Hi Martin,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on mkp-scsi/for-next]
[also build test ERROR on scsi/for-next v5.11-rc3 next-20210111]
[cannot apply to target/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Martin-Kepplinger/scsi-add-runtime-PM-workaround-for-SD-cardreaders/20210111-232622
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: riscv-randconfig-r023-20210111 (attached as .config)
compiler: riscv32-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/7d6cb920cac7d125589fb70381b8ab199d7f0d67
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Martin-Kepplinger/scsi-add-runtime-PM-workaround-for-SD-cardreaders/20210111-232622
        git checkout 7d6cb920cac7d125589fb70381b8ab199d7f0d67
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/scsi/sd.c:611:21: error: 'sd_resume_runtime' undeclared here (not in a function); did you mean 'sd_suspend_runtime'?
     611 |  .runtime_resume  = sd_resume_runtime,
         |                     ^~~~~~~~~~~~~~~~~
         |                     sd_suspend_runtime
   drivers/scsi/sd.c: In function 'sd_remove':
   drivers/scsi/sd.c:3513:8: warning: variable 'devt' set but not used [-Wunused-but-set-variable]
    3513 |  dev_t devt;
         |        ^~~~
   drivers/scsi/sd.c: In function 'sd_resume_runtime':
   drivers/scsi/sd.c:3705:6: warning: unused variable 'ret' [-Wunused-variable]
    3705 |  int ret;
         |      ^~~
   At top level:
   drivers/scsi/sd.c:3702:12: warning: 'sd_resume_runtime' defined but not used [-Wunused-function]
    3702 | static int sd_resume_runtime(struct device *dev)
         |            ^~~~~~~~~~~~~~~~~


vim +611 drivers/scsi/sd.c

   604	
   605	static const struct dev_pm_ops sd_pm_ops = {
   606		.suspend		= sd_suspend_system,
   607		.resume			= sd_resume,
   608		.poweroff		= sd_suspend_system,
   609		.restore		= sd_resume,
   610		.runtime_suspend	= sd_suspend_runtime,
 > 611		.runtime_resume		= sd_resume_runtime,
   612	};
   613	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35241 bytes --]