[+cc linux-pci]
On Thu, Dec 16, 2021 at 05:30:07PM -0600, Bjorn Helgaas wrote:
> [+cc Vaibhav since my main question is about converting to generic PM]
>
> On Thu, May 20, 2021 at 08:55:42PM +0530, Kashyap Desai wrote:
> > Signed-off-by: Kashyap Desai <kashyap.desai@xxxxxxxxxxxx>
> > Reviewed-by: Hannes Reinecke <hare@xxxxxxx>
> > Reviewed-by: Tomas Henzl <thenzl@xxxxxxxxxx>
> > Reviewed-by: Himanshu Madhani <himanshu.madhani@xxxxxxxxxx>
> >
> > Cc: sathya.prakash@xxxxxxxxxxxx
> > ---
> > drivers/scsi/mpi3mr/mpi3mr_os.c | 84 +++++++++++++++++++++++++++++++++
> > 1 file changed, 84 insertions(+)
> >
> > diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> > index e63db7c..dd71cdc 100644
> > --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> > +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> > @@ -3389,6 +3389,86 @@ static void mpi3mr_shutdown(struct pci_dev *pdev)
> > mpi3mr_cleanup_ioc(mrioc, 0);
> > }
> >
> > +#ifdef CONFIG_PM
> > +/**
> > + * mpi3mr_suspend - PCI power management suspend callback
> > + * @pdev: PCI device instance
> > + * @state: New power state
> > + *
> > + * Change the power state to the given value and cleanup the IOC
> > + * by issuing MUR and shutdown notification
> > + *
> > + * Return: 0 always.
> > + */
> > +static int mpi3mr_suspend(struct pci_dev *pdev, pm_message_t state)
> > +{
> > + struct Scsi_Host *shost = pci_get_drvdata(pdev);
> > + struct mpi3mr_ioc *mrioc;
> > + pci_power_t device_state;
> > +
> > + if (!shost)
> > + return 0;
>
> This test of "shost" is unnecessary. If you set the drvdata before
> the .probe() method returns success, you will always get valid drvdata
> here. And you do:
>
> mpi3mr_probe
> mpi3mr_init_ioc
> mpi3mr_setup_resources
> pci_set_drvdata(pdev, mrioc->shost)
>
> Sure, it's *possible* that a random memory corruption will clear out
> the drvdata pointer, but there's no point in testing for that.
>
> > + mrioc = shost_priv(shost);
> > + while (mrioc->reset_in_progress || mrioc->is_driver_loading)
> > + ssleep(1);
> > + mrioc->stop_drv_processing = 1;
> > + mpi3mr_cleanup_fwevt_list(mrioc);
> > + scsi_block_requests(shost);
> > + mpi3mr_stop_watchdog(mrioc);
> > + mpi3mr_cleanup_ioc(mrioc, 1);
>
> This looks possibly wrong. mpi3mr_cleanup_ioc() takes a "reason",
> which looks like it should be MPI3MR_COMPLETE_CLEANUP (0),
> MPI3MR_REINIT_FAILURE (1), or MPI3MR_SUSPEND (2).
>
> This should at least use the enum, and it looks like it should use
> MPI3MR_SUSPEND instead of passing the MPI3MR_REINIT_FAILURE value.
>
> > + device_state = pci_choose_state(pdev, state);
> > + ioc_info(mrioc, "pdev=0x%p, slot=%s, entering operating state [D%d]\n",
> > + pdev, pci_name(pdev), device_state);
> > + pci_save_state(pdev);
> > + pci_set_power_state(pdev, device_state);
> > + mpi3mr_cleanup_resources(mrioc);
>
> Why is mpi3mr_cleanup_resources() needed here? It frees IRQs,
> iounmaps registers, calls pci_release_selected_regions(), etc.
>
> Very few other drivers do this, and I don't think it's necessary for
> suspend. And if you don't clean it up here, you won't need to
> re-initialize it during resume.
>
> I'm asking because I want to convert this from the legacy PCI power
> management model to the generic PM model. The generic model works
> like this:
>
> suspend_devices_and_enter
> dpm_suspend_start(PMSG_SUSPEND)
> pci_pm_suspend # PCI bus .suspend() method
> mpi3mr_suspend <-- device-specific stuff
> suspend_enter
> dpm_suspend_noirq(PMSG_SUSPEND)
> pci_pm_suspend_noirq # PCI bus .suspend_noirq() method
> pci_save_state <-- generic PCI
> pci_prepare_to_sleep <-- generic PCI
> pci_set_power_state
> ...
> dpm_resume_end(PMSG_RESUME)
> pci_pm_resume # PCI bus .resume() method
> pci_restore_standard_config
> pci_set_power_state(PCI_D0) <-- generic PCI
> pci_restore_state <-- generic PCI
> mpi3mr_resume # dev->driver->pm->resume
>
> Notice that in the generic model the PCI core takes care of
> pci_save_state(), pci_set_power_state(), etc., and the device-specific
> stuff is done before the generic PCI suspend, and after the generic
> PCI resume.
>
> mpi3mr_cleanup_resources() is problematic because the device-specific
> stuff should be done *before* the generic PCI part, but you call it
> *after* the generic PCI part. I think it would be *possible* to do it
> after by making a mpi3mr_suspend_noirq() method, but there are hardly
> any drivers that do that, and I think mpi3mr_cleanup_resources() is
> unnecessary here anyway.
>
> > + return 0;
> > +}
> > +
> > +/**
> > + * mpi3mr_resume - PCI power management resume callback
> > + * @pdev: PCI device instance
> > + *
> > + * Restore the power state to D0 and reinitialize the controller
> > + * and resume I/O operations to the target devices
> > + *
> > + * Return: 0 on success, non-zero on failure
> > + */
> > +static int mpi3mr_resume(struct pci_dev *pdev)
> > +{
> > + struct Scsi_Host *shost = pci_get_drvdata(pdev);
> > + struct mpi3mr_ioc *mrioc;
> > + pci_power_t device_state = pdev->current_state;
> > + int r;
> > +
> > + mrioc = shost_priv(shost);
> > +
> > + ioc_info(mrioc, "pdev=0x%p, slot=%s, previous operating state [D%d]\n",
> > + pdev, pci_name(pdev), device_state);
> > + pci_set_power_state(pdev, PCI_D0);
> > + pci_enable_wake(pdev, PCI_D0, 0);
> > + pci_restore_state(pdev);
> > + mrioc->pdev = pdev;
> > + mrioc->cpu_count = num_online_cpus();
> > + r = mpi3mr_setup_resources(mrioc);
> > + if (r) {
> > + ioc_info(mrioc, "%s: Setup resources failed[%d]\n",
> > + __func__, r);
> > + return r;
> > + }
> > +
> > + mrioc->stop_drv_processing = 0;
> > + mpi3mr_init_ioc(mrioc, 1);
> > + scsi_unblock_requests(shost);
> > + mpi3mr_start_watchdog(mrioc);
> > +
> > + return 0;
> > +}
> > +#endif
> > +
> > static const struct pci_device_id mpi3mr_pci_id_table[] = {
> > {
> > PCI_DEVICE_SUB(PCI_VENDOR_ID_LSI_LOGIC, 0x00A5,
> > @@ -3404,6 +3484,10 @@ static struct pci_driver mpi3mr_pci_driver = {
> > .probe = mpi3mr_probe,
> > .remove = mpi3mr_remove,
> > .shutdown = mpi3mr_shutdown,
> > +#ifdef CONFIG_PM
> > + .suspend = mpi3mr_suspend,
> > + .resume = mpi3mr_resume,
> > +#endif
> > };
> >
> > static int __init mpi3mr_init(void)
> > --
> > 2.18.1
> >
>
>
