| Message ID | 1664368034-114991-7-git-send-email-john.garry@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: libsas: Use request tag in more drivers | expand |
On 9/28/22 21:27, John Garry wrote: > The request associated with a scsi command coming from the block layer > has a unique tag, so use that when possible for getting a slot. > > Unfortunately we don't support reserved commands in the SCSI midlayer yet. > As such, SMP tasks - as an example - will not have a request associated, so > in the interim continue to manage those tags for that type of sas_task > internally. > > We reserve an arbitrary 4 tags for these internal tags. Indeed, we already > decrement MVS_RSVD_SLOTS by 2 for the shost can_queue when flag > MVF_FLAG_SOC is set. This change was made in commit 20b09c2992fef > ("[PATCH] [SCSI] mvsas: add support for 94xx; layout change; bug fixes"), > but what those 2 slots are used for is not obvious. > > Signed-off-by: John Garry <john.garry@huawei.com> > --- > drivers/scsi/mvsas/mv_defs.h | 1 + > drivers/scsi/mvsas/mv_init.c | 4 ++-- > drivers/scsi/mvsas/mv_sas.c | 22 +++++++++++++++++----- > drivers/scsi/mvsas/mv_sas.h | 1 - > 4 files changed, 20 insertions(+), 8 deletions(-) > > diff --git a/drivers/scsi/mvsas/mv_defs.h b/drivers/scsi/mvsas/mv_defs.h > index 7123a2efbf58..8ef174cd4d37 100644 > --- a/drivers/scsi/mvsas/mv_defs.h > +++ b/drivers/scsi/mvsas/mv_defs.h > @@ -40,6 +40,7 @@ enum driver_configuration { > MVS_ATA_CMD_SZ = 96, /* SATA command table buffer size */ > MVS_OAF_SZ = 64, /* Open address frame buffer size */ > MVS_QUEUE_SIZE = 64, /* Support Queue depth */ > + MVS_RSVD_SLOTS = 4, > MVS_SOC_CAN_QUEUE = MVS_SOC_SLOTS - 2, > }; > > diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c > index c85fb812ad43..d834ed9e8e4a 100644 > --- a/drivers/scsi/mvsas/mv_init.c > +++ b/drivers/scsi/mvsas/mv_init.c > @@ -284,7 +284,7 @@ static int mvs_alloc(struct mvs_info *mvi, struct Scsi_Host *shost) > printk(KERN_DEBUG "failed to create dma pool %s.\n", pool_name); > goto err_out; > } > - mvi->tags_num = slot_nr; > + mvi->tags_num = MVS_RSVD_SLOTS; Same comment as for pm8001: do you really need this field if the value is always MVS_RSVD_SLOTS ? > > return 0; > err_out: > @@ -367,7 +367,7 @@ static struct mvs_info *mvs_pci_alloc(struct pci_dev *pdev, > mvi->sas = sha; > mvi->shost = shost; > > - mvi->tags = kzalloc(MVS_CHIP_SLOT_SZ>>3, GFP_KERNEL); > + mvi->tags = kzalloc(MVS_RSVD_SLOTS, GFP_KERNEL); Field name ? reserved_tags ? Also, the alloc seems wrong. This will allocate 4 bytes, but you only need 4 bits. You could make this an unsigned long and not allocate anything. Same remark for pm8001 by the way. That would cap MVS_RSVD_SLOTS to BITS_PER_LONG maximum, but that is easy to check at compile time with a #if/#error. > if (!mvi->tags) > goto err_out; > > diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c > index 0810e6c930e1..549d2ec89f60 100644 > --- a/drivers/scsi/mvsas/mv_sas.c > +++ b/drivers/scsi/mvsas/mv_sas.c > @@ -20,7 +20,7 @@ static int mvs_find_tag(struct mvs_info *mvi, struct sas_task *task, u32 *tag) > return 0; > } > > -void mvs_tag_clear(struct mvs_info *mvi, u32 tag) > +static void mvs_tag_clear(struct mvs_info *mvi, u32 tag) > { > void *bitmap = mvi->tags; > clear_bit(tag, bitmap); > @@ -28,6 +28,11 @@ void mvs_tag_clear(struct mvs_info *mvi, u32 tag) > > void mvs_tag_free(struct mvs_info *mvi, u32 tag) > { > + if (tag < mvi->shost->can_queue) > + return; > + > + tag -= mvi->shost->can_queue; > + > mvs_tag_clear(mvi, tag); > } > > @@ -47,6 +52,7 @@ inline int mvs_tag_alloc(struct mvs_info *mvi, u32 *tag_out) > if (tag >= mvi->tags_num) > return -SAS_QUEUE_FULL; > mvs_tag_set(mvi, tag); > + tag += mvi->shost->can_queue; > *tag_out = tag; > return 0; > } > @@ -696,6 +702,7 @@ static int mvs_task_prep(struct sas_task *task, struct mvs_info *mvi, int is_tmf > struct mvs_task_exec_info tei; > struct mvs_slot_info *slot; > u32 tag = 0xdeadbeef, n_elem = 0; > + struct request *rq; > int rc = 0; > > if (!dev->port) { > @@ -760,9 +767,14 @@ static int mvs_task_prep(struct sas_task *task, struct mvs_info *mvi, int is_tmf > n_elem = task->num_scatter; > } > > - rc = mvs_tag_alloc(mvi, &tag); > - if (rc) > - goto err_out; > + rq = sas_task_find_rq(task); > + if (rq) { > + tag = rq->tag; > + } else { > + rc = mvs_tag_alloc(mvi, &tag); > + if (rc) > + goto err_out; > + } > > slot = &mvi->slot_info[tag]; > > @@ -857,7 +869,7 @@ int mvs_queue_command(struct sas_task *task, gfp_t gfp_flags) > static void mvs_slot_free(struct mvs_info *mvi, u32 rx_desc) > { > u32 slot_idx = rx_desc & RXQ_SLOT_MASK; > - mvs_tag_clear(mvi, slot_idx); > + mvs_tag_free(mvi, slot_idx); > } > > static void mvs_slot_task_free(struct mvs_info *mvi, struct sas_task *task, > diff --git a/drivers/scsi/mvsas/mv_sas.h b/drivers/scsi/mvsas/mv_sas.h > index fe57665bdb50..e6c70786ded9 100644 > --- a/drivers/scsi/mvsas/mv_sas.h > +++ b/drivers/scsi/mvsas/mv_sas.h > @@ -424,7 +424,6 @@ struct mvs_task_exec_info { > > /******************** function prototype *********************/ > void mvs_get_sas_addr(void *buf, u32 buflen); > -void mvs_tag_clear(struct mvs_info *mvi, u32 tag); > void mvs_tag_free(struct mvs_info *mvi, u32 tag); > void mvs_tag_set(struct mvs_info *mvi, unsigned int tag); > int mvs_tag_alloc(struct mvs_info *mvi, u32 *tag_out);
On 29/09/2022 03:22, Damien Le Moal wrote: > On 9/28/22 21:27, John Garry wrote: >> The request associated with a scsi command coming from the block layer >> has a unique tag, so use that when possible for getting a slot. >> >> Unfortunately we don't support reserved commands in the SCSI midlayer yet. >> As such, SMP tasks - as an example - will not have a request associated, so >> in the interim continue to manage those tags for that type of sas_task >> internally. >> >> We reserve an arbitrary 4 tags for these internal tags. Indeed, we already >> decrement MVS_RSVD_SLOTS by 2 for the shost can_queue when flag >> MVF_FLAG_SOC is set. This change was made in commit 20b09c2992fef >> ("[PATCH] [SCSI] mvsas: add support for 94xx; layout change; bug fixes"), >> but what those 2 slots are used for is not obvious. >> >> Signed-off-by: John Garry <john.garry@huawei.com> >> --- >> drivers/scsi/mvsas/mv_defs.h | 1 + >> drivers/scsi/mvsas/mv_init.c | 4 ++-- >> drivers/scsi/mvsas/mv_sas.c | 22 +++++++++++++++++----- >> drivers/scsi/mvsas/mv_sas.h | 1 - >> 4 files changed, 20 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/scsi/mvsas/mv_defs.h b/drivers/scsi/mvsas/mv_defs.h >> index 7123a2efbf58..8ef174cd4d37 100644 >> --- a/drivers/scsi/mvsas/mv_defs.h >> +++ b/drivers/scsi/mvsas/mv_defs.h >> @@ -40,6 +40,7 @@ enum driver_configuration { >> MVS_ATA_CMD_SZ = 96, /* SATA command table buffer size */ >> MVS_OAF_SZ = 64, /* Open address frame buffer size */ >> MVS_QUEUE_SIZE = 64, /* Support Queue depth */ >> + MVS_RSVD_SLOTS = 4, >> MVS_SOC_CAN_QUEUE = MVS_SOC_SLOTS - 2, >> }; >> >> diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c >> index c85fb812ad43..d834ed9e8e4a 100644 >> --- a/drivers/scsi/mvsas/mv_init.c >> +++ b/drivers/scsi/mvsas/mv_init.c >> @@ -284,7 +284,7 @@ static int mvs_alloc(struct mvs_info *mvi, struct Scsi_Host *shost) >> printk(KERN_DEBUG "failed to create dma pool %s.\n", pool_name); >> goto err_out; >> } >> - mvi->tags_num = slot_nr; >> + mvi->tags_num = MVS_RSVD_SLOTS; > > Same comment as for pm8001: do you really need this field if the value > is always MVS_RSVD_SLOTS ? Right, I don't need this struct member. Again I can just use this macro directly. > >> >> return 0; >> err_out: >> @@ -367,7 +367,7 @@ static struct mvs_info *mvs_pci_alloc(struct pci_dev *pdev, >> mvi->sas = sha; >> mvi->shost = shost; >> >> - mvi->tags = kzalloc(MVS_CHIP_SLOT_SZ>>3, GFP_KERNEL); >> + mvi->tags = kzalloc(MVS_RSVD_SLOTS, GFP_KERNEL); > > Field name ? reserved_tags ? > Also, the alloc seems wrong. This will allocate 4 bytes, but you only > need 4 bits. You could make this an unsigned long and not allocate > anything. Well spotted. I should have questioned more why they had >>3 previously. But I would rather keep as a bitmap, i.e. *unsigned long for simplicity. > Same remark for pm8001 by the way. I think it's ok as it uses bitmap_zalloc() > > That would cap MVS_RSVD_SLOTS to BITS_PER_LONG maximum, but that is easy > to check at compile time with a #if/#error. > As above, I'd rather keep as a bitmap. It's a little inefficient, but is a one off in the driver. Thanks, John
On 9/29/22 16:49, John Garry wrote: > On 29/09/2022 03:22, Damien Le Moal wrote: >> On 9/28/22 21:27, John Garry wrote: >>> The request associated with a scsi command coming from the block layer >>> has a unique tag, so use that when possible for getting a slot. >>> >>> Unfortunately we don't support reserved commands in the SCSI midlayer yet. >>> As such, SMP tasks - as an example - will not have a request associated, so >>> in the interim continue to manage those tags for that type of sas_task >>> internally. >>> >>> We reserve an arbitrary 4 tags for these internal tags. Indeed, we already >>> decrement MVS_RSVD_SLOTS by 2 for the shost can_queue when flag >>> MVF_FLAG_SOC is set. This change was made in commit 20b09c2992fef >>> ("[PATCH] [SCSI] mvsas: add support for 94xx; layout change; bug fixes"), >>> but what those 2 slots are used for is not obvious. >>> >>> Signed-off-by: John Garry <john.garry@huawei.com> >>> --- >>> drivers/scsi/mvsas/mv_defs.h | 1 + >>> drivers/scsi/mvsas/mv_init.c | 4 ++-- >>> drivers/scsi/mvsas/mv_sas.c | 22 +++++++++++++++++----- >>> drivers/scsi/mvsas/mv_sas.h | 1 - >>> 4 files changed, 20 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/scsi/mvsas/mv_defs.h b/drivers/scsi/mvsas/mv_defs.h >>> index 7123a2efbf58..8ef174cd4d37 100644 >>> --- a/drivers/scsi/mvsas/mv_defs.h >>> +++ b/drivers/scsi/mvsas/mv_defs.h >>> @@ -40,6 +40,7 @@ enum driver_configuration { >>> MVS_ATA_CMD_SZ = 96, /* SATA command table buffer size */ >>> MVS_OAF_SZ = 64, /* Open address frame buffer size */ >>> MVS_QUEUE_SIZE = 64, /* Support Queue depth */ >>> + MVS_RSVD_SLOTS = 4, >>> MVS_SOC_CAN_QUEUE = MVS_SOC_SLOTS - 2, >>> }; >>> >>> diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c >>> index c85fb812ad43..d834ed9e8e4a 100644 >>> --- a/drivers/scsi/mvsas/mv_init.c >>> +++ b/drivers/scsi/mvsas/mv_init.c >>> @@ -284,7 +284,7 @@ static int mvs_alloc(struct mvs_info *mvi, struct Scsi_Host *shost) >>> printk(KERN_DEBUG "failed to create dma pool %s.\n", pool_name); >>> goto err_out; >>> } >>> - mvi->tags_num = slot_nr; >>> + mvi->tags_num = MVS_RSVD_SLOTS; >> >> Same comment as for pm8001: do you really need this field if the value >> is always MVS_RSVD_SLOTS ? > > Right, I don't need this struct member. Again I can just use this macro > directly. > >> >>> >>> return 0; >>> err_out: >>> @@ -367,7 +367,7 @@ static struct mvs_info *mvs_pci_alloc(struct pci_dev *pdev, >>> mvi->sas = sha; >>> mvi->shost = shost; >>> >>> - mvi->tags = kzalloc(MVS_CHIP_SLOT_SZ>>3, GFP_KERNEL); >>> + mvi->tags = kzalloc(MVS_RSVD_SLOTS, GFP_KERNEL); >> >> Field name ? reserved_tags ? >> Also, the alloc seems wrong. This will allocate 4 bytes, but you only >> need 4 bits. You could make this an unsigned long and not allocate >> anything. > > Well spotted. I should have questioned more why they had >>3 previously. > > But I would rather keep as a bitmap, i.e. *unsigned long for simplicity.> >> Same remark for pm8001 by the way. > > I think it's ok as it uses bitmap_zalloc() Yes ! > >> >> That would cap MVS_RSVD_SLOTS to BITS_PER_LONG maximum, but that is easy >> to check at compile time with a #if/#error. >> > > As above, I'd rather keep as a bitmap. It's a little inefficient, but is > a one off in the driver. > > Thanks, > John > >
diff --git a/drivers/scsi/mvsas/mv_defs.h b/drivers/scsi/mvsas/mv_defs.h index 7123a2efbf58..8ef174cd4d37 100644 --- a/drivers/scsi/mvsas/mv_defs.h +++ b/drivers/scsi/mvsas/mv_defs.h @@ -40,6 +40,7 @@ enum driver_configuration { MVS_ATA_CMD_SZ = 96, /* SATA command table buffer size */ MVS_OAF_SZ = 64, /* Open address frame buffer size */ MVS_QUEUE_SIZE = 64, /* Support Queue depth */ + MVS_RSVD_SLOTS = 4, MVS_SOC_CAN_QUEUE = MVS_SOC_SLOTS - 2, }; diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c index c85fb812ad43..d834ed9e8e4a 100644 --- a/drivers/scsi/mvsas/mv_init.c +++ b/drivers/scsi/mvsas/mv_init.c @@ -284,7 +284,7 @@ static int mvs_alloc(struct mvs_info *mvi, struct Scsi_Host *shost) printk(KERN_DEBUG "failed to create dma pool %s.\n", pool_name); goto err_out; } - mvi->tags_num = slot_nr; + mvi->tags_num = MVS_RSVD_SLOTS; return 0; err_out: @@ -367,7 +367,7 @@ static struct mvs_info *mvs_pci_alloc(struct pci_dev *pdev, mvi->sas = sha; mvi->shost = shost; - mvi->tags = kzalloc(MVS_CHIP_SLOT_SZ>>3, GFP_KERNEL); + mvi->tags = kzalloc(MVS_RSVD_SLOTS, GFP_KERNEL); if (!mvi->tags) goto err_out; diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c index 0810e6c930e1..549d2ec89f60 100644 --- a/drivers/scsi/mvsas/mv_sas.c +++ b/drivers/scsi/mvsas/mv_sas.c @@ -20,7 +20,7 @@ static int mvs_find_tag(struct mvs_info *mvi, struct sas_task *task, u32 *tag) return 0; } -void mvs_tag_clear(struct mvs_info *mvi, u32 tag) +static void mvs_tag_clear(struct mvs_info *mvi, u32 tag) { void *bitmap = mvi->tags; clear_bit(tag, bitmap); @@ -28,6 +28,11 @@ void mvs_tag_clear(struct mvs_info *mvi, u32 tag) void mvs_tag_free(struct mvs_info *mvi, u32 tag) { + if (tag < mvi->shost->can_queue) + return; + + tag -= mvi->shost->can_queue; + mvs_tag_clear(mvi, tag); } @@ -47,6 +52,7 @@ inline int mvs_tag_alloc(struct mvs_info *mvi, u32 *tag_out) if (tag >= mvi->tags_num) return -SAS_QUEUE_FULL; mvs_tag_set(mvi, tag); + tag += mvi->shost->can_queue; *tag_out = tag; return 0; } @@ -696,6 +702,7 @@ static int mvs_task_prep(struct sas_task *task, struct mvs_info *mvi, int is_tmf struct mvs_task_exec_info tei; struct mvs_slot_info *slot; u32 tag = 0xdeadbeef, n_elem = 0; + struct request *rq; int rc = 0; if (!dev->port) { @@ -760,9 +767,14 @@ static int mvs_task_prep(struct sas_task *task, struct mvs_info *mvi, int is_tmf n_elem = task->num_scatter; } - rc = mvs_tag_alloc(mvi, &tag); - if (rc) - goto err_out; + rq = sas_task_find_rq(task); + if (rq) { + tag = rq->tag; + } else { + rc = mvs_tag_alloc(mvi, &tag); + if (rc) + goto err_out; + } slot = &mvi->slot_info[tag]; @@ -857,7 +869,7 @@ int mvs_queue_command(struct sas_task *task, gfp_t gfp_flags) static void mvs_slot_free(struct mvs_info *mvi, u32 rx_desc) { u32 slot_idx = rx_desc & RXQ_SLOT_MASK; - mvs_tag_clear(mvi, slot_idx); + mvs_tag_free(mvi, slot_idx); } static void mvs_slot_task_free(struct mvs_info *mvi, struct sas_task *task, diff --git a/drivers/scsi/mvsas/mv_sas.h b/drivers/scsi/mvsas/mv_sas.h index fe57665bdb50..e6c70786ded9 100644 --- a/drivers/scsi/mvsas/mv_sas.h +++ b/drivers/scsi/mvsas/mv_sas.h @@ -424,7 +424,6 @@ struct mvs_task_exec_info { /******************** function prototype *********************/ void mvs_get_sas_addr(void *buf, u32 buflen); -void mvs_tag_clear(struct mvs_info *mvi, u32 tag); void mvs_tag_free(struct mvs_info *mvi, u32 tag); void mvs_tag_set(struct mvs_info *mvi, unsigned int tag); int mvs_tag_alloc(struct mvs_info *mvi, u32 *tag_out);
The request associated with a scsi command coming from the block layer has a unique tag, so use that when possible for getting a slot. Unfortunately we don't support reserved commands in the SCSI midlayer yet. As such, SMP tasks - as an example - will not have a request associated, so in the interim continue to manage those tags for that type of sas_task internally. We reserve an arbitrary 4 tags for these internal tags. Indeed, we already decrement MVS_RSVD_SLOTS by 2 for the shost can_queue when flag MVF_FLAG_SOC is set. This change was made in commit 20b09c2992fef ("[PATCH] [SCSI] mvsas: add support for 94xx; layout change; bug fixes"), but what those 2 slots are used for is not obvious. Signed-off-by: John Garry <john.garry@huawei.com> --- drivers/scsi/mvsas/mv_defs.h | 1 + drivers/scsi/mvsas/mv_init.c | 4 ++-- drivers/scsi/mvsas/mv_sas.c | 22 +++++++++++++++++----- drivers/scsi/mvsas/mv_sas.h | 1 - 4 files changed, 20 insertions(+), 8 deletions(-)