ufs: fix bugs in probing and removing driver paths

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'.

Signed-off-by Seungwon Jeon <[email protected]>

---
  drivers/scsi/ufs/ufshcd.c |    3 +--
  1 files changed, 1 insertions(+), 2 deletions(-)

drivers/scsi/ufs/ufshcd.c +0 -0

index b743bd6..4cf3a2d 100644

drivers/scsi/ufs/ufshcd.c +0 -1

  		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,

Quoted 19 lines · Seungwon Jeon

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'.

Signed-off-by Seungwon Jeon <[email protected]>

---
  drivers/scsi/ufs/ufshcd.c |    3 +--
  1 files changed, 1 insertions(+), 2 deletions(-)

drivers/scsi/ufs/ufshcd.c +0 -0

index b743bd6..4cf3a2d 100644
@@ -1199,7 +1199,7 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 

drivers/scsi/ufs/ufshcd.c ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) · line 1204

Quoted 6 lines

drivers/scsi/ufs/ufshcd.c +1 -1

  	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.

Quoted 20 lines · Subhash Jadavani

Quoted 19 lines · Seungwon Jeon

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'.

Signed-off-by Seungwon Jeon <[email protected]>

---
   drivers/scsi/ufs/ufshcd.c |    3 +--
   1 files changed, 1 insertions(+), 2 deletions(-)

drivers/scsi/ufs/ufshcd.c +0 -0

index b743bd6..4cf3a2d 100644
@@ -1199,7 +1199,7 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 

Quoted 13 lines

drivers/scsi/ufs/ufshcd.c ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) · line 1204

Quoted 6 lines

drivers/scsi/ufs/ufshcd.c +1 -1

   	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.

Response data of 'ucd_rsp_ptr' will be updated by UFS device if response UPIU is completed. There is nowhere driver clears 'ucd_rsp_ptr', though.

But if it's really needed, it can be another patch?

Thanks,
Seungwon Jeon

Quoted 7 lines · Seungwon Jeon

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'.

Quoted 11 lines

Signed-off-by Seungwon Jeon <[email protected]>

---
  drivers/scsi/ufs/ufshcd.c |    3 +--
  1 files changed, 1 insertions(+), 2 deletions(-)

drivers/scsi/ufs/ufshcd.c +0 -0

index b743bd6..4cf3a2d 100644

drivers/scsi/ufs/ufshcd.c ufshcd_transfer_rsp_status(struct ufs_hba *hba, · line 1204

Quoted 7 lines

drivers/scsi/ufs/ufshcd.c +1 -1

@@ -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

Quoted 10 lines

drivers/scsi/ufs/ufshcd.c +0 -1

          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,

Quoted 8 lines · Subhash Jadavani

Quoted 7 lines · Seungwon Jeon

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."

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?

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.

Quoted 30 lines

Quoted 11 lines

Signed-off-by Seungwon Jeon <[email protected]>

---
  drivers/scsi/ufs/ufshcd.c |    3 +--
  1 files changed, 1 insertions(+), 2 deletions(-)

drivers/scsi/ufs/ufshcd.c +0 -0

index b743bd6..4cf3a2d 100644

drivers/scsi/ufs/ufshcd.c ufshcd_transfer_rsp_status(struct ufs_hba *hba, · line 1204

Quoted 7 lines

drivers/scsi/ufs/ufshcd.c +1 -1

@@ -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

Quoted 11 lines

Quoted 10 lines

drivers/scsi/ufs/ufshcd.c +0 -1

          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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 9 lines · Sujit Reddy Thumma

Quoted 8 lines · Subhash Jadavani

Quoted 7 lines · Seungwon Jeon

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'.

Quoted 4 lines

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.

Quoted 6 lines

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?

"When Response field is 'Target Failure', if host controller updates the OCS field with 'SUCCESS', it's not proper behavior."

Thanks,
Seungwon Jeon

Quoted 50 lines

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.

Quoted 28 lines

Quoted 10 lines

Signed-off-by Seungwon Jeon <[email protected]>

---
   drivers/scsi/ufs/ufshcd.c |    3 +--
   1 files changed, 1 insertions(+), 2 deletions(-)

drivers/scsi/ufs/ufshcd.c +0 -0

index b743bd6..4cf3a2d 100644

drivers/scsi/ufs/ufshcd.c ufshcd_transfer_rsp_status(struct ufs_hba *hba, · line 1204

Quoted 7 lines

drivers/scsi/ufs/ufshcd.c +1 -1

@@ -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

Quoted 10 lines

Quoted 10 lines

drivers/scsi/ufs/ufshcd.c +0 -1

           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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 16 lines · Seungwon Jeon

Quoted 9 lines · Sujit Reddy Thumma

Quoted 8 lines · Subhash Jadavani

Quoted 7 lines · Seungwon Jeon

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'.

Quoted 4 lines

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.

Quoted 8 lines

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

"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.

Regards,
Subhash

Quoted 54 lines

Thanks,
Seungwon Jeon

Quoted 50 lines

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.

Quoted 28 lines

Quoted 10 lines

Signed-off-by Seungwon Jeon <[email protected]>

---
   drivers/scsi/ufs/ufshcd.c |    3 +--
   1 files changed, 1 insertions(+), 2 deletions(-)

drivers/scsi/ufs/ufshcd.c +0 -0

index b743bd6..4cf3a2d 100644

drivers/scsi/ufs/ufshcd.c ufshcd_transfer_rsp_status(struct ufs_hba *hba, · line 1204

Quoted 7 lines

drivers/scsi/ufs/ufshcd.c +1 -1

@@ -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

Quoted 10 lines

Quoted 10 lines

drivers/scsi/ufs/ufshcd.c +0 -1

           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 [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 35 lines · Subhash Jadavani

Quoted 16 lines · Seungwon Jeon

Quoted 9 lines · Sujit Reddy Thumma

Quoted 8 lines · Subhash Jadavani

Quoted 7 lines · Seungwon Jeon

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'.

Quoted 4 lines

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.

Quoted 8 lines

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 6 lines

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?

Quoted 6 lines

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.

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.

Quoted 11 lines

Quoted 2 lines

"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.

Quoted 2 lines

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

Quoted 55 lines

Regards,
Subhash

Quoted 52 lines

Thanks,
Seungwon Jeon

Quoted 49 lines

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.

Quoted 28 lines

Quoted 10 lines

Signed-off-by Seungwon Jeon <[email protected]>

---
    drivers/scsi/ufs/ufshcd.c |    3 +--
    1 files changed, 1 insertions(+), 2 deletions(-)

drivers/scsi/ufs/ufshcd.c +0 -0

index b743bd6..4cf3a2d 100644

drivers/scsi/ufs/ufshcd.c ufshcd_transfer_rsp_status(struct ufs_hba *hba, · line 1204

Quoted 7 lines

drivers/scsi/ufs/ufshcd.c +1 -1

@@ -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

Quoted 10 lines

Quoted 10 lines

drivers/scsi/ufs/ufshcd.c +0 -1

            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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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'.

Signed-off-by Seungwon Jeon <[email protected]>

---
 drivers/scsi/ufs/ufshcd.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

drivers/scsi/ufs/ufshcd.c +1 -3

index b743bd6..4cf3a2d 100644
@@ -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);
                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,
--

1.7.0.4

This path series contain driver's fixes and updates.

Changes in v2:
- [scsi: ufs: amend the ocs handling with fatal error] is excluded. Host behavior needs to be defined clearly in UFSHCI specification.
- Some minor changes are applied with comments from Subhash, Sujit and Santosh.

Seungwon Jeon (6):
scsi: ufs: find out sense data over scsi status values
scsi: ufs: fix the setting interrupt aggregation counter
scsi: ufs: add dme configuration primitives
scsi: ufs: add unipro attribute IDs
scsi: ufs: add operation for the uic power mode change
scsi: ufs: configure the attribute for power mode

 drivers/scsi/ufs/ufs.h    |    1 +
 drivers/scsi/ufs/ufshcd.c |  336
++++++++++++++++++++++++++++++++++++++-------
 drivers/scsi/ufs/ufshcd.h |   54 +++++++
 drivers/scsi/ufs/ufshci.h |   22 +++-
 drivers/scsi/ufs/unipro.h |  151 ++++++++++++++++++++
 5 files changed, 513 insertions(+), 51 deletions(-)

Thanks,
Seungwon Jeon

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

This path series contain driver's fixes and updates.

Changes in v3:
- Rebased with scsi-next

Changes in v2:
- [scsi: ufs: amend the ocs handling with fatal error] is excluded. Host behavior needs to be defined clearly in UFSHCI specification.
- Some minor changes are applied with comments from Subhash, Sujit and Santosh.

Seungwon Jeon (6):
scsi: ufs: find out sense data over scsi status values
scsi: ufs: fix the setting interrupt aggregation counter
scsi: ufs: add dme configuration primitives
scsi: ufs: add unipro attribute IDs
scsi: ufs: add operation for the uic power mode change
scsi: ufs: configure the attribute for power mode

  drivers/scsi/ufs/ufs.h    |    1 +
  drivers/scsi/ufs/ufshcd.c |  328 ++++++++++++++++++++++++++++++++++++++-------
  drivers/scsi/ufs/ufshcd.h |   54 ++++++++
  drivers/scsi/ufs/ufshci.h |   22 +++-
  drivers/scsi/ufs/unipro.h |  151 +++++++++++++++++++++
  5 files changed, 507 insertions(+), 49 deletions(-)

Thanks,
Seungwon Jeon

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

Except for 'GOOD' and 'CHECK CONDITION', other status value

in Response UPIU may or may contain sense data. If a non-zero

value is in the Data Segment Length field, it means that UPIU has Sense Data in the Data Segment area.

Signed-off-by Seungwon Jeon <[email protected]>

---
  drivers/scsi/ufs/ufs.h    |    1 +
  drivers/scsi/ufs/ufshcd.c |   27 +++++++++++++++++++--------
  2 files changed, 20 insertions(+), 8 deletions(-)

drivers/scsi/ufs/ufs.h +1 -0

index 139bc06..737c31b 100644
@@ -114,6 +114,7 @@ enum {
  	MASK_SCSI_STATUS	= 0xFF,
  	MASK_TASK_RESPONSE	= 0xFF00,
  	MASK_RSP_UPIU_RESULT	= 0xFFFF,
+	MASK_RSP_UPIU_DATA_SEG_LEN	= 0xFFFF,
  };
 
  /* Task management service response */

drivers/scsi/ufs/ufshcd.c +14 -0

index 4cf3a2d..688ae0e 100644
@@ -218,6 +218,20 @@ ufshcd_get_rsp_upiu_result(struct utp_upiu_rsp *ucd_rsp_ptr)
  	return be32_to_cpu(ucd_rsp_ptr->header.dword_1) & MASK_RSP_UPIU_RESULT;
  }
 
+/*
+ * ufshcd_get_rsp_upiu_data_seg_len - Get the data segment length
+ *				from response UPIU
+ * @ucd_rsp_ptr: pointer to response UPIU
+ *
+ * Return the data segment length.
+ */
+static inline int
+ufshcd_get_rsp_upiu_data_seg_len(struct utp_upiu_rsp *ucd_rsp_ptr)
+{
+	return be32_to_cpu(ucd_rsp_ptr->header.dword_2) &
+		MASK_RSP_UPIU_DATA_SEG_LEN;
+}
+
  /**
   * ufshcd_config_int_aggr - Configure interrupt aggregation values.
   *		Currently there is no use case where we want to configure

drivers/scsi/ufs/ufshcd.c +3 -7

  		len = be16_to_cpu(lrbp->ucd_rsp_ptr->sense_data_len);
  		memcpy(lrbp->sense_buffer,
  			lrbp->ucd_rsp_ptr->sense_data,

@@ -1156,21 +1171,17 @@ ufshcd_scsi_cmd_status(struct ufshcd_lrb *lrbp, int scsi_status)
  			  SAM_STAT_CHECK_CONDITION;
  		ufshcd_copy_sense_data(lrbp);
  		break;
-	case SAM_STAT_BUSY:
-		result |= SAM_STAT_BUSY;
-		break;
  	case SAM_STAT_TASK_SET_FULL:
-
  		/*
  		 * If a LUN reports SAM_STAT_TASK_SET_FULL, then the LUN queue
  		 * depth needs to be adjusted to the exact number of
  		 * outstanding commands the LUN can handle at any given time.
  		 */
  		ufshcd_adjust_lun_qdepth(lrbp->cmd);
-		result |= SAM_STAT_TASK_SET_FULL;
-		break;
+	case SAM_STAT_BUSY:
  	case SAM_STAT_TASK_ABORTED:
-		result |= SAM_STAT_TASK_ABORTED;
+		result |= scsi_status;
+		ufshcd_copy_sense_data(lrbp);
  		break;
  	default:
  		result |= DID_ERROR << 16;

Quoted 1 line · Seungwon Jeon

Except for 'GOOD' and 'CHECK CONDITION', other status value

This is what UFS device spec says: "A GOOD status indicates successful SCSI completion and therefore no Sense Data will be returned." So please change the commit text accordingly.

Quoted 1 line

in Response UPIU may or may contain sense data. If a non-zero

s/"may or may contain ..."/"may or may not contain"

Quoted 49 lines

value is in the Data Segment Length field, it means that UPIU has Sense Data in the Data Segment area.

Signed-off-by Seungwon Jeon <[email protected]>

---
  drivers/scsi/ufs/ufs.h    |    1 +
  drivers/scsi/ufs/ufshcd.c |   27 +++++++++++++++++++--------
  2 files changed, 20 insertions(+), 8 deletions(-)

drivers/scsi/ufs/ufs.h +1 -0

index 139bc06..737c31b 100644
@@ -114,6 +114,7 @@ enum {
  	MASK_SCSI_STATUS	= 0xFF,
  	MASK_TASK_RESPONSE	= 0xFF00,
  	MASK_RSP_UPIU_RESULT	= 0xFFFF,
+	MASK_RSP_UPIU_DATA_SEG_LEN	= 0xFFFF,
  };
 
  /* Task management service response */

drivers/scsi/ufs/ufshcd.c +14 -0

index 4cf3a2d..688ae0e 100644
@@ -218,6 +218,20 @@ ufshcd_get_rsp_upiu_result(struct utp_upiu_rsp *ucd_rsp_ptr)
  	return be32_to_cpu(ucd_rsp_ptr->header.dword_1) & MASK_RSP_UPIU_RESULT;
  }
 
+/*
+ * ufshcd_get_rsp_upiu_data_seg_len - Get the data segment length
+ *				from response UPIU
+ * @ucd_rsp_ptr: pointer to response UPIU
+ *
+ * Return the data segment length.
+ */
+static inline int
+ufshcd_get_rsp_upiu_data_seg_len(struct utp_upiu_rsp *ucd_rsp_ptr)
+{
+	return be32_to_cpu(ucd_rsp_ptr->header.dword_2) &
+		MASK_RSP_UPIU_DATA_SEG_LEN;
+}
+
  /**
   * ufshcd_config_int_aggr - Configure interrupt aggregation values.
   *		Currently there is no use case where we want to configure

@@ -298,7 +312,8 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag)
  static inline void ufshcd_copy_sense_data(struct ufshcd_lrb *lrbp)
  {

drivers/scsi/ufs/ufshcd.c void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag) · line 316

Quoted 4 lines

drivers/scsi/ufs/ufshcd.c +2 -1

  	int len;
-	if (lrbp->sense_buffer) {
+	if (lrbp->sense_buffer &&
+	    !ufshcd_get_rsp_upiu_data_seg_len(lrbp->ucd_rsp_ptr)) {

why if(!data_seg_len)? I guess you want to copy the sense data tot sense buffer only if the data segment length is non zero so in that case it should be "if(data_seg_len)".

Except for 'GOOD' and 'CHECK CONDITION', other status value in Response UPIU may or may contain sense data. If a non-zero value is in the Data Segment Length field, it means that UPIU has Sense Data in the Data Segment area.

Signed-off-by Seungwon Jeon <[email protected]>

---
  drivers/scsi/ufs/ufs.h    |    1 +
  drivers/scsi/ufs/ufshcd.c |   27 +++++++++++++++++++--------
  2 files changed, 20 insertions(+), 8 deletions(-)

drivers/scsi/ufs/ufs.h +1 -0

index 139bc06..737c31b 100644
@@ -114,6 +114,7 @@ enum {
  	MASK_SCSI_STATUS	= 0xFF,
  	MASK_TASK_RESPONSE	= 0xFF00,
  	MASK_RSP_UPIU_RESULT	= 0xFFFF,
+	MASK_RSP_UPIU_DATA_SEG_LEN	= 0xFFFF,
  };
 
  /* Task management service response */

drivers/scsi/ufs/ufshcd.c +16 -1

index 4cf3a2d..688ae0e 100644
@@ -218,6 +218,20 @@ ufshcd_get_rsp_upiu_result(struct utp_upiu_rsp *ucd_rsp_ptr)
  	return be32_to_cpu(ucd_rsp_ptr->header.dword_1) & MASK_RSP_UPIU_RESULT;
  }
 
+/*
+ * ufshcd_get_rsp_upiu_data_seg_len - Get the data segment length
+ *				from response UPIU
+ * @ucd_rsp_ptr: pointer to response UPIU
+ *
+ * Return the data segment length.
+ */
+static inline int
+ufshcd_get_rsp_upiu_data_seg_len(struct utp_upiu_rsp *ucd_rsp_ptr)
+{
+	return be32_to_cpu(ucd_rsp_ptr->header.dword_2) &
+		MASK_RSP_UPIU_DATA_SEG_LEN;
+}
+
  /**
   * ufshcd_config_int_aggr - Configure interrupt aggregation values.
   *		Currently there is no use case where we want to configure

@@ -298,7 +312,8 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag)
  static inline void ufshcd_copy_sense_data(struct ufshcd_lrb *lrbp)
  {
  	int len;
-	if (lrbp->sense_buffer) {
+	if (lrbp->sense_buffer &&
+	    !ufshcd_get_rsp_upiu_data_seg_len(lrbp->ucd_rsp_ptr)) {
  		len = be16_to_cpu(lrbp->ucd_rsp_ptr->sense_data_len);
  		memcpy(lrbp->sense_buffer,
  			lrbp->ucd_rsp_ptr->sense_data,

drivers/scsi/ufs/ufshcd.c +3 -7

  		ufshcd_copy_sense_data(lrbp);
  		break;
-	case SAM_STAT_BUSY:
-		result |= SAM_STAT_BUSY;
-		break;
  	case SAM_STAT_TASK_SET_FULL:
-
  		/*
  		 * If a LUN reports SAM_STAT_TASK_SET_FULL, then the LUN queue
  		 * depth needs to be adjusted to the exact number of
  		 * outstanding commands the LUN can handle at any given time.
  		 */
  		ufshcd_adjust_lun_qdepth(lrbp->cmd);
-		result |= SAM_STAT_TASK_SET_FULL;
-		break;
+	case SAM_STAT_BUSY:
  	case SAM_STAT_TASK_ABORTED:
-		result |= SAM_STAT_TASK_ABORTED;
+		result |= scsi_status;
+		ufshcd_copy_sense_data(lrbp);
  		break;
  	default:
  		result |= DID_ERROR << 16;

Quoted 56 lines · Seungwon Jeon

Except for 'GOOD' and 'CHECK CONDITION', other status value in Response UPIU may or may contain sense data. If a non-zero value is in the Data Segment Length field, it means that UPIU has Sense Data in the Data Segment area.

Signed-off-by Seungwon Jeon <[email protected]>

---
  drivers/scsi/ufs/ufs.h    |    1 +
  drivers/scsi/ufs/ufshcd.c |   27 +++++++++++++++++++--------
  2 files changed, 20 insertions(+), 8 deletions(-)

drivers/scsi/ufs/ufs.h +1 -0

index 139bc06..737c31b 100644
@@ -114,6 +114,7 @@ enum {
  	MASK_SCSI_STATUS	= 0xFF,
  	MASK_TASK_RESPONSE	= 0xFF00,
  	MASK_RSP_UPIU_RESULT	= 0xFFFF,
+	MASK_RSP_UPIU_DATA_SEG_LEN	= 0xFFFF,
  };
 
  /* Task management service response */

drivers/scsi/ufs/ufshcd.c +16 -1

index 4cf3a2d..688ae0e 100644
@@ -218,6 +218,20 @@ ufshcd_get_rsp_upiu_result(struct utp_upiu_rsp *ucd_rsp_ptr)
  	return be32_to_cpu(ucd_rsp_ptr->header.dword_1) & MASK_RSP_UPIU_RESULT;
  }
 
+/*
+ * ufshcd_get_rsp_upiu_data_seg_len - Get the data segment length
+ *				from response UPIU
+ * @ucd_rsp_ptr: pointer to response UPIU
+ *
+ * Return the data segment length.
+ */
+static inline int
+ufshcd_get_rsp_upiu_data_seg_len(struct utp_upiu_rsp *ucd_rsp_ptr)
+{
+	return be32_to_cpu(ucd_rsp_ptr->header.dword_2) &
+		MASK_RSP_UPIU_DATA_SEG_LEN;
+}
+
  /**
   * ufshcd_config_int_aggr - Configure interrupt aggregation values.
   *		Currently there is no use case where we want to configure

@@ -298,7 +312,8 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag)
  static inline void ufshcd_copy_sense_data(struct ufshcd_lrb *lrbp)
  {
  	int len;
-	if (lrbp->sense_buffer) {
+	if (lrbp->sense_buffer &&
+	    !ufshcd_get_rsp_upiu_data_seg_len(lrbp->ucd_rsp_ptr)) {
  		len = be16_to_cpu(lrbp->ucd_rsp_ptr->sense_data_len);

drivers/scsi/ufs/ufshcd.c ufshcd_scsi_cmd_status(struct ufshcd_lrb *lrbp, int scsi_status) · line 1171

Quoted 4 lines

drivers/scsi/ufs/ufshcd.c +0 -0

  		memcpy(lrbp->sense_buffer,
  			lrbp->ucd_rsp_ptr->sense_data,

@@ -1156,21 +1171,17 @@ ufshcd_scsi_cmd_status(struct ufshcd_lrb *lrbp, int scsi_status)
  			  SAM_STAT_CHECK_CONDITION;

The whole function is doing things wrong. It is redundant to interpret the scsi_status at the LLD layer and make decisions while otherwise the SCSI layer anyway knows how handle them properly.

Please see scsi_decide_disposition().

Ideally, this is what we should do -
if (response_upiu_sense_data_len > 0 && lrbp->sense_buffer) {
ufshcd_copy_sense_data();
}
result = set_host_byte(DID_OK) |

set_msg_byte(COMMAND_COMPLETE) |

		(scsi_status & 0xff);

Quoted 14 lines

drivers/scsi/ufs/ufshcd.c +0 -4

  		ufshcd_copy_sense_data(lrbp);
  		break;
-	case SAM_STAT_BUSY:
-		result |= SAM_STAT_BUSY;
-		break;
  	case SAM_STAT_TASK_SET_FULL:
-
  		/*
  		 * If a LUN reports SAM_STAT_TASK_SET_FULL, then the LUN queue
  		 * depth needs to be adjusted to the exact number of
  		 * outstanding commands the LUN can handle at any given time.
  		 */
  		ufshcd_adjust_lun_qdepth(lrbp->cmd);

drivers/scsi/ufs/ufshcd.c ufshcd_scsi_cmd_status(struct ufshcd_lrb *lrbp, int scsi_status) · line 1187

Quoted 10 lines

drivers/scsi/ufs/ufshcd.c +3 -3

-		result |= SAM_STAT_TASK_SET_FULL;
-		break;
+	case SAM_STAT_BUSY:
  	case SAM_STAT_TASK_ABORTED:
-		result |= SAM_STAT_TASK_ABORTED;
+		result |= scsi_status;
+		ufshcd_copy_sense_data(lrbp);
  		break;
  	default:
  		result |= DID_ERROR << 16;

--
Regards,
Sujit
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Except for 'GOOD' and 'CHECK CONDITION', other status value in Response UPIU may or may contain sense data. If a non-zero value is in the Data Segment Length field, it means that UPIU has Sense Data in the Data Segment area.

Signed-off-by Seungwon Jeon <[email protected]>

---
 drivers/scsi/ufs/ufs.h    |    1 +
 drivers/scsi/ufs/ufshcd.c |   27 +++++++++++++++++++--------
 2 files changed, 20 insertions(+), 8 deletions(-)

drivers/scsi/ufs/ufs.h +1 -0

index 139bc06..737c31b 100644
@@ -114,6 +114,7 @@ enum {
        MASK_SCSI_STATUS        = 0xFF,
        MASK_TASK_RESPONSE      = 0xFF00,
        MASK_RSP_UPIU_RESULT    = 0xFFFF,
+       MASK_RSP_UPIU_DATA_SEG_LEN      = 0xFFFF,
 };
 
 /* Task management service response */

drivers/scsi/ufs/ufshcd.c +14 -0

index 4cf3a2d..688ae0e 100644
@@ -218,6 +218,20 @@ ufshcd_get_rsp_upiu_result(struct utp_upiu_rsp *ucd_rsp_ptr)
        return be32_to_cpu(ucd_rsp_ptr->header.dword_1) & MASK_RSP_UPIU_RESULT;
 }
 
+/*
+ * ufshcd_get_rsp_upiu_data_seg_len - Get the data segment length
+ *                             from response UPIU
+ * @ucd_rsp_ptr: pointer to response UPIU
+ *
+ * Return the data segment length.
+ */
+static inline int
+ufshcd_get_rsp_upiu_data_seg_len(struct utp_upiu_rsp *ucd_rsp_ptr)
+{
+       return be32_to_cpu(ucd_rsp_ptr->header.dword_2) &
+               MASK_RSP_UPIU_DATA_SEG_LEN;
+}
+
 /**
  * ufshcd_config_int_aggr - Configure interrupt aggregation values.
  *             Currently there is no use case where we want to configure

drivers/scsi/ufs/ufshcd.c +3 -8

@@ -1156,21 +1171,17 @@ ufshcd_scsi_cmd_status(struct ufshcd_lrb *lrbp, int scsi_status)
                          SAM_STAT_CHECK_CONDITION;
                ufshcd_copy_sense_data(lrbp);
                break;
-       case SAM_STAT_BUSY:
-               result |= SAM_STAT_BUSY;
-               break;
        case SAM_STAT_TASK_SET_FULL:
-
                /*
                 * If a LUN reports SAM_STAT_TASK_SET_FULL, then the LUN queue
                 * depth needs to be adjusted to the exact number of
                 * outstanding commands the LUN can handle at any given time.
                 */
                ufshcd_adjust_lun_qdepth(lrbp->cmd);
-               result |= SAM_STAT_TASK_SET_FULL;
-               break;
+       case SAM_STAT_BUSY:
        case SAM_STAT_TASK_ABORTED:
-               result |= SAM_STAT_TASK_ABORTED;
+               result |= scsi_status;
+               ufshcd_copy_sense_data(lrbp);
                break;
        default:
                result |= DID_ERROR << 16;
--

1.7.0.4

Quoted 49 lines · Seungwon Jeon

Except for 'GOOD' and 'CHECK CONDITION', other status value in Response UPIU may or may contain sense data. If a non-zero value is in the Data Segment Length field, it means that UPIU has Sense Data in the Data Segment area.

Signed-off-by Seungwon Jeon <[email protected]>

---
 drivers/scsi/ufs/ufs.h    |    1 +
 drivers/scsi/ufs/ufshcd.c |   27 +++++++++++++++++++--------
 2 files changed, 20 insertions(+), 8 deletions(-)

drivers/scsi/ufs/ufs.h +1 -0

index 139bc06..737c31b 100644
@@ -114,6 +114,7 @@ enum {
        MASK_SCSI_STATUS        = 0xFF,
        MASK_TASK_RESPONSE      = 0xFF00,
        MASK_RSP_UPIU_RESULT    = 0xFFFF,
+       MASK_RSP_UPIU_DATA_SEG_LEN      = 0xFFFF,
 };
 
 /* Task management service response */

drivers/scsi/ufs/ufshcd.c +14 -0

index 4cf3a2d..688ae0e 100644
@@ -218,6 +218,20 @@ ufshcd_get_rsp_upiu_result(struct utp_upiu_rsp *ucd_rsp_ptr)
        return be32_to_cpu(ucd_rsp_ptr->header.dword_1) & MASK_RSP_UPIU_RESULT;
 }
 
+/*
+ * ufshcd_get_rsp_upiu_data_seg_len - Get the data segment length
+ *                             from response UPIU
+ * @ucd_rsp_ptr: pointer to response UPIU
+ *
+ * Return the data segment length.
+ */
+static inline int
+ufshcd_get_rsp_upiu_data_seg_len(struct utp_upiu_rsp *ucd_rsp_ptr)
+{
+       return be32_to_cpu(ucd_rsp_ptr->header.dword_2) &
+               MASK_RSP_UPIU_DATA_SEG_LEN;
+}
+
 /**
  * ufshcd_config_int_aggr - Configure interrupt aggregation values.
  *             Currently there is no use case where we want to configure

drivers/scsi/ufs/ufshcd.c void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag) · line 319

Quoted 9 lines

drivers/scsi/ufs/ufshcd.c +2 -1

@@ -298,7 +312,8 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag)
 static inline void ufshcd_copy_sense_data(struct ufshcd_lrb *lrbp)
 {
        int len;
-       if (lrbp->sense_buffer) {
+       if (lrbp->sense_buffer &&
+           !ufshcd_get_rsp_upiu_data_seg_len(lrbp->ucd_rsp_ptr)) {
                len = be16_to_cpu(lrbp->ucd_rsp_ptr->sense_data_len);
                memcpy(lrbp->sense_buffer,
                        lrbp->ucd_rsp_ptr->sense_data,

Also please change the SCSI_SENSE_BUFFERSIZE to 18-bytes as per the spec, *in case* if wrong 'data segment length' is returned, there won't be a need to memcpy extra bytes ;-).

#define UFS_SENSE_BUFFERSIZE 18
memcpy(lrbp->sense_buffer,

			lrbp->ucd_rsp_ptr->sense_data,
			min_t(int, len, UFS_SENSE_BUFFERSIZE));

Quoted 28 lines

drivers/scsi/ufs/ufshcd.c +3 -8

@@ -1156,21 +1171,17 @@ ufshcd_scsi_cmd_status(struct ufshcd_lrb *lrbp, int scsi_status)
                          SAM_STAT_CHECK_CONDITION;
                ufshcd_copy_sense_data(lrbp);
                break;
-       case SAM_STAT_BUSY:
-               result |= SAM_STAT_BUSY;
-               break;
        case SAM_STAT_TASK_SET_FULL:
-
                /*
                 * If a LUN reports SAM_STAT_TASK_SET_FULL, then the LUN queue
                 * depth needs to be adjusted to the exact number of
                 * outstanding commands the LUN can handle at any given time.
                 */
                ufshcd_adjust_lun_qdepth(lrbp->cmd);
-               result |= SAM_STAT_TASK_SET_FULL;
-               break;
+       case SAM_STAT_BUSY:
        case SAM_STAT_TASK_ABORTED:
-               result |= SAM_STAT_TASK_ABORTED;
+               result |= scsi_status;
+               ufshcd_copy_sense_data(lrbp);
                break;
        default:
                result |= DID_ERROR << 16;
--

Quoted 3 lines

1.7.0.4

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

-----Original Message-----
From: [email protected] [mailto:linux-scsi- [email protected]] On Behalf Of Seungwon Jeon
Sent: Tuesday, 30 July, 2013 8:03 AM
To: 'Santosh Y'
Cc: [email protected]; 'Vinayak Holikatti'; 'James E.J. Bottomley'
Subject: RE: [PATCH 2/7] scsi: ufs: find out sense data over scsi status values

Quoted 1 line · Santosh Y

On Fri, Jul 26, 2013 at 7:16 PM, Seungwon Jeon <[email protected]>

Quoted 22 lines · wrote:

Quoted 22 lines

Except for 'GOOD' and 'CHECK CONDITION', other status value in Response UPIU may or may contain sense data. If a non-zero value is in the Data Segment Length field, it means that UPIU has Sense Data in the Data Segment area.

Signed-off-by Seungwon Jeon <[email protected]>

---
 drivers/scsi/ufs/ufs.h    |    1 +
 drivers/scsi/ufs/ufshcd.c |   27 +++++++++++++++++++--------
 2 files changed, 20 insertions(+), 8 deletions(-)

drivers/scsi/ufs/ufs.h +1 -0

index 139bc06..737c31b 100644
@@ -114,6 +114,7 @@ enum {
        MASK_SCSI_STATUS        = 0xFF,
        MASK_TASK_RESPONSE      = 0xFF00,
        MASK_RSP_UPIU_RESULT    = 0xFFFF,
+       MASK_RSP_UPIU_DATA_SEG_LEN      = 0xFFFF,
 };
 

Quoted 6 lines

drivers/scsi/ufs/ufshcd.c ufshcd_get_rsp_upiu_result(struct utp_upiu_rsp

Quoted 6 lines

drivers/scsi/ufs/ufs.h +0 -0

 /* Task management service response */

drivers/scsi/ufs/ufshcd.c +0 -0

index 4cf3a2d..688ae0e 100644
@@ -218,6 +218,20 @@ ufshcd_get_rsp_upiu_result(struct utp_upiu_rsp

*ucd_rsp_ptr)

Quoted 1 line

drivers/scsi/ufs/ufshcd.c ufshcd_get_rsp_upiu_result(struct utp_upiu_rsp · line 218

Quoted 1 line

drivers/scsi/ufs/ufshcd.c +0 -0

        return be32_to_cpu(ucd_rsp_ptr->header.dword_1) &

MASK_RSP_UPIU_RESULT;

Quoted 12 lines

Quoted 12 lines

drivers/scsi/ufs/ufshcd.c +10 -0

 }
 
+/*
+ * ufshcd_get_rsp_upiu_data_seg_len - Get the data segment length
+ *                             from response UPIU
+ * @ucd_rsp_ptr: pointer to response UPIU
+ *
+ * Return the data segment length.
+ */
+static inline int
+ufshcd_get_rsp_upiu_data_seg_len(struct utp_upiu_rsp *ucd_rsp_ptr)
+{

Quoted 8 lines

drivers/scsi/ufs/ufshcd.c void ufshcd_send_command(struct ufs_hba

Quoted 8 lines

drivers/scsi/ufs/ufshcd.c +4 -0

+       return be32_to_cpu(ucd_rsp_ptr->header.dword_2) &
+               MASK_RSP_UPIU_DATA_SEG_LEN;
+}
+
 /**
  * ufshcd_config_int_aggr - Configure interrupt aggregation values.
  *             Currently there is no use case where we want to configure

@@ -298,7 +312,8 @@ void ufshcd_send_command(struct ufs_hba

*hba, unsigned int task_tag)

Quoted 10 lines

drivers/scsi/ufs/ufshcd.c void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag) · line 319

Quoted 9 lines

drivers/scsi/ufs/ufshcd.c +2 -1

 static inline void ufshcd_copy_sense_data(struct ufshcd_lrb *lrbp)
 {
        int len;
-       if (lrbp->sense_buffer) {
+       if (lrbp->sense_buffer &&
+           !ufshcd_get_rsp_upiu_data_seg_len(lrbp->ucd_rsp_ptr)) {
                len = be16_to_cpu(lrbp->ucd_rsp_ptr->sense_data_len);
                memcpy(lrbp->sense_buffer,
                        lrbp->ucd_rsp_ptr->sense_data,

Quoted 8 lines

Also please change the SCSI_SENSE_BUFFERSIZE to 18-bytes as per the spec, *in case* if wrong 'data segment length' is returned, there won't be a need to memcpy extra bytes ;-).

#define UFS_SENSE_BUFFERSIZE 18
memcpy(lrbp->sense_buffer,

			lrbp->ucd_rsp_ptr->sense_data,
			min_t(int, len, UFS_SENSE_BUFFERSIZE));

Ok. I'll check.
But considering the extension with spec. revision, it's not bad to be kept.
Current: [#define SCSI_SENSE_BUFFERSIZE 96]

Thanks,
Seungwon Jeon

Quoted 3 lines

drivers/scsi/ufs/ufshcd.c ufshcd_scsi_cmd_status(struct ufshcd_lrb

Quoted 3 lines

drivers/scsi/ufs/ufshcd.c +0 -0

@@ -1156,21 +1171,17 @@ ufshcd_scsi_cmd_status(struct ufshcd_lrb

*lrbp, int scsi_status)

Quoted 10 lines

drivers/scsi/ufs/ufshcd.c ufshcd_scsi_cmd_status(struct ufshcd_lrb *lrbp, int scsi_status)

Quoted 10 lines

drivers/scsi/ufs/ufshcd.c +0 -4

                          SAM_STAT_CHECK_CONDITION;
                ufshcd_copy_sense_data(lrbp);
                break;
-       case SAM_STAT_BUSY:
-               result |= SAM_STAT_BUSY;
-               break;
        case SAM_STAT_TASK_SET_FULL:
-
                /*
                 * If a LUN reports SAM_STAT_TASK_SET_FULL, then the LUN

queue

Quoted 21 lines

Quoted 15 lines

drivers/scsi/ufs/ufshcd.c +3 -4

                 * depth needs to be adjusted to the exact number of
                 * outstanding commands the LUN can handle at any given time.
                 */
                ufshcd_adjust_lun_qdepth(lrbp->cmd);
-               result |= SAM_STAT_TASK_SET_FULL;
-               break;
+       case SAM_STAT_BUSY:
        case SAM_STAT_TASK_ABORTED:
-               result |= SAM_STAT_TASK_ABORTED;
+               result |= scsi_status;
+               ufshcd_copy_sense_data(lrbp);
                break;
        default:
                result |= DID_ERROR << 16;
--

Quoted 3 lines

1.7.0.4

Quoted 6 lines

--
~Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

I don't know how well UFS keeps up with SCSI architecture, but SCSI has expanded from the old 18-byte fixed format sense data to a variable-length descriptor format sense data, in which an 8 byte header precedes a variable number of descriptors. This was devised to support 8-byte LBAs in the INFORMATION field and COMMAND-SPECIFIC INFORMATION field for >2 TiB drives.

A few other new features:

1. Extended INQUIRY Data VPD page
MAXIMUM SUPPORTED SENSE DATA LENGTH field:
Report how much sense data the device server is capable of returning
2. Control Extensions mode page
MAXIMUM SENSE DATA LENGTH field:
Specify how much sense data the device server is allowed to return
3. Descriptor format sense data header
SDAT_OVFL bit:
Reports that the device server had to drop some descriptors to fit in the specified length
4. "Direct-access block device sense data descriptor" Devised for the use by Base feature set compliant designs. Combined with the 8 byte header, results in 32 bytes of sense data. Always carries an INFORMATION field, COMMAND-SPECIFIC INFORMATION field, sense-key specific information, field replaceable unit code, and ILI bit.

Quoted 131 lines

-----Original Message-----
From: [email protected] [mailto:linux-scsi- [email protected]] On Behalf Of Seungwon Jeon
Sent: Tuesday, 30 July, 2013 8:03 AM
To: 'Santosh Y'
Cc: [email protected]; 'Vinayak Holikatti'; 'James E.J. Bottomley'
Subject: RE: [PATCH 2/7] scsi: ufs: find out sense data over scsi status values

Quoted 1 line · Santosh Y

On Fri, Jul 26, 2013 at 7:16 PM, Seungwon Jeon <[email protected]>

Quoted 22 lines · wrote:

Quoted 22 lines

Except for 'GOOD' and 'CHECK CONDITION', other status value in Response UPIU may or may contain sense data. If a non-zero value is in the Data Segment Length field, it means that UPIU has Sense Data in the Data Segment area.

Signed-off-by Seungwon Jeon <[email protected]>

---
 drivers/scsi/ufs/ufs.h    |    1 +
 drivers/scsi/ufs/ufshcd.c |   27 +++++++++++++++++++--------
 2 files changed, 20 insertions(+), 8 deletions(-)

drivers/scsi/ufs/ufs.h +1 -0

index 139bc06..737c31b 100644
@@ -114,6 +114,7 @@ enum {
        MASK_SCSI_STATUS        = 0xFF,
        MASK_TASK_RESPONSE      = 0xFF00,
        MASK_RSP_UPIU_RESULT    = 0xFFFF,
+       MASK_RSP_UPIU_DATA_SEG_LEN      = 0xFFFF,
 };
 

Quoted 6 lines

drivers/scsi/ufs/ufshcd.c ufshcd_get_rsp_upiu_result(struct utp_upiu_rsp

Quoted 6 lines

drivers/scsi/ufs/ufs.h +0 -0

 /* Task management service response */

drivers/scsi/ufs/ufshcd.c +0 -0

index 4cf3a2d..688ae0e 100644
@@ -218,6 +218,20 @@ ufshcd_get_rsp_upiu_result(struct utp_upiu_rsp

*ucd_rsp_ptr)

Quoted 1 line

drivers/scsi/ufs/ufshcd.c ufshcd_get_rsp_upiu_result(struct utp_upiu_rsp · line 218

Quoted 1 line

drivers/scsi/ufs/ufshcd.c +0 -0

        return be32_to_cpu(ucd_rsp_ptr->header.dword_1) &

MASK_RSP_UPIU_RESULT;

Quoted 12 lines

Quoted 12 lines

drivers/scsi/ufs/ufshcd.c +10 -0

 }
 
+/*
+ * ufshcd_get_rsp_upiu_data_seg_len - Get the data segment length
+ *                             from response UPIU
+ * @ucd_rsp_ptr: pointer to response UPIU
+ *
+ * Return the data segment length.
+ */
+static inline int
+ufshcd_get_rsp_upiu_data_seg_len(struct utp_upiu_rsp *ucd_rsp_ptr)
+{

Quoted 8 lines

drivers/scsi/ufs/ufshcd.c void ufshcd_send_command(struct ufs_hba

Quoted 8 lines

drivers/scsi/ufs/ufshcd.c +4 -0

+       return be32_to_cpu(ucd_rsp_ptr->header.dword_2) &
+               MASK_RSP_UPIU_DATA_SEG_LEN;
+}
+
 /**
  * ufshcd_config_int_aggr - Configure interrupt aggregation values.
  *             Currently there is no use case where we want to configure

@@ -298,7 +312,8 @@ void ufshcd_send_command(struct ufs_hba

*hba, unsigned int task_tag)

Quoted 10 lines

drivers/scsi/ufs/ufshcd.c void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag) · line 319

Quoted 9 lines

drivers/scsi/ufs/ufshcd.c +2 -1

 static inline void ufshcd_copy_sense_data(struct ufshcd_lrb *lrbp)
 {
        int len;
-       if (lrbp->sense_buffer) {
+       if (lrbp->sense_buffer &&
+           !ufshcd_get_rsp_upiu_data_seg_len(lrbp->ucd_rsp_ptr)) {
                len = be16_to_cpu(lrbp->ucd_rsp_ptr->sense_data_len);
                memcpy(lrbp->sense_buffer,
                        lrbp->ucd_rsp_ptr->sense_data,

Quoted 8 lines

Also please change the SCSI_SENSE_BUFFERSIZE to 18-bytes as per the spec, *in case* if wrong 'data segment length' is returned, there won't be a need to memcpy extra bytes ;-).

#define UFS_SENSE_BUFFERSIZE 18
memcpy(lrbp->sense_buffer,

			lrbp->ucd_rsp_ptr->sense_data,
			min_t(int, len, UFS_SENSE_BUFFERSIZE));

Ok. I'll check.
But considering the extension with spec. revision, it's not bad to be kept.
Current: [#define SCSI_SENSE_BUFFERSIZE 96]

Thanks,
Seungwon Jeon

Quoted 3 lines

drivers/scsi/ufs/ufshcd.c ufshcd_scsi_cmd_status(struct ufshcd_lrb

Quoted 3 lines

drivers/scsi/ufs/ufshcd.c +0 -0

@@ -1156,21 +1171,17 @@ ufshcd_scsi_cmd_status(struct ufshcd_lrb

*lrbp, int scsi_status)

Quoted 10 lines

drivers/scsi/ufs/ufshcd.c ufshcd_scsi_cmd_status(struct ufshcd_lrb *lrbp, int scsi_status)

Quoted 10 lines

drivers/scsi/ufs/ufshcd.c +0 -4

                          SAM_STAT_CHECK_CONDITION;
                ufshcd_copy_sense_data(lrbp);
                break;
-       case SAM_STAT_BUSY:
-               result |= SAM_STAT_BUSY;
-               break;
        case SAM_STAT_TASK_SET_FULL:
-
                /*
                 * If a LUN reports SAM_STAT_TASK_SET_FULL, then the LUN

queue

Quoted 21 lines

Quoted 15 lines

drivers/scsi/ufs/ufshcd.c +3 -4

                 * depth needs to be adjusted to the exact number of
                 * outstanding commands the LUN can handle at any given time.
                 */
                ufshcd_adjust_lun_qdepth(lrbp->cmd);
-               result |= SAM_STAT_TASK_SET_FULL;
-               break;
+       case SAM_STAT_BUSY:
        case SAM_STAT_TASK_ABORTED:
-               result |= SAM_STAT_TASK_ABORTED;
+               result |= scsi_status;
+               ufshcd_copy_sense_data(lrbp);
                break;
        default:
                result |= DID_ERROR << 16;
--

Quoted 3 lines

1.7.0.4

Quoted 6 lines

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

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Unlike 'GOOD' and 'CHECK CONDITION', other status values in Response UPIU may or may not contain sense data. That is returning sense data isn't obvious. So, in this case the Data Segment Length field should be checked. If a non-zero value, it means that UPIU has Sense Data in the Data Segment area.

Signed-off-by Seungwon Jeon <[email protected]>

Reviewed-by Subhash Jadavani <[email protected]>

---
  drivers/scsi/ufs/ufs.h    |    1 +
  drivers/scsi/ufs/ufshcd.c |   37 ++++++++++++++++++++++---------------
  2 files changed, 23 insertions(+), 15 deletions(-)

drivers/scsi/ufs/ufs.h +1 -0

index bce09a6..7210500 100644
@@ -177,6 +177,7 @@ enum {
  	MASK_TASK_RESPONSE              = 0xFF00,
  	MASK_RSP_UPIU_RESULT            = 0xFFFF,
  	MASK_QUERY_DATA_SEG_LEN         = 0xFFFF,
+	MASK_RSP_UPIU_DATA_SEG_LEN	= 0xFFFF,
  	MASK_RSP_EXCEPTION_EVENT        = 0x10000,
  };
 

drivers/scsi/ufs/ufshcd.c +22 -15

index 1b99c0a..6ff16c9 100644
@@ -310,6 +310,20 @@ ufshcd_get_rsp_upiu_result(struct utp_upiu_rsp *ucd_rsp_ptr)
  	return be32_to_cpu(ucd_rsp_ptr->header.dword_1) & MASK_RSP_UPIU_RESULT;
  }
 
+/*
+ * ufshcd_get_rsp_upiu_data_seg_len - Get the data segment length
+ *				from response UPIU
+ * @ucd_rsp_ptr: pointer to response UPIU
+ *
+ * Return the data segment length.
+ */
+static inline unsigned int
+ufshcd_get_rsp_upiu_data_seg_len(struct utp_upiu_rsp *ucd_rsp_ptr)
+{
+	return be32_to_cpu(ucd_rsp_ptr->header.dword_2) &
+		MASK_RSP_UPIU_DATA_SEG_LEN;
+}
+
  /**
   * ufshcd_is_exception_event - Check if the device raised an exception event
   * @ucd_rsp_ptr: pointer to response UPIU

@@ -405,7 +419,8 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag)
  static inline void ufshcd_copy_sense_data(struct ufshcd_lrb *lrbp)
  {
  	int len;
-	if (lrbp->sense_buffer) {
+	if (lrbp->sense_buffer &&
+	    ufshcd_get_rsp_upiu_data_seg_len(lrbp->ucd_rsp_ptr)) {
  		len = be16_to_cpu(lrbp->ucd_rsp_ptr->sr.sense_data_len);
  		memcpy(lrbp->sense_buffer,
  			lrbp->ucd_rsp_ptr->sr.sense_data,

@@ -1789,32 +1804,24 @@ ufshcd_scsi_cmd_status(struct ufshcd_lrb *lrbp, int scsi_status)
  	int result = 0;
 
  	switch (scsi_status) {
-	case SAM_STAT_GOOD:
-		result |= DID_OK << 16 |
-			  COMMAND_COMPLETE << 8 |
-			  SAM_STAT_GOOD;
-		break;
  	case SAM_STAT_CHECK_CONDITION:
+		ufshcd_copy_sense_data(lrbp);
+	case SAM_STAT_GOOD:
  		result |= DID_OK << 16 |
  			  COMMAND_COMPLETE << 8 |
-			  SAM_STAT_CHECK_CONDITION;
-		ufshcd_copy_sense_data(lrbp);
-		break;
-	case SAM_STAT_BUSY:
-		result |= SAM_STAT_BUSY;
+			  scsi_status;
  		break;
  	case SAM_STAT_TASK_SET_FULL:
-
  		/*
  		 * If a LUN reports SAM_STAT_TASK_SET_FULL, then the LUN queue
  		 * depth needs to be adjusted to the exact number of
  		 * outstanding commands the LUN can handle at any given time.
  		 */
  		ufshcd_adjust_lun_qdepth(lrbp->cmd);
-		result |= SAM_STAT_TASK_SET_FULL;
-		break;
+	case SAM_STAT_BUSY:
  	case SAM_STAT_TASK_ABORTED:
-		result |= SAM_STAT_TASK_ABORTED;
+		ufshcd_copy_sense_data(lrbp);
+		result |= scsi_status;
  		break;
  	default:
  		result |= DID_ERROR << 16;

IACTH(Interrupt aggregation counter threshold) value is allowed up to 0x1F and current setting value is the maximum. This value is related with NUTRS(max:0x20) of HCI's capability. Considering HCI controller doesn't support the maximum, IACTH setting should be adjusted with possible value.
For that, existing 'ufshcd_config_int_aggr' is split into two part [reset, configure].

Signed-off-by Seungwon Jeon <[email protected]>

Reviewed-by Subhash Jadavani <[email protected]>

---
  drivers/scsi/ufs/ufshcd.c |   53 +++++++++++++++++++++-----------------------
  drivers/scsi/ufs/ufshci.h |    4 +-
  2 files changed, 27 insertions(+), 30 deletions(-)

drivers/scsi/ufs/ufshcd.c +25 -28

index 6ff16c9..c90b88a 100644
@@ -59,6 +59,9 @@
  /* Expose the flag value from utp_upiu_query.value */
  #define MASK_QUERY_UPIU_FLAG_LOC 0xFF
 
+/* Interrupt aggregation default timeout, unit: 40us */
+#define INT_AGGR_DEF_TO	0x02
+
  enum {
  	UFSHCD_MAX_CHANNEL	= 0,
  	UFSHCD_MAX_ID		= 1,

@@ -94,12 +97,6 @@ enum {
  	UFSHCD_INT_CLEAR,
  };
 
-/* Interrupt aggregation options */
-enum {
-	INT_AGGR_RESET,
-	INT_AGGR_CONFIG,
-};
-
  #define ufshcd_set_eh_in_progress(h) \
  	(h->eh_flags |= UFSHCD_EH_IN_PROGRESS)
  #define ufshcd_eh_in_progress(h) \

@@ -340,30 +337,30 @@ static inline bool ufshcd_is_exception_event(struct utp_upiu_rsp *ucd_rsp_ptr)
  }
 
  /**
- * ufshcd_config_int_aggr - Configure interrupt aggregation values.
- *		Currently there is no use case where we want to configure
- *		interrupt aggregation dynamically. So to configure interrupt
- *		aggregation, #define INT_AGGR_COUNTER_THRESHOLD_VALUE and
- *		INT_AGGR_TIMEOUT_VALUE are used.
+ * ufshcd_reset_intr_aggr - Reset interrupt aggregation values.
   * @hba: per adapter instance
- * @option: Interrupt aggregation option
   */
  static inline void
-ufshcd_config_int_aggr(struct ufs_hba *hba, int option)
+ufshcd_reset_intr_aggr(struct ufs_hba *hba)
  {
-	switch (option) {
-	case INT_AGGR_RESET:
-		ufshcd_writel(hba, INT_AGGR_ENABLE |
-			      INT_AGGR_COUNTER_AND_TIMER_RESET,
-			      REG_UTP_TRANSFER_REQ_INT_AGG_CONTROL);
-		break;
-	case INT_AGGR_CONFIG:
-		ufshcd_writel(hba, INT_AGGR_ENABLE | INT_AGGR_PARAM_WRITE |
-			      INT_AGGR_COUNTER_THRESHOLD_VALUE |
-			      INT_AGGR_TIMEOUT_VALUE,
-			      REG_UTP_TRANSFER_REQ_INT_AGG_CONTROL);
-		break;
-	}
+	ufshcd_writel(hba, INT_AGGR_ENABLE |
+		      INT_AGGR_COUNTER_AND_TIMER_RESET,
+		      REG_UTP_TRANSFER_REQ_INT_AGG_CONTROL);
+}
+
+/**
+ * ufshcd_config_intr_aggr - Configure interrupt aggregation values.
+ * @hba: per adapter instance
+ * @cnt: Interrupt aggregation counter threshold
+ * @tmout: Interrupt aggregation timeout value
+ */
+static inline void
+ufshcd_config_intr_aggr(struct ufs_hba *hba, u8 cnt, u8 tmout)
+{
+	ufshcd_writel(hba, INT_AGGR_ENABLE | INT_AGGR_PARAM_WRITE |
+		      INT_AGGR_COUNTER_THLD_VAL(cnt) |
+		      INT_AGGR_TIMEOUT_VAL(tmout),
+		      REG_UTP_TRANSFER_REQ_INT_AGG_CONTROL);
  }
 
  /**

@@ -1523,7 +1520,7 @@ static int ufshcd_make_hba_operational(struct ufs_hba *hba)
  	ufshcd_enable_intr(hba, UFSHCD_ENABLE_INTRS);
 
  	/* Configure interrupt aggregation */
-	ufshcd_config_int_aggr(hba, INT_AGGR_CONFIG);
+	ufshcd_config_intr_aggr(hba, hba->nutrs - 1, INT_AGGR_DEF_TO);
 
  	/* Configure UTRL and UTMRL base address registers */
  	ufshcd_writel(hba, lower_32_bits(hba->utrdl_dma_addr),

@@ -1971,7 +1968,7 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
 
  	/* Reset interrupt aggregation counters */
  	if (int_aggr_reset)
-		ufshcd_config_int_aggr(hba, INT_AGGR_RESET);
+		ufshcd_reset_intr_aggr(hba);
  }
 
  /**

drivers/scsi/ufs/ufshci.h +2 -2

index f1e1b74..739ae3a 100644
@@ -226,8 +226,8 @@ enum {
 
  #define MASK_UIC_COMMAND_RESULT			0xFF
 
-#define INT_AGGR_COUNTER_THRESHOLD_VALUE	(0x1F << 8)
-#define INT_AGGR_TIMEOUT_VALUE			(0x02)
+#define INT_AGGR_COUNTER_THLD_VAL(c)	(((c) & 0x1F) << 8)
+#define INT_AGGR_TIMEOUT_VAL(t)		(((t) & 0xFF) << 0)
 
  /* Interrupt disable masks */
  enum {

Implements to support GET and SET operations of the DME. These operations are used to configure the behavior of the UNIPRO. Along with basic operation, {Peer/AttrSetType} can be mixed.

Signed-off-by Seungwon Jeon <[email protected]>

Reviewed-by Subhash Jadavani <[email protected]>

---
  drivers/scsi/ufs/ufshcd.c |   88 +++++++++++++++++++++++++++++++++++++++++++++
  drivers/scsi/ufs/ufshcd.h |   51 ++++++++++++++++++++++++++
  drivers/scsi/ufs/ufshci.h |    6 +++
  3 files changed, 145 insertions(+), 0 deletions(-)

drivers/scsi/ufs/ufshcd.c +88 -0

index c90b88a..00bfc1a 100644
@@ -285,6 +285,18 @@ static inline int ufshcd_get_uic_cmd_result(struct ufs_hba *hba)
  }
 
  /**
+ * ufshcd_get_dme_attr_val - Get the value of attribute returned by UIC command
+ * @hba: Pointer to adapter instance
+ *
+ * This function gets UIC command argument3
+ * Returns 0 on success, non zero value on error
+ */
+static inline u32 ufshcd_get_dme_attr_val(struct ufs_hba *hba)
+{
+	return ufshcd_readl(hba, REG_UIC_COMMAND_ARG_3);
+}
+
+/**
   * ufshcd_get_req_rsp - returns the TR response transaction type
   * @ucd_rsp_ptr: pointer to response UPIU
   */

@@ -1440,6 +1452,80 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
  }
 
  /**
+ * ufshcd_dme_set_attr - UIC command for DME_SET, DME_PEER_SET
+ * @hba: per adapter instance
+ * @attr_sel: uic command argument1
+ * @attr_set: attribute set type as uic command argument2
+ * @mib_val: setting value as uic command argument3
+ * @peer: indicate whether peer or local
+ *
+ * Returns 0 on success, non-zero value on failure
+ */
+int ufshcd_dme_set_attr(struct ufs_hba *hba, u32 attr_sel,
+			u8 attr_set, u32 mib_val, u8 peer)
+{
+	struct uic_command uic_cmd = {0};
+	static const char *const action[] = {
+		"dme-set",
+		"dme-peer-set"
+	};
+	const char *set = action[!!peer];
+	int ret;
+
+	uic_cmd.command = peer ?
+		UIC_CMD_DME_PEER_SET : UIC_CMD_DME_SET;
+	uic_cmd.argument1 = attr_sel;
+	uic_cmd.argument2 = UIC_ARG_ATTR_TYPE(attr_set);
+	uic_cmd.argument3 = mib_val;
+
+	ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
+	if (ret)
+		dev_err(hba->dev, "%s: attr-id 0x%x val 0x%x error code %d\n",
+			set, UIC_GET_ATTR_ID(attr_sel), mib_val, ret);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(ufshcd_dme_set_attr);
+
+/**
+ * ufshcd_dme_get_attr - UIC command for DME_GET, DME_PEER_GET
+ * @hba: per adapter instance
+ * @attr_sel: uic command argument1
+ * @mib_val: the value of the attribute as returned by the UIC command
+ * @peer: indicate whether peer or local
+ *
+ * Returns 0 on success, non-zero value on failure
+ */
+int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 attr_sel,
+			u32 *mib_val, u8 peer)
+{
+	struct uic_command uic_cmd = {0};
+	static const char *const action[] = {
+		"dme-get",
+		"dme-peer-get"
+	};
+	const char *get = action[!!peer];
+	int ret;
+
+	uic_cmd.command = peer ?
+		UIC_CMD_DME_PEER_GET : UIC_CMD_DME_GET;
+	uic_cmd.argument1 = attr_sel;
+
+	ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
+	if (ret) {
+		dev_err(hba->dev, "%s: attr-id 0x%x error code %d\n",
+			get, UIC_GET_ATTR_ID(attr_sel), ret);
+		goto out;
+	}
+
+	if (mib_val)
+		*mib_val = uic_cmd.argument3;
+out:
+	return ret;
+}
+EXPORT_SYMBOL_GPL(ufshcd_dme_get_attr);
+
+/**
   * ufshcd_complete_dev_init() - checks device readiness
   * hba: per-adapter instance
   *

@@ -1912,6 +1998,8 @@ static void ufshcd_uic_cmd_compl(struct ufs_hba *hba)
  	if (hba->active_uic_cmd) {
  		hba->active_uic_cmd->argument2 |=
  			ufshcd_get_uic_cmd_result(hba);
+		hba->active_uic_cmd->argument3 =
+			ufshcd_get_dme_attr_val(hba);
  		complete(&hba->active_uic_cmd->done);
  	}
  }

drivers/scsi/ufs/ufshcd.h +51 -0

index 8f5624e..ab1518d 100644
@@ -275,4 +275,55 @@ static inline void check_upiu_size(void)
  extern int ufshcd_runtime_suspend(struct ufs_hba *hba);
  extern int ufshcd_runtime_resume(struct ufs_hba *hba);
  extern int ufshcd_runtime_idle(struct ufs_hba *hba);
+extern int ufshcd_dme_set_attr(struct ufs_hba *hba, u32 attr_sel,
+			       u8 attr_set, u32 mib_val, u8 peer);
+extern int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 attr_sel,
+			       u32 *mib_val, u8 peer);
+
+/* UIC command interfaces for DME primitives */
+#define DME_LOCAL	0
+#define DME_PEER	1
+#define ATTR_SET_NOR	0	/* NORMAL */
+#define ATTR_SET_ST	1	/* STATIC */
+
+static inline int ufshcd_dme_set(struct ufs_hba *hba, u32 attr_sel,
+				 u32 mib_val)
+{
+	return ufshcd_dme_set_attr(hba, attr_sel, ATTR_SET_NOR,
+				   mib_val, DME_LOCAL);
+}
+
+static inline int ufshcd_dme_st_set(struct ufs_hba *hba, u32 attr_sel,
+				    u32 mib_val)
+{
+	return ufshcd_dme_set_attr(hba, attr_sel, ATTR_SET_ST,
+				   mib_val, DME_LOCAL);
+}
+
+static inline int ufshcd_dme_peer_set(struct ufs_hba *hba, u32 attr_sel,
+				      u32 mib_val)
+{
+	return ufshcd_dme_set_attr(hba, attr_sel, ATTR_SET_NOR,
+				   mib_val, DME_PEER);
+}
+
+static inline int ufshcd_dme_peer_st_set(struct ufs_hba *hba, u32 attr_sel,
+					 u32 mib_val)
+{
+	return ufshcd_dme_set_attr(hba, attr_sel, ATTR_SET_ST,
+				   mib_val, DME_PEER);
+}
+
+static inline int ufshcd_dme_get(struct ufs_hba *hba,
+				 u32 attr_sel, u32 *mib_val)
+{
+	return ufshcd_dme_get_attr(hba, attr_sel, mib_val, DME_LOCAL);
+}
+
+static inline int ufshcd_dme_peer_get(struct ufs_hba *hba,
+				      u32 attr_sel, u32 *mib_val)
+{
+	return ufshcd_dme_get_attr(hba, attr_sel, mib_val, DME_PEER);
+}
+
  #endif /* End of Header */

drivers/scsi/ufs/ufshci.h +6 -0

index 739ae3a..1e1fe26 100644
@@ -191,6 +191,12 @@ enum {
  #define CONFIG_RESULT_CODE_MASK		0xFF
  #define GENERIC_ERROR_CODE_MASK		0xFF
 
+#define UIC_ARG_MIB_SEL(attr, sel)	((((attr) & 0xFFFF) << 16) |\
+					 ((sel) & 0xFFFF))
+#define UIC_ARG_MIB(attr)		UIC_ARG_MIB_SEL(attr, 0)
+#define UIC_ARG_ATTR_TYPE(t)		(((t) & 0xFF) << 16)
+#define UIC_GET_ATTR_ID(v)		(((v) >> 16) & 0xFFFF)
+
  /* UIC Commands */
  enum {
  	UIC_CMD_DME_GET			= 0x01,