Re: [PATCH 1/4] scsi: ufs: Fix broken task management command implementation

[Sujit Reddy Thumma <sthumma@codeaurora.org>]

On 7/3/2013 9:53 PM, Santosh Y wrote:
> On Wed, Jul 3, 2013 at 9:22 PM, Sujit Reddy Thumma
> <sthumma@codeaurora.org> wrote:
>> On 7/2/2013 9:21 PM, Santosh Y wrote:
>>>
>>> On Fri, Jun 28, 2013 at 5:02 PM, Sujit Reddy Thumma
>>> <sthumma@codeaurora.org> wrote:
>>>>
>>>> On 6/27/2013 4:49 PM, Santosh Y wrote:
>>>>>>
>>>>>>
>>>>>>> +       spin_lock_irqsave(host->host_lock, flags);
>>>>>>>           task_req_descp = hba->utmrdl_base_addr;
>>>>>>>           task_req_descp += free_slot;
>>>>>>>
>>>>>>> @@ -2353,38 +2387,39 @@ ufshcd_issue_tm_cmd(struct ufs_hba *hba,
>>>>>>>                   (struct utp_upiu_task_req *)
>>>>>>> task_req_descp->task_req_upiu;
>>>>>>>           task_req_upiup->header.dword_0 =
>>>>>>>                   UPIU_HEADER_DWORD(UPIU_TRANSACTION_TASK_REQ, 0,
>>>>>>> -                                             lrbp->lun,
>>>>>>> lrbp->task_tag);
>>>>>>> +                               lun_id, free_slot);
>>>>>
>>>>>
>>>>> Actually it still doesn't fix the problem. The*task tag*  used here
>>>>>
>>>>> should be unique across the SCSI/Query and Task Managment UPIUs.
>>>>
>>>>
>>>>
>>>> I am sorry, I didn't get that. Why should it be unique across the
>>>> SCSI/Query? For example, if a machine supports 32 request slots and 8
>>>> task
>>>> management slots, then the task management command tag can be anything
>>>> out
>>>> of 8 slots.
>>>>
>>>
>>> The spec(ufs 1.1) has the requirement under  '10.5.2 Basic Header
>>> Format'->'Task Tag'.
>>> Couple of devices I came across had similar behavior. The tracking of
>>> UPIUs --even belonging to a separate group-- seemed to be based on the
>>> 'task tag' value rather than 'type of UPIU'->'task tag'.
>>
>>
>> Thanks for the clarification. So to make the task tag unique we should do
>> something like below:
>>
>> @@ -2940,9 +2941,10 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba *hba,
>> int lun_id, int task_id,
>>          /* Configure task request UPIU */
>>          task_req_upiup =
>>
>>                  (struct utp_upiu_task_req *) task_req_descp->task_req_upiu;
>> +       task_tag = hba->nutrs + free_slot;
>
> Yes, this was exactly my internal fix at the time :-)

Okay. I will update the patch with this. Thanks.

>
>>
>>          task_req_upiup->header.dword_0 =
>>                  UPIU_HEADER_DWORD(UPIU_TRANSACTION_TASK_REQ, 0,
>> -                               lun_id, free_slot);
>> +                               lun_id, task_tag);
>>
>> Will this work for the devices you came across?
>>


-- 
Regards,
Sujit