Re: [PATCH 1/2] scsi: ufs: Add support for sending NOP OUT UPIU

[Santosh Y <santoshsy@gmail.com>]

On Fri, Jun 14, 2013 at 1:10 PM, Sujit Reddy Thumma
<sthumma@codeaurora.org> wrote:
> On 6/13/2013 10:03 AM, Sujit Reddy Thumma wrote:
>>>
>>>   static struct scsi_host_template ufshcd_driver_template = {
>>> @@ -1771,8 +2064,8 @@ int ufshcd_init(struct device *dev, struct
>>> ufs_hba **hba_handle,
>>>          /* Configure LRB */
>>>          ufshcd_host_memory_configure(hba);
>>>
>>> -       host->can_queue = hba->nutrs;
>>> -       host->cmd_per_lun = hba->nutrs;
>>> +       host->can_queue = SCSI_CMD_QUEUE_SIZE;
>>
>>
>>
>> I don't think this is appropriate. Reserving a slot exclusively for
>> query/DM requests is not optimal.  can_queue should be changed
>> dynamically, scsi_adjust_queue_depth() maybe?
>
>
> The motivation to change the design for this patch  is that routing
> query command through SCSI layer raised problems when we are trying to
> improve the fatal error handling as we need to block the SCSI layer and
> recover the link. Hence, the need to have the query/DM command send as
> internal commands. Which probably makes sense.
>

If fatal error handling is the only concern then the only required
query/DM commands, I guess you are talking about fDeviceInit and NOP,
for the recovery can be sent internally. Moreover, it doesn't matter
on which available slot the query/DM commands are sent during the
fatal error handling since the SCSI layer is blocked. Once the
recovery completes,  query commands can go on without reserving an
exclusive slot.

> One possible option was to look for a free command slot whenever we
> try to send an internal command, but getting a free slot is not always
> guaranteed.
>
> Even if we get hold of a tag, there is no way we can explain this to
> SCSI/block layer that particular tag is in use internally (case where
> normal query requests are sent in tandem with SCSI requests). In which
> case, other option is to use tag[31] as you have said on need basis
> and change the queue depth to 31. This again has problems - one
> changing queue depth doesn't take effect immediately but for the next
> command. Second, if the command in tag[31] is the root cause of the
> fatal error and is stuck, then the internal command has to wait forever
> (until scsi timesout) to plan recovery. Considering, all these factors
> it is better to reserve a tag and use it for internal commands instead of
> playing around with the tags internally without/partially informing
> SCSI/block layers.
>
> I am open for discussion, if there are any suggestions to handle such LLD
> requirements in the SCSI layer optimally.
>
> Coming to how optimal is to work with 31 slots instead of h/w defined 32 is
> something which we can answer when we have true multi queueing. AFAIK, there
> may not exist real-world applications which utilize full 32 tags at
> particular instant. SATA AHCI controller driver which is ubiquitous in
> modern systems also reserves a slot for internal commands which is used only
> in case of error handling and AFAIK, no one has ever reported performance
> problems with this (its about 7 years the commit to reserve a slot is merged
> into Linux tree).
>
> I hope this explains the intent. Please let me know what do you think.
>

Always nutrs=32 is the assumption you are making. Spec mentions that
(1 <= nutrs <=32) and (1 <= nutmrs <= 8), and unfortunately I did come
across a host controller with nutrs=4, nutmrs=2 sometime back.
Reserving an exclusive slot for query in such cases would make a
difference. Hope this helps.


-- 
~Santosh