| Message ID | 1646309930-138960-1-git-send-email-john.garry@huawei.com |
|---|---|
| Headers | show |
| Series | scsi: libsas and users: Factor out internal abort code | expand |
On 2022/03/03 14:18, John Garry wrote: > The internal abort feature is common to hisi_sas and pm8001 HBAs, and the > driver support is similar also, so add a common handler. > > Two modes of operation will be supported: > - single: Abort a single tagged command > - device: Abort all commands associated with a specific domain device > > A new protocol is added, SAS_PROTOCOL_INTERNAL_ABORT, so the common queue > command API may be re-used. > > Only add "single" support as a first step. > > Signed-off-by: John Garry <john.garry@huawei.com> > --- > drivers/scsi/libsas/sas_scsi_host.c | 75 +++++++++++++++++++++++++++++ > include/scsi/libsas.h | 14 ++++++ > include/scsi/sas.h | 2 + > 3 files changed, 91 insertions(+) > > diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c > index 5b5747e33dbd..0d05826e6e8c 100644 > --- a/drivers/scsi/libsas/sas_scsi_host.c > +++ b/drivers/scsi/libsas/sas_scsi_host.c > @@ -920,6 +920,81 @@ void sas_task_internal_timedout(struct timer_list *t) > #define TASK_TIMEOUT (20 * HZ) > #define TASK_RETRY 3 > > +static int sas_execute_internal_abort(struct domain_device *device, > + enum sas_internal_abort type, u16 tag, > + unsigned int qid, void *data) > +{ > + struct sas_ha_struct *ha = device->port->ha; > + struct sas_internal *i = to_sas_internal(ha->core.shost->transportt); > + struct sas_task *task = NULL; > + int res, retry; > + > + for (retry = 0; retry < TASK_RETRY; retry++) { > + task = sas_alloc_slow_task(GFP_KERNEL); > + if (!task) > + return -ENOMEM; > + > + task->dev = device; > + task->task_proto = SAS_PROTOCOL_INTERNAL_ABORT; > + task->task_done = sas_task_internal_done; > + task->slow_task->timer.function = sas_task_internal_timedout; > + task->slow_task->timer.expires = jiffies + TASK_TIMEOUT; > + add_timer(&task->slow_task->timer); > + > + task->abort_task.tag = tag; > + task->abort_task.type = type; > + > + res = i->dft->lldd_execute_task(task, GFP_KERNEL); > + if (res) { > + del_timer_sync(&task->slow_task->timer); > + pr_err("Executing internal abort failed %016llx (%d)\n", > + SAS_ADDR(device->sas_addr), res); > + break; > + } > + > + wait_for_completion(&task->slow_task->completion); > + res = TMF_RESP_FUNC_FAILED; > + > + /* Even if the internal abort timed out, return direct. */ > + if (task->task_state_flags & SAS_TASK_STATE_ABORTED) { > + pr_err("Internal abort: timeout %016llx\n", > + SAS_ADDR(device->sas_addr)); > + Nit: blank line not needed here ? > + res = -EIO; > + break; > + } > + > + if (task->task_status.resp == SAS_TASK_COMPLETE && > + task->task_status.stat == SAS_SAM_STAT_GOOD) { > + res = TMF_RESP_FUNC_COMPLETE; > + break; > + } > + > + if (task->task_status.resp == SAS_TASK_COMPLETE && > + task->task_status.stat == TMF_RESP_FUNC_SUCC) { > + res = TMF_RESP_FUNC_SUCC; > + break; > + } > + > + pr_err("Internal abort: task to dev %016llx response: 0x%x status 0x%x\n", > + SAS_ADDR(device->sas_addr), task->task_status.resp, > + task->task_status.stat); > + sas_free_task(task); > + task = NULL; > + } > + BUG_ON(retry == TASK_RETRY && task != NULL); > + sas_free_task(task); > + return res; > +} > + > +int sas_execute_internal_abort_single(struct domain_device *device, u16 tag, > + unsigned int qid, void *data) > +{ > + return sas_execute_internal_abort(device, SAS_INTERNAL_ABORT_SINGLE, > + tag, qid, data); > +} > +EXPORT_SYMBOL_GPL(sas_execute_internal_abort_single); > + > int sas_execute_tmf(struct domain_device *device, void *parameter, > int para_len, int force_phy_id, > struct sas_tmf_task *tmf) > diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h > index df2c8fc43429..2d30d57916e5 100644 > --- a/include/scsi/libsas.h > +++ b/include/scsi/libsas.h > @@ -557,6 +557,16 @@ struct sas_ata_task { > int force_phy_id; > }; > > +/* LLDDs rely on these values */ > +enum sas_internal_abort { > + SAS_INTERNAL_ABORT_SINGLE = 0, > +}; > + > +struct sas_internal_abort_task { > + enum sas_internal_abort type; > + u16 tag; > +}; > + > struct sas_smp_task { > struct scatterlist smp_req; > struct scatterlist smp_resp; > @@ -596,6 +606,7 @@ struct sas_task { > struct sas_ata_task ata_task; > struct sas_smp_task smp_task; > struct sas_ssp_task ssp_task; > + struct sas_internal_abort_task abort_task; > }; > > struct scatterlist *scatter; > @@ -683,6 +694,9 @@ extern int sas_slave_configure(struct scsi_device *); > extern int sas_change_queue_depth(struct scsi_device *, int new_depth); > extern int sas_bios_param(struct scsi_device *, struct block_device *, > sector_t capacity, int *hsc); > +int sas_execute_internal_abort_single(struct domain_device *device, > + u16 tag, unsigned int qid, > + void *data); > extern struct scsi_transport_template * > sas_domain_attach_transport(struct sas_domain_function_template *); > extern struct device_attribute dev_attr_phy_event_threshold; > diff --git a/include/scsi/sas.h b/include/scsi/sas.h > index 332a463d08ef..acfc69fd72d0 100644 > --- a/include/scsi/sas.h > +++ b/include/scsi/sas.h > @@ -95,6 +95,8 @@ enum sas_protocol { > SAS_PROTOCOL_SSP = 0x08, > SAS_PROTOCOL_ALL = 0x0E, > SAS_PROTOCOL_STP_ALL = SAS_PROTOCOL_STP|SAS_PROTOCOL_SATA, > + /* these are internal to libsas */ > + SAS_PROTOCOL_INTERNAL_ABORT = 0x10, > }; > > /* From the spec; local phys only */
On 03/03/2022 16:31, Damien Le Moal wrote: >> + >> + wait_for_completion(&task->slow_task->completion); >> + res = TMF_RESP_FUNC_FAILED; >> + >> + /* Even if the internal abort timed out, return direct. */ >> + if (task->task_state_flags & SAS_TASK_STATE_ABORTED) { >> + pr_err("Internal abort: timeout %016llx\n", >> + SAS_ADDR(device->sas_addr)); >> + > Nit: blank line not needed here ? Ok, I can add it. Thanks, John > >> + res = -EIO; >> + break; >> + }
On 03/03/2022 16:36, Damien Le Moal wrote: >> - rc = send_task_abort(pm8001_ha, opc, device_id, flag, >> - task_tag, cmd_tag); >> + rc = send_task_abort(pm8001_ha, opc, device_id, abort->type, >> + abort->tag, ccb->ccb_tag); > Nit: Can you indent this together with "(" ? I find it easier to read:) ok, I can align it. > >> if (rc != TMF_RESP_FUNC_COMPLETE) >> pm8001_dbg(pm8001_ha, EH, "rc= %d\n", rc); >> return rc; >> diff --git a/drivers/scsi/pm8001/pm8001_hwi.h b/drivers/scsi/pm8001/pm8001_hwi.h >> index d1f3aa93325b..961d0465b923 100644 >> --- a/drivers/scsi/pm8001/pm8001_hwi.h >> +++ b/drivers/scsi/pm8001/pm8001_hwi.h >> @@ -434,11 +434,6 @@ struct task_abort_req { >> u32 reserved[11]; >> } __attribute__((packed, aligned(4))); >> >> -/* These flags used for SSP SMP & SATA Abort */ >> -#define ABORT_MASK 0x3 >> -#define ABORT_SINGLE 0x0 >> -#define ABORT_ALL 0x1 >> - >> /** >> * brief the data structure of SSP SATA SMP Abort Response >> * use to describe SSP SMP & SATA Abort Response ( 64 bytes) >> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c >> index ac9c40c95070..d1224674173e 100644 >> --- a/drivers/scsi/pm8001/pm8001_sas.c >> +++ b/drivers/scsi/pm8001/pm8001_sas.c >> @@ -324,6 +324,18 @@ static int pm8001_task_prep_ata(struct pm8001_hba_info *pm8001_ha, >> return PM8001_CHIP_DISP->sata_req(pm8001_ha, ccb); >> } >> >> +/** >> + * pm8001_task_prep_internal_abort - the dispatcher function, prepare data >> + * for internal abort task >> + * @pm8001_ha: our hba card information >> + * @ccb: the ccb which attached to sata task >> + */ >> +static int pm8001_task_prep_internal_abort(struct pm8001_hba_info *pm8001_ha, >> + struct pm8001_ccb_info *ccb) >> +{ >> + return PM8001_CHIP_DISP->task_abort(pm8001_ha, ccb); >> +} >> + >> /** >> * pm8001_task_prep_ssp_tm - the dispatcher function, prepare task management data >> * @pm8001_ha: our hba card information >> @@ -367,6 +379,43 @@ static int sas_find_local_port_id(struct domain_device *dev) >> #define DEV_IS_GONE(pm8001_dev) \ >> ((!pm8001_dev || (pm8001_dev->dev_type == SAS_PHY_UNUSED))) >> >> + >> +static int pm8001_deliver_command(struct pm8001_hba_info *pm8001_ha, >> + struct pm8001_ccb_info *ccb) >> +{ >> + struct sas_task *task = ccb->task; >> + enum sas_protocol task_proto = task->task_proto; >> + struct sas_tmf_task *tmf = task->tmf; >> + int is_tmf = !!tmf; >> + int rc; >> + >> + switch (task_proto) { >> + case SAS_PROTOCOL_SMP: >> + rc = pm8001_task_prep_smp(pm8001_ha, ccb); >> + break; >> + case SAS_PROTOCOL_SSP: >> + if (is_tmf) >> + rc = pm8001_task_prep_ssp_tm(pm8001_ha, ccb, tmf); >> + else >> + rc = pm8001_task_prep_ssp(pm8001_ha, ccb); >> + break; >> + case SAS_PROTOCOL_SATA: >> + case SAS_PROTOCOL_STP: >> + rc = pm8001_task_prep_ata(pm8001_ha, ccb); >> + break; >> + case SAS_PROTOCOL_INTERNAL_ABORT: >> + rc = pm8001_task_prep_internal_abort(pm8001_ha, ccb); >> + break; >> + default: >> + dev_err(pm8001_ha->dev, "unknown sas_task proto: 0x%x\n", >> + task_proto); >> + rc = -EINVAL; >> + break; >> + } >> + >> + return rc; > rc variable is not very useful here. Why not use return directly for each case ? ok, I can make that change. > >> +} >> + >> /** >> * pm8001_queue_command - register for upper layer used, all IO commands sent >> * to HBA are from this interface. >> @@ -379,16 +428,15 @@ int pm8001_queue_command(struct sas_task *task, gfp_t gfp_flags) >> enum sas_protocol task_proto = task->task_proto; >> struct domain_device *dev = task->dev; >> struct pm8001_device *pm8001_dev = dev->lldd_dev; >> + bool internal_abort = sas_is_internal_abort(task); >> struct pm8001_hba_info *pm8001_ha; >> struct pm8001_port *port = NULL; >> struct pm8001_ccb_info *ccb; >> - struct sas_tmf_task *tmf = task->tmf; >> - int is_tmf = !!task->tmf; >> unsigned long flags; >> u32 n_elem = 0; >> int rc = 0; >> >> - if (!dev->port) { >> + if (!internal_abort && !dev->port) { >> ts->resp = SAS_TASK_UNDELIVERED; >> ts->stat = SAS_PHY_DOWN; >> if (dev->dev_type != SAS_SATA_DEV) >> @@ -410,7 +458,8 @@ int pm8001_queue_command(struct sas_task *task, gfp_t gfp_flags) >> pm8001_dev = dev->lldd_dev; >> port = &pm8001_ha->port[sas_find_local_port_id(dev)]; >> >> - if (DEV_IS_GONE(pm8001_dev) || !port->port_attached) { >> + if (!internal_abort && (DEV_IS_GONE(pm8001_dev) || >> + !port->port_attached)) { > Wrapping the line after "&&" would make this a lot cleaner and easier to read. Agreed, I can do it. But if you can see any neater ways to skip these checks for internal abort in the common queue command path then I would be glad to hear it. > >> ts->resp = SAS_TASK_UNDELIVERED; >> ts->stat = SAS_PHY_DOWN; >> if (sas_protocol_ata(task_proto)) { >> @@ -448,27 +497,7 @@ int pm8001_queue_command(struct sas_task *task, gfp_t gfp_flags) >> Thanks, John
On 3/3/22 21:18, John Garry wrote: > This is a follow-on from the series to factor out the TMF code shared > between libsas LLDDs. > > The hisi_sas and pm8001 have an internal abort feature to abort pending > commands in the host controller, prior to being sent to the target. The > driver support implementation is naturally quite similar, so factor it > out. > > Again, testing and review would be appreciated. I ran my usual set of tests with fio and also libzbc tests to exercise the failure/abort path. No problems detected. All good to me. Feel free to add: Tested-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> To your V2 with the cosmetic fixes. > > This is based on mkp-scsi 5.18 staging queue @ commit f2ddbbea7780 > > John Garry (4): > scsi: libsas: Add sas_execute_internal_abort_single() > scsi: libsas: Add sas_execute_internal_abort_dev() > scsi: pm8001: Use libsas internal abort support > scsi: hisi_sas: Use libsas internal abort support > > drivers/scsi/hisi_sas/hisi_sas.h | 8 +- > drivers/scsi/hisi_sas/hisi_sas_main.c | 453 +++++++++---------------- > drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 11 +- > drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 18 +- > drivers/scsi/libsas/sas_scsi_host.c | 89 +++++ > drivers/scsi/pm8001/pm8001_hwi.c | 27 +- > drivers/scsi/pm8001/pm8001_hwi.h | 5 - > drivers/scsi/pm8001/pm8001_sas.c | 186 ++++------ > drivers/scsi/pm8001/pm8001_sas.h | 6 +- > drivers/scsi/pm8001/pm80xx_hwi.h | 5 - > include/scsi/libsas.h | 24 ++ > include/scsi/sas.h | 2 + > 12 files changed, 368 insertions(+), 466 deletions(-) >
On 3/4/22 18:41, John Garry wrote: >>> /** >>> * pm8001_queue_command - register for upper layer used, all IO commands sent >>> * to HBA are from this interface. >>> @@ -379,16 +428,15 @@ int pm8001_queue_command(struct sas_task *task, gfp_t gfp_flags) >>> enum sas_protocol task_proto = task->task_proto; >>> struct domain_device *dev = task->dev; >>> struct pm8001_device *pm8001_dev = dev->lldd_dev; >>> + bool internal_abort = sas_is_internal_abort(task); >>> struct pm8001_hba_info *pm8001_ha; >>> struct pm8001_port *port = NULL; >>> struct pm8001_ccb_info *ccb; >>> - struct sas_tmf_task *tmf = task->tmf; >>> - int is_tmf = !!task->tmf; >>> unsigned long flags; >>> u32 n_elem = 0; >>> int rc = 0; >>> >>> - if (!dev->port) { >>> + if (!internal_abort && !dev->port) { >>> ts->resp = SAS_TASK_UNDELIVERED; >>> ts->stat = SAS_PHY_DOWN; >>> if (dev->dev_type != SAS_SATA_DEV) >>> @@ -410,7 +458,8 @@ int pm8001_queue_command(struct sas_task *task, gfp_t gfp_flags) >>> pm8001_dev = dev->lldd_dev; >>> port = &pm8001_ha->port[sas_find_local_port_id(dev)]; >>> >>> - if (DEV_IS_GONE(pm8001_dev) || !port->port_attached) { >>> + if (!internal_abort && (DEV_IS_GONE(pm8001_dev) || >>> + !port->port_attached)) { >> Wrapping the line after "&&" would make this a lot cleaner and easier to read. > > Agreed, I can do it. > > But if you can see any neater ways to skip these checks for internal > abort in the common queue command path then I would be glad to hear it. Would need to check the locking context, but if there is no race possible with the context setting the device as gone, libata should already be aware of it and not issuing the command in the first place. So these checks should not be needed at all. Will try to have a look when I have some time. There are several things that needs better integration with libata anyway, like the "manual" read log on error. We should try to address these for 5.19.