As reported in [0], the pm8001 driver NCQ error handling more or less
duplicates what libata does in link error handling, as follows:
- abort all commands
- do autopsy with read log ext 10 command
- reset the target to recover, if necessary
Indeed for the hisi_sas driver we want to add similar handling for NCQ
errors.
This series add a new libsas API - sas_ata_device_link_abort() - to handle
host NCQ errors, and fixes up pm8001 and hisi_sas drivers to use it.
A difference in the pm8001 driver NCQ error handling is that we send
SATA_ABORT per-task prior to read log ext10, but I feel that this should
not make a difference to the error handling.
Damien kindly tested previous the series for pm8001, but any further pm8001
testing would be appreciated as I have since tweaked pm8001 handling again.
This is because the pm8001 driver hangs on my arm64 machine read log ext10
command.
Finally with these changes we can make the libsas task alloc/free APIs
private, which they should always have been.
Based on v6.0-rc4
[0] https://lore.kernel.org/linux-scsi/[email protected]/
Changes since v2:
- Stop sending SATA_ABORT all for pm8001 handling
- Make "reset" optional in sas_ata_device_link_abort()
Changes since v1:
- Rename sas_ata_link_abort() -> sas_ata_device_link_abort()
- Set EH RESET flag in sas_ata_device_link_abort()
- Add Jack's Ack tags
- Rebase
John Garry (5):
scsi: pm8001: Modify task abort handling for SATA task
scsi: libsas: Add sas_ata_device_link_abort()
scsi: pm8001: Use sas_ata_device_link_abort() to handle NCQ errors
scsi: hisi_sas: Don't issue ATA softreset in hisi_sas_abort_task()
scsi: libsas: Make sas_{alloc, alloc_slow, free}_task() private
Xingui Yang (1):
scsi: hisi_sas: Add SATA_DISK_ERR bit handling for v3 hw
drivers/scsi/hisi_sas/hisi_sas_main.c | 5 +-
drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 22 ++-
drivers/scsi/libsas/sas_ata.c | 12 ++
drivers/scsi/libsas/sas_init.c | 3 -
drivers/scsi/libsas/sas_internal.h | 4 +
drivers/scsi/pm8001/pm8001_hwi.c | 186 ++++---------------------
drivers/scsi/pm8001/pm8001_sas.c | 8 ++
drivers/scsi/pm8001/pm8001_sas.h | 4 -
drivers/scsi/pm8001/pm80xx_hwi.c | 177 +++--------------------
include/scsi/libsas.h | 4 -
include/scsi/sas_ata.h | 6 +
11 files changed, 96 insertions(+), 335 deletions(-)
--
2.35.3
It is not necessary to issue an ATA softreset in hisi_sas_abort_task().
This is because in the libsas EH scheme we will defer to ATA EH which will
deal with any device reset during recovery.
Signed-off-by: John Garry <[email protected]>
---
drivers/scsi/hisi_sas/hisi_sas_main.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index 33af5b8dede2..2fe5290ac56c 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -1598,13 +1598,16 @@ static int hisi_sas_abort_task(struct sas_task *task)
} else if (task->task_proto & SAS_PROTOCOL_SATA ||
task->task_proto & SAS_PROTOCOL_STP) {
if (task->dev->dev_type == SAS_SATA_DEV) {
+ struct hisi_sas_slot *slot = task->lldd_task;
+
+ slot->task = NULL;
rc = hisi_sas_internal_task_abort_dev(sas_dev, false);
if (rc < 0) {
dev_err(dev, "abort task: internal abort failed\n");
goto out;
}
hisi_sas_dereg_device(hisi_hba, device);
- rc = hisi_sas_softreset_ata_disk(device);
+ rc = TMF_RESP_FUNC_COMPLETE;
}
} else if (task->lldd_task && task->task_proto & SAS_PROTOCOL_SMP) {
/* SMP */
--
2.35.3
We have no users outside libsas any longer, so make sas_alloc_task(),
sas_alloc_slow_task(), and sas_free_task() private.
Signed-off-by: John Garry <[email protected]>
---
drivers/scsi/libsas/sas_init.c | 3 ---
drivers/scsi/libsas/sas_internal.h | 4 ++++
include/scsi/libsas.h | 4 ----
3 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index e4f77072a58d..f2c05ebeb72f 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -35,7 +35,6 @@ struct sas_task *sas_alloc_task(gfp_t flags)
return task;
}
-EXPORT_SYMBOL_GPL(sas_alloc_task);
struct sas_task *sas_alloc_slow_task(gfp_t flags)
{
@@ -56,7 +55,6 @@ struct sas_task *sas_alloc_slow_task(gfp_t flags)
return task;
}
-EXPORT_SYMBOL_GPL(sas_alloc_slow_task);
void sas_free_task(struct sas_task *task)
{
@@ -65,7 +63,6 @@ void sas_free_task(struct sas_task *task)
kmem_cache_free(sas_task_cache, task);
}
}
-EXPORT_SYMBOL_GPL(sas_free_task);
/*------------ SAS addr hash -----------*/
void sas_hash_addr(u8 *hashed, const u8 *sas_addr)
diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
index 8d0ad3abc7b5..b54bcf3c9a9d 100644
--- a/drivers/scsi/libsas/sas_internal.h
+++ b/drivers/scsi/libsas/sas_internal.h
@@ -52,6 +52,10 @@ void sas_unregister_phys(struct sas_ha_struct *sas_ha);
struct asd_sas_event *sas_alloc_event(struct asd_sas_phy *phy, gfp_t gfp_flags);
void sas_free_event(struct asd_sas_event *event);
+struct sas_task *sas_alloc_task(gfp_t flags);
+struct sas_task *sas_alloc_slow_task(gfp_t flags);
+void sas_free_task(struct sas_task *task);
+
int sas_register_ports(struct sas_ha_struct *sas_ha);
void sas_unregister_ports(struct sas_ha_struct *sas_ha);
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 2dbead74a2af..f86b56bf7833 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -639,10 +639,6 @@ struct sas_task_slow {
#define SAS_TASK_STATE_ABORTED 4
#define SAS_TASK_NEED_DEV_RESET 8
-extern struct sas_task *sas_alloc_task(gfp_t flags);
-extern struct sas_task *sas_alloc_slow_task(gfp_t flags);
-extern void sas_free_task(struct sas_task *task);
-
static inline bool sas_is_internal_abort(struct sas_task *task)
{
return task->task_proto == SAS_PROTOCOL_INTERNAL_ABORT;
--
2.35.3
In commit c6b9ef5779c3 ("[SCSI] pm80xx: NCQ error handling changes") the
driver had support added to handle NCQ errors but much of what is done
in this handling is duplicated from the libata EH.
In that named commit we handle in 2x main steps:
a. Issue read log ext10 to examine and clear the errors
b. Issue SATA_ABORT all command
Indeed, in libata EH, we do similar to above:
a. ata_do_eh() -> ata_eh_autopsy() -> ata_eh_link_autopsy() ->
ata_eh_analyze_ncq_error() -> ata_eh_read_log_10h()
b. ata_do_eh() -> ata_eh_recover() which will issue a device soft reset
or hard reset
Since there is so much duplication, use sas_ata_device_link_abort() which
will abort all pending IOs and kick of ATA EH which will do the steps,
above.
However we will not follow the advisory to send the SATA_ABORT all command
after the autopsy in read log ext10. Indeed, in libsas EH, we already send
a per-task SATA_ABORT command, and this is prior to the ATA EH kicking in
and issuing the read log ext10 in the recovery process. I judge that this
is ok as the SATA_ABORT command does not actually send any protocol on the
link to abort IO on the other side, so would not change any state on the
disk (for the read log ext10 command).
Signed-off-by: John Garry <[email protected]>
---
drivers/scsi/pm8001/pm8001_hwi.c | 171 +++----------------------------
drivers/scsi/pm8001/pm8001_sas.h | 4 -
drivers/scsi/pm8001/pm80xx_hwi.c | 163 ++---------------------------
3 files changed, 19 insertions(+), 319 deletions(-)
diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c
index 1c8a43bf54ad..0bc16994f0b5 100644
--- a/drivers/scsi/pm8001/pm8001_hwi.c
+++ b/drivers/scsi/pm8001/pm8001_hwi.c
@@ -1724,7 +1724,14 @@ void pm8001_work_fn(struct work_struct *work)
pm8001_free_dev(pm8001_dev);
}
}
- } break;
+ }
+ break;
+ case IO_XFER_ERROR_ABORTED_NCQ_MODE:
+ {
+ dev = pm8001_dev->sas_device;
+ sas_ata_device_link_abort(dev, false);
+ }
+ break;
}
kfree(pw);
}
@@ -1748,110 +1755,6 @@ int pm8001_handle_event(struct pm8001_hba_info *pm8001_ha, void *data,
return ret;
}
-static void pm8001_send_abort_all(struct pm8001_hba_info *pm8001_ha,
- struct pm8001_device *pm8001_ha_dev)
-{
- struct pm8001_ccb_info *ccb;
- struct sas_task *task;
- struct task_abort_req task_abort;
- u32 opc = OPC_INB_SATA_ABORT;
- int ret;
-
- pm8001_ha_dev->id |= NCQ_ABORT_ALL_FLAG;
- pm8001_ha_dev->id &= ~NCQ_READ_LOG_FLAG;
-
- task = sas_alloc_slow_task(GFP_ATOMIC);
- if (!task) {
- pm8001_dbg(pm8001_ha, FAIL, "cannot allocate task\n");
- return;
- }
-
- task->task_done = pm8001_task_done;
-
- ccb = pm8001_ccb_alloc(pm8001_ha, pm8001_ha_dev, task);
- if (!ccb) {
- sas_free_task(task);
- return;
- }
-
- memset(&task_abort, 0, sizeof(task_abort));
- task_abort.abort_all = cpu_to_le32(1);
- task_abort.device_id = cpu_to_le32(pm8001_ha_dev->device_id);
- task_abort.tag = cpu_to_le32(ccb->ccb_tag);
-
- ret = pm8001_mpi_build_cmd(pm8001_ha, 0, opc, &task_abort,
- sizeof(task_abort), 0);
- if (ret) {
- sas_free_task(task);
- pm8001_ccb_free(pm8001_ha, ccb);
- }
-}
-
-static void pm8001_send_read_log(struct pm8001_hba_info *pm8001_ha,
- struct pm8001_device *pm8001_ha_dev)
-{
- struct sata_start_req sata_cmd;
- int res;
- struct pm8001_ccb_info *ccb;
- struct sas_task *task = NULL;
- struct host_to_dev_fis fis;
- struct domain_device *dev;
- u32 opc = OPC_INB_SATA_HOST_OPSTART;
-
- task = sas_alloc_slow_task(GFP_ATOMIC);
- if (!task) {
- pm8001_dbg(pm8001_ha, FAIL, "cannot allocate task !!!\n");
- return;
- }
- task->task_done = pm8001_task_done;
-
- /*
- * Allocate domain device by ourselves as libsas is not going to
- * provide any.
- */
- dev = kzalloc(sizeof(struct domain_device), GFP_ATOMIC);
- if (!dev) {
- sas_free_task(task);
- pm8001_dbg(pm8001_ha, FAIL,
- "Domain device cannot be allocated\n");
- return;
- }
- task->dev = dev;
- task->dev->lldd_dev = pm8001_ha_dev;
-
- ccb = pm8001_ccb_alloc(pm8001_ha, pm8001_ha_dev, task);
- if (!ccb) {
- sas_free_task(task);
- kfree(dev);
- return;
- }
-
- pm8001_ha_dev->id |= NCQ_READ_LOG_FLAG;
- pm8001_ha_dev->id |= NCQ_2ND_RLE_FLAG;
-
- /* construct read log FIS */
- memset(&fis, 0, sizeof(struct host_to_dev_fis));
- fis.fis_type = 0x27;
- fis.flags = 0x80;
- fis.command = ATA_CMD_READ_LOG_EXT;
- fis.lbal = 0x10;
- fis.sector_count = 0x1;
-
- memset(&sata_cmd, 0, sizeof(sata_cmd));
- sata_cmd.tag = cpu_to_le32(ccb->ccb_tag);
- sata_cmd.device_id = cpu_to_le32(pm8001_ha_dev->device_id);
- sata_cmd.ncqtag_atap_dir_m = cpu_to_le32((0x1 << 7) | (0x5 << 9));
- memcpy(&sata_cmd.sata_fis, &fis, sizeof(struct host_to_dev_fis));
-
- res = pm8001_mpi_build_cmd(pm8001_ha, 0, opc, &sata_cmd,
- sizeof(sata_cmd), 0);
- if (res) {
- sas_free_task(task);
- pm8001_ccb_free(pm8001_ha, ccb);
- kfree(dev);
- }
-}
-
/**
* mpi_ssp_completion- process the event that FW response to the SSP request.
* @pm8001_ha: our hba card information
@@ -2301,8 +2204,7 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
return;
}
- if ((pm8001_dev && !(pm8001_dev->id & NCQ_READ_LOG_FLAG))
- && unlikely(!t || !t->lldd_task || !t->dev)) {
+ if (pm8001_dev && unlikely(!t || !t->lldd_task || !t->dev)) {
pm8001_dbg(pm8001_ha, FAIL, "task or dev null\n");
return;
}
@@ -2360,15 +2262,6 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
if (param == 0) {
ts->resp = SAS_TASK_COMPLETE;
ts->stat = SAS_SAM_STAT_GOOD;
- /* check if response is for SEND READ LOG */
- if (pm8001_dev &&
- (pm8001_dev->id & NCQ_READ_LOG_FLAG)) {
- pm8001_send_abort_all(pm8001_ha, pm8001_dev);
- /* Free the tag */
- pm8001_tag_free(pm8001_ha, tag);
- sas_free_task(t);
- return;
- }
} else {
u8 len;
ts->resp = SAS_TASK_COMPLETE;
@@ -2666,9 +2559,10 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, void *piomb)
if (event == IO_XFER_ERROR_ABORTED_NCQ_MODE) {
/* find device using device id */
pm8001_dev = pm8001_find_dev(pm8001_ha, dev_id);
- /* send read log extension */
if (pm8001_dev)
- pm8001_send_read_log(pm8001_ha, pm8001_dev);
+ pm8001_handle_event(pm8001_ha,
+ pm8001_dev,
+ IO_XFER_ERROR_ABORTED_NCQ_MODE);
return;
}
@@ -3645,12 +3539,7 @@ int pm8001_mpi_task_abort_resp(struct pm8001_hba_info *pm8001_ha, void *piomb)
pm8001_ccb_task_free(pm8001_ha, ccb);
mb();
- if (pm8001_dev->id & NCQ_ABORT_ALL_FLAG) {
- sas_free_task(t);
- pm8001_dev->id &= ~NCQ_ABORT_ALL_FLAG;
- } else {
- t->task_done(t);
- }
+ t->task_done(t);
return 0;
}
@@ -4202,7 +4091,6 @@ static int pm8001_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
u64 phys_addr;
u32 ATAP = 0x0;
u32 dir;
- unsigned long flags;
u32 opc = OPC_INB_SATA_HOST_OPSTART;
memset(&sata_cmd, 0, sizeof(sata_cmd));
@@ -4257,39 +4145,6 @@ static int pm8001_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
sata_cmd.esgl = 0;
}
- /* Check for read log for failed drive and return */
- if (sata_cmd.sata_fis.command == 0x2f) {
- if (((pm8001_ha_dev->id & NCQ_READ_LOG_FLAG) ||
- (pm8001_ha_dev->id & NCQ_ABORT_ALL_FLAG) ||
- (pm8001_ha_dev->id & NCQ_2ND_RLE_FLAG))) {
- struct task_status_struct *ts;
-
- pm8001_ha_dev->id &= 0xDFFFFFFF;
- ts = &task->task_status;
-
- spin_lock_irqsave(&task->task_state_lock, flags);
- ts->resp = SAS_TASK_COMPLETE;
- ts->stat = SAS_SAM_STAT_GOOD;
- task->task_state_flags &= ~SAS_TASK_STATE_PENDING;
- task->task_state_flags |= SAS_TASK_STATE_DONE;
- if (unlikely((task->task_state_flags &
- SAS_TASK_STATE_ABORTED))) {
- spin_unlock_irqrestore(&task->task_state_lock,
- flags);
- pm8001_dbg(pm8001_ha, FAIL,
- "task 0x%p resp 0x%x stat 0x%x but aborted by upper layer\n",
- task, ts->resp,
- ts->stat);
- pm8001_ccb_task_free(pm8001_ha, ccb);
- } else {
- spin_unlock_irqrestore(&task->task_state_lock,
- flags);
- pm8001_ccb_task_free_done(pm8001_ha, ccb);
- return 0;
- }
- }
- }
-
return pm8001_mpi_build_cmd(pm8001_ha, 0, opc, &sata_cmd,
sizeof(sata_cmd), 0);
}
diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
index c5e3f380a01c..571ed927298b 100644
--- a/drivers/scsi/pm8001/pm8001_sas.h
+++ b/drivers/scsi/pm8001/pm8001_sas.h
@@ -579,10 +579,6 @@ struct pm8001_fw_image_header {
#define FLASH_UPDATE_DNLD_NOT_SUPPORTED 0x10
#define FLASH_UPDATE_DISABLED 0x11
-#define NCQ_READ_LOG_FLAG 0x80000000
-#define NCQ_ABORT_ALL_FLAG 0x40000000
-#define NCQ_2ND_RLE_FLAG 0x20000000
-
/* Device states */
#define DS_OPERATIONAL 0x01
#define DS_PORT_IN_RESET 0x02
diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
index dd0e06983cd3..4484c498bcb6 100644
--- a/drivers/scsi/pm8001/pm80xx_hwi.c
+++ b/drivers/scsi/pm8001/pm80xx_hwi.c
@@ -1778,113 +1778,6 @@ pm80xx_chip_interrupt_disable(struct pm8001_hba_info *pm8001_ha, u8 vec)
pm80xx_chip_intx_interrupt_disable(pm8001_ha);
}
-static void pm80xx_send_abort_all(struct pm8001_hba_info *pm8001_ha,
- struct pm8001_device *pm8001_ha_dev)
-{
- struct pm8001_ccb_info *ccb;
- struct sas_task *task;
- struct task_abort_req task_abort;
- u32 opc = OPC_INB_SATA_ABORT;
- int ret;
-
- pm8001_ha_dev->id |= NCQ_ABORT_ALL_FLAG;
- pm8001_ha_dev->id &= ~NCQ_READ_LOG_FLAG;
-
- task = sas_alloc_slow_task(GFP_ATOMIC);
- if (!task) {
- pm8001_dbg(pm8001_ha, FAIL, "cannot allocate task\n");
- return;
- }
- task->task_done = pm8001_task_done;
-
- ccb = pm8001_ccb_alloc(pm8001_ha, pm8001_ha_dev, task);
- if (!ccb) {
- sas_free_task(task);
- return;
- }
-
- memset(&task_abort, 0, sizeof(task_abort));
- task_abort.abort_all = cpu_to_le32(1);
- task_abort.device_id = cpu_to_le32(pm8001_ha_dev->device_id);
- task_abort.tag = cpu_to_le32(ccb->ccb_tag);
-
- ret = pm8001_mpi_build_cmd(pm8001_ha, 0, opc, &task_abort,
- sizeof(task_abort), 0);
- pm8001_dbg(pm8001_ha, FAIL, "Executing abort task end\n");
- if (ret) {
- sas_free_task(task);
- pm8001_ccb_free(pm8001_ha, ccb);
- }
-}
-
-static void pm80xx_send_read_log(struct pm8001_hba_info *pm8001_ha,
- struct pm8001_device *pm8001_ha_dev)
-{
- struct sata_start_req sata_cmd;
- int res;
- struct pm8001_ccb_info *ccb;
- struct sas_task *task = NULL;
- struct host_to_dev_fis fis;
- struct domain_device *dev;
- u32 opc = OPC_INB_SATA_HOST_OPSTART;
-
- task = sas_alloc_slow_task(GFP_ATOMIC);
- if (!task) {
- pm8001_dbg(pm8001_ha, FAIL, "cannot allocate task !!!\n");
- return;
- }
- task->task_done = pm8001_task_done;
-
- /*
- * Allocate domain device by ourselves as libsas is not going to
- * provide any.
- */
- dev = kzalloc(sizeof(struct domain_device), GFP_ATOMIC);
- if (!dev) {
- sas_free_task(task);
- pm8001_dbg(pm8001_ha, FAIL,
- "Domain device cannot be allocated\n");
- return;
- }
-
- task->dev = dev;
- task->dev->lldd_dev = pm8001_ha_dev;
-
- ccb = pm8001_ccb_alloc(pm8001_ha, pm8001_ha_dev, task);
- if (!ccb) {
- sas_free_task(task);
- kfree(dev);
- return;
- }
-
- pm8001_ha_dev->id |= NCQ_READ_LOG_FLAG;
- pm8001_ha_dev->id |= NCQ_2ND_RLE_FLAG;
-
- memset(&sata_cmd, 0, sizeof(sata_cmd));
-
- /* construct read log FIS */
- memset(&fis, 0, sizeof(struct host_to_dev_fis));
- fis.fis_type = 0x27;
- fis.flags = 0x80;
- fis.command = ATA_CMD_READ_LOG_EXT;
- fis.lbal = 0x10;
- fis.sector_count = 0x1;
-
- sata_cmd.tag = cpu_to_le32(ccb->ccb_tag);
- sata_cmd.device_id = cpu_to_le32(pm8001_ha_dev->device_id);
- sata_cmd.ncqtag_atap_dir_m_dad = cpu_to_le32(((0x1 << 7) | (0x5 << 9)));
- memcpy(&sata_cmd.sata_fis, &fis, sizeof(struct host_to_dev_fis));
-
- res = pm8001_mpi_build_cmd(pm8001_ha, 0, opc, &sata_cmd,
- sizeof(sata_cmd), 0);
- pm8001_dbg(pm8001_ha, FAIL, "Executing read log end\n");
- if (res) {
- sas_free_task(task);
- pm8001_ccb_free(pm8001_ha, ccb);
- kfree(dev);
- }
-}
-
/**
* mpi_ssp_completion - process the event that FW response to the SSP request.
* @pm8001_ha: our hba card information
@@ -2402,11 +2295,9 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha,
return;
}
- if ((pm8001_dev && !(pm8001_dev->id & NCQ_READ_LOG_FLAG))
- && unlikely(!t || !t->lldd_task || !t->dev)) {
- pm8001_dbg(pm8001_ha, FAIL, "task or dev null\n");
+
+ if (pm8001_dev && unlikely(!t->lldd_task || !t->dev))
return;
- }
ts = &t->task_status;
@@ -2463,15 +2354,6 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha,
if (param == 0) {
ts->resp = SAS_TASK_COMPLETE;
ts->stat = SAS_SAM_STAT_GOOD;
- /* check if response is for SEND READ LOG */
- if (pm8001_dev &&
- (pm8001_dev->id & NCQ_READ_LOG_FLAG)) {
- pm80xx_send_abort_all(pm8001_ha, pm8001_dev);
- /* Free the tag */
- pm8001_tag_free(pm8001_ha, tag);
- sas_free_task(t);
- return;
- }
} else {
u8 len;
ts->resp = SAS_TASK_COMPLETE;
@@ -2806,9 +2688,11 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha,
if (event == IO_XFER_ERROR_ABORTED_NCQ_MODE) {
/* find device using device id */
pm8001_dev = pm8001_find_dev(pm8001_ha, dev_id);
- /* send read log extension */
+ /* send read log extension by aborting the link - libata does what we want */
if (pm8001_dev)
- pm80xx_send_read_log(pm8001_ha, pm8001_dev);
+ pm8001_handle_event(pm8001_ha,
+ pm8001_dev,
+ IO_XFER_ERROR_ABORTED_NCQ_MODE);
return;
}
@@ -4556,7 +4440,6 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
u32 end_addr_high, end_addr_low;
u32 ATAP = 0x0;
u32 dir;
- unsigned long flags;
u32 opc = OPC_INB_SATA_HOST_OPSTART;
memset(&sata_cmd, 0, sizeof(sata_cmd));
@@ -4735,40 +4618,6 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
(task->ata_task.atapi_packet[15] << 24)));
}
- /* Check for read log for failed drive and return */
- if (sata_cmd.sata_fis.command == 0x2f) {
- if (pm8001_ha_dev && ((pm8001_ha_dev->id & NCQ_READ_LOG_FLAG) ||
- (pm8001_ha_dev->id & NCQ_ABORT_ALL_FLAG) ||
- (pm8001_ha_dev->id & NCQ_2ND_RLE_FLAG))) {
- struct task_status_struct *ts;
-
- pm8001_ha_dev->id &= 0xDFFFFFFF;
- ts = &task->task_status;
-
- spin_lock_irqsave(&task->task_state_lock, flags);
- ts->resp = SAS_TASK_COMPLETE;
- ts->stat = SAS_SAM_STAT_GOOD;
- task->task_state_flags &= ~SAS_TASK_STATE_PENDING;
- task->task_state_flags |= SAS_TASK_STATE_DONE;
- if (unlikely((task->task_state_flags &
- SAS_TASK_STATE_ABORTED))) {
- spin_unlock_irqrestore(&task->task_state_lock,
- flags);
- pm8001_dbg(pm8001_ha, FAIL,
- "task 0x%p resp 0x%x stat 0x%x but aborted by upper layer\n",
- task, ts->resp,
- ts->stat);
- pm8001_ccb_task_free(pm8001_ha, ccb);
- return 0;
- } else {
- spin_unlock_irqrestore(&task->task_state_lock,
- flags);
- pm8001_ccb_task_free_done(pm8001_ha, ccb);
- atomic_dec(&pm8001_ha_dev->running_req);
- return 0;
- }
- }
- }
trace_pm80xx_request_issue(pm8001_ha->id,
ccb->device ? ccb->device->attached_phy : PM8001_MAX_PHYS,
ccb->ccb_tag, opc,
--
2.35.3
Similar to how AHCI handles NCQ errors in ahci_error_intr() ->
ata_port_abort() -> ata_do_link_abort(), add an NCQ error handler for LLDDs
to call to initiate a link abort.
This will mark all outstanding QCs as failed and kick-off EH.
Note:
A "force reset" argument is added for drivers which require the ATA error
handling to always reset the device.
A driver may require this feature for when SATA device per-SCSI cmnd
resources are only released during reset for ATA EH. As such, we need an
option to force reset to be done, regardless of what any EH autopsy
decides.
Suggested-by: Damien Le Moal <[email protected]>
Signed-off-by: John Garry <[email protected]>
---
drivers/scsi/libsas/sas_ata.c | 12 ++++++++++++
include/scsi/sas_ata.h | 6 ++++++
2 files changed, 18 insertions(+)
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index d35c9296f738..bdffb6852dcf 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -861,6 +861,18 @@ void sas_ata_wait_eh(struct domain_device *dev)
ata_port_wait_eh(ap);
}
+void sas_ata_device_link_abort(struct domain_device *device, bool force_reset)
+{
+ struct ata_port *ap = device->sata_dev.ap;
+ struct ata_link *link = &ap->link;
+
+ link->eh_info.err_mask |= AC_ERR_DEV;
+ if (force_reset)
+ link->eh_info.action |= ATA_EH_RESET;
+ ata_link_abort(link);
+}
+EXPORT_SYMBOL_GPL(sas_ata_device_link_abort);
+
int sas_execute_ata_cmd(struct domain_device *device, u8 *fis, int force_phy_id)
{
struct sas_tmf_task tmf_task = {};
diff --git a/include/scsi/sas_ata.h b/include/scsi/sas_ata.h
index a1df4f9d57a3..e47f0aec0722 100644
--- a/include/scsi/sas_ata.h
+++ b/include/scsi/sas_ata.h
@@ -32,6 +32,7 @@ void sas_probe_sata(struct asd_sas_port *port);
void sas_suspend_sata(struct asd_sas_port *port);
void sas_resume_sata(struct asd_sas_port *port);
void sas_ata_end_eh(struct ata_port *ap);
+void sas_ata_device_link_abort(struct domain_device *dev, bool force_reset);
int sas_execute_ata_cmd(struct domain_device *device, u8 *fis,
int force_phy_id);
int sas_ata_wait_after_reset(struct domain_device *dev, unsigned long deadline);
@@ -87,6 +88,11 @@ static inline void sas_ata_end_eh(struct ata_port *ap)
{
}
+static inline void sas_ata_device_link_abort(struct domain_device *dev,
+ bool force_reset)
+{
+}
+
static inline int sas_execute_ata_cmd(struct domain_device *device, u8 *fis,
int force_phy_id)
{
--
2.35.3
On 9/7/22 00:08, John Garry wrote:
> As reported in [0], the pm8001 driver NCQ error handling more or less
> duplicates what libata does in link error handling, as follows:
> - abort all commands
> - do autopsy with read log ext 10 command
> - reset the target to recover, if necessary
>
> Indeed for the hisi_sas driver we want to add similar handling for NCQ
> errors.
>
> This series add a new libsas API - sas_ata_device_link_abort() - to handle
> host NCQ errors, and fixes up pm8001 and hisi_sas drivers to use it.
>
> A difference in the pm8001 driver NCQ error handling is that we send
> SATA_ABORT per-task prior to read log ext10, but I feel that this should
> not make a difference to the error handling.
>
> Damien kindly tested previous the series for pm8001, but any further pm8001
> testing would be appreciated as I have since tweaked pm8001 handling again.
> This is because the pm8001 driver hangs on my arm64 machine read log ext10
> command.
I tested this series on top of the current Linus tree. All look good. As
ususal, I hammered an SMR drive connected to a pm80xx adapter and
generated lots of invalid commands to check EH. No issues that I can see.
E.g., an unaligned write error look like this:
[ 5384.194853] pm80xx0:: mpi_sata_event 2685: SATA EVENT 0x23
[ 5384.238720] sas: Enter sas_scsi_recover_host busy: 1 failed: 1
[ 5384.246171] sas: sas_scsi_find_task: aborting task 0x0000000050be2a4b
[ 5384.254654] pm80xx0:: mpi_sata_completion 2292: task null, freeing
CCB tag 2
[ 5384.254676] sas: sas_scsi_find_task: task 0x0000000050be2a4b is aborted
[ 5384.294659] sas: sas_eh_handle_sas_errors: task 0x0000000050be2a4b is
aborted
[ 5384.318691] ata22.00: exception Emask 0x0 SAct 0x200000 SErr 0x0
action 0x0
[ 5384.328425] ata22.00: failed command: WRITE FPDMA QUEUED
[ 5384.336277] ata22.00: cmd 61/01:00:01:00:ea/00:00:02:00:00/40 tag 21
ncq dma 4096 out
[ 5384.336277] res 43/04:01:01:00:ea/00:00:02:00:00/00 Emask
0x400 (NCQ error) <F>
[ 5384.357299] ata22.00: status: { DRDY SENSE ERR }
[ 5384.364459] ata22.00: error: { ABRT }
[ 5384.553177] ata22.00: configured for UDMA/133
[ 5384.560343] sd 19:0:3:0: [sdl] tag#80 FAILED Result: hostbyte=DID_OK
driverbyte=DRIVER_OK cmd_age=0s
[ 5384.572175] sd 19:0:3:0: [sdl] tag#80 Sense Key : Illegal Request
[current]
[ 5384.581765] sd 19:0:3:0: [sdl] tag#80 Add. Sense: Unaligned write command
[ 5384.591126] sd 19:0:3:0: [sdl] tag#80 CDB: Write(16) 8a 00 00 00 00
00 02 ea 00 01 00 00 00 01 00 00
[ 5384.602938] I/O error, dev sdl, sector 391118856 op 0x1:(WRITE) flags
0x8800 phys_seg 1 prio class 2
[ 5384.613854] ata22: EH complete
[ 5384.618570] sas: --- Exit sas_scsi_recover_host: busy: 0 failed: 1
tries: 1
So looks good to me.
Feel free to add:
Tested-by: Damien Le Moal <[email protected]>
>
> Finally with these changes we can make the libsas task alloc/free APIs
> private, which they should always have been.
>
> Based on v6.0-rc4
>
> [0] https://lore.kernel.org/linux-scsi/[email protected]/
>
> Changes since v2:
> - Stop sending SATA_ABORT all for pm8001 handling
> - Make "reset" optional in sas_ata_device_link_abort()
>
> Changes since v1:
> - Rename sas_ata_link_abort() -> sas_ata_device_link_abort()
> - Set EH RESET flag in sas_ata_device_link_abort()
> - Add Jack's Ack tags
> - Rebase
>
> John Garry (5):
> scsi: pm8001: Modify task abort handling for SATA task
> scsi: libsas: Add sas_ata_device_link_abort()
> scsi: pm8001: Use sas_ata_device_link_abort() to handle NCQ errors
> scsi: hisi_sas: Don't issue ATA softreset in hisi_sas_abort_task()
> scsi: libsas: Make sas_{alloc, alloc_slow, free}_task() private
>
> Xingui Yang (1):
> scsi: hisi_sas: Add SATA_DISK_ERR bit handling for v3 hw
>
> drivers/scsi/hisi_sas/hisi_sas_main.c | 5 +-
> drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 22 ++-
> drivers/scsi/libsas/sas_ata.c | 12 ++
> drivers/scsi/libsas/sas_init.c | 3 -
> drivers/scsi/libsas/sas_internal.h | 4 +
> drivers/scsi/pm8001/pm8001_hwi.c | 186 ++++---------------------
> drivers/scsi/pm8001/pm8001_sas.c | 8 ++
> drivers/scsi/pm8001/pm8001_sas.h | 4 -
> drivers/scsi/pm8001/pm80xx_hwi.c | 177 +++--------------------
> include/scsi/libsas.h | 4 -
> include/scsi/sas_ata.h | 6 +
> 11 files changed, 96 insertions(+), 335 deletions(-)
>
--
Damien Le Moal
Western Digital Research
On 21/09/2022 04:59, Damien Le Moal wrote:
> I tested this series on top of the current Linus tree. All look good. As
> ususal, I hammered an SMR drive connected to a pm80xx adapter and
> generated lots of invalid commands to check EH. No issues that I can see.
> E.g., an unaligned write error look like this:
>
> [ 5384.194853] pm80xx0:: mpi_sata_event 2685: SATA EVENT 0x23
> [ 5384.238720] sas: Enter sas_scsi_recover_host busy: 1 failed: 1
> [ 5384.246171] sas: sas_scsi_find_task: aborting task 0x0000000050be2a4b
> [ 5384.254654] pm80xx0:: mpi_sata_completion 2292: task null, freeing
> CCB tag 2
> [ 5384.254676] sas: sas_scsi_find_task: task 0x0000000050be2a4b is aborted
> [ 5384.294659] sas: sas_eh_handle_sas_errors: task 0x0000000050be2a4b is
> aborted
> [ 5384.318691] ata22.00: exception Emask 0x0 SAct 0x200000 SErr 0x0
> action 0x0
> [ 5384.328425] ata22.00: failed command: WRITE FPDMA QUEUED
> [ 5384.336277] ata22.00: cmd 61/01:00:01:00:ea/00:00:02:00:00/40 tag 21
> ncq dma 4096 out
> [ 5384.336277] res 43/04:01:01:00:ea/00:00:02:00:00/00 Emask
> 0x400 (NCQ error) <F>
> [ 5384.357299] ata22.00: status: { DRDY SENSE ERR }
> [ 5384.364459] ata22.00: error: { ABRT }
> [ 5384.553177] ata22.00: configured for UDMA/133
> [ 5384.560343] sd 19:0:3:0: [sdl] tag#80 FAILED Result: hostbyte=DID_OK
> driverbyte=DRIVER_OK cmd_age=0s
> [ 5384.572175] sd 19:0:3:0: [sdl] tag#80 Sense Key : Illegal Request
> [current]
> [ 5384.581765] sd 19:0:3:0: [sdl] tag#80 Add. Sense: Unaligned write
> command
> [ 5384.591126] sd 19:0:3:0: [sdl] tag#80 CDB: Write(16) 8a 00 00 00 00
> 00 02 ea 00 01 00 00 00 01 00 00
> [ 5384.602938] I/O error, dev sdl, sector 391118856 op 0x1:(WRITE) flags
> 0x8800 phys_seg 1 prio class 2
> [ 5384.613854] ata22: EH complete
> [ 5384.618570] sas: --- Exit sas_scsi_recover_host: busy: 0 failed: 1
> tries: 1
>
> So looks good to me.
>
> Feel free to add:
>
> Tested-by: Damien Le Moal <[email protected]>
Great, thanks very much. I'll send an updated version based on mkp-scsi
6.1 queue today.
John