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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
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@xxxxxxxxxx>
> > Signed-off-by: Igor Pylypiv <ipylypiv@xxxxxxxxxx>
> > ---
> > 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);
>
