Re: [PATCH] scsi: pm80xx: Remove pm8001_tag_init()

[Igor Pylypiv <ipylypiv@google.com>]

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);
>