[PATCH 0/2] Some small fixes for hisi_sas

[PATCH 0/2] Some small fixes for hisi_sas

[chenxiang]

From: Xiang Chen <chenxiang66@hisilicon.com>

This series contain two small fixes including:
- Handle the NCQ error returned by D2H frame;
- Correct the deadline value for ata_wait_after_reset() in
function hisi_sas_debug_I_T_nexus_reset;

Xingui Yang (1):
  scsi: hisi_sas: Handle the NCQ error returned by D2H frame

Yihang Li (1):
  scsi: hisi_sas: Modify the deadline for ata_wait_after_reset()

 drivers/scsi/hisi_sas/hisi_sas_main.c  | 4 +++-
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 9 ++++++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

-- 
2.30.0

[PATCH 1/2] scsi: hisi_sas: Handle the NCQ error returned by D2H frame

[chenxiang]

From: Xingui Yang <yangxingui@huawei.com>

We find that some disks use D2H frame instead of SDB frame to return NCQ
error. Currently, only the I/O corresponding to the D2H frame is processed
in this scenario, which does not meet the processing requirements of the
NCQ error scenario.
So we set dev_status to HISI_SAS_DEV_NCQ_ERR and abort all I/Os of the disk
in this scenario.

Signed-off-by: Xingui Yang <yangxingui@huawei.com>
Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
---
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index 7d2a33514538..3935fa6bc72b 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -2244,7 +2244,14 @@ slot_err_v3_hw(struct hisi_hba *hisi_hba, struct sas_task *task,
 	case SAS_PROTOCOL_SATA | SAS_PROTOCOL_STP:
 		if ((dw0 & CMPLT_HDR_RSPNS_XFRD_MSK) &&
 		    (sipc_rx_err_type & RX_FIS_STATUS_ERR_MSK)) {
-			ts->stat = SAS_PROTO_RESPONSE;
+			if (task->ata_task.use_ncq) {
+				struct domain_device *device = task->dev;
+				struct hisi_sas_device *sas_dev =
+						device->lldd_dev;
+				sas_dev->dev_status = HISI_SAS_DEV_NCQ_ERR;
+				slot->abort = 1;
+			} else
+				ts->stat = SAS_PROTO_RESPONSE;
 		} else if (dma_rx_err_type & RX_DATA_LEN_UNDERFLOW_MSK) {
 			ts->residual = trans_tx_fail_type;
 			ts->stat = SAS_DATA_UNDERRUN;
-- 
2.30.0

[PATCH 2/2] scsi: hisi_sas: Modify the deadline for ata_wait_after_reset()

[chenxiang]

From: Yihang Li <liyihang9@huawei.com>

We found that the second parameter of function ata_wait_after_reset() is
incorrectly used. We call smp_ata_check_ready_type() to poll the device
type until the 30s timeout, so the correct deadline should be (jiffies +
30000).

Fixes: 3c2673a09cf1 ("scsi: hisi_sas: Fix SATA devices missing issue during I_T nexus reset")
Signed-off-by: xiabing <xiabing12@h-partners.com>
Signed-off-by: Yihang Li <liyihang9@huawei.com>
Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
---
 drivers/scsi/hisi_sas/hisi_sas_main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index 097dfe4b620d..7245600aedb2 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -1796,8 +1796,10 @@ static int hisi_sas_debug_I_T_nexus_reset(struct domain_device *device)
 
 	if (dev_is_sata(device)) {
 		struct ata_link *link = &device->sata_dev.ap->link;
+		unsigned long deadline = ata_deadline(jiffies,
+				HISI_SAS_WAIT_PHYUP_TIMEOUT / HZ * 1000);
 
-		rc = ata_wait_after_reset(link, HISI_SAS_WAIT_PHYUP_TIMEOUT,
+		rc = ata_wait_after_reset(link, deadline,
 					  smp_ata_check_ready_type);
 	} else {
 		msleep(2000);
-- 
2.30.0

Re: [PATCH 2/2] scsi: hisi_sas: Modify the deadline for ata_wait_after_reset()

[Damien Le Moal]

On 4/1/24 14:49, chenxiang wrote:
> From: Yihang Li <liyihang9@huawei.com>
> 
> We found that the second parameter of function ata_wait_after_reset() is
> incorrectly used. We call smp_ata_check_ready_type() to poll the device
> type until the 30s timeout, so the correct deadline should be (jiffies +
> 30000).
> 
> Fixes: 3c2673a09cf1 ("scsi: hisi_sas: Fix SATA devices missing issue during I_T nexus reset")
> Signed-off-by: xiabing <xiabing12@h-partners.com>
> Signed-off-by: Yihang Li <liyihang9@huawei.com>
> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
> ---
>  drivers/scsi/hisi_sas/hisi_sas_main.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
> index 097dfe4b620d..7245600aedb2 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> @@ -1796,8 +1796,10 @@ static int hisi_sas_debug_I_T_nexus_reset(struct domain_device *device)
>  
>  	if (dev_is_sata(device)) {
>  		struct ata_link *link = &device->sata_dev.ap->link;
> +		unsigned long deadline = ata_deadline(jiffies,
> +				HISI_SAS_WAIT_PHYUP_TIMEOUT / HZ * 1000);

At the very least, you should use jiffies_to_msecs() here. But I do not see the
point though given that ata_deadline will do "jiffies + msecs_to_jiffies()".

So may be calling:

	rc = ata_wait_after_reset(link, jiffies + HISI_SAS_WAIT_PHYUP_TIMEOUT,
				  smp_ata_check_ready_type);

May be simpler and more readable.

>  
> -		rc = ata_wait_after_reset(link, HISI_SAS_WAIT_PHYUP_TIMEOUT,
> +		rc = ata_wait_after_reset(link, deadline,
>  					  smp_ata_check_ready_type);
>  	} else {
>  		msleep(2000);

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/2] scsi: hisi_sas: Handle the NCQ error returned by D2H frame

[Damien Le Moal]

On 4/1/24 14:49, chenxiang wrote:
> From: Xingui Yang <yangxingui@huawei.com>
> 
> We find that some disks use D2H frame instead of SDB frame to return NCQ
> error. Currently, only the I/O corresponding to the D2H frame is processed
> in this scenario, which does not meet the processing requirements of the
> NCQ error scenario.
> So we set dev_status to HISI_SAS_DEV_NCQ_ERR and abort all I/Os of the disk
> in this scenario.
> 
> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
> ---
>  drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> index 7d2a33514538..3935fa6bc72b 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> @@ -2244,7 +2244,14 @@ slot_err_v3_hw(struct hisi_hba *hisi_hba, struct sas_task *task,
>  	case SAS_PROTOCOL_SATA | SAS_PROTOCOL_STP:
>  		if ((dw0 & CMPLT_HDR_RSPNS_XFRD_MSK) &&
>  		    (sipc_rx_err_type & RX_FIS_STATUS_ERR_MSK)) {
> -			ts->stat = SAS_PROTO_RESPONSE;
> +			if (task->ata_task.use_ncq) {
> +				struct domain_device *device = task->dev;> +				struct hisi_sas_device *sas_dev =
> +						device->lldd_dev;

Missing blank line after the declaration. And why the line split for the above
declaration ? That fits in 80 chars line...

> +				sas_dev->dev_status = HISI_SAS_DEV_NCQ_ERR;
> +				slot->abort = 1;
> +			} else
> +				ts->stat = SAS_PROTO_RESPONSE;

Missing the curly brackets here.

>  		} else if (dma_rx_err_type & RX_DATA_LEN_UNDERFLOW_MSK) {
>  			ts->residual = trans_tx_fail_type;
>  			ts->stat = SAS_DATA_UNDERRUN;

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/2] scsi: hisi_sas: Handle the NCQ error returned by D2H frame

[chenxiang (M)]

Hi Damien,


在 2024/4/1 星期一 15:10, Damien Le Moal 写道:
> On 4/1/24 14:49, chenxiang wrote:
>> From: Xingui Yang <yangxingui@huawei.com>
>>
>> We find that some disks use D2H frame instead of SDB frame to return NCQ
>> error. Currently, only the I/O corresponding to the D2H frame is processed
>> in this scenario, which does not meet the processing requirements of the
>> NCQ error scenario.
>> So we set dev_status to HISI_SAS_DEV_NCQ_ERR and abort all I/Os of the disk
>> in this scenario.
>>
>> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
>> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
>> ---
>>   drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
>> index 7d2a33514538..3935fa6bc72b 100644
>> --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
>> +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
>> @@ -2244,7 +2244,14 @@ slot_err_v3_hw(struct hisi_hba *hisi_hba, struct sas_task *task,
>>   	case SAS_PROTOCOL_SATA | SAS_PROTOCOL_STP:
>>   		if ((dw0 & CMPLT_HDR_RSPNS_XFRD_MSK) &&
>>   		    (sipc_rx_err_type & RX_FIS_STATUS_ERR_MSK)) {
>> -			ts->stat = SAS_PROTO_RESPONSE;
>> +			if (task->ata_task.use_ncq) {
>> +				struct domain_device *device = task->dev;> +				struct hisi_sas_device *sas_dev =
>> +						device->lldd_dev;
> Missing blank line after the declaration. And why the line split for the above
> declaration ? That fits in 80 chars line...

Will change it in next version.

>
>> +				sas_dev->dev_status = HISI_SAS_DEV_NCQ_ERR;
>> +				slot->abort = 1;
>> +			} else
>> +				ts->stat = SAS_PROTO_RESPONSE;
> Missing the curly brackets here.


Will change it in next version.

>
>>   		} else if (dma_rx_err_type & RX_DATA_LEN_UNDERFLOW_MSK) {
>>   			ts->residual = trans_tx_fail_type;
>>   			ts->stat = SAS_DATA_UNDERRUN;

Re: [PATCH 2/2] scsi: hisi_sas: Modify the deadline for ata_wait_after_reset()

[chenxiang (M)]

Hi Damien,


在 2024/4/1 星期一 15:08, Damien Le Moal 写道:
> On 4/1/24 14:49, chenxiang wrote:
>> From: Yihang Li <liyihang9@huawei.com>
>>
>> We found that the second parameter of function ata_wait_after_reset() is
>> incorrectly used. We call smp_ata_check_ready_type() to poll the device
>> type until the 30s timeout, so the correct deadline should be (jiffies +
>> 30000).
>>
>> Fixes: 3c2673a09cf1 ("scsi: hisi_sas: Fix SATA devices missing issue during I_T nexus reset")
>> Signed-off-by: xiabing <xiabing12@h-partners.com>
>> Signed-off-by: Yihang Li <liyihang9@huawei.com>
>> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
>> ---
>>   drivers/scsi/hisi_sas/hisi_sas_main.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
>> index 097dfe4b620d..7245600aedb2 100644
>> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
>> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
>> @@ -1796,8 +1796,10 @@ static int hisi_sas_debug_I_T_nexus_reset(struct domain_device *device)
>>   
>>   	if (dev_is_sata(device)) {
>>   		struct ata_link *link = &device->sata_dev.ap->link;
>> +		unsigned long deadline = ata_deadline(jiffies,
>> +				HISI_SAS_WAIT_PHYUP_TIMEOUT / HZ * 1000);
> At the very least, you should use jiffies_to_msecs() here. But I do not see the
> point though given that ata_deadline will do "jiffies + msecs_to_jiffies()".

We found the issue when reading the code of  libata-eh which uses 
ata_deadline() and ata_wait_after_reset().
So we modify it as above.

>
> So may be calling:
>
> 	rc = ata_wait_after_reset(link, jiffies + HISI_SAS_WAIT_PHYUP_TIMEOUT,
> 				  smp_ata_check_ready_type);
>
> May be simpler and more readable.

OK, it is more simpler and readable. Will change it in next version.

>
>>   
>> -		rc = ata_wait_after_reset(link, HISI_SAS_WAIT_PHYUP_TIMEOUT,
>> +		rc = ata_wait_after_reset(link, deadline,
>>   					  smp_ata_check_ready_type);
>>   	} else {
>>   		msleep(2000);