Re: [PATCH v2 11/20] scsi: ufs: Switch to scsi_(get|put)_internal_cmd()

[Adrian Hunter <adrian.hunter@intel.com>]

On 29/11/2021 21:32, Bart Van Assche wrote:
> On 11/24/21 3:02 AM, Adrian Hunter wrote:
>> On 19/11/2021 21:57, Bart Van Assche wrote:
>>> The only functional change in this patch is the addition of a
>>> blk_mq_start_request() call in ufshcd_issue_devman_upiu_cmd().
>>>
>>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>>> ---
>>>   drivers/scsi/ufs/ufshcd.c | 46 +++++++++++++++++++++++++--------------
>>>   1 file changed, 30 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>> index fced4528ee90..dfa5f127342b 100644
>>> --- a/drivers/scsi/ufs/ufshcd.c
>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>> @@ -2933,6 +2933,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
>>>   {
>>>       struct request_queue *q = hba->cmd_queue;
>>>       DECLARE_COMPLETION_ONSTACK(wait);
>>> +    struct scsi_cmnd *scmd;
>>>       struct request *req;
>>>       struct ufshcd_lrb *lrbp;
>>>       int err;
>>> @@ -2945,15 +2946,18 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
>>>        * Even though we use wait_event() which sleeps indefinitely,
>>>        * the maximum wait time is bounded by SCSI request timeout.
>>>        */
>>> -    req = blk_mq_alloc_request(q, REQ_OP_DRV_OUT, 0);
>>> -    if (IS_ERR(req)) {
>>> -        err = PTR_ERR(req);
>>> +    scmd = scsi_get_internal_cmd(q, DMA_TO_DEVICE, 0);
>>
>> We do not need the block layer, nor SCSI commands which begs the question,
>> why involve them at all?
>>
>> For example, the following is much simpler and seems to work:
>> [ ... ]
> 
> That patch bypasses the block layer for device management commands. So that
> patch breaks a very basic assumption on which the block layer has been built,
> namely that the block layer core knows whether or not any requests are ongoing.
> That patch breaks at least the following functionality:
> * Run-time power management. blk_pre_runtime_suspend() checks whether
>   q_usage_counter is zero before initiating runtime power management.
>   q_usage_counter is increased by blk_mq_alloc_request() and decreased by
>   blk_mq_free_request(). I don't think it is safe to change the power state
>   while a device management request is in progress.

hba->cmd_queue does not have runtime PM.

I suspect making it do runtime PM might open up deadlock issues similar to
the issues that were seen with sending sending clear WLUN UA from the
UFS error handler.

> * The code in blk_cleanup_queue() that waits for pending requests to finish
>   before resources associated with the request queue are freed.
>   ufshcd_remove() calls blk_cleanup_queue(hba->cmd_queue) and hence waits until
>   pending device management commands have finished. That would no longer be the
>   case if the block layer is bypassed to submit device management commands.

cmd_queue is used only by the UFS driver, so if the driver is racing with
itself at "remove", then that should be fixed. The risk is not that the UFS
driver might use requests, but that it might still be operating when hba or
other resources get freed.

So the question remains, for device commands, we do not need the block
layer, nor SCSI commands which still begs the question, why involve them
at all?