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

[John Garry <john.garry@xxxxxxxxxx>]

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/

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