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

Bjorn Helgaas
2024-01-04T23:11:05Z

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