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

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

On 30/11/2021 19:51, Bart Van Assche wrote:
> On 11/29/21 10:41 PM, Adrian Hunter wrote:
>> On 29/11/2021 21:32, Bart Van Assche wrote:
>>> * 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?
> 
> By using the block layer request allocation functions the block layer guarantees
> that each tag is in use in only one context. When bypassing the block layer code
> would have to be inserted in ufshcd_exec_dev_cmd() and ufshcd_issue_devman_upiu_cmd()
> to serialize these functions.

They already are serialized, but you are essentially saying the functionality
being duplicated is just a lock.  What you are proposing seems awfully
complicated just to get the functionality of a lock.

> In other words, we would be duplicating existing
> functionality if we bypass the block layer. The recommended approach in the Linux
> kernel is not to duplicate existing functionality.

More accurately, the functionality would not be being used at all, so not
really any duplication.