| Message ID | 20220428000326.3622230-1-ipylypiv@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: pm80xx: Remove pm8001_tag_init() | expand |
On 28/04/2022 01:03, Igor Pylypiv wrote: > In commit 5a141315ed7c ("scsi: pm80xx: Increase the number of outstanding > I/O supported to 1024") the pm8001_ha->tags allocation was moved into > pm8001_init_ccb_tag(). This changed the execution order of allocation. > pm8001_tag_init() used to be called after the pm8001_ha->tags allocation > and now it is called before the allocation. > > Before: > > pm8001_pci_probe() > `--> pm8001_pci_alloc() > `--> pm8001_alloc() > `--> pm8001_ha->tags = kzalloc(...) > `--> pm8001_tag_init(pm8001_ha); // OK: tags are allocated > > After: > > pm8001_pci_probe() > `--> pm8001_pci_alloc() > | `--> pm8001_alloc() > | `--> pm8001_tag_init(pm8001_ha); // NOK: tags are not allocated > | > `--> pm8001_init_ccb_tag() > `--> pm8001_ha->tags = kzalloc(...) // today it is bitmap_zalloc() > > Since pm8001_ha->tags_num is zero when pm8001_tag_init() is called it does > nothing. Tags memory is allocated with bitmap_zalloc() so there is no need > to manually clear each bit with pm8001_tag_free(). Your change looks ok. But please note the following discussed in the following link, there was/is a bug in the lateness in which the tags are allocated: https://lore.kernel.org/linux-scsi/PH0PR11MB5112D8C17D9EA268C197893FEC579@PH0PR11MB5112.namprd11.prod.outlook.com/ I don't think that it has been fixed yet... Thanks, John > > Fixes: 5a141315ed7c ("scsi: pm80xx: Increase the number of outstanding > I/O supported to 1024") > Reviewed-by: Changyuan Lyu <changyuanl@google.com> > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > --- > drivers/scsi/pm8001/pm8001_init.c | 2 -- > drivers/scsi/pm8001/pm8001_sas.c | 7 ------- > drivers/scsi/pm8001/pm8001_sas.h | 1 - > 3 files changed, 10 deletions(-) > > diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c > index 9b04f1a6a67d..7040cecd861b 100644 > --- a/drivers/scsi/pm8001/pm8001_init.c > +++ b/drivers/scsi/pm8001/pm8001_init.c > @@ -420,8 +420,6 @@ static int pm8001_alloc(struct pm8001_hba_info *pm8001_ha, > atomic_set(&pm8001_ha->devices[i].running_req, 0); > } > pm8001_ha->flags = PM8001F_INIT_TIME; > - /* Initialize tags */ > - pm8001_tag_init(pm8001_ha); > return 0; > > err_out_nodev: > diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c > index 3a863d776724..dc689055341b 100644 > --- a/drivers/scsi/pm8001/pm8001_sas.c > +++ b/drivers/scsi/pm8001/pm8001_sas.c > @@ -92,13 +92,6 @@ int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out) > return 0; > } > > -void pm8001_tag_init(struct pm8001_hba_info *pm8001_ha) > -{ > - int i; > - for (i = 0; i < pm8001_ha->tags_num; ++i) > - pm8001_tag_free(pm8001_ha, i); > -} > - > /** > * pm8001_mem_alloc - allocate memory for pm8001. > * @pdev: pci device. > diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h > index 060ab680a7ed..ba959f986c1e 100644 > --- a/drivers/scsi/pm8001/pm8001_sas.h > +++ b/drivers/scsi/pm8001/pm8001_sas.h > @@ -633,7 +633,6 @@ extern struct workqueue_struct *pm8001_wq; > > /******************** function prototype *********************/ > int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out); > -void pm8001_tag_init(struct pm8001_hba_info *pm8001_ha); > u32 pm8001_get_ncq_tag(struct sas_task *task, u32 *tag); > void pm8001_ccb_task_free(struct pm8001_hba_info *pm8001_ha, > struct pm8001_ccb_info *ccb);
On Fri, Apr 29, 2022 at 10:06:06AM +0100, John Garry wrote: > On 28/04/2022 01:03, Igor Pylypiv wrote: > > In commit 5a141315ed7c ("scsi: pm80xx: Increase the number of outstanding > > I/O supported to 1024") the pm8001_ha->tags allocation was moved into > > pm8001_init_ccb_tag(). This changed the execution order of allocation. > > pm8001_tag_init() used to be called after the pm8001_ha->tags allocation > > and now it is called before the allocation. > > > > Before: > > > > pm8001_pci_probe() > > `--> pm8001_pci_alloc() > > `--> pm8001_alloc() > > `--> pm8001_ha->tags = kzalloc(...) > > `--> pm8001_tag_init(pm8001_ha); // OK: tags are allocated > > > > After: > > > > pm8001_pci_probe() > > `--> pm8001_pci_alloc() > > | `--> pm8001_alloc() > > | `--> pm8001_tag_init(pm8001_ha); // NOK: tags are not allocated > > | > > `--> pm8001_init_ccb_tag() > > `--> pm8001_ha->tags = kzalloc(...) // today it is bitmap_zalloc() > > > > Since pm8001_ha->tags_num is zero when pm8001_tag_init() is called it does > > nothing. Tags memory is allocated with bitmap_zalloc() so there is no need > > to manually clear each bit with pm8001_tag_free(). > > Your change looks ok. But please note the following discussed in the > following link, there was/is a bug in the lateness in which the tags are > allocated: > > https://lore.kernel.org/linux-scsi/PH0PR11MB5112D8C17D9EA268C197893FEC579@PH0PR11MB5112.namprd11.prod.outlook.com/ Thanks for pointing out the discussion, John. My patch should still apply because clearing bits is redundant for memory allocated with bitmap_zalloc(). > > I don't think that it has been fixed yet... Adding Ajish to comment on the patch readiness for the tags allocation lateness issue. Thanks, Igor > > Thanks, > John > > > > > Fixes: 5a141315ed7c ("scsi: pm80xx: Increase the number of outstanding > > I/O supported to 1024") > > Reviewed-by: Changyuan Lyu <changyuanl@google.com> > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > > --- > > drivers/scsi/pm8001/pm8001_init.c | 2 -- > > drivers/scsi/pm8001/pm8001_sas.c | 7 ------- > > drivers/scsi/pm8001/pm8001_sas.h | 1 - > > 3 files changed, 10 deletions(-) > > > > diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c > > index 9b04f1a6a67d..7040cecd861b 100644 > > --- a/drivers/scsi/pm8001/pm8001_init.c > > +++ b/drivers/scsi/pm8001/pm8001_init.c > > @@ -420,8 +420,6 @@ static int pm8001_alloc(struct pm8001_hba_info *pm8001_ha, > > atomic_set(&pm8001_ha->devices[i].running_req, 0); > > } > > pm8001_ha->flags = PM8001F_INIT_TIME; > > - /* Initialize tags */ > > - pm8001_tag_init(pm8001_ha); > > return 0; > > err_out_nodev: > > diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c > > index 3a863d776724..dc689055341b 100644 > > --- a/drivers/scsi/pm8001/pm8001_sas.c > > +++ b/drivers/scsi/pm8001/pm8001_sas.c > > @@ -92,13 +92,6 @@ int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out) > > return 0; > > } > > -void pm8001_tag_init(struct pm8001_hba_info *pm8001_ha) > > -{ > > - int i; > > - for (i = 0; i < pm8001_ha->tags_num; ++i) > > - pm8001_tag_free(pm8001_ha, i); > > -} > > - > > /** > > * pm8001_mem_alloc - allocate memory for pm8001. > > * @pdev: pci device. > > diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h > > index 060ab680a7ed..ba959f986c1e 100644 > > --- a/drivers/scsi/pm8001/pm8001_sas.h > > +++ b/drivers/scsi/pm8001/pm8001_sas.h > > @@ -633,7 +633,6 @@ extern struct workqueue_struct *pm8001_wq; > > /******************** function prototype *********************/ > > int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out); > > -void pm8001_tag_init(struct pm8001_hba_info *pm8001_ha); > > u32 pm8001_get_ncq_tag(struct sas_task *task, u32 *tag); > > void pm8001_ccb_task_free(struct pm8001_hba_info *pm8001_ha, > > struct pm8001_ccb_info *ccb); >
diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c index 9b04f1a6a67d..7040cecd861b 100644 --- a/drivers/scsi/pm8001/pm8001_init.c +++ b/drivers/scsi/pm8001/pm8001_init.c @@ -420,8 +420,6 @@ static int pm8001_alloc(struct pm8001_hba_info *pm8001_ha, atomic_set(&pm8001_ha->devices[i].running_req, 0); } pm8001_ha->flags = PM8001F_INIT_TIME; - /* Initialize tags */ - pm8001_tag_init(pm8001_ha); return 0; err_out_nodev: diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c index 3a863d776724..dc689055341b 100644 --- a/drivers/scsi/pm8001/pm8001_sas.c +++ b/drivers/scsi/pm8001/pm8001_sas.c @@ -92,13 +92,6 @@ int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out) return 0; } -void pm8001_tag_init(struct pm8001_hba_info *pm8001_ha) -{ - int i; - for (i = 0; i < pm8001_ha->tags_num; ++i) - pm8001_tag_free(pm8001_ha, i); -} - /** * pm8001_mem_alloc - allocate memory for pm8001. * @pdev: pci device. diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h index 060ab680a7ed..ba959f986c1e 100644 --- a/drivers/scsi/pm8001/pm8001_sas.h +++ b/drivers/scsi/pm8001/pm8001_sas.h @@ -633,7 +633,6 @@ extern struct workqueue_struct *pm8001_wq; /******************** function prototype *********************/ int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out); -void pm8001_tag_init(struct pm8001_hba_info *pm8001_ha); u32 pm8001_get_ncq_tag(struct sas_task *task, u32 *tag); void pm8001_ccb_task_free(struct pm8001_hba_info *pm8001_ha, struct pm8001_ccb_info *ccb);
In commit 5a141315ed7c ("scsi: pm80xx: Increase the number of outstanding I/O supported to 1024") the pm8001_ha->tags allocation was moved into pm8001_init_ccb_tag(). This changed the execution order of allocation. pm8001_tag_init() used to be called after the pm8001_ha->tags allocation and now it is called before the allocation. Before: pm8001_pci_probe() `--> pm8001_pci_alloc() `--> pm8001_alloc() `--> pm8001_ha->tags = kzalloc(...) `--> pm8001_tag_init(pm8001_ha); // OK: tags are allocated After: pm8001_pci_probe() `--> pm8001_pci_alloc() | `--> pm8001_alloc() | `--> pm8001_tag_init(pm8001_ha); // NOK: tags are not allocated | `--> pm8001_init_ccb_tag() `--> pm8001_ha->tags = kzalloc(...) // today it is bitmap_zalloc() Since pm8001_ha->tags_num is zero when pm8001_tag_init() is called it does nothing. Tags memory is allocated with bitmap_zalloc() so there is no need to manually clear each bit with pm8001_tag_free(). Fixes: 5a141315ed7c ("scsi: pm80xx: Increase the number of outstanding I/O supported to 1024") Reviewed-by: Changyuan Lyu <changyuanl@google.com> Signed-off-by: Igor Pylypiv <ipylypiv@google.com> --- drivers/scsi/pm8001/pm8001_init.c | 2 -- drivers/scsi/pm8001/pm8001_sas.c | 7 ------- drivers/scsi/pm8001/pm8001_sas.h | 1 - 3 files changed, 10 deletions(-)