Re: [PATCH v1 2/4] mpi3mr: Support PCIe Error Recovery callback handlers

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Dec 06, 2023 at 08:55:11PM +0530, Ranjan Kumar wrote:
> The driver has been upgraded to include support for the
> PCIe error recovery callback handler which is crucial for
> the recovery of the controllers. This feature is
> necessary for addressing the errors reported by
> the PCIe AER (Advanced Error Reporting) mechanism.
> 
> Signed-off-by: Sathya Prakash <sathya.prakash@broadcom.com>
> Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
> ...

> +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 -1;
> +	}
> +
> +	*mrioc = shost_priv(*shost);
> +	if (*mrioc == NULL) {
> +		dev_err(&pdev->dev, "shost's private data is null\n");
> +		*shost = NULL;
> +		return -1;

I'm a little bit skeptical about these checks for NULL, although I do
see that the existing code has similar "if (!shost)" checks.

Usually these checks will only find memory corruption or logic errors,
and silently bailing out, as the previous "if (!shost)" checks do,
just masks a serious problem.  Logging errors, as you do here, is a
little better, but I think it's better to just take the exception when
we dereference the NULL pointer later because that's impossible to
ignore and usually gives more clues about what went wrong.

> +}
> +	return 0;
> +}

The addition and use of mpi3mr_get_shost_and_mrioc() looks like it
could be a separate patch.  If so, it might be nice to split this into
several smaller, simpler patches.

>  static int __maybe_unused
>  mpi3mr_suspend(struct device *dev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> -	struct Scsi_Host *shost = pci_get_drvdata(pdev);
> +	struct Scsi_Host *shost;
>  	struct mpi3mr_ioc *mrioc;
>  
> -	if (!shost)
> -		return 0;
> +	if (mpi3mr_get_shost_and_mrioc(pdev, &shost, &mrioc))
> +		return -1;

Is -1 really the best return value here?  It seems like usually a
negative errno is returned.

Bjorn