diff mbox series

[v6,4/8] scsi: hisi_sas: Modify v3 HW SATA disk error state completion processing

Message ID 1665998435-199946-5-git-send-email-john.garry@huawei.com
State New
Headers
Series libsas and drivers: NCQ error handling |

Commit Message

John Garry Oct. 17, 2022, 9:20 a.m. UTC 
  From: Xingui Yang <yangxingui@huawei.com>

When an NCQ error occurs, the  controller will abnormally complete the I/Os
that are newly delivered to disk, and bit8 in CQ dw3 will be set which
indicates that the SATA disk is in error state. The current processing flow
is to set ts->stat to SAS_OPEN_REJECT and then sas_ata_task_done() will
set FIS stat to ATA_ERR. After analyzing the IO by ata_eh_analyze_tf(),
err_mask will set to AC_ERR_HSM. If media error occurs for four times
within 10 minutes and the chip rejects new I/Os for four times, NCQ will
be disabled due to excessive errors, which is undesirable.

Therefore, use sas_task_abort() to handle abnormally completed I/Os when
SATA disk is in error state, as these abnormally completed I/Os are already
processed by sas_ata_device_link_abort() and qc->flag are set to
ATA_QCFLAG_FAILED. If sas_task_abort() is used, qc->err_mask will not be
modified in EH. Unlike the current process flow, it will not increase
the count of ECAT_TOUT_HSM and not turn off NCQ. Like other I/Os on the
disk that do not have an error but do not return after the NCQ error, they
are retried after the EH.

Signed-off-by: Xingui Yang <yangxingui@huawei.com>
Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Niklas Cassel Oct. 17, 2022, 10:45 a.m. UTC | #1 
On Mon, Oct 17, 2022 at 05:20:31PM +0800, John Garry wrote:
> From: Xingui Yang <yangxingui@huawei.com>
> 
> When an NCQ error occurs, the  controller will abnormally complete the I/Os
> that are newly delivered to disk, and bit8 in CQ dw3 will be set which
> indicates that the SATA disk is in error state. The current processing flow
> is to set ts->stat to SAS_OPEN_REJECT and then sas_ata_task_done() will
> set FIS stat to ATA_ERR. After analyzing the IO by ata_eh_analyze_tf(),
> err_mask will set to AC_ERR_HSM. If media error occurs for four times
> within 10 minutes and the chip rejects new I/Os for four times, NCQ will
> be disabled due to excessive errors, which is undesirable.

With patch 8/8 in this series, will it still set err_mask to AC_ERR_HSM?
If not, is this patch still needed with patch 8/8?


Kind regards,
Niklas

> 
> Therefore, use sas_task_abort() to handle abnormally completed I/Os when
> SATA disk is in error state, as these abnormally completed I/Os are already
> processed by sas_ata_device_link_abort() and qc->flag are set to
> ATA_QCFLAG_FAILED. If sas_task_abort() is used, qc->err_mask will not be
> modified in EH. Unlike the current process flow, it will not increase
> the count of ECAT_TOUT_HSM and not turn off NCQ. Like other I/Os on the
> disk that do not have an error but do not return after the NCQ error, they
> are retried after the EH.
> 
> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 5 ++++-
>  1 file changed, 4 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 0ae8a60aaf93..0c3fcb807806 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> @@ -428,6 +428,8 @@
>  #define CMPLT_HDR_DEV_ID_OFF		16
>  #define CMPLT_HDR_DEV_ID_MSK		(0xffff << CMPLT_HDR_DEV_ID_OFF)
>  /* dw3 */
> +#define SATA_DISK_IN_ERROR_STATUS_OFF	8
> +#define SATA_DISK_IN_ERROR_STATUS_MSK	(0x1 << SATA_DISK_IN_ERROR_STATUS_OFF)
>  #define CMPLT_HDR_SATA_DISK_ERR_OFF	16
>  #define CMPLT_HDR_SATA_DISK_ERR_MSK	(0x1 << CMPLT_HDR_SATA_DISK_ERR_OFF)
>  #define CMPLT_HDR_IO_IN_TARGET_OFF	17
> @@ -2219,7 +2221,8 @@ slot_err_v3_hw(struct hisi_hba *hisi_hba, struct sas_task *task,
>  		} else if (dma_rx_err_type & RX_DATA_LEN_UNDERFLOW_MSK) {
>  			ts->residual = trans_tx_fail_type;
>  			ts->stat = SAS_DATA_UNDERRUN;
> -		} else if (dw3 & CMPLT_HDR_IO_IN_TARGET_MSK) {
> +		} else if ((dw3 & CMPLT_HDR_IO_IN_TARGET_MSK) ||
> +			   (dw3 & SATA_DISK_IN_ERROR_STATUS_MSK)) {
>  			ts->stat = SAS_PHY_DOWN;
>  			slot->abort = 1;
>  		} else {
> -- 
> 2.35.3
>
John Garry Oct. 17, 2022, 11:01 a.m. UTC | #2 
On 17/10/2022 11:45, Niklas Cassel wrote:
> On Mon, Oct 17, 2022 at 05:20:31PM +0800, John Garry wrote:
>> From: Xingui Yang<yangxingui@huawei.com>
>>
>> When an NCQ error occurs, the  controller will abnormally complete the I/Os
>> that are newly delivered to disk, and bit8 in CQ dw3 will be set which
>> indicates that the SATA disk is in error state. The current processing flow
>> is to set ts->stat to SAS_OPEN_REJECT and then sas_ata_task_done() will
>> set FIS stat to ATA_ERR. After analyzing the IO by ata_eh_analyze_tf(),
>> err_mask will set to AC_ERR_HSM. If media error occurs for four times
>> within 10 minutes and the chip rejects new I/Os for four times, NCQ will
>> be disabled due to excessive errors, which is undesirable.
> With patch 8/8 in this series, will it still set err_mask to AC_ERR_HSM?
> If not, is this patch still needed with patch 8/8?
> 

I spoke to colleague Xingui Yang and he does not think that it is still 
required. But I was leaving it in as it still looked like a sensible 
change. I suppose I can drop it.

Thanks,
John
diff mbox series

Patch

diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index 0ae8a60aaf93..0c3fcb807806 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -428,6 +428,8 @@ 
 #define CMPLT_HDR_DEV_ID_OFF		16
 #define CMPLT_HDR_DEV_ID_MSK		(0xffff << CMPLT_HDR_DEV_ID_OFF)
 /* dw3 */
+#define SATA_DISK_IN_ERROR_STATUS_OFF	8
+#define SATA_DISK_IN_ERROR_STATUS_MSK	(0x1 << SATA_DISK_IN_ERROR_STATUS_OFF)
 #define CMPLT_HDR_SATA_DISK_ERR_OFF	16
 #define CMPLT_HDR_SATA_DISK_ERR_MSK	(0x1 << CMPLT_HDR_SATA_DISK_ERR_OFF)
 #define CMPLT_HDR_IO_IN_TARGET_OFF	17
@@ -2219,7 +2221,8 @@  slot_err_v3_hw(struct hisi_hba *hisi_hba, struct sas_task *task,
 		} else if (dma_rx_err_type & RX_DATA_LEN_UNDERFLOW_MSK) {
 			ts->residual = trans_tx_fail_type;
 			ts->stat = SAS_DATA_UNDERRUN;
-		} else if (dw3 & CMPLT_HDR_IO_IN_TARGET_MSK) {
+		} else if ((dw3 & CMPLT_HDR_IO_IN_TARGET_MSK) ||
+			   (dw3 & SATA_DISK_IN_ERROR_STATUS_MSK)) {
 			ts->stat = SAS_PHY_DOWN;
 			slot->abort = 1;
 		} else {