| Message ID | 20210328102531.1114535-1-martin.kepplinger@puri.sm |
|---|---|
| Headers | show |
| Series | scsi: add runtime PM workaround for SD cardreaders | expand |
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
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.
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.
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);
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(-)