Re: [PATCH 1/7] scsi: ufs: amend the ocs handling with fatal error

[Subhash Jadavani <subhashj@codeaurora.org>]

On 8/13/2013 5:20 PM, Seungwon Jeon wrote:
> On Mon, August 12, 2013, Subhash Jadavani wrote:
>> On 7/30/2013 6:32 PM, Seungwon Jeon wrote:
>>> On Mon, July 29, 2013, Sujit Reddy Thumma wrote:
>>>> On 7/29/2013 11:47 AM, Subhash Jadavani wrote:
>>>>> On 7/26/2013 7:15 PM, Seungwon Jeon wrote:
>>>>>> Fatal error in OCS(overall command status) field indicates
>>>>>> error conditions which is not covered by UFSHCI.
>>>>>> It means that host cannot define the result of command status
>>>>>> and therefore host may need to check transfer response UPIU's
>>>>>> response and status field.
>>>>>> It was actually found that 'CHECK CONDITION' is stored in status
>>>>>> field of UPIU where OCS is 'FATAL ERROR'.
>>>> It looks like you are assuming that there will be some kind of response
>>>> from the device. But the spec. mentions - "OCS_FATAL ERROR: within host
>>>> controller that is not covered by the error conditions described above
>>>> in this table."
>>> Yes, error interrupt doesn't happen actually.
>>>
>>>> So spec. left everything to implementers on how to trigger this error.
>>>> Also, it says "within host controller" so ufshcd_is_valid_req_rsp()
>>>> might fail as well.
>>>>
>>>> I couldn't understand why there is a need for a host controller to
>>>> interpret the SCSI command status and raise an OCS_FATAL_ERROR?
>>> I feel like OCS values are related to syntax problem except OCS_FATAL_ERROR.
>>> Basically, host controller may need to refer the Response field[Target Success/Target Failure] of
>> response UPIU.
>>> It's a overall result from response UPIU.
>>> When Response field is 'Target Failure', if host controller updates the OCS field with 'SUCCESS',
>> it's not proper behavior.
>>> In this case host may refer the Status field of response UPIU to decide the OCS result.
>>> Of course, it's not clear thing and could depends on host's implementation,
>>> because there is no obvious description for that.
>>> But if we consider the way to report UTP error from UFSHCI 8.2.4,
>>> we can check the Response UPIU for more detailed error condition regardless OCS value.
>>> Could you check your host side?
>> This is what our host documentation says: " SW may check the contents of
>> Response UPIU when OCS !=0 for debug purposes. It may (or may not) help
>> to understand the error scenario better. This is needed only for debug.
>> When system is stable, OCS should be 0." So this clearly means that we
>> shouldn't rely on the response UPIU if the OCS is non-zero hence simply
>> ignore it.
> Isn't  there any description for FATAL ERROR of OCS?
> Can I take like below?
> Even though Response field is 'Target Failure', OCS field can get a success result.
Yes, that's true. OCS could give success result for response field can 
be 'Target Failure'
> I just has focused what spec. says below:
> - FATAL ERROR within host controller that is not covered by the error conditions described above in this table.
> - The field contains the status code for the request.
> -  Host software may need to check Transfer Response UPIU's Response, and possibly Sense Data to find out more details for the cause of the error.
> Because host doesn't know and decide the error status, host could need to check.
No, i don't think it should be interpreted to read the response UPIU 
when OCS is equal to FATAL ERROR.
>
>>   > "When Response field is 'Target Failure', if host controller updates
>> the OCS field with 'SUCCESS', it's not proper behavior."
>>
>> I doubt whether your above understanding is true from Host controller
>> specification point of view. Host controller would not decode the
>> “Response” field of the “response UPIU”, it would only decode the
>> “Transaction Type” & “Task Tag” (and may be “LUN”) from the response
>> UPIU in order to find out the correct slot in the transfer/task request
>> list. Even 7.3.2.3 in UFSHCI spec also mentions the same.
>>
>> I guess with this i don't see a need for this patch unless you feel
>> otherwise.
> It may depend on host's implementation. HCI spec. doesn't guide for OCS in detail.
> Anyway, it's not clear just now.
>
> Thanks,
> Seungwon Jeon
>
>> Regards,
>> Subhash
>>
>>> Thanks,
>>> Seungwon Jeon
>>>
>>>> If it is clarified by the spec. then we can have generic implementation
>>>> otherwise I would prefer to make this specific to those host controllers
>>>> that raise OCS_FATAL_ERROR for CHECK_CONDITION.
>>>>
>>>>>> Signed-off-by: Seungwon Jeon <tgih.jun@samsung.com>
>>>>>> ---
>>>>>>     drivers/scsi/ufs/ufshcd.c |    3 +--
>>>>>>     1 files changed, 1 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>>>>> index b743bd6..4cf3a2d 100644
>>>>>> --- a/drivers/scsi/ufs/ufshcd.c
>>>>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>>>>> @@ -1199,7 +1199,7 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba,
>>>>>> struct ufshcd_lrb *lrbp)
>>>>>>         switch (ocs) {
>>>>>>         case OCS_SUCCESS:
>>>>>> -
>>>>>> +    case OCS_FATAL_ERROR:
>>>>>>             /* check if the returned transfer response is valid */
>>>>>>             result = ufshcd_is_valid_req_rsp(lrbp->ucd_rsp_ptr);
>>>>> I don't see the response UPIU data of the last command response is
>>>>> cleared anywhere in the driver. This means its quite possible that if
>>>>> the current command failed (and if it is using the same tag as the last
>>>>> succeeded command) with the OCS_FATAL_ERROR then response UPIU (pointed
>>>>> by lrbp->ucd_rsp_ptr) content may be still the same as the last one. If
>>>>> we ensure to clear the response UPIU data after every command completion
>>>>> then only we can rely on the response  UPIU content in case of fatal
>>>>> errors.
>>>>>
>>>>> Regards,
>>>>> Subhash
>>>>>>             if (result) {
>>>>>> @@ -1229,7 +1229,6 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba,
>>>>>> struct ufshcd_lrb *lrbp)
>>>>>>         case OCS_MISMATCH_DATA_BUF_SIZE:
>>>>>>         case OCS_MISMATCH_RESP_UPIU_SIZE:
>>>>>>         case OCS_PEER_COMM_FAILURE:
>>>>>> -    case OCS_FATAL_ERROR:
>>>>>>         default:
>>>>>>             result |= DID_ERROR << 16;
>>>>>>             dev_err(hba->dev,
>>>> --
>>>> Regards,
>>>> Sujit
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html