Re: [PATCH v4 09/10] scsi: ufs: Introduce the function ufshcd_execute_start_stop()

Mike Christie
2022-10-20T17:21:42Z

On 10/19/22 4:13 PM, Bart Van Assche wrote:
> On 10/19/22 12:57, Mike Christie wrote:
>> On 10/18/22 3:29 PM, Bart Van Assche wrote:
>>> Open-code scsi_execute() because a later patch will modify scmd->flags
>>> and because scsi_execute() does not support setting scmd->flags. No
>>> functionality is changed.
>>>
>>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>>> ---
>>>   drivers/ufs/core/ufshcd.c | 39 ++++++++++++++++++++++++++++++++++-----
>>>   1 file changed, 34 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>>> index 2a32bcc93d2e..c5ccc7ba583b 100644
>>> --- a/drivers/ufs/core/ufshcd.c
>>> +++ b/drivers/ufs/core/ufshcd.c
>>> @@ -8729,6 +8729,39 @@ static void ufshcd_hba_exit(struct ufs_hba *hba)
>>>       }
>>>   }
>>>   +static int ufshcd_execute_start_stop(struct scsi_device *sdev,
>>> +                     enum ufs_dev_pwr_mode pwr_mode,
>>> +                     struct scsi_sense_hdr *sshdr)
>>> +{
>>> +    unsigned char cdb[6] = { START_STOP, 0, 0, 0, pwr_mode << 4, 0 };
>>> +    struct request *req;
>>> +    struct scsi_cmnd *scmd;
>>> +    int ret;
>>> +
>>> +    req = scsi_alloc_request(sdev->request_queue, REQ_OP_DRV_IN,
>>> +                 BLK_MQ_REQ_PM);
>>>
>>
>> Can you hit a case where we have run out of tags (__blk_mq_alloc_requests
>> is hitting the blk_mq_get_tag == BLK_MQ_NO_TAG check), the host has gone
>> into recovery and so commands are completing to add a tag back and then we
>> try to call this and get stuck waiting on a tag? Or for passthrough do we
>> have some special reserve?
>>
>> If so do you need to use BLK_MQ_REQ_NOWAIT here? Maybe do the retry loop
>> yourself like:
>>
>> retry:
>>     if host is in recovery
>>         return failure
>>
>>     req = scsi_alloc_request(.... BLK_MQ_REQ_NOWAIT)
>>     if (!req and we have not hit some retry limit)
>>         goto retry
>>
>>
>> or have some special reserve command/tag.
>
> Hi Mike,
>
> No other SCSI commands should be in progress when ufshcd_execute_start_stop() is called because that function is only called during system suspend and resume and no other I/O should be in progress at that time. Additionally, there is a mutual exclusion mechanism in the UFS driver to serialize system suspend/resume activity and error handling.

Looks ok to me then.

Reviewed-by: Mike Christie <michael.christie@oracle.com>