| Message ID | 20210520152545.2710479-22-kashyap.desai@broadcom.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Introducing mpi3mr driver | expand |
[+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@broadcom.com> > Reviewed-by: Hannes Reinecke <hare@suse.de> > Reviewed-by: Tomas Henzl <thenzl@redhat.com> > Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com> > > Cc: sathya.prakash@broadcom.com > --- > 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 >
[+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@broadcom.com> > > Reviewed-by: Hannes Reinecke <hare@suse.de> > > Reviewed-by: Tomas Henzl <thenzl@redhat.com> > > Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com> > > > > Cc: sathya.prakash@broadcom.com > > --- > > 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 > > > >
[-cc Peter Rivera (bouncing)] On Thu, Dec 16, 2021 at 05:32:51PM -0600, Bjorn Helgaas wrote: > On Thu, Dec 16, 2021 at 05:30:07PM -0600, Bjorn Helgaas wrote: > > On Thu, May 20, 2021 at 08:55:42PM +0530, Kashyap Desai wrote: > > > +static int mpi3mr_suspend(struct pci_dev *pdev, pm_message_t state) > > > +{ > > > + 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. Sorry for the noise about this part, I see you fixed this later with 0da66348c26f ("scsi: mpi3mr: Set up IRQs in resume path").
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; + + 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); + + 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); + + 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)