Re: [PATCH 2/6] hisi_sas: add hisi_sas_slot_abort()

[John Garry <john.garry@huawei.com>]

On 16/02/2016 15:22, Hannes Reinecke wrote:
> On 02/16/2016 01:22 PM, John Garry wrote:
>> Add a function to abort a slot (task) in the device
>> (if it is in the task set) and then cleanup and
>> complete the task.
>> The function is called from work queue context as
>> it cannot be called from the context where it is
>> triggered (interrupt).
>>
>> Signed-off-by: John Garry <john.garry@huawei.com>
>> ---
>>   drivers/scsi/hisi_sas/hisi_sas.h      |  1 +
>>   drivers/scsi/hisi_sas/hisi_sas_main.c | 43 +++++++++++++++++++++++++++++++++++
>>   2 files changed, 44 insertions(+)
>>
>> diff --git a/drivers/scsi/hisi_sas/hisi_sas.h b/drivers/scsi/hisi_sas/hisi_sas.h
>> index 02da7e4..a05ce71 100644
>> --- a/drivers/scsi/hisi_sas/hisi_sas.h
>> +++ b/drivers/scsi/hisi_sas/hisi_sas.h
>> @@ -120,6 +120,7 @@ struct hisi_sas_slot {
>>   	dma_addr_t command_table_dma;
>>   	struct hisi_sas_sge_page *sge_page;
>>   	dma_addr_t sge_page_dma;
>> +	struct work_struct abort_slot;
>>   };
>>
>>   struct hisi_sas_tmf_task {
>> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
>> index c600f5e..65509eb 100644
>> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
>> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
>> @@ -15,6 +15,9 @@
>>   #define DEV_IS_GONE(dev) \
>>   	((!dev) || (dev->dev_type == SAS_PHY_UNUSED))
>>
>> +static int hisi_sas_debug_issue_ssp_tmf(struct domain_device *device,
>> +				u8 *lun, struct hisi_sas_tmf_task *tmf);
>> +
>>   static struct hisi_hba *dev_to_hisi_hba(struct domain_device *device)
>>   {
>>   	return device->port->ha->lldd_ha;
>> @@ -113,6 +116,45 @@ static int hisi_sas_task_prep_ata(struct hisi_hba *hisi_hba,
>>   	return hisi_hba->hw->prep_stp(hisi_hba, slot);
>>   }
>>
>> +/*
>> + * This function will issue an abort TMF if a task is still in
>> + * the target. Then it will do the task complete cleanup and
>> + * callbacks.
>> + */
>> +static void hisi_sas_slot_abort(struct work_struct *work)
>> +{
>> +	struct hisi_sas_slot *abort_slot =
>> +		container_of(work, struct hisi_sas_slot, abort_slot);
>> +	struct sas_task *task = abort_slot->task;
>> +	struct hisi_hba *hisi_hba = dev_to_hisi_hba(task->dev);
>> +	struct scsi_cmnd *cmnd = task->uldd_task;
>> +	struct hisi_sas_tmf_task tmf_task;
>> +	struct domain_device *device = task->dev;
>> +	struct hisi_sas_device *sas_dev = device->lldd_dev;
>> +	struct scsi_lun lun;
>> +	int tag = abort_slot->idx, rc;
>> +
>> +	int_to_scsilun(cmnd->device->lun, &lun);
>> +	tmf_task.tmf = TMF_QUERY_TASK;
>> +	tmf_task.tag_of_task_to_be_managed = cpu_to_le16(tag);
>> +
>> +	rc = hisi_sas_debug_issue_ssp_tmf(task->dev, lun.scsi_lun, &tmf_task);
>> +
>> +	/* TMF_RESP_FUNC_SUCC means task is present in the task set */
>> +	if (rc != TMF_RESP_FUNC_SUCC)
>> +		goto out;
>> +	tmf_task.tmf = TMF_ABORT_TASK;
>> +	tmf_task.tag_of_task_to_be_managed = cpu_to_le16(tag);
>> +
>> +	rc = hisi_sas_debug_issue_ssp_tmf(task->dev, lun.scsi_lun, &tmf_task);
>> +out:
>> +	hisi_sas_slot_task_free(hisi_hba, task, abort_slot);
>> +	if (task->task_done)
>> +		task->task_done(task);
>> +	if (sas_dev && sas_dev->running_req)
>> +		sas_dev->running_req--;
>> +}
>> +
>>   static int hisi_sas_task_prep(struct sas_task *task, struct hisi_hba *hisi_hba,
>>   			      int is_tmf, struct hisi_sas_tmf_task *tmf,
>>   			      int *pass)
> Do you really need to query the task first?
> As per SAM a successful return from an ABORT TASK TMF has this meaning:
>
> A response of FUNCTION COMPLETE shall indicate that the task was
> aborted or was not in the task set.
>
> Ie it doesn't matter if the task was present or not.
>
> Cheers,
>
> Hannes
>

I think that it should be ok to the abort without first querying. I'll 
just test this to double-check. (This would make the first patch in the 
series superflous)

Cheers,
John