All of lore.kernel.org
 help / color / mirror / Atom feed
From: Martin Kepplinger <martin.kepplinger@puri.sm>
To: James Bottomley <James.Bottomley@HansenPartnership.com>,
	Alan Stern <stern@rowland.harvard.edu>
Cc: Bart Van Assche <bvanassche@acm.org>,
	Can Guo <cang@codeaurora.org>,
	martin.petersen@oracle.com, linux-scsi@vger.kernel.org,
	linux-kernel@vger.kernel.org, kernel@puri.sm
Subject: Re: [PATCH] scsi: sd: add runtime pm to open / release
Date: Thu, 30 Jul 2020 10:52:14 +0200	[thread overview]
Message-ID: <d3fe36a9-b785-a5c4-c90d-b8fa10f4272f@puri.sm> (raw)
In-Reply-To: <1596047349.4356.84.camel@HansenPartnership.com>

On 29.07.20 20:29, James Bottomley wrote:
> On Wed, 2020-07-29 at 14:25 -0400, Alan Stern wrote:
>> On Wed, Jul 29, 2020 at 06:43:48PM +0200, Martin Kepplinger wrote:
> [...]
>>>>> --- a/drivers/scsi/scsi_error.c
>>>>> +++ b/drivers/scsi/scsi_error.c
>>>>> @@ -554,16 +554,8 @@ int scsi_check_sense(struct scsi_cmnd
>>>>> *scmd)
>>>>>                  * so that we can deal with it there.
>>>>>                  */
>>>>>                 if (scmd->device->expecting_cc_ua) {
>>>>> -                       /*
>>>>> -                        * Because some device does not queue
>>>>> unit
>>>>> -                        * attentions correctly, we carefully
>>>>> check
>>>>> -                        * additional sense code and qualifier
>>>>> so as
>>>>> -                        * not to squash media change unit
>>>>> attention.
>>>>> -                        */
>>>>> -                       if (sshdr.asc != 0x28 || sshdr.ascq !=
>>>>> 0x00)
>>>>> {
>>>>> -                               scmd->device->expecting_cc_ua =
>>>>> 0;
>>>>> -                               return NEEDS_RETRY;
>>>>> -                       }
>>>>> +                       scmd->device->expecting_cc_ua = 0;
>>>>> +                       return NEEDS_RETRY;
>>>>
>>>> Well, yes, but you can't do this because it would lose us media
>>>> change events in the non-suspend/resume case which we really
>>>> don't want.  That's why I was suggesting a new flag.
>>>>
>>>> James
>>>
>>> also if I set expecting_cc_ua in resume() only, like I did?
>>
>> That wouldn't make any difference.  The information sent by your
>> card reader has sshdr.asc == 0x28 and sshdr.ascq == 0x00 (you can see
>> it in the log).  So because of the code here in scsi_check_sense(),
>> which you can't change, the Unit Attention sent by the card reader
>> would not be  retried even if you do set the flag in resume().
> 
> But if we had a new flag, like expecting_media_change, you could set
> that in resume and we could condition the above test in the code on it
> and reset it and do a retry if it gets set.  I'm not saying we have to
> do it this way, but it sounds like we have to do something in the
> kernel, so I think the flag will become necessary but there might be a
> bit of policy based dance around how it gets set in the kernel (to
> avoid losing accurate media change events).
> 
> James
> 

Maybe I should just start a new discussion with a patch, but the below
is what makes sense to me (when I understand you correctly) and seems to
work. I basically add a new flag, so that the old flags behave unchanged
and only call it during *runtime* resume for SD cards:


--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -553,15 +553,21 @@ int scsi_check_sense(struct scsi_cmnd *scmd)
                 * information that we should pass up to the upper-level
driver
                 * so that we can deal with it there.
                 */
-               if (scmd->device->expecting_cc_ua) {
+               if (scmd->device->expecting_cc_ua ||
+                   scmd->device->expecting_media_change) {
                        /*
                         * Because some device does not queue unit
                         * attentions correctly, we carefully check
                         * additional sense code and qualifier so as
-                        * not to squash media change unit attention.
+                        * not to squash media change unit attention;
+                        * unless expecting_media_change is set, indicating
+                        * that the media (most likely) didn't change
+                        * but a device only believes so (for example
+                        * because of suspend/resume).
                         */
-                       if (sshdr.asc != 0x28 || sshdr.ascq != 0x00) {
-                               scmd->device->expecting_cc_ua = 0;
+                       if ((sshdr.asc != 0x28 || sshdr.ascq != 0x00) ||
+                           scmd->device->expecting_media_change) {
+                               scmd->device->expecting_media_change = 0;
                                return NEEDS_RETRY;
                        }
                }
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index d90fefffe31b..b647fab2b663 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);
@@ -574,7 +575,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 = {
@@ -3652,6 +3653,21 @@ 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);
+
+       /* Some SD cardreaders report media change when resuming from
suspend
+        * because they can't keep track during suspend. */
+
+       /* XXX This is not unproblematic though: We won't notice when a card
+        * was really changed during runtime suspend! We basically rely
on users
+        * to unmount or suspend before doing so. */
+       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/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index bc5909033d13..8c8f053f71c8 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -169,6 +169,8 @@ 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 change
ASC/ASCQ
+                                             when it actually doesn't
change */
        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 */

  reply	other threads:[~2020-07-30  8:52 UTC|newest]

Thread overview: 68+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-23 11:10 [PATCH] scsi: sd: add runtime pm to open / release Martin Kepplinger
2020-06-24 13:33 ` Bart Van Assche
2020-06-25  8:16   ` Martin Kepplinger
2020-06-25 14:52     ` Alan Stern
2020-06-26  3:53     ` Bart Van Assche
2020-06-26 15:07     ` Bart Van Assche
2020-06-26 15:44       ` Alan Stern
2020-06-28  2:37         ` Bart Van Assche
2020-06-28 13:10           ` Alan Stern
2020-06-29  9:42         ` Martin Kepplinger
2020-06-29 16:15           ` Alan Stern
2020-06-29 16:56             ` Bart Van Assche
2020-06-29 17:40               ` Alan Stern
2020-06-30  3:33             ` Martin Kepplinger
2020-06-30 13:38               ` Alan Stern
2020-06-30 15:59             ` Bart Van Assche
2020-06-30 18:02               ` Alan Stern
2020-06-30 19:23                 ` Bart Van Assche
2020-06-30 19:38                   ` Alan Stern
2020-06-30 23:31                     ` Bart Van Assche
2020-07-01  0:49                       ` Alan Stern
2020-07-06 16:41                         ` Alan Stern
2020-07-28  7:02                           ` Martin Kepplinger
2020-07-28 20:02                             ` Alan Stern
2020-07-29 14:12                               ` Martin Kepplinger
2020-07-29 14:32                                 ` Alan Stern
2020-07-29 14:44                                   ` Martin K. Petersen
2020-07-29 14:56                                     ` Alan Stern
2020-07-29 14:46                                   ` James Bottomley
2020-07-29 14:53                                     ` James Bottomley
2020-07-29 15:40                                       ` Martin Kepplinger
2020-07-29 15:44                                         ` James Bottomley
2020-07-29 16:43                                           ` Martin Kepplinger
2020-07-29 18:25                                             ` Alan Stern
2020-07-29 18:29                                               ` James Bottomley
2020-07-30  8:52                                                 ` Martin Kepplinger [this message]
2020-07-30  8:54                                                   ` Martin Kepplinger
2020-07-30 15:10                                                   ` Alan Stern
2020-08-04  9:39                                                     ` Martin Kepplinger
2020-08-07  9:51                                                       ` Martin Kepplinger
2020-08-07 14:30                                                         ` Alan Stern
2020-08-08  6:59                                                           ` Martin Kepplinger
2020-08-08 15:05                                                             ` Alan Stern
2020-08-09  9:20                                                               ` Martin Kepplinger
2020-08-09 15:26                                                                 ` Alan Stern
2020-08-10 12:03                                                                   ` Martin Kepplinger
2020-08-10 14:13                                                                     ` Alan Stern
2020-08-11  7:55                                                                       ` Martin Kepplinger
2020-08-11 13:48                                                                         ` Alan Stern
2020-08-23 14:57                                                                         ` [PATCH] block: Fix bug in runtime-resume handling Alan Stern
2020-08-24 17:48                                                                           ` Bart Van Assche
2020-08-24 20:13                                                                             ` Alan Stern
2020-08-26  7:48                                                                               ` Martin Kepplinger
2020-08-27 17:42                                                                                 ` Martin Kepplinger
2020-08-27 20:29                                                                                   ` Alan Stern
2020-08-29  7:24                                                                                     ` Martin Kepplinger
2020-08-29 15:26                                                                                       ` Alan Stern
2020-08-29 16:33                                                                                         ` Martin Kepplinger
2020-08-29 18:56                                                                                           ` Alan Stern
2020-08-30  0:38                                                                                             ` Bart Van Assche
2020-08-30  1:06                                                                                               ` Alan Stern
2020-07-29 15:40                                       ` [PATCH] scsi: sd: add runtime pm to open / release Alan Stern
2020-07-29 15:49                                         ` James Bottomley
2020-07-29 16:17                                           ` Alan Stern
2020-07-29 15:52                                         ` Martin Kepplinger
2020-07-29 18:10                                   ` Douglas Gilbert
2020-07-30  8:05             ` Martin Kepplinger
2020-07-30 15:14               ` Alan Stern

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d3fe36a9-b785-a5c4-c90d-b8fa10f4272f@puri.sm \
    --to=martin.kepplinger@puri.sm \
    --cc=James.Bottomley@HansenPartnership.com \
    --cc=bvanassche@acm.org \
    --cc=cang@codeaurora.org \
    --cc=kernel@puri.sm \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    --cc=stern@rowland.harvard.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.