| Message ID | 20210916170211.8564-1-adrian.hunter@intel.com |
|---|---|
| Headers | show |
| Series | scsi: ufs: Let devices remain runtime suspended during system suspend | expand |
On 9/16/21 10:02 AM, Adrian Hunter wrote: > -static void ufshcd_request_sense_done(struct request *rq, blk_status_t error) > +static int ufshcd_request_sense_direct(struct ufs_hba *hba, u8 wlun) > { [ ... ] > + /* The command queue cannot be frozen */ > + req = blk_get_request(q, REQ_OP_DRV_OUT, 0); hba->cmd_queue shares a tag set with the request queues associated with SCSI LUNs. Since this blk_get_request() call happens from the context of the error handler before SCSI requests are unblocked, it will hang if all tags are in use for SCSI requests before the error handler starts. Bart.
On 17/09/21 7:09 pm, Bart Van Assche wrote: > On 9/16/21 10:02 AM, Adrian Hunter wrote: >> -static void ufshcd_request_sense_done(struct request *rq, blk_status_t error) >> +static int ufshcd_request_sense_direct(struct ufs_hba *hba, u8 wlun) >> { > [ ... ] >> + /* The command queue cannot be frozen */ >> + req = blk_get_request(q, REQ_OP_DRV_OUT, 0); > > hba->cmd_queue shares a tag set with the request queues associated with SCSI > LUNs. Since this blk_get_request() call happens from the context of the error > handler before SCSI requests are unblocked, it will hang if all tags are > in use for SCSI requests before the error handler starts. All the commands sent by ufshcd_probe_hba() take the same approach, so no difference there.
On 9/17/21 10:39 AM, Adrian Hunter wrote: > On 17/09/21 7:09 pm, Bart Van Assche wrote: >> On 9/16/21 10:02 AM, Adrian Hunter wrote: >>> -static void ufshcd_request_sense_done(struct request *rq, blk_status_t error) >>> +static int ufshcd_request_sense_direct(struct ufs_hba *hba, u8 wlun) >>> { >> [ ... ] >>> + /* The command queue cannot be frozen */ >>> + req = blk_get_request(q, REQ_OP_DRV_OUT, 0); >> >> hba->cmd_queue shares a tag set with the request queues associated with SCSI >> LUNs. Since this blk_get_request() call happens from the context of the error >> handler before SCSI requests are unblocked, it will hang if all tags are >> in use for SCSI requests before the error handler starts. > > All the commands sent by ufshcd_probe_hba() take the same approach, so no > difference there. The same comment applies to the commands sent by ufshcd_probe_hba() I think. If you would like to address this issue, how about passing the flag BLK_MQ_REQ_RESERVED to the blk_get_request() call in ufshcd_exec_dev_cmd()? The following additional changes are required to make this work: * Add a 'reserved_tags' member to struct scsi_host_template, e.g. close to tag_alloc_policy. * Modify scsi_mq_setup_tags() such that it copies the reserved_tags value from the host template into tag_set->reserved_tags. * Set 'reserved_tags' in the UFS driver SCSI host template. Thanks, Bart.
On 17/09/21 8:58 pm, Bart Van Assche wrote: > On 9/17/21 10:39 AM, Adrian Hunter wrote: >> On 17/09/21 7:09 pm, Bart Van Assche wrote: >>> On 9/16/21 10:02 AM, Adrian Hunter wrote: >>>> -static void ufshcd_request_sense_done(struct request *rq, blk_status_t error) >>>> +static int ufshcd_request_sense_direct(struct ufs_hba *hba, u8 wlun) >>>> { >>> [ ... ] >>>> + /* The command queue cannot be frozen */ >>>> + req = blk_get_request(q, REQ_OP_DRV_OUT, 0); >>> >>> hba->cmd_queue shares a tag set with the request queues associated with SCSI >>> LUNs. Since this blk_get_request() call happens from the context of the error >>> handler before SCSI requests are unblocked, it will hang if all tags are >>> in use for SCSI requests before the error handler starts. >> >> All the commands sent by ufshcd_probe_hba() take the same approach, so no >> difference there. > > The same comment applies to the commands sent by ufshcd_probe_hba() I think. > If you would like to address this issue, how about passing the flag > BLK_MQ_REQ_RESERVED to the blk_get_request() call in ufshcd_exec_dev_cmd()? > The following additional changes are required to make this work: > * Add a 'reserved_tags' member to struct scsi_host_template, e.g. close to > tag_alloc_policy. > * Modify scsi_mq_setup_tags() such that it copies the reserved_tags value > from the host template into tag_set->reserved_tags. > * Set 'reserved_tags' in the UFS driver SCSI host template. I think these things only happen on the reset path, so all the slots should be free, even if all the tags are allocated. With some care, it might be possible simply to grab a free slot.
On 9/20/21 10:30 AM, Adrian Hunter wrote: > On 17/09/21 8:58 pm, Bart Van Assche wrote: >> The same comment applies to the commands sent by ufshcd_probe_hba() I think. >> If you would like to address this issue, how about passing the flag >> BLK_MQ_REQ_RESERVED to the blk_get_request() call in ufshcd_exec_dev_cmd()? >> The following additional changes are required to make this work: >> * Add a 'reserved_tags' member to struct scsi_host_template, e.g. close to >> tag_alloc_policy. >> * Modify scsi_mq_setup_tags() such that it copies the reserved_tags value >> from the host template into tag_set->reserved_tags. >> * Set 'reserved_tags' in the UFS driver SCSI host template. > > I think these things only happen on the reset path, so all the slots should > be free, even if all the tags are allocated. With some care, it might be > possible simply to grab a free slot. Hmm ... my understanding is that ufshcd_try_to_abort_task() may fail and hence that it is not guaranteed that there will be free slot. Anyway, I'm fine with the patch in its current shape and implementing the mechanism explained in my previous email at a later time. Bart.
Hi UFS devices can remain runtime suspended at system suspend time, if the conditions are right. Add support for that, first fixing the impediments. Changes in V4: scsi: ufs: Fix error handler clear ua deadlock Do request-sense directly Changes in V3: scsi: ufs: Fix error handler clear ua deadlock Correct commit message. Amend stable tags to add dependent cherry picks Changes in V2: scsi: ufs: Let devices remain runtime suspended during system suspend The ufs-hisi driver uses different RPM and SPM, but it is made explicit by a new parameter to suspend prepare. Adrian Hunter (3): scsi: ufs: Fix error handler clear ua deadlock scsi: ufs: Fix runtime PM dependencies getting broken scsi: ufs: Let devices remain runtime suspended during system suspend drivers/scsi/scsi_pm.c | 16 ++- drivers/scsi/ufs/ufs-hisi.c | 8 +- drivers/scsi/ufs/ufs.h | 3 +- drivers/scsi/ufs/ufshcd.c | 255 ++++++++++++++++++++++++++++++++------------ drivers/scsi/ufs/ufshcd.h | 21 +++- include/scsi/scsi_device.h | 1 + include/trace/events/ufs.h | 5 +- 7 files changed, 231 insertions(+), 78 deletions(-) Regards Adrian