Re: [PATCH v2 1/6] mpi3mr: Creation of helper function

[Bjorn Helgaas <helgaas@kernel.org>]

I wasn't cc'd on this series, so apologies if it's already been
applied and these comments are moot.

BTW, the cover doesn't say what this applies to, and it doesn't apply
cleanly to v6.7-rc1.

On Fri, Dec 15, 2023 at 02:28:55AM +0530, Ranjan Kumar wrote:
> Use of helper function to get controller and shost details.
> 
> Signed-off-by: Sathya Prakash <sathya.prakash@broadcom.com>
> Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
> ---
>  drivers/scsi/mpi3mr/mpi3mr_os.c | 54 ++++++++++++++++++++++++++-------
>  1 file changed, 43 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 1bffd629c124..76ba31a9517d 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -5230,6 +5230,35 @@ mpi3mr_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	return retval;
>  }
>  
> +/**
> + * mpi3mr_get_shost_and_mrioc - get shost and ioc reference if
> + *                     they are valid
> + * @pdev: PCI device struct
> + * @shost: address to store scsi host reference
> + * @mrioc: address store HBA adapter reference
> + *
> + * Return: 0 if *shost and *ioc are not NULL otherwise -1.
> + */
> +

Spurious blank line (I see that mpi3mr_probe() has a blank line here,
but most functions in this file do not).

> +static int
> +mpi3mr_get_shost_and_mrioc(struct pci_dev *pdev,

Most functions in this file use this style:

  static int mpi3mr_get_shost_and_mrioc(struct pci_dev *pdev,

> +	struct Scsi_Host **shost, struct mpi3mr_ioc **mrioc)
> +{
> +	*shost = pci_get_drvdata(pdev);
> +	if (*shost == NULL) {
> +		dev_err(&pdev->dev, "pdev's driver data is null\n");
> +		return -ENXIO;
> +	}
> +
> +	*mrioc = shost_priv(*shost);
> +	if (*mrioc == NULL) {
> +		dev_err(&pdev->dev, "shost's private data is null\n");
> +		*shost = NULL;
> +		return -ENXIO;
> +		}
> +	return 0;
> +}
> +
>  /**
>   * mpi3mr_remove - PCI remove callback
>   * @pdev: PCI device instance
> @@ -5242,7 +5271,7 @@ mpi3mr_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   */
>  static void mpi3mr_remove(struct pci_dev *pdev)
>  {
> -	struct Scsi_Host *shost = pci_get_drvdata(pdev);
> +	struct Scsi_Host *shost;
>  	struct mpi3mr_ioc *mrioc;
>  	struct workqueue_struct	*wq;
>  	unsigned long flags;
> @@ -5250,7 +5279,7 @@ static void mpi3mr_remove(struct pci_dev *pdev)
>  	struct mpi3mr_hba_port *port, *hba_port_next;
>  	struct mpi3mr_sas_node *sas_expander, *sas_expander_next;
>  
> -	if (!shost)
> +	if (mpi3mr_get_shost_and_mrioc(pdev, &shost, &mrioc))
>  		return;
>  
>  	mrioc = shost_priv(shost);

Maybe I'm missing something, but it looks like this patch should
remove "mrioc = shost_priv(shost)" every time it adds a call to
mpi3mr_get_shost_and_mrioc().

Bjorn