| Message ID | 1646309930-138960-2-git-send-email-john.garry@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| 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 3/3/22 13: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)); > + > + 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, > +}; > + Why don't you use the existing TMF_XXX values here? Your 'single' method pretty much _is_ a TMF_ABORT_TASK, and the 'device' method _is_ a TMF_ABORT_TASK_SET, no? Cheers, Hannes
On 20/04/2022 13:21, Hannes Reinecke wrote: >> 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, >> +}; >> + > > Why don't you use the existing TMF_XXX values here? > Your 'single' method pretty much _is_ a TMF_ABORT_TASK, and the 'device' > method _is_ a TMF_ABORT_TASK_SET, no? Sure, they are doing the same as TMFs and there is equivalence in the 'single' and 'device' methods, as you say. However, as mentioned in the comment, the LLDDs rely on the values in enum sas_internal_abort, which do not match the values in TMF_ABORT{_TASK, _TASK_SET}. Thanks, John
On 4/25/22 10:27, John Garry wrote: > On 20/04/2022 13:21, Hannes Reinecke wrote: >>> 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, >>> +}; >>> + >> >> Why don't you use the existing TMF_XXX values here? >> Your 'single' method pretty much _is_ a TMF_ABORT_TASK, and the >> 'device' method _is_ a TMF_ABORT_TASK_SET, no? > > Sure, they are doing the same as TMFs and there is equivalence in the > 'single' and 'device' methods, as you say. > > However, as mentioned in the comment, the LLDDs rely on the values in > enum sas_internal_abort, which do not match the values in > TMF_ABORT{_TASK, _TASK_SET}. > How can they rely on a value which you just introduced? Cheers, Hannes
On 25/04/2022 09:34, Hannes Reinecke wrote: >>>> +/* LLDDs rely on these values */ >>>> +enum sas_internal_abort { >>>> + SAS_INTERNAL_ABORT_SINGLE = 0, >>>> +}; >>>> + >>> >>> Why don't you use the existing TMF_XXX values here? >>> Your 'single' method pretty much _is_ a TMF_ABORT_TASK, and the >>> 'device' method _is_ a TMF_ABORT_TASK_SET, no? >> >> Sure, they are doing the same as TMFs and there is equivalence in the >> 'single' and 'device' methods, as you say. >> >> However, as mentioned in the comment, the LLDDs rely on the values in >> enum sas_internal_abort, which do not match the values in >> TMF_ABORT{_TASK, _TASK_SET}. >> > How can they rely on a value which you just introduced? I am relying on no one changing those values in enum sas_internal_abort. Both hisi_sas and pm8001 use value of 0 for single abort and 1 for device abort in their own internal abort HW frames structs. And if some other controller comes along which wants to support this feature and the values in enum sas_internal_abort don't match then they would need to do some translation. I could use TMF values and do the translation in hisi_sas and pm8001 drivers today, but I don't see much much gain in that. Thanks, John
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)); + + 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 */
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(+)