On Wed, Dec 6, 2023 at 9:56 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxx>
> > Signed-off-by: Ranjan Kumar <ranjan.kumar@xxxxxxxxxxxx>
> > ...
>
> > +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.
>
Agree, however, I wouldn't want to crash the system from this driver.
Will add WARN notices and move this into a separate patch later. For
now, will check this inside the PM functions as we do currently for
other cases.
> > +}
> > + 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
Will detach this from this patch and will submit another separate patch.
>
> > 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.
The __suspend_report_result just prints the return value, so i thought
-1 is fine as we already print a error message. Do you recommend to
use another negative (non -1) number for differentiation between
generic error and this?
>
> Bjorn
>
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
