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

[John Garry]

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@xxxxxxxxxx>
> > > ---
> > >   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@xxxxxxxxxx
> http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm

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

Thanks,
John