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 <[email protected]>
---
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,
+};
+
+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 */
--
2.26.2
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 <[email protected]>
> ---
> 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 */
--
Damien Le Moal
Western Digital Research
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 <[email protected]>
> ---
> 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
--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
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
--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
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
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