| Message ID | 20200720133427.454400-2-vaibhavgupta40@gmail.com (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
| Series | scsi: use generic power management | expand |
On Mon, Jul 20, 2020 at 07:04:14PM +0530, Vaibhav Gupta wrote: > With legacy PM hooks, it was the responsibility of a driver to manage PCI > states and also the device's power state. The generic approach is to let > the PCI core handle the work. > > PCI core passes "struct device*" as an argument to the .suspend() and > .resume() callbacks. As the .suspend() work with "struct instance*", > extract it from "struct device*" using dev_get_drv_data(). > > Driver was also using PCI helper functions like pci_save/restore_state(), > pci_disable/enable_device(), pci_set_power_state() and pci_enable_wake(). > They should not be invoked by the driver. > > Compile-tested only. > > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com> > --- > drivers/scsi/megaraid/megaraid_sas_base.c | 61 ++++++----------------- > 1 file changed, 16 insertions(+), 45 deletions(-) > > diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c > index 00668335c2af..4a6ee7778977 100644 > --- a/drivers/scsi/megaraid/megaraid_sas_base.c > +++ b/drivers/scsi/megaraid/megaraid_sas_base.c > @@ -7539,25 +7539,21 @@ static void megasas_shutdown_controller(struct megasas_instance *instance, > megasas_return_cmd(instance, cmd); > } > > -#ifdef CONFIG_PM > /** > * megasas_suspend - driver suspend entry point > - * @pdev: PCI device structure > - * @state: PCI power state to suspend routine > + * @dev: Device structure > */ > -static int > -megasas_suspend(struct pci_dev *pdev, pm_message_t state) > +static int __maybe_unused > +megasas_suspend(struct device *dev) > { > - struct megasas_instance *instance; > - > - instance = pci_get_drvdata(pdev); > + struct megasas_instance *instance = dev_get_drvdata(dev); > > if (!instance) > return 0; > > instance->unload = 1; > > - dev_info(&pdev->dev, "%s is called\n", __func__); > + dev_info(dev, "%s is called\n", __func__); > > /* Shutdown SR-IOV heartbeat timer */ > if (instance->requestorId && !instance->skip_heartbeat_timer_del) > @@ -7579,7 +7575,7 @@ megasas_suspend(struct pci_dev *pdev, pm_message_t state) > > tasklet_kill(&instance->isr_tasklet); > > - pci_set_drvdata(instance->pdev, instance); > + dev_set_drvdata(dev, instance); > instance->instancet->disable_intr(instance); > > megasas_destroy_irqs(instance); > @@ -7587,48 +7583,28 @@ megasas_suspend(struct pci_dev *pdev, pm_message_t state) > if (instance->msix_vectors) > pci_free_irq_vectors(instance->pdev); > > - pci_save_state(pdev); > - pci_disable_device(pdev); > - > - pci_set_power_state(pdev, pci_choose_state(pdev, state)); > - > return 0; > } > > /** > * megasas_resume- driver resume entry point > - * @pdev: PCI device structure > + * @dev: Device structure > */ > -static int > -megasas_resume(struct pci_dev *pdev) > +static int __maybe_unused > +megasas_resume(struct device *dev) > { > int rval; > struct Scsi_Host *host; > - struct megasas_instance *instance; > + struct megasas_instance *instance = dev_get_drvdata(dev); > u32 status_reg; > > - instance = pci_get_drvdata(pdev); > - > if (!instance) > return 0; > > host = instance->host; > - pci_set_power_state(pdev, PCI_D0); > - pci_enable_wake(pdev, PCI_D0, 0); > - pci_restore_state(pdev); > + device_wakeup_disable(dev); > > - dev_info(&pdev->dev, "%s is called\n", __func__); > - /* > - * PCI prepping: enable device set bus mastering and dma mask > - */ > - rval = pci_enable_device_mem(pdev); > - > - if (rval) { > - dev_err(&pdev->dev, "Enable device failed\n"); > - return rval; > - } > - > - pci_set_master(pdev); > + dev_info(dev, "%s is called\n", __func__); > > /* > * We expect the FW state to be READY > @@ -7754,14 +7730,8 @@ megasas_resume(struct pci_dev *pdev) > fail_set_dma_mask: > fail_ready_state: > > - pci_disable_device(pdev); > - > return -ENODEV; > } > -#else > -#define megasas_suspend NULL > -#define megasas_resume NULL > -#endif > > static inline int > megasas_wait_for_adapter_operational(struct megasas_instance *instance) > @@ -7931,7 +7901,7 @@ static void megasas_detach_one(struct pci_dev *pdev) > > /** > * megasas_shutdown - Shutdown entry point > - * @device: Generic device structure > + * @pdev: PCI device structure > */ > static void megasas_shutdown(struct pci_dev *pdev) > { > @@ -8508,6 +8478,8 @@ static const struct file_operations megasas_mgmt_fops = { > .llseek = noop_llseek, > }; > > +static SIMPLE_DEV_PM_OPS(megasas_pm_ops, megasas_suspend, megasas_resume); > + > /* > * PCI hotplug support registration structure > */ > @@ -8517,8 +8489,7 @@ static struct pci_driver megasas_pci_driver = { > .id_table = megasas_pci_table, > .probe = megasas_probe_one, > .remove = megasas_detach_one, > - .suspend = megasas_suspend, > - .resume = megasas_resume, > + .driver.pm = &megasas_pm_ops, > .shutdown = megasas_shutdown, > }; > > -- > 2.27.0 > .
On Mon, Jul 20, 2020 at 07:04:14PM +0530, Vaibhav Gupta wrote: > With legacy PM hooks, it was the responsibility of a driver to manage PCI > states and also the device's power state. The generic approach is to let > the PCI core handle the work. > > PCI core passes "struct device*" as an argument to the .suspend() and > .resume() callbacks. As the .suspend() work with "struct instance*", > extract it from "struct device*" using dev_get_drv_data(). > > Driver was also using PCI helper functions like pci_save/restore_state(), > pci_disable/enable_device(), pci_set_power_state() and pci_enable_wake(). > They should not be invoked by the driver. > > Compile-tested only. > > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com> > --- > drivers/scsi/megaraid/megaraid_sas_base.c | 61 ++++++----------------- > 1 file changed, 16 insertions(+), 45 deletions(-) > > diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c > index 00668335c2af..4a6ee7778977 100644 > --- a/drivers/scsi/megaraid/megaraid_sas_base.c > +++ b/drivers/scsi/megaraid/megaraid_sas_base.c > @@ -7539,25 +7539,21 @@ static void megasas_shutdown_controller(struct megasas_instance *instance, > megasas_return_cmd(instance, cmd); > } > > -#ifdef CONFIG_PM > /** > * megasas_suspend - driver suspend entry point > - * @pdev: PCI device structure > - * @state: PCI power state to suspend routine > + * @dev: Device structure > */ > -static int > -megasas_suspend(struct pci_dev *pdev, pm_message_t state) > +static int __maybe_unused > +megasas_suspend(struct device *dev) > { > - struct megasas_instance *instance; > - > - instance = pci_get_drvdata(pdev); > + struct megasas_instance *instance = dev_get_drvdata(dev); > > if (!instance) > return 0; > > instance->unload = 1; > > - dev_info(&pdev->dev, "%s is called\n", __func__); > + dev_info(dev, "%s is called\n", __func__); > > /* Shutdown SR-IOV heartbeat timer */ > if (instance->requestorId && !instance->skip_heartbeat_timer_del) > @@ -7579,7 +7575,7 @@ megasas_suspend(struct pci_dev *pdev, pm_message_t state) > > tasklet_kill(&instance->isr_tasklet); > > - pci_set_drvdata(instance->pdev, instance); > + dev_set_drvdata(dev, instance); It *might* be correct to replace "instance->pdev" with "dev", but it's not obvious and deserves some explanation. It's true that you can replace &pdev->dev with dev, but I don't know anything about instance->dev. I don't think this change is actually necessary, is it? "instance->pdev" is still a pci_dev pointer, so pci_set_drvdata() should work fine. It looks goofy to use pci_set_drvdata() or dev_set_drvdata() in a suspend routine, but I didn't bother trying to figure out what's going on here. > instance->instancet->disable_intr(instance); > > megasas_destroy_irqs(instance); > @@ -7587,48 +7583,28 @@ megasas_suspend(struct pci_dev *pdev, pm_message_t state) > if (instance->msix_vectors) > pci_free_irq_vectors(instance->pdev); > > - pci_save_state(pdev); > - pci_disable_device(pdev); > - > - pci_set_power_state(pdev, pci_choose_state(pdev, state)); > - > return 0; > } > > /** > * megasas_resume- driver resume entry point > - * @pdev: PCI device structure > + * @dev: Device structure > */ > -static int > -megasas_resume(struct pci_dev *pdev) > +static int __maybe_unused > +megasas_resume(struct device *dev) > { > int rval; > struct Scsi_Host *host; > - struct megasas_instance *instance; > + struct megasas_instance *instance = dev_get_drvdata(dev); > u32 status_reg; > > - instance = pci_get_drvdata(pdev); > - > if (!instance) > return 0; > > host = instance->host; > - pci_set_power_state(pdev, PCI_D0); > - pci_enable_wake(pdev, PCI_D0, 0); > - pci_restore_state(pdev); > + device_wakeup_disable(dev); Shouldn't there be a corresponding device_wakeup_enable() or similar elsewhere? Maybe the fact that megasas disables wakeup but never enables it is a latent bug? > - dev_info(&pdev->dev, "%s is called\n", __func__); > - /* > - * PCI prepping: enable device set bus mastering and dma mask > - */ > - rval = pci_enable_device_mem(pdev); > - > - if (rval) { > - dev_err(&pdev->dev, "Enable device failed\n"); > - return rval; > - } > - > - pci_set_master(pdev); > + dev_info(dev, "%s is called\n", __func__); > > /* > * We expect the FW state to be READY > @@ -7754,14 +7730,8 @@ megasas_resume(struct pci_dev *pdev) > fail_set_dma_mask: > fail_ready_state: > > - pci_disable_device(pdev); > - > return -ENODEV; > } > -#else > -#define megasas_suspend NULL > -#define megasas_resume NULL > -#endif > > static inline int > megasas_wait_for_adapter_operational(struct megasas_instance *instance) > @@ -7931,7 +7901,7 @@ static void megasas_detach_one(struct pci_dev *pdev) > > /** > * megasas_shutdown - Shutdown entry point > - * @device: Generic device structure > + * @pdev: PCI device structure Looks like an unrelated typo fix? I would put this in a separate patch. > */ > static void megasas_shutdown(struct pci_dev *pdev) > { > @@ -8508,6 +8478,8 @@ static const struct file_operations megasas_mgmt_fops = { > .llseek = noop_llseek, > }; > > +static SIMPLE_DEV_PM_OPS(megasas_pm_ops, megasas_suspend, megasas_resume); > + > /* > * PCI hotplug support registration structure > */ > @@ -8517,8 +8489,7 @@ static struct pci_driver megasas_pci_driver = { > .id_table = megasas_pci_table, > .probe = megasas_probe_one, > .remove = megasas_detach_one, > - .suspend = megasas_suspend, > - .resume = megasas_resume, > + .driver.pm = &megasas_pm_ops, > .shutdown = megasas_shutdown, > }; > > -- > 2.27.0 >
On Tue, Sep 08, 2020 at 12:32:09PM -0500, Bjorn Helgaas wrote: > On Mon, Jul 20, 2020 at 07:04:14PM +0530, Vaibhav Gupta wrote: > > With legacy PM hooks, it was the responsibility of a driver to manage PCI > > states and also the device's power state. The generic approach is to let > > the PCI core handle the work. > > > > PCI core passes "struct device*" as an argument to the .suspend() and > > .resume() callbacks. As the .suspend() work with "struct instance*", > > extract it from "struct device*" using dev_get_drv_data(). > > > > Driver was also using PCI helper functions like pci_save/restore_state(), > > pci_disable/enable_device(), pci_set_power_state() and pci_enable_wake(). > > They should not be invoked by the driver. > > > > Compile-tested only. > > > > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com> > > --- > > drivers/scsi/megaraid/megaraid_sas_base.c | 61 ++++++----------------- > > 1 file changed, 16 insertions(+), 45 deletions(-) > > > > diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c > > index 00668335c2af..4a6ee7778977 100644 > > --- a/drivers/scsi/megaraid/megaraid_sas_base.c > > +++ b/drivers/scsi/megaraid/megaraid_sas_base.c > > @@ -7539,25 +7539,21 @@ static void megasas_shutdown_controller(struct megasas_instance *instance, > > megasas_return_cmd(instance, cmd); > > } > > > > -#ifdef CONFIG_PM > > /** > > * megasas_suspend - driver suspend entry point > > - * @pdev: PCI device structure > > - * @state: PCI power state to suspend routine > > + * @dev: Device structure > > */ > > -static int > > -megasas_suspend(struct pci_dev *pdev, pm_message_t state) > > +static int __maybe_unused > > +megasas_suspend(struct device *dev) > > { > > - struct megasas_instance *instance; > > - > > - instance = pci_get_drvdata(pdev); > > + struct megasas_instance *instance = dev_get_drvdata(dev); > > > > if (!instance) > > return 0; > > > > instance->unload = 1; > > > > - dev_info(&pdev->dev, "%s is called\n", __func__); > > + dev_info(dev, "%s is called\n", __func__); > > > > /* Shutdown SR-IOV heartbeat timer */ > > if (instance->requestorId && !instance->skip_heartbeat_timer_del) > > @@ -7579,7 +7575,7 @@ megasas_suspend(struct pci_dev *pdev, pm_message_t state) > > > > tasklet_kill(&instance->isr_tasklet); > > > > - pci_set_drvdata(instance->pdev, instance); > > + dev_set_drvdata(dev, instance); > > It *might* be correct to replace "instance->pdev" with "dev", but it's > not obvious and deserves some explanation. It's true that you can > replace &pdev->dev with dev, but I don't know anything about > instance->dev. > > I don't think this change is actually necessary, is it? > "instance->pdev" is still a pci_dev pointer, so pci_set_drvdata() > should work fine. > > It looks goofy to use pci_set_drvdata() or dev_set_drvdata() in a > suspend routine, but I didn't bother trying to figure out what's going > on here. > There is no instance->dev . The 'dev' passed dev_set_drvdata() is same &pdev->dev. The dev pointer used here, points to same value. pci_get_drvdata() and pci_set_drvdata() invoke dev_get_drvdata() and dev_set_drvdata() respectively. And they do nothing else. Seems like additional unnecessary function calls and operations. We can get rid of these PCI helper functions too. > > instance->instancet->disable_intr(instance); > > > > megasas_destroy_irqs(instance); > > @@ -7587,48 +7583,28 @@ megasas_suspend(struct pci_dev *pdev, pm_message_t state) > > if (instance->msix_vectors) > > pci_free_irq_vectors(instance->pdev); > > > > - pci_save_state(pdev); > > - pci_disable_device(pdev); > > - > > - pci_set_power_state(pdev, pci_choose_state(pdev, state)); > > - > > return 0; > > } > > > > /** > > * megasas_resume- driver resume entry point > > - * @pdev: PCI device structure > > + * @dev: Device structure > > */ > > -static int > > -megasas_resume(struct pci_dev *pdev) > > +static int __maybe_unused > > +megasas_resume(struct device *dev) > > { > > int rval; > > struct Scsi_Host *host; > > - struct megasas_instance *instance; > > + struct megasas_instance *instance = dev_get_drvdata(dev); > > u32 status_reg; > > > > - instance = pci_get_drvdata(pdev); > > - > > if (!instance) > > return 0; > > > > host = instance->host; > > - pci_set_power_state(pdev, PCI_D0); > > - pci_enable_wake(pdev, PCI_D0, 0); > > - pci_restore_state(pdev); > > + device_wakeup_disable(dev); > > Shouldn't there be a corresponding device_wakeup_enable() or similar > elsewhere? > > Maybe the fact that megasas disables wakeup but never enables it is a > latent bug? > Yeah, I guess I sent this patch a lot earlier than we figured this out and didn't notice it later. I will send v3 for it. > > - dev_info(&pdev->dev, "%s is called\n", __func__); > > - /* > > - * PCI prepping: enable device set bus mastering and dma mask > > - */ > > - rval = pci_enable_device_mem(pdev); > > - > > - if (rval) { > > - dev_err(&pdev->dev, "Enable device failed\n"); > > - return rval; > > - } > > - > > - pci_set_master(pdev); > > + dev_info(dev, "%s is called\n", __func__); > > > > /* > > * We expect the FW state to be READY > > @@ -7754,14 +7730,8 @@ megasas_resume(struct pci_dev *pdev) > > fail_set_dma_mask: > > fail_ready_state: > > > > - pci_disable_device(pdev); > > - > > return -ENODEV; > > } > > -#else > > -#define megasas_suspend NULL > > -#define megasas_resume NULL > > -#endif > > > > static inline int > > megasas_wait_for_adapter_operational(struct megasas_instance *instance) > > @@ -7931,7 +7901,7 @@ static void megasas_detach_one(struct pci_dev *pdev) > > > > /** > > * megasas_shutdown - Shutdown entry point > > - * @device: Generic device structure > > + * @pdev: PCI device structure > > Looks like an unrelated typo fix? I would put this in a separate > patch. > Okay. > > */ > > static void megasas_shutdown(struct pci_dev *pdev) > > { > > @@ -8508,6 +8478,8 @@ static const struct file_operations megasas_mgmt_fops = { > > .llseek = noop_llseek, > > }; > > > > +static SIMPLE_DEV_PM_OPS(megasas_pm_ops, megasas_suspend, megasas_resume); > > + > > /* > > * PCI hotplug support registration structure > > */ > > @@ -8517,8 +8489,7 @@ static struct pci_driver megasas_pci_driver = { > > .id_table = megasas_pci_table, > > .probe = megasas_probe_one, > > .remove = megasas_detach_one, > > - .suspend = megasas_suspend, > > - .resume = megasas_resume, > > + .driver.pm = &megasas_pm_ops, > > .shutdown = megasas_shutdown, > > }; > > > > -- > > 2.27.0 > >
On Wed, Sep 09, 2020 at 03:33:15PM +0530, Vaibhav Gupta wrote: > On Tue, Sep 08, 2020 at 12:32:09PM -0500, Bjorn Helgaas wrote: > > On Mon, Jul 20, 2020 at 07:04:14PM +0530, Vaibhav Gupta wrote: > > > With legacy PM hooks, it was the responsibility of a driver to manage PCI > > > states and also the device's power state. The generic approach is to let > > > the PCI core handle the work. > > > > > > PCI core passes "struct device*" as an argument to the .suspend() and > > > .resume() callbacks. As the .suspend() work with "struct instance*", > > > extract it from "struct device*" using dev_get_drv_data(). > > > > > > Driver was also using PCI helper functions like pci_save/restore_state(), > > > pci_disable/enable_device(), pci_set_power_state() and pci_enable_wake(). > > > They should not be invoked by the driver. > > > > > > Compile-tested only. > > > > > > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com> > > > --- > > > drivers/scsi/megaraid/megaraid_sas_base.c | 61 ++++++----------------- > > > 1 file changed, 16 insertions(+), 45 deletions(-) > > > > > > diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c > > > index 00668335c2af..4a6ee7778977 100644 > > > --- a/drivers/scsi/megaraid/megaraid_sas_base.c > > > +++ b/drivers/scsi/megaraid/megaraid_sas_base.c > > > @@ -7539,25 +7539,21 @@ static void megasas_shutdown_controller(struct megasas_instance *instance, > > > megasas_return_cmd(instance, cmd); > > > } > > > > > > -#ifdef CONFIG_PM > > > /** > > > * megasas_suspend - driver suspend entry point > > > - * @pdev: PCI device structure > > > - * @state: PCI power state to suspend routine > > > + * @dev: Device structure > > > */ > > > -static int > > > -megasas_suspend(struct pci_dev *pdev, pm_message_t state) > > > +static int __maybe_unused > > > +megasas_suspend(struct device *dev) > > > { > > > - struct megasas_instance *instance; > > > - > > > - instance = pci_get_drvdata(pdev); > > > + struct megasas_instance *instance = dev_get_drvdata(dev); > > > > > > if (!instance) > > > return 0; > > > > > > instance->unload = 1; > > > > > > - dev_info(&pdev->dev, "%s is called\n", __func__); > > > + dev_info(dev, "%s is called\n", __func__); > > > > > > /* Shutdown SR-IOV heartbeat timer */ > > > if (instance->requestorId && !instance->skip_heartbeat_timer_del) > > > @@ -7579,7 +7575,7 @@ megasas_suspend(struct pci_dev *pdev, pm_message_t state) > > > > > > tasklet_kill(&instance->isr_tasklet); > > > > > > - pci_set_drvdata(instance->pdev, instance); > > > + dev_set_drvdata(dev, instance); > > > > It *might* be correct to replace "instance->pdev" with "dev", but it's > > not obvious and deserves some explanation. It's true that you can > > replace &pdev->dev with dev, but I don't know anything about > > instance->dev. Sorry, I meant "instance->pdev" here. > > I don't think this change is actually necessary, is it? > > "instance->pdev" is still a pci_dev pointer, so pci_set_drvdata() > > should work fine. ... > > > There is no instance->dev . The 'dev' passed dev_set_drvdata() is > same &pdev->dev. Yes, it's true that "dev" here is the same as the "&pdev->dev" we had previously. But we passed "instance->pdev" (not "pdev") to pci_set_drvdata(). So the question is whether instance->pdev->dev == dev. They *might* be the same, but I don't think it's obvious. > The dev pointer used here, points to same value. > > pci_get_drvdata() and pci_set_drvdata() invoke dev_get_drvdata() and > dev_set_drvdata() respectively. And they do nothing else. Seems like > additional unnecessary function calls and operations.
On Wed, Sep 09, 2020 at 08:23:32AM -0500, Bjorn Helgaas wrote: > On Wed, Sep 09, 2020 at 03:33:15PM +0530, Vaibhav Gupta wrote: > > On Tue, Sep 08, 2020 at 12:32:09PM -0500, Bjorn Helgaas wrote: > > > On Mon, Jul 20, 2020 at 07:04:14PM +0530, Vaibhav Gupta wrote: > > > > With legacy PM hooks, it was the responsibility of a driver to manage PCI > > > > states and also the device's power state. The generic approach is to let > > > > the PCI core handle the work. > > > > > > > > PCI core passes "struct device*" as an argument to the .suspend() and > > > > .resume() callbacks. As the .suspend() work with "struct instance*", > > > > extract it from "struct device*" using dev_get_drv_data(). > > > > > > > > Driver was also using PCI helper functions like pci_save/restore_state(), > > > > pci_disable/enable_device(), pci_set_power_state() and pci_enable_wake(). > > > > They should not be invoked by the driver. > > > > > > > > Compile-tested only. > > > > > > > > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com> > > > > --- > > > > drivers/scsi/megaraid/megaraid_sas_base.c | 61 ++++++----------------- > > > > 1 file changed, 16 insertions(+), 45 deletions(-) > > > > > > > > diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c > > > > index 00668335c2af..4a6ee7778977 100644 > > > > --- a/drivers/scsi/megaraid/megaraid_sas_base.c > > > > +++ b/drivers/scsi/megaraid/megaraid_sas_base.c > > > > @@ -7539,25 +7539,21 @@ static void megasas_shutdown_controller(struct megasas_instance *instance, > > > > megasas_return_cmd(instance, cmd); > > > > } > > > > > > > > -#ifdef CONFIG_PM > > > > /** > > > > * megasas_suspend - driver suspend entry point > > > > - * @pdev: PCI device structure > > > > - * @state: PCI power state to suspend routine > > > > + * @dev: Device structure > > > > */ > > > > -static int > > > > -megasas_suspend(struct pci_dev *pdev, pm_message_t state) > > > > +static int __maybe_unused > > > > +megasas_suspend(struct device *dev) > > > > { > > > > - struct megasas_instance *instance; > > > > - > > > > - instance = pci_get_drvdata(pdev); > > > > + struct megasas_instance *instance = dev_get_drvdata(dev); > > > > > > > > if (!instance) > > > > return 0; > > > > > > > > instance->unload = 1; > > > > > > > > - dev_info(&pdev->dev, "%s is called\n", __func__); > > > > + dev_info(dev, "%s is called\n", __func__); > > > > > > > > /* Shutdown SR-IOV heartbeat timer */ > > > > if (instance->requestorId && !instance->skip_heartbeat_timer_del) > > > > @@ -7579,7 +7575,7 @@ megasas_suspend(struct pci_dev *pdev, pm_message_t state) > > > > > > > > tasklet_kill(&instance->isr_tasklet); > > > > > > > > - pci_set_drvdata(instance->pdev, instance); > > > > + dev_set_drvdata(dev, instance); > > > > > > It *might* be correct to replace "instance->pdev" with "dev", but it's > > > not obvious and deserves some explanation. It's true that you can > > > replace &pdev->dev with dev, but I don't know anything about > > > instance->dev. > > Sorry, I meant "instance->pdev" here. > > > > I don't think this change is actually necessary, is it? > > > "instance->pdev" is still a pci_dev pointer, so pci_set_drvdata() > > > should work fine. ... > > > > > There is no instance->dev . The 'dev' passed dev_set_drvdata() is > > same &pdev->dev. > > Yes, it's true that "dev" here is the same as the "&pdev->dev" we had > previously. But we passed "instance->pdev" (not "pdev") to > pci_set_drvdata(). So the question is whether instance->pdev->dev == > dev. > > They *might* be the same, but I don't think it's obvious. > Yes, they are same. driver/pci/pci-driver.c : 'dev' is passed as parameter to both pci_device_probe() and pci_pm_suspend() From 'dev', pci_device_probe() extracts "struct pci_dev*" and passes it to the probe callback of this driver. In the proble function - megasas_probe_one() :7347 instance->pdev = pdev; :7386 pci_set_drvdata(pdev, instance); The proble function is using "struct pci_dev*" variable "pdev" provided by core and same we replaced &pdev->dev with "struct device *dev". So the instance->pdev->dev and 'dev' can only differ if 'dev' passed to pci_device_probe() and pci_pm_suspend() are different. > > The dev pointer used here, points to same value. > > > > pci_get_drvdata() and pci_set_drvdata() invoke dev_get_drvdata() and > > dev_set_drvdata() respectively. And they do nothing else. Seems like > > additional unnecessary function calls and operations.
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c index 00668335c2af..4a6ee7778977 100644 --- a/drivers/scsi/megaraid/megaraid_sas_base.c +++ b/drivers/scsi/megaraid/megaraid_sas_base.c @@ -7539,25 +7539,21 @@ static void megasas_shutdown_controller(struct megasas_instance *instance, megasas_return_cmd(instance, cmd); } -#ifdef CONFIG_PM /** * megasas_suspend - driver suspend entry point - * @pdev: PCI device structure - * @state: PCI power state to suspend routine + * @dev: Device structure */ -static int -megasas_suspend(struct pci_dev *pdev, pm_message_t state) +static int __maybe_unused +megasas_suspend(struct device *dev) { - struct megasas_instance *instance; - - instance = pci_get_drvdata(pdev); + struct megasas_instance *instance = dev_get_drvdata(dev); if (!instance) return 0; instance->unload = 1; - dev_info(&pdev->dev, "%s is called\n", __func__); + dev_info(dev, "%s is called\n", __func__); /* Shutdown SR-IOV heartbeat timer */ if (instance->requestorId && !instance->skip_heartbeat_timer_del) @@ -7579,7 +7575,7 @@ megasas_suspend(struct pci_dev *pdev, pm_message_t state) tasklet_kill(&instance->isr_tasklet); - pci_set_drvdata(instance->pdev, instance); + dev_set_drvdata(dev, instance); instance->instancet->disable_intr(instance); megasas_destroy_irqs(instance); @@ -7587,48 +7583,28 @@ megasas_suspend(struct pci_dev *pdev, pm_message_t state) if (instance->msix_vectors) pci_free_irq_vectors(instance->pdev); - pci_save_state(pdev); - pci_disable_device(pdev); - - pci_set_power_state(pdev, pci_choose_state(pdev, state)); - return 0; } /** * megasas_resume- driver resume entry point - * @pdev: PCI device structure + * @dev: Device structure */ -static int -megasas_resume(struct pci_dev *pdev) +static int __maybe_unused +megasas_resume(struct device *dev) { int rval; struct Scsi_Host *host; - struct megasas_instance *instance; + struct megasas_instance *instance = dev_get_drvdata(dev); u32 status_reg; - instance = pci_get_drvdata(pdev); - if (!instance) return 0; host = instance->host; - pci_set_power_state(pdev, PCI_D0); - pci_enable_wake(pdev, PCI_D0, 0); - pci_restore_state(pdev); + device_wakeup_disable(dev); - dev_info(&pdev->dev, "%s is called\n", __func__); - /* - * PCI prepping: enable device set bus mastering and dma mask - */ - rval = pci_enable_device_mem(pdev); - - if (rval) { - dev_err(&pdev->dev, "Enable device failed\n"); - return rval; - } - - pci_set_master(pdev); + dev_info(dev, "%s is called\n", __func__); /* * We expect the FW state to be READY @@ -7754,14 +7730,8 @@ megasas_resume(struct pci_dev *pdev) fail_set_dma_mask: fail_ready_state: - pci_disable_device(pdev); - return -ENODEV; } -#else -#define megasas_suspend NULL -#define megasas_resume NULL -#endif static inline int megasas_wait_for_adapter_operational(struct megasas_instance *instance) @@ -7931,7 +7901,7 @@ static void megasas_detach_one(struct pci_dev *pdev) /** * megasas_shutdown - Shutdown entry point - * @device: Generic device structure + * @pdev: PCI device structure */ static void megasas_shutdown(struct pci_dev *pdev) { @@ -8508,6 +8478,8 @@ static const struct file_operations megasas_mgmt_fops = { .llseek = noop_llseek, }; +static SIMPLE_DEV_PM_OPS(megasas_pm_ops, megasas_suspend, megasas_resume); + /* * PCI hotplug support registration structure */ @@ -8517,8 +8489,7 @@ static struct pci_driver megasas_pci_driver = { .id_table = megasas_pci_table, .probe = megasas_probe_one, .remove = megasas_detach_one, - .suspend = megasas_suspend, - .resume = megasas_resume, + .driver.pm = &megasas_pm_ops, .shutdown = megasas_shutdown, };
With legacy PM hooks, it was the responsibility of a driver to manage PCI states and also the device's power state. The generic approach is to let the PCI core handle the work. PCI core passes "struct device*" as an argument to the .suspend() and .resume() callbacks. As the .suspend() work with "struct instance*", extract it from "struct device*" using dev_get_drv_data(). Driver was also using PCI helper functions like pci_save/restore_state(), pci_disable/enable_device(), pci_set_power_state() and pci_enable_wake(). They should not be invoked by the driver. Compile-tested only. Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com> --- drivers/scsi/megaraid/megaraid_sas_base.c | 61 ++++++----------------- 1 file changed, 16 insertions(+), 45 deletions(-)