Date:	Thu, 18 Feb 2016 09:30:56 +0000
From:	John Garry <john.garry@...wei.com>
To:	Hannes Reinecke <hare@...e.de>, <JBottomley@...n.com>,
	<martin.petersen@...cle.com>
CC:	<linux-scsi@...r.kernel.org>, <john.garry2@...l.dcu.ie>,
	<linux-kernel@...r.kernel.org>, <linuxarm@...wei.com>,
	<zhangfei.gao@...aro.org>
Subject: Re: [PATCH 2/6] hisi_sas: add hisi_sas_slot_abort()

On 16/02/2016 15:41, John Garry wrote:
> 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@...wei.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
>
>
> _______________________________________________
> linuxarm mailing list
> linuxarm@...wei.com
> http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm

As suggested, it is ok to remove the query tmf.

Thanks,
John

Powered by blists - more mailing lists