Re: [RFC PATCH 2/6] isci: task (libsas interface support)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thanks David, comments below.

>> +       if (task->task_state_flags & SAS_TASK_STATE_ABORTED) {
>> +
>> +               isci_task_complete_for_upper_layer(
>> +                       task,
>> +                       SAS_TASK_UNDELIVERED,
>> +                       SAM_STAT_TASK_ABORTED,
>> +                       isci_perform_normal_io_completion
>> +                       );
>> +
>> +               return 0;  /* The I/O was accepted (and failed). */
>
> Dan, is this the right status to return to libsas?
>
> Looks like it checks (->lldd_execute_task) for non-zero result.

I think this is self defense that can be deleted, how can a task be
aborted before it has even returned from the submission routine.

>
>> +       }
>> +       if ((task->dev == NULL) || (task->dev->port == NULL)) {
>> +
>> +               /* Indicate SAS_TASK_UNDELIVERED, so that the scsi
>> midlayer
>> +                * removes the target.
>> +                */
>> +               isci_task_complete_for_upper_layer(
>> +                       task,
>> +                       SAS_TASK_UNDELIVERED,
>> +                       SAS_DEVICE_UNKNOWN,
>> +                       isci_perform_normal_io_completion
>> +                       );
>> +               return 0;  /* The I/O was accepted (and failed). */
>
> Same here?

Yeah, we should always be able to dereference task->dev because ->dev
is referenced counted against outstanding commands.  If we have a task
by definition we have a dev.

[..]
>> +        */
>> +       isci_task_build_tmf(&tmf, isci_device, isci_tmf_ssp_lun_reset,
>> NULL,
>> +                           NULL);
>> +
>> +       #define ISCI_LU_RESET_TIMEOUT_MS 2000 /* 2 second timeout. */
>
> Maybe move #define to isci_task.h?

Ok.

>
>> +       ret = isci_task_execute_tmf(isci_host, &tmf,
>> ISCI_LU_RESET_TIMEOUT_MS);
>> +
>> +       if (ret == TMF_RESP_FUNC_COMPLETE)
>> +               dev_dbg(&isci_host->pdev->dev,
>> +                       "%s: %p: TMF_LU_RESET passed\n",
>> +                       __func__, isci_device);
>> +       else
>> +               dev_dbg(&isci_host->pdev->dev,
>> +                       "%s: %p: TMF_LU_RESET failed (%x)\n",
>> +                       __func__, isci_device, ret);
>> +
>> +       return ret;
>> +}
>> +
>> +/**
>
>> +
>> +/*      int (*lldd_clear_nexus_port)(struct asd_sas_port *); */
>> +int isci_task_clear_nexus_port(struct asd_sas_port *port)
>> +{
>> +       return TMF_RESP_FUNC_FAILED;
>> +}
>> +
>> +
>> +
>> +int isci_task_clear_nexus_ha(struct sas_ha_struct *ha)
>> +{
>> +       return TMF_RESP_FUNC_FAILED;
>> +}
>> +
>> +int isci_task_I_T_nexus_reset(struct domain_device *dev)
>> +{
>> +       return TMF_RESP_FUNC_FAILED;
>> +}
>> +
>
> These will be used during libsas error handling.

The driver implements abort_task, and promotes the rest to lu_reset.
Assuming the documentation is not out of date:

     A SAS LLDD should also implement at least one of the Task
     Management Functions (TMFs) described in SAM:

...how urgent is the need for these other task management routines?

Speaking of mandatory libsas methods all other libsas drivers outside
of aic94xx forgo implementing

             /* Port and Adapter management */
             int (*lldd_clear_nexus_port)(struct sas_port *);
             int (*lldd_clear_nexus_ha)(struct sas_ha_struct *);

     A SAS LLDD should implement at least one of those.

...not clear that at least one of those is mandatory.

>
>
>> +/**
>> + * isci_task_abort_task() - This function is one of the SAS Domain
>> Template
>> + *    functions. This function is called by libsas to abort a specified
>> task.
>> + * @task: This parameter specifies the SAS task to abort.
>> + *
>> + * status, zero indicates success.
>> + */
>> +int isci_task_abort_task(struct sas_task *task)
>> +{
>
> scic_lock, flags);
>
>
>> +
>> +               #define ISCI_ABORT_TASK_TIMEOUT_MS 500 /* half second
>> timeout. */
>
> Maybe move this #define to isci_task.h?

Ok.

[..]
>> + * isci_task_abort_task_set() - This function is one of the SAS Domain
>> Template
>> + *    functions. This is one of the Task Management functoins called by
>> libsas,
>> + *    to abort all task for the given lun.
>> + * @d_device: This parameter specifies the domain device associated with
>> this
>> + *    request.
>> + * @lun: This parameter specifies the lun associated with this request.
>> + *
>> + * status, zero indicates success.
>> + */
>> +int isci_task_abort_task_set(
>> +       struct domain_device *d_device,
>> +       u8 *lun)
>> +{
>> +       return TMF_RESP_FUNC_FAILED;
>> +}
>> +
>> +
>> +/**
>> + * isci_task_clear_aca() - This function is one of the SAS Domain
>> Template
>> + *    functions. This is one of the Task Management functoins called by
>> libsas.
>> + * @d_device: This parameter specifies the domain device associated with
>> this
>> + *    request.
>> + * @lun: This parameter specifies the lun       associated with this
>> request.
>> + *
>> + * status, zero indicates success.
>> + */
>> +int isci_task_clear_aca(
>> +       struct domain_device *d_device,
>> +       u8 *lun)
>> +{
>> +       return TMF_RESP_FUNC_FAILED;
>> +}
>> +
>> +
>> +
>> +/**
>> + * isci_task_clear_task_set() - This function is one of the SAS Domain
>> Template
>> + *    functions. This is one of the Task Management functoins called by
>> libsas.
>> + * @d_device: This parameter specifies the domain device associated with
>> this
>> + *    request.
>> + * @lun: This parameter specifies the lun       associated with this
>> request.
>> + *
>> + * status, zero indicates success.
>> + */
>> +int isci_task_clear_task_set(
>> +       struct domain_device *d_device,
>> +       u8 *lun)
>> +{
>> +       return TMF_RESP_FUNC_FAILED;
>> +}
>
> More libsas error recovery functions.

Same comment as above the driver has the little and the big recovery
hammers, the medium hammers can be prioritized behind other fixes I
think.

--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]
  Powered by Linux