Re: [PATCH v3 1/3] mpi3mr: Support PCIe Error Recovery callback handlers

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Jun 14, 2024 at 12:30:20AM +0530, Sumit Saxena wrote:
> This patch adds support for the PCIe error recovery callback handlers which is
> crucial for the recovery of the controllers. This feature is necessary for
> addressing the errors reported by the PCI Error Recovery mechanism.

> +++ b/drivers/scsi/mpi3mr/mpi3mr.h

> +mpi3mr_pcierr_detected(struct pci_dev *pdev, pci_channel_state_t state)
> +{
> +	struct Scsi_Host *shost;
> +	struct mpi3mr_ioc *mrioc;
> +	unsigned int timeout = MPI3MR_RESET_TIMEOUT;
> +
> +	dev_info(&pdev->dev, "%s: callback invoked state(%d)\n", __func__,
> +	    state);
> +
> +	shost = pci_get_drvdata(pdev);
> +	mrioc = shost_priv(shost);

This will be a NULL pointer dereference if shost is NULL.  But I think
that's OK, and you don't need the check below, because we should never
get here if either shost or mrioc is NULL unless the code is broken,
and in that case we *want* the NULL pointer oops so we can fix it.

> +	if (!shost || !mrioc) {
> +		dev_err(&pdev->dev, "device not available\n");
> +		return PCI_ERS_RESULT_DISCONNECT;
> +	}

> +static pci_ers_result_t mpi3mr_pcierr_slot_reset(struct pci_dev *pdev)
> +{
> +	struct Scsi_Host *shost;
> +	struct mpi3mr_ioc *mrioc;
> +	unsigned int timeout = MPI3MR_RESET_TIMEOUT;
> +
> +	dev_info(&pdev->dev, "%s: callback invoked\n", __func__);
> +
> +	shost = pci_get_drvdata(pdev);
> +	mrioc = shost_priv(shost);
> +
> +	if (!shost || !mrioc) {
> +		dev_err(&pdev->dev, "device not available\n");
> +		return PCI_ERS_RESULT_DISCONNECT;
> +	}

Same here.

> +static void mpi3mr_pcierr_resume(struct pci_dev *pdev)
> +{
> +	struct Scsi_Host *shost;
> +	struct mpi3mr_ioc *mrioc;
> +
> +	dev_info(&pdev->dev, "%s: callback invoked\n", __func__);
> +
> +	shost = pci_get_drvdata(pdev);
> +	mrioc = shost_priv(shost);
> +
> +	if (!shost || !mrioc) {
> +		dev_err(&pdev->dev, "device not available\n");
> +		return;
> +	}

Same here.

> +	pci_aer_clear_nonfatal_status(pdev);

Why is there here?  No other driver does this.

> +static pci_ers_result_t mpi3mr_pcierr_mmio_enabled(struct pci_dev *pdev)
> +{
> +	struct Scsi_Host *shost;
> +	struct mpi3mr_ioc *mrioc;
> +
> +	dev_info(&pdev->dev, "%s: callback invoked\n", __func__);
> +
> +	shost = pci_get_drvdata(pdev);
> +	mrioc = shost_priv(shost);
> +
> +	if (!shost || !mrioc) {
> +		dev_err(&pdev->dev, "device not available\n");
> +		return PCI_ERS_RESULT_DISCONNECT;
> +	}

Same here.

> +static struct pci_error_handlers mpi3mr_err_handler = {
> +	.error_detected = mpi3mr_pcierr_detected,

I think it's nice if the function name includes the function pointer
name, i.e., ".error_detected = mpi3mr_error_detected" (or
"mpi3mr_pci_error_detected" if you prefer).

That way 'git grep -A5 ".*pci_ers_result_t.*error_detected"' finds
most of them for comparison.

> +	.mmio_enabled = mpi3mr_pcierr_mmio_enabled,
> +	.slot_reset = mpi3mr_pcierr_slot_reset,
> +	.resume = mpi3mr_pcierr_resume,
> +};