| Message ID | 1664368034-114991-2-git-send-email-john.garry@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: libsas: Use request tag in more drivers | expand |
On 28.09.22 14:34, John Garry wrote: > blk-mq already provides a unique tag per request. Some libsas LLDDs - like > hisi_sas - already use this tag as the unique per-IO HW tag. > > Add a common function to provide the request associated with a sas_task > for all libsas LLDDs. > > Signed-off-by: John Garry <john.garry@huawei.com> > --- > include/scsi/libsas.h | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h > index f86b56bf7833..bc51756a3317 100644 > --- a/include/scsi/libsas.h > +++ b/include/scsi/libsas.h > @@ -644,6 +644,28 @@ static inline bool sas_is_internal_abort(struct sas_task *task) > return task->task_proto == SAS_PROTOCOL_INTERNAL_ABORT; > } > > +static inline struct request *sas_task_find_rq(struct sas_task *task) > +{ > + struct scsi_cmnd *scmd; > + > + if (!task || !task->uldd_task) > + return NULL; Is anyone actually calling sas_task_find_rq with a NULL task? That doesn't make a lot of sense from an API POV for me, having the only argument allowed to be NULL (and not being a *free() kind of function). > + > + if (task->task_proto & SAS_PROTOCOL_STP_ALL) { > + struct ata_queued_cmd *qc; > + > + qc = task->uldd_task; > + scmd = qc->scsicmd; > + } else { > + scmd = task->uldd_task; > + } > + > + if (!scmd) > + return NULL; > + > + return scsi_cmd_to_rq(scmd); > +} > + > struct sas_domain_function_template { > /* The class calls these to notify the LLDD of an event. */ > void (*lldd_port_formed)(struct asd_sas_phy *);
On 28/09/2022 14:17, Johannes Thumshirn wrote: >> +static inline struct request *sas_task_find_rq(struct sas_task *task) >> +{ >> + struct scsi_cmnd *scmd; >> + >> + if (!task || !task->uldd_task) >> + return NULL; > Is anyone actually calling sas_task_find_rq with a NULL task? Yeah, unfortunately. An example - the only one I think - is in the pm8001 driver: pm8001_ccb_alloc() function which takes a task pointer which may be NULL, and this function calls sas_task_find_rq() > That doesn't make a lot of sense from an API POV for me, having > the only argument allowed to be NULL (and not being a *free() > kind of function). I suppose that I could change to only call sas_task_find_rq() for non-NULL sas_task pointers (and remove the task check). Thanks, John
On 28.09.22 15:56, John Garry wrote: > On 28/09/2022 14:17, Johannes Thumshirn wrote: >>> +static inline struct request *sas_task_find_rq(struct sas_task *task) >>> +{ >>> + struct scsi_cmnd *scmd; >>> + >>> + if (!task || !task->uldd_task) >>> + return NULL; >> Is anyone actually calling sas_task_find_rq with a NULL task? > > Yeah, unfortunately. An example - the only one I think - is in the > pm8001 driver: pm8001_ccb_alloc() function which takes a task pointer > which may be NULL, and this function calls sas_task_find_rq() > >> That doesn't make a lot of sense from an API POV for me, having >> the only argument allowed to be NULL (and not being a *free() >> kind of function). > > I suppose that I could change to only call sas_task_find_rq() for > non-NULL sas_task pointers (and remove the task check). If this is done in only a few drivers I'd go that route TBH. AFAICS hishi sas doesn't call sas_task_find_rq with a NULL task, so it's only pm8001
On 9/28/22 21:27, John Garry wrote: > blk-mq already provides a unique tag per request. Some libsas LLDDs - like > hisi_sas - already use this tag as the unique per-IO HW tag. > > Add a common function to provide the request associated with a sas_task > for all libsas LLDDs. > > Signed-off-by: John Garry <john.garry@huawei.com> > --- > include/scsi/libsas.h | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h > index f86b56bf7833..bc51756a3317 100644 > --- a/include/scsi/libsas.h > +++ b/include/scsi/libsas.h > @@ -644,6 +644,28 @@ static inline bool sas_is_internal_abort(struct sas_task *task) > return task->task_proto == SAS_PROTOCOL_INTERNAL_ABORT; > } > > +static inline struct request *sas_task_find_rq(struct sas_task *task) > +{ > + struct scsi_cmnd *scmd; > + > + if (!task || !task->uldd_task) > + return NULL; > + > + if (task->task_proto & SAS_PROTOCOL_STP_ALL) { > + struct ata_queued_cmd *qc; > + > + qc = task->uldd_task; I would change these 2 lines into a single line: struct ata_queued_cmd *qc = task->uldd_task; And no cast as suggested. > + scmd = qc->scsicmd; > + } else { > + scmd = task->uldd_task; > + } > + > + if (!scmd) > + return NULL; > + > + return scsi_cmd_to_rq(scmd); > +} > + > struct sas_domain_function_template { > /* The class calls these to notify the LLDD of an event. */ > void (*lldd_port_formed)(struct asd_sas_phy *);
On 29/09/2022 03:09, Damien Le Moal wrote: > On 9/28/22 21:27, John Garry wrote: >> blk-mq already provides a unique tag per request. Some libsas LLDDs - like >> hisi_sas - already use this tag as the unique per-IO HW tag. >> >> Add a common function to provide the request associated with a sas_task >> for all libsas LLDDs. >> >> Signed-off-by: John Garry <john.garry@huawei.com> >> --- >> include/scsi/libsas.h | 22 ++++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> >> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h >> index f86b56bf7833..bc51756a3317 100644 >> --- a/include/scsi/libsas.h >> +++ b/include/scsi/libsas.h >> @@ -644,6 +644,28 @@ static inline bool sas_is_internal_abort(struct sas_task *task) >> return task->task_proto == SAS_PROTOCOL_INTERNAL_ABORT; >> } >> >> +static inline struct request *sas_task_find_rq(struct sas_task *task) >> +{ >> + struct scsi_cmnd *scmd; >> + >> + if (!task || !task->uldd_task) >> + return NULL; >> + >> + if (task->task_proto & SAS_PROTOCOL_STP_ALL) { >> + struct ata_queued_cmd *qc; >> + >> + qc = task->uldd_task; > > I would change these 2 lines into a single line: > > struct ata_queued_cmd *qc = task->uldd_task; > > And no cast as suggested. > >> + scmd = qc->scsicmd; So do you prefer: scmd = ((struct ata_queued_cmd *)task->uldd_task)->scsicmd As Jason suggested? Thanks, John
On 9/29/22 16:33, John Garry wrote: > On 29/09/2022 03:09, Damien Le Moal wrote: >> On 9/28/22 21:27, John Garry wrote: >>> blk-mq already provides a unique tag per request. Some libsas LLDDs - like >>> hisi_sas - already use this tag as the unique per-IO HW tag. >>> >>> Add a common function to provide the request associated with a sas_task >>> for all libsas LLDDs. >>> >>> Signed-off-by: John Garry <john.garry@huawei.com> >>> --- >>> include/scsi/libsas.h | 22 ++++++++++++++++++++++ >>> 1 file changed, 22 insertions(+) >>> >>> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h >>> index f86b56bf7833..bc51756a3317 100644 >>> --- a/include/scsi/libsas.h >>> +++ b/include/scsi/libsas.h >>> @@ -644,6 +644,28 @@ static inline bool sas_is_internal_abort(struct sas_task *task) >>> return task->task_proto == SAS_PROTOCOL_INTERNAL_ABORT; >>> } >>> >>> +static inline struct request *sas_task_find_rq(struct sas_task *task) >>> +{ >>> + struct scsi_cmnd *scmd; >>> + >>> + if (!task || !task->uldd_task) >>> + return NULL; >>> + >>> + if (task->task_proto & SAS_PROTOCOL_STP_ALL) { >>> + struct ata_queued_cmd *qc; >>> + >>> + qc = task->uldd_task; >> >> I would change these 2 lines into a single line: >> >> struct ata_queued_cmd *qc = task->uldd_task; >> >> And no cast as suggested. >> >>> + scmd = qc->scsicmd; > > So do you prefer: > > scmd = ((struct ata_queued_cmd *)task->uldd_task)->scsicmd Not a fan of the cast approach suggested by Jason. I prefer my above suggestion, but no strong feeling about it. Either way will be OK. > > As Jason suggested? > > Thanks, > John
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h index f86b56bf7833..bc51756a3317 100644 --- a/include/scsi/libsas.h +++ b/include/scsi/libsas.h @@ -644,6 +644,28 @@ static inline bool sas_is_internal_abort(struct sas_task *task) return task->task_proto == SAS_PROTOCOL_INTERNAL_ABORT; } +static inline struct request *sas_task_find_rq(struct sas_task *task) +{ + struct scsi_cmnd *scmd; + + if (!task || !task->uldd_task) + return NULL; + + if (task->task_proto & SAS_PROTOCOL_STP_ALL) { + struct ata_queued_cmd *qc; + + qc = task->uldd_task; + scmd = qc->scsicmd; + } else { + scmd = task->uldd_task; + } + + if (!scmd) + return NULL; + + return scsi_cmd_to_rq(scmd); +} + struct sas_domain_function_template { /* The class calls these to notify the LLDD of an event. */ void (*lldd_port_formed)(struct asd_sas_phy *);
blk-mq already provides a unique tag per request. Some libsas LLDDs - like hisi_sas - already use this tag as the unique per-IO HW tag. Add a common function to provide the request associated with a sas_task for all libsas LLDDs. Signed-off-by: John Garry <john.garry@huawei.com> --- include/scsi/libsas.h | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)