* [Linux-kernel-mentees] [PATCH v1 1/2] realtek/8139too: Remove Legacy Power Management @ 2020-04-23 13:27 Vaibhav Gupta 2020-04-23 13:28 ` [Linux-kernel-mentees] [PATCH v1 2/2] realtek/8139cp: " Vaibhav Gupta 2020-04-24 3:43 ` [Linux-kernel-mentees] [PATCH v1 1/2] realtek/8139too: " Bjorn Helgaas 0 siblings, 2 replies; 12+ messages in thread From: Vaibhav Gupta @ 2020-04-23 13:27 UTC (permalink / raw) To: linux-kernel-mentees, bjorn, skhan, rjw; +Cc: Vaibhav Gupta Upgrade Power Management from legacy to generic using dev_pm_ops. Remove struct pci_driver.suspend and struct pci_driver.resume bindings, and add struct pci_driver.driver.pm . Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com> --- drivers/net/ethernet/realtek/8139too.c | 26 +++++++------------------- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/drivers/net/ethernet/realtek/8139too.c b/drivers/net/ethernet/realtek/8139too.c index 5caeb8368eab..b7c98b165256 100644 --- a/drivers/net/ethernet/realtek/8139too.c +++ b/drivers/net/ethernet/realtek/8139too.c @@ -2603,17 +2603,13 @@ static void rtl8139_set_rx_mode (struct net_device *dev) spin_unlock_irqrestore (&tp->lock, flags); } -#ifdef CONFIG_PM - -static int rtl8139_suspend (struct pci_dev *pdev, pm_message_t state) +static int rtl8139_suspend(struct device *device) { - struct net_device *dev = pci_get_drvdata (pdev); + struct net_device *dev = dev_get_drvdata(device); struct rtl8139_private *tp = netdev_priv(dev); void __iomem *ioaddr = tp->mmio_addr; unsigned long flags; - pci_save_state (pdev); - if (!netif_running (dev)) return 0; @@ -2631,38 +2627,30 @@ static int rtl8139_suspend (struct pci_dev *pdev, pm_message_t state) spin_unlock_irqrestore (&tp->lock, flags); - pci_set_power_state (pdev, PCI_D3hot); - return 0; } - -static int rtl8139_resume (struct pci_dev *pdev) +static int rtl8139_resume(struct device *device) { - struct net_device *dev = pci_get_drvdata (pdev); + struct net_device *dev = dev_get_drvdata(device); - pci_restore_state (pdev); if (!netif_running (dev)) return 0; - pci_set_power_state (pdev, PCI_D0); + rtl8139_init_ring (dev); rtl8139_hw_start (dev); netif_device_attach (dev); return 0; } -#endif /* CONFIG_PM */ - +static SIMPLE_DEV_PM_OPS(rtl8139_pm_ops, rtl8139_suspend, rtl8139_resume); static struct pci_driver rtl8139_pci_driver = { .name = DRV_NAME, .id_table = rtl8139_pci_tbl, .probe = rtl8139_init_one, .remove = rtl8139_remove_one, -#ifdef CONFIG_PM - .suspend = rtl8139_suspend, - .resume = rtl8139_resume, -#endif /* CONFIG_PM */ + .driver.pm = &rtl8139_pm_ops, }; -- 2.26.2 _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Linux-kernel-mentees] [PATCH v1 2/2] realtek/8139cp: Remove Legacy Power Management 2020-04-23 13:27 [Linux-kernel-mentees] [PATCH v1 1/2] realtek/8139too: Remove Legacy Power Management Vaibhav Gupta @ 2020-04-23 13:28 ` Vaibhav Gupta 2020-04-24 3:47 ` Bjorn Helgaas 2020-04-24 3:43 ` [Linux-kernel-mentees] [PATCH v1 1/2] realtek/8139too: " Bjorn Helgaas 1 sibling, 1 reply; 12+ messages in thread From: Vaibhav Gupta @ 2020-04-23 13:28 UTC (permalink / raw) To: linux-kernel-mentees, bjorn, skhan, rjw; +Cc: Vaibhav Gupta Upgrade Power Management from legacy to generic using dev_pm_ops. Remove pci_save_sate(), pci_set_state(), etc. No need of directive '#ifdef CONFIG_PM' as, if CONFIG_PM is not defined, the binding will be by deafult set to NULL. Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com> --- drivers/net/ethernet/realtek/8139cp.c | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c index 60d342f82fb3..e2e79cf4e1fa 100644 --- a/drivers/net/ethernet/realtek/8139cp.c +++ b/drivers/net/ethernet/realtek/8139cp.c @@ -2054,10 +2054,9 @@ static void cp_remove_one (struct pci_dev *pdev) free_netdev(dev); } -#ifdef CONFIG_PM -static int cp_suspend (struct pci_dev *pdev, pm_message_t state) +static int cp_suspend(struct device *device) { - struct net_device *dev = pci_get_drvdata(pdev); + struct net_device *dev = dev_get_drvdata(device); struct cp_private *cp = netdev_priv(dev); unsigned long flags; @@ -2075,16 +2074,12 @@ static int cp_suspend (struct pci_dev *pdev, pm_message_t state) spin_unlock_irqrestore (&cp->lock, flags); - pci_save_state(pdev); - pci_enable_wake(pdev, pci_choose_state(pdev, state), cp->wol_enabled); - pci_set_power_state(pdev, pci_choose_state(pdev, state)); - return 0; } -static int cp_resume (struct pci_dev *pdev) +static int cp_resume(struct device *device) { - struct net_device *dev = pci_get_drvdata (pdev); + struct net_device *dev = dev_get_drvdata(device); struct cp_private *cp = netdev_priv(dev); unsigned long flags; @@ -2093,10 +2088,6 @@ static int cp_resume (struct pci_dev *pdev) netif_device_attach (dev); - pci_set_power_state(pdev, PCI_D0); - pci_restore_state(pdev); - pci_enable_wake(pdev, PCI_D0, 0); - /* FIXME: sh*t may happen if the Rx ring buffer is depleted */ cp_init_rings_index (cp); cp_init_hw (cp); @@ -2111,7 +2102,6 @@ static int cp_resume (struct pci_dev *pdev) return 0; } -#endif /* CONFIG_PM */ static const struct pci_device_id cp_pci_tbl[] = { { PCI_DEVICE(PCI_VENDOR_ID_REALTEK, PCI_DEVICE_ID_REALTEK_8139), }, @@ -2120,15 +2110,14 @@ static const struct pci_device_id cp_pci_tbl[] = { }; MODULE_DEVICE_TABLE(pci, cp_pci_tbl); +static SIMPLE_DEV_PM_OPS(cp_pm_ops, cp_suspend, cp_resume); + static struct pci_driver cp_driver = { .name = DRV_NAME, .id_table = cp_pci_tbl, .probe = cp_init_one, .remove = cp_remove_one, -#ifdef CONFIG_PM - .resume = cp_resume, - .suspend = cp_suspend, -#endif + .driver.pm = &cp_pm_ops, }; module_pci_driver(cp_driver); -- 2.26.2 _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1 2/2] realtek/8139cp: Remove Legacy Power Management 2020-04-23 13:28 ` [Linux-kernel-mentees] [PATCH v1 2/2] realtek/8139cp: " Vaibhav Gupta @ 2020-04-24 3:47 ` Bjorn Helgaas 2020-04-24 10:31 ` Vaibhav Gupta 0 siblings, 1 reply; 12+ messages in thread From: Bjorn Helgaas @ 2020-04-24 3:47 UTC (permalink / raw) To: Vaibhav Gupta; +Cc: rjw, linux-kernel-mentees On Thu, Apr 23, 2020 at 06:58:01PM +0530, Vaibhav Gupta wrote: > Upgrade Power Management from legacy to generic using dev_pm_ops. > > Remove pci_save_sate(), pci_set_state(), etc. No need of directive > '#ifdef CONFIG_PM' as, if CONFIG_PM is not defined, the binding > will be by deafult set to NULL. s/deafult/default/ I didn't quite follow this argument. SIMPLE_DEV_PM_OPS() is defined to SET_SYSTEM_SLEEP_PM_OPS(), and SET_SYSTEM_SLEEP_PM_OPS() is defined to nothing unless CONFIG_PM_SLEEP (not CONFIG_PM). But in any case, we don't want cp_suspend() and cp_resume() to be compiled at all if they're not referenced. So I think converting the "#ifdef CONFIG_PM" to "#ifdef CONFIG_PM_SLEEP" is the right thing. > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com> > --- > drivers/net/ethernet/realtek/8139cp.c | 25 +++++++------------------ > 1 file changed, 7 insertions(+), 18 deletions(-) > > diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c > index 60d342f82fb3..e2e79cf4e1fa 100644 > --- a/drivers/net/ethernet/realtek/8139cp.c > +++ b/drivers/net/ethernet/realtek/8139cp.c > @@ -2054,10 +2054,9 @@ static void cp_remove_one (struct pci_dev *pdev) > free_netdev(dev); > } > > -#ifdef CONFIG_PM > -static int cp_suspend (struct pci_dev *pdev, pm_message_t state) > +static int cp_suspend(struct device *device) > { > - struct net_device *dev = pci_get_drvdata(pdev); > + struct net_device *dev = dev_get_drvdata(device); > struct cp_private *cp = netdev_priv(dev); > unsigned long flags; > > @@ -2075,16 +2074,12 @@ static int cp_suspend (struct pci_dev *pdev, pm_message_t state) > > spin_unlock_irqrestore (&cp->lock, flags); > > - pci_save_state(pdev); > - pci_enable_wake(pdev, pci_choose_state(pdev, state), cp->wol_enabled); > - pci_set_power_state(pdev, pci_choose_state(pdev, state)); > - > return 0; > } > > -static int cp_resume (struct pci_dev *pdev) > +static int cp_resume(struct device *device) > { > - struct net_device *dev = pci_get_drvdata (pdev); > + struct net_device *dev = dev_get_drvdata(device); > struct cp_private *cp = netdev_priv(dev); > unsigned long flags; > > @@ -2093,10 +2088,6 @@ static int cp_resume (struct pci_dev *pdev) > > netif_device_attach (dev); > > - pci_set_power_state(pdev, PCI_D0); > - pci_restore_state(pdev); > - pci_enable_wake(pdev, PCI_D0, 0); > - > /* FIXME: sh*t may happen if the Rx ring buffer is depleted */ > cp_init_rings_index (cp); > cp_init_hw (cp); > @@ -2111,7 +2102,6 @@ static int cp_resume (struct pci_dev *pdev) > > return 0; > } > -#endif /* CONFIG_PM */ > > static const struct pci_device_id cp_pci_tbl[] = { > { PCI_DEVICE(PCI_VENDOR_ID_REALTEK, PCI_DEVICE_ID_REALTEK_8139), }, > @@ -2120,15 +2110,14 @@ static const struct pci_device_id cp_pci_tbl[] = { > }; > MODULE_DEVICE_TABLE(pci, cp_pci_tbl); > > +static SIMPLE_DEV_PM_OPS(cp_pm_ops, cp_suspend, cp_resume); > + > static struct pci_driver cp_driver = { > .name = DRV_NAME, > .id_table = cp_pci_tbl, > .probe = cp_init_one, > .remove = cp_remove_one, > -#ifdef CONFIG_PM > - .resume = cp_resume, > - .suspend = cp_suspend, > -#endif > + .driver.pm = &cp_pm_ops, > }; > > module_pci_driver(cp_driver); > -- > 2.26.2 > _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1 2/2] realtek/8139cp: Remove Legacy Power Management 2020-04-24 3:47 ` Bjorn Helgaas @ 2020-04-24 10:31 ` Vaibhav Gupta 2020-04-24 12:44 ` Bjorn Helgaas 0 siblings, 1 reply; 12+ messages in thread From: Vaibhav Gupta @ 2020-04-24 10:31 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: linux-kernel-mentees, Rafael J. Wysocki, Vaibhav Gupta On Fri, 24 Apr 2020 at 09:17, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Thu, Apr 23, 2020 at 06:58:01PM +0530, Vaibhav Gupta wrote: > > Upgrade Power Management from legacy to generic using dev_pm_ops. > > > > Remove pci_save_sate(), pci_set_state(), etc. No need of directive > > '#ifdef CONFIG_PM' as, if CONFIG_PM is not defined, the binding > > will be by deafult set to NULL. > > s/deafult/default/ > > I didn't quite follow this argument. SIMPLE_DEV_PM_OPS() is defined > to SET_SYSTEM_SLEEP_PM_OPS(), and SET_SYSTEM_SLEEP_PM_OPS() is defined > to nothing unless CONFIG_PM_SLEEP (not CONFIG_PM). yes, and we can find about CONFIG_PM_SLEEP inside kernel/power/kconfig It is defined as: config PM_SLEEP def_bool y depends on SUSPEND || HIBERNATE_CALLBACKS select PM select SRCU Hence it is selecting CONFIG_PM. So any of the macro check works.So i should go with #ifded CONFIG_PM or #ifdef CONFIG_PM_SLEEP ? > > But in any case, we don't want cp_suspend() and cp_resume() to be > compiled at all if they're not referenced. So I think converting the > "#ifdef CONFIG_PM" to "#ifdef CONFIG_PM_SLEEP" is the right thing. Actually I removed it because Andy also removed them in 226e6b866d74, thought it has to be done that way. --Vaibhav Gupta > > > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com> > > --- > > drivers/net/ethernet/realtek/8139cp.c | 25 +++++++------------------ > > 1 file changed, 7 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c > > index 60d342f82fb3..e2e79cf4e1fa 100644 > > --- a/drivers/net/ethernet/realtek/8139cp.c > > +++ b/drivers/net/ethernet/realtek/8139cp.c > > @@ -2054,10 +2054,9 @@ static void cp_remove_one (struct pci_dev *pdev) > > free_netdev(dev); > > } > > > > -#ifdef CONFIG_PM > > -static int cp_suspend (struct pci_dev *pdev, pm_message_t state) > > +static int cp_suspend(struct device *device) > > { > > - struct net_device *dev = pci_get_drvdata(pdev); > > + struct net_device *dev = dev_get_drvdata(device); > > struct cp_private *cp = netdev_priv(dev); > > unsigned long flags; > > > > @@ -2075,16 +2074,12 @@ static int cp_suspend (struct pci_dev *pdev, pm_message_t state) > > > > spin_unlock_irqrestore (&cp->lock, flags); > > > > - pci_save_state(pdev); > > - pci_enable_wake(pdev, pci_choose_state(pdev, state), cp->wol_enabled); > > - pci_set_power_state(pdev, pci_choose_state(pdev, state)); > > - > > return 0; > > } > > > > -static int cp_resume (struct pci_dev *pdev) > > +static int cp_resume(struct device *device) > > { > > - struct net_device *dev = pci_get_drvdata (pdev); > > + struct net_device *dev = dev_get_drvdata(device); > > struct cp_private *cp = netdev_priv(dev); > > unsigned long flags; > > > > @@ -2093,10 +2088,6 @@ static int cp_resume (struct pci_dev *pdev) > > > > netif_device_attach (dev); > > > > - pci_set_power_state(pdev, PCI_D0); > > - pci_restore_state(pdev); > > - pci_enable_wake(pdev, PCI_D0, 0); > > - > > /* FIXME: sh*t may happen if the Rx ring buffer is depleted */ > > cp_init_rings_index (cp); > > cp_init_hw (cp); > > @@ -2111,7 +2102,6 @@ static int cp_resume (struct pci_dev *pdev) > > > > return 0; > > } > > -#endif /* CONFIG_PM */ > > > > static const struct pci_device_id cp_pci_tbl[] = { > > { PCI_DEVICE(PCI_VENDOR_ID_REALTEK, PCI_DEVICE_ID_REALTEK_8139), }, > > @@ -2120,15 +2110,14 @@ static const struct pci_device_id cp_pci_tbl[] = { > > }; > > MODULE_DEVICE_TABLE(pci, cp_pci_tbl); > > > > +static SIMPLE_DEV_PM_OPS(cp_pm_ops, cp_suspend, cp_resume); > > + > > static struct pci_driver cp_driver = { > > .name = DRV_NAME, > > .id_table = cp_pci_tbl, > > .probe = cp_init_one, > > .remove = cp_remove_one, > > -#ifdef CONFIG_PM > > - .resume = cp_resume, > > - .suspend = cp_suspend, > > -#endif > > + .driver.pm = &cp_pm_ops, > > }; > > > > module_pci_driver(cp_driver); > > -- > > 2.26.2 > > > _______________________________________________ > Linux-kernel-mentees mailing list > Linux-kernel-mentees@lists.linuxfoundation.org > https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1 2/2] realtek/8139cp: Remove Legacy Power Management 2020-04-24 10:31 ` Vaibhav Gupta @ 2020-04-24 12:44 ` Bjorn Helgaas 2020-04-24 13:00 ` Vaibhav Gupta 0 siblings, 1 reply; 12+ messages in thread From: Bjorn Helgaas @ 2020-04-24 12:44 UTC (permalink / raw) To: Vaibhav Gupta; +Cc: linux-kernel-mentees, Rafael J. Wysocki, Vaibhav Gupta On Fri, Apr 24, 2020 at 04:01:43PM +0530, Vaibhav Gupta wrote: > On Fri, 24 Apr 2020 at 09:17, Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > On Thu, Apr 23, 2020 at 06:58:01PM +0530, Vaibhav Gupta wrote: > > > Upgrade Power Management from legacy to generic using dev_pm_ops. > > > > > > Remove pci_save_sate(), pci_set_state(), etc. No need of directive > > > '#ifdef CONFIG_PM' as, if CONFIG_PM is not defined, the binding > > > will be by deafult set to NULL. > > > > s/deafult/default/ > > > > I didn't quite follow this argument. SIMPLE_DEV_PM_OPS() is defined > > to SET_SYSTEM_SLEEP_PM_OPS(), and SET_SYSTEM_SLEEP_PM_OPS() is defined > > to nothing unless CONFIG_PM_SLEEP (not CONFIG_PM). > yes, and we can find about CONFIG_PM_SLEEP inside kernel/power/kconfig > It is defined as: > config PM_SLEEP > def_bool y > depends on SUSPEND || HIBERNATE_CALLBACKS > select PM > select SRCU > > Hence it is selecting CONFIG_PM. So any of the macro check works.So i > should go with > #ifded CONFIG_PM > or > #ifdef CONFIG_PM_SLEEP ? Well, the point was that it's possible to have CONFIG_PM=y but CONFIG_PM_SLEEP unset. In that case, using "#ifdef CONFIG_PM" will mean those functions will be compiled but not referenced. > > But in any case, we don't want cp_suspend() and cp_resume() to be > > compiled at all if they're not referenced. So I think converting the > > "#ifdef CONFIG_PM" to "#ifdef CONFIG_PM_SLEEP" is the right thing. > Actually I removed it because Andy also removed them in > 226e6b866d74, thought it has to be done that way. Hmm, yes, I see that. I wish we had one canonical way to do this. In 226e6b866d74 ("gpio: pch: Convert to dev_pm_ops"), I see that Andy also added "__maybe_unused", which I think turns off the compiler warning about the function being unused. I had been looking at recent additions of SIMPLE_DEV_PM_OPS() via $ git log -p drivers/ and searching with "/^\+.*SIMPLE_DEV_PM_OPS". There's a mix of using "#ifdef CONFIG_PM_SLEEP" and using "__maybe_unused", but it does look like "__maybe_unused" is winning. It leaves unusable code in the image but arguably looks a little prettier. So I guess I'm fine with either way. But I do think we need either "#ifdef CONFIG_PM_SLEEP" or "__maybe_unused" so we don't get compiler warnings. Bjorn _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1 2/2] realtek/8139cp: Remove Legacy Power Management 2020-04-24 12:44 ` Bjorn Helgaas @ 2020-04-24 13:00 ` Vaibhav Gupta 2020-04-24 13:59 ` Bjorn Helgaas 0 siblings, 1 reply; 12+ messages in thread From: Vaibhav Gupta @ 2020-04-24 13:00 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: linux-kernel-mentees, Rafael J. Wysocki, Vaibhav Gupta On Fri, 24 Apr 2020 at 18:14, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, Apr 24, 2020 at 04:01:43PM +0530, Vaibhav Gupta wrote: > > On Fri, 24 Apr 2020 at 09:17, Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > > > On Thu, Apr 23, 2020 at 06:58:01PM +0530, Vaibhav Gupta wrote: > > > > Upgrade Power Management from legacy to generic using dev_pm_ops. > > > > > > > > Remove pci_save_sate(), pci_set_state(), etc. No need of directive > > > > '#ifdef CONFIG_PM' as, if CONFIG_PM is not defined, the binding > > > > will be by deafult set to NULL. > > > > > > s/deafult/default/ > > > > > > I didn't quite follow this argument. SIMPLE_DEV_PM_OPS() is defined > > > to SET_SYSTEM_SLEEP_PM_OPS(), and SET_SYSTEM_SLEEP_PM_OPS() is defined > > > to nothing unless CONFIG_PM_SLEEP (not CONFIG_PM). > > yes, and we can find about CONFIG_PM_SLEEP inside kernel/power/kconfig > > It is defined as: > > config PM_SLEEP > > def_bool y > > depends on SUSPEND || HIBERNATE_CALLBACKS > > select PM > > select SRCU > > > > Hence it is selecting CONFIG_PM. So any of the macro check works.So i > > should go with > > #ifded CONFIG_PM > > or > > #ifdef CONFIG_PM_SLEEP ? > > Well, the point was that it's possible to have CONFIG_PM=y but > CONFIG_PM_SLEEP unset. In that case, using "#ifdef CONFIG_PM" will > mean those functions will be compiled but not referenced. > > > > But in any case, we don't want cp_suspend() and cp_resume() to be > > > compiled at all if they're not referenced. So I think converting the > > > "#ifdef CONFIG_PM" to "#ifdef CONFIG_PM_SLEEP" is the right thing. > > Actually I removed it because Andy also removed them in > > 226e6b866d74, thought it has to be done that way. > > Hmm, yes, I see that. I wish we had one canonical way to do this. > > In 226e6b866d74 ("gpio: pch: Convert to dev_pm_ops"), I see that Andy > also added "__maybe_unused", which I think turns off the compiler > warning about the function being unused. > > I had been looking at recent additions of SIMPLE_DEV_PM_OPS() via > > $ git log -p drivers/ > > and searching with "/^\+.*SIMPLE_DEV_PM_OPS". There's a mix of using > "#ifdef CONFIG_PM_SLEEP" and using "__maybe_unused", but it does look > like "__maybe_unused" is winning. It leaves unusable code in the > image but arguably looks a little prettier. > > So I guess I'm fine with either way. But I do think we need either > "#ifdef CONFIG_PM_SLEEP" or "__maybe_unused" so we don't get compiler > warnings. I totally agree with your argument. But in case of "#ifdef CONFIG_PM_SLEEP", neither the suspend() & resume() are compiled and neither ".driver.pm" gets bind. But in case of "__maybe_unused", we don't keep a check while binding ".driver.pm". Hence, if CONFIG_PM_SLEEP is not defined, still the ".driver.pm" is bind with a structure with some random initial values. We can use the mix of both? __maybe_unused xyz_suspend(){} __maybe_unused xyz_resume(){} ... #ifdef CONIFG_PM_SLEEP .driver.pm = &xyz_pm_ops; #endif -- Vaibhav Gupta > > Bjorn _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1 2/2] realtek/8139cp: Remove Legacy Power Management 2020-04-24 13:00 ` Vaibhav Gupta @ 2020-04-24 13:59 ` Bjorn Helgaas 2020-04-24 14:04 ` Vaibhav Gupta 0 siblings, 1 reply; 12+ messages in thread From: Bjorn Helgaas @ 2020-04-24 13:59 UTC (permalink / raw) To: Vaibhav Gupta; +Cc: linux-kernel-mentees, Rafael J. Wysocki, Vaibhav Gupta On Fri, Apr 24, 2020 at 06:30:01PM +0530, Vaibhav Gupta wrote: > On Fri, 24 Apr 2020 at 18:14, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, Apr 24, 2020 at 04:01:43PM +0530, Vaibhav Gupta wrote: > > > On Fri, 24 Apr 2020 at 09:17, Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > On Thu, Apr 23, 2020 at 06:58:01PM +0530, Vaibhav Gupta wrote: > > > > > Upgrade Power Management from legacy to generic using dev_pm_ops. > > > > > > > > > > Remove pci_save_sate(), pci_set_state(), etc. No need of directive > > > > > '#ifdef CONFIG_PM' as, if CONFIG_PM is not defined, the binding > > > > > will be by deafult set to NULL. > > > > > > > > s/deafult/default/ > > > > > > > > I didn't quite follow this argument. SIMPLE_DEV_PM_OPS() is defined > > > > to SET_SYSTEM_SLEEP_PM_OPS(), and SET_SYSTEM_SLEEP_PM_OPS() is defined > > > > to nothing unless CONFIG_PM_SLEEP (not CONFIG_PM). > > > yes, and we can find about CONFIG_PM_SLEEP inside kernel/power/kconfig > > > It is defined as: > > > config PM_SLEEP > > > def_bool y > > > depends on SUSPEND || HIBERNATE_CALLBACKS > > > select PM > > > select SRCU > > > > > > Hence it is selecting CONFIG_PM. So any of the macro check works.So i > > > should go with > > > #ifded CONFIG_PM > > > or > > > #ifdef CONFIG_PM_SLEEP ? > > > > Well, the point was that it's possible to have CONFIG_PM=y but > > CONFIG_PM_SLEEP unset. In that case, using "#ifdef CONFIG_PM" will > > mean those functions will be compiled but not referenced. > > > > > > But in any case, we don't want cp_suspend() and cp_resume() to be > > > > compiled at all if they're not referenced. So I think converting the > > > > "#ifdef CONFIG_PM" to "#ifdef CONFIG_PM_SLEEP" is the right thing. > > > Actually I removed it because Andy also removed them in > > > 226e6b866d74, thought it has to be done that way. > > > > Hmm, yes, I see that. I wish we had one canonical way to do this. > > > > In 226e6b866d74 ("gpio: pch: Convert to dev_pm_ops"), I see that Andy > > also added "__maybe_unused", which I think turns off the compiler > > warning about the function being unused. > > > > I had been looking at recent additions of SIMPLE_DEV_PM_OPS() via > > > > $ git log -p drivers/ > > > > and searching with "/^\+.*SIMPLE_DEV_PM_OPS". There's a mix of using > > "#ifdef CONFIG_PM_SLEEP" and using "__maybe_unused", but it does look > > like "__maybe_unused" is winning. It leaves unusable code in the > > image but arguably looks a little prettier. > > > > So I guess I'm fine with either way. But I do think we need either > > "#ifdef CONFIG_PM_SLEEP" or "__maybe_unused" so we don't get compiler > > warnings. > > I totally agree with your argument. But in case of "#ifdef CONFIG_PM_SLEEP", > neither the suspend() & resume() are compiled and neither ".driver.pm" > gets bind. > > But in case of "__maybe_unused", we don't keep a check while binding > ".driver.pm". > Hence, if CONFIG_PM_SLEEP is not defined, still the ".driver.pm" is > bind with a structure > with some random initial values. > > We can use the mix of both? > __maybe_unused xyz_suspend(){} > __maybe_unused xyz_resume(){} > ... > #ifdef CONFIG_PM_SLEEP > .driver.pm = &xyz_pm_ops; > #endif I don't think we need the mix because when CONFIG_PM_SLEEP isn't defined, SIMPLE_DEV_PM_OPS() compiles to: const struct dev_pm_ops xyz_pm_ops = { } and the xyz_pm_ops struct should be zero-filled. The #ifdef around ".driver.pm = &xyz_pm_ops" doesn't buy us anything -- the space for the driver.pm pointer is there regardless, so we don't save any space, and the default initialization is zero-filled, so the result is the same. _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1 2/2] realtek/8139cp: Remove Legacy Power Management 2020-04-24 13:59 ` Bjorn Helgaas @ 2020-04-24 14:04 ` Vaibhav Gupta 2020-04-24 15:01 ` Bjorn Helgaas 0 siblings, 1 reply; 12+ messages in thread From: Vaibhav Gupta @ 2020-04-24 14:04 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: linux-kernel-mentees, Rafael J. Wysocki, Vaibhav Gupta > > > > But in case of "__maybe_unused", we don't keep a check while binding > > ".driver.pm". > > Hence, if CONFIG_PM_SLEEP is not defined, still the ".driver.pm" is > > bind with a structure > > with some random initial values. > > > > We can use the mix of both? > > __maybe_unused xyz_suspend(){} > > __maybe_unused xyz_resume(){} > > ... > > #ifdef CONFIG_PM_SLEEP > > .driver.pm = &xyz_pm_ops; > > #endif > > I don't think we need the mix because when CONFIG_PM_SLEEP isn't > defined, SIMPLE_DEV_PM_OPS() compiles to: > > const struct dev_pm_ops xyz_pm_ops = { } > > and the xyz_pm_ops struct should be zero-filled. The #ifdef around > ".driver.pm = &xyz_pm_ops" doesn't buy us anything -- the space for > the driver.pm pointer is there regardless, so we don't save any space, > and the default initialization is zero-filled, so the result is the > same. Then, I guess my step to remove "#ifdef CONFIG_PM" won't go complete waste, I should just use "__maybe_unused" with suspend() and resume() to suppress warnings. Right? -- Vaibhav Gupta _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1 2/2] realtek/8139cp: Remove Legacy Power Management 2020-04-24 14:04 ` Vaibhav Gupta @ 2020-04-24 15:01 ` Bjorn Helgaas 2020-04-24 15:13 ` Vaibhav Gupta 0 siblings, 1 reply; 12+ messages in thread From: Bjorn Helgaas @ 2020-04-24 15:01 UTC (permalink / raw) To: Vaibhav Gupta; +Cc: linux-kernel-mentees, Rafael J. Wysocki, Vaibhav Gupta On Fri, Apr 24, 2020 at 07:34:50PM +0530, Vaibhav Gupta wrote: > > > > > > But in case of "__maybe_unused", we don't keep a check while binding > > > ".driver.pm". > > > Hence, if CONFIG_PM_SLEEP is not defined, still the ".driver.pm" is > > > bind with a structure > > > with some random initial values. > > > > > > We can use the mix of both? > > > __maybe_unused xyz_suspend(){} > > > __maybe_unused xyz_resume(){} > > > ... > > > #ifdef CONFIG_PM_SLEEP > > > .driver.pm = &xyz_pm_ops; > > > #endif > > > > I don't think we need the mix because when CONFIG_PM_SLEEP isn't > > defined, SIMPLE_DEV_PM_OPS() compiles to: > > > > const struct dev_pm_ops xyz_pm_ops = { } > > > > and the xyz_pm_ops struct should be zero-filled. The #ifdef around > > ".driver.pm = &xyz_pm_ops" doesn't buy us anything -- the space for > > the driver.pm pointer is there regardless, so we don't save any space, > > and the default initialization is zero-filled, so the result is the > > same. > > Then, I guess my step to remove "#ifdef CONFIG_PM" won't go complete > waste, I should just use "__maybe_unused" with suspend() and resume() > to suppress warnings. Right? Yes, that's what I'd do. This is all window dressing; the important thing is getting the semantic change right, i.e., the transformations of the xyz_suspend() and xyz_resume() functions. This #ifdef and __maybe_unused stuff can be trivially changed if necessary. Bjorn _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1 2/2] realtek/8139cp: Remove Legacy Power Management 2020-04-24 15:01 ` Bjorn Helgaas @ 2020-04-24 15:13 ` Vaibhav Gupta 0 siblings, 0 replies; 12+ messages in thread From: Vaibhav Gupta @ 2020-04-24 15:13 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: linux-kernel-mentees, Rafael J. Wysocki, Vaibhav Gupta On Fri, 24 Apr 2020 at 20:31, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, Apr 24, 2020 at 07:34:50PM +0530, Vaibhav Gupta wrote: > > > > > > > > But in case of "__maybe_unused", we don't keep a check while binding > > > > ".driver.pm". > > > > Hence, if CONFIG_PM_SLEEP is not defined, still the ".driver.pm" is > > > > bind with a structure > > > > with some random initial values. > > > > > > > > We can use the mix of both? > > > > __maybe_unused xyz_suspend(){} > > > > __maybe_unused xyz_resume(){} > > > > ... > > > > #ifdef CONFIG_PM_SLEEP > > > > .driver.pm = &xyz_pm_ops; > > > > #endif > > > > > > I don't think we need the mix because when CONFIG_PM_SLEEP isn't > > > defined, SIMPLE_DEV_PM_OPS() compiles to: > > > > > > const struct dev_pm_ops xyz_pm_ops = { } > > > > > > and the xyz_pm_ops struct should be zero-filled. The #ifdef around > > > ".driver.pm = &xyz_pm_ops" doesn't buy us anything -- the space for > > > the driver.pm pointer is there regardless, so we don't save any space, > > > and the default initialization is zero-filled, so the result is the > > > same. > > > > Then, I guess my step to remove "#ifdef CONFIG_PM" won't go complete > > waste, I should just use "__maybe_unused" with suspend() and resume() > > to suppress warnings. Right? > > Yes, that's what I'd do. This is all window dressing; the important > thing is getting the semantic change right, i.e., the transformations > of the xyz_suspend() and xyz_resume() functions. This #ifdef and > __maybe_unused stuff can be trivially changed if necessary. Yes, I will send a new patch series for this one. -- Vaibhav Gupta > > Bjorn _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1 1/2] realtek/8139too: Remove Legacy Power Management 2020-04-23 13:27 [Linux-kernel-mentees] [PATCH v1 1/2] realtek/8139too: Remove Legacy Power Management Vaibhav Gupta 2020-04-23 13:28 ` [Linux-kernel-mentees] [PATCH v1 2/2] realtek/8139cp: " Vaibhav Gupta @ 2020-04-24 3:43 ` Bjorn Helgaas 2020-04-24 10:00 ` Vaibhav Gupta 1 sibling, 1 reply; 12+ messages in thread From: Bjorn Helgaas @ 2020-04-24 3:43 UTC (permalink / raw) To: Vaibhav Gupta; +Cc: rjw, linux-kernel-mentees If you post more that one patch in a series, it's best if you include a cover letter with the patches being responses to the cover letter. In this case, it would be: [0/2] cover letter -> [1/2] realtek/8139too: Remove ... -> [2/2] realtek/8139cp: Remove ... On Thu, Apr 23, 2020 at 06:57:59PM +0530, Vaibhav Gupta wrote: > Upgrade Power Management from legacy to generic using dev_pm_ops. > > Remove struct pci_driver.suspend and struct pci_driver.resume bindings, > and add struct pci_driver.driver.pm . > > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com> > --- > drivers/net/ethernet/realtek/8139too.c | 26 +++++++------------------- > 1 file changed, 7 insertions(+), 19 deletions(-) > > diff --git a/drivers/net/ethernet/realtek/8139too.c b/drivers/net/ethernet/realtek/8139too.c > index 5caeb8368eab..b7c98b165256 100644 > --- a/drivers/net/ethernet/realtek/8139too.c > +++ b/drivers/net/ethernet/realtek/8139too.c > @@ -2603,17 +2603,13 @@ static void rtl8139_set_rx_mode (struct net_device *dev) > spin_unlock_irqrestore (&tp->lock, flags); > } > > -#ifdef CONFIG_PM > - > -static int rtl8139_suspend (struct pci_dev *pdev, pm_message_t state) > +static int rtl8139_suspend(struct device *device) > { > - struct net_device *dev = pci_get_drvdata (pdev); > + struct net_device *dev = dev_get_drvdata(device); > struct rtl8139_private *tp = netdev_priv(dev); > void __iomem *ioaddr = tp->mmio_addr; > unsigned long flags; > > - pci_save_state (pdev); > - > if (!netif_running (dev)) > return 0; > > @@ -2631,38 +2627,30 @@ static int rtl8139_suspend (struct pci_dev *pdev, pm_message_t state) > > spin_unlock_irqrestore (&tp->lock, flags); > > - pci_set_power_state (pdev, PCI_D3hot); > - > return 0; > } > > - > -static int rtl8139_resume (struct pci_dev *pdev) > +static int rtl8139_resume(struct device *device) > { > - struct net_device *dev = pci_get_drvdata (pdev); > + struct net_device *dev = dev_get_drvdata(device); > > - pci_restore_state (pdev); > if (!netif_running (dev)) > return 0; > - pci_set_power_state (pdev, PCI_D0); > + > rtl8139_init_ring (dev); > rtl8139_hw_start (dev); > netif_device_attach (dev); > return 0; > } > > -#endif /* CONFIG_PM */ > - > +static SIMPLE_DEV_PM_OPS(rtl8139_pm_ops, rtl8139_suspend, rtl8139_resume); The common pattern here seems to be: #ifdef CONFIG_PM_SLEEP static int *_suspend(struct device *device) { ... } static int *_resume(struct device *device) { ... } #endif static SIMPLE_DEV_PM_OPS(*_pm_ops, *_suspend, *_resume); So your patch looks great except that I think you should keep the #ifdef and convert it to CONFIG_PM_SLEEP. > static struct pci_driver rtl8139_pci_driver = { > .name = DRV_NAME, > .id_table = rtl8139_pci_tbl, > .probe = rtl8139_init_one, > .remove = rtl8139_remove_one, > -#ifdef CONFIG_PM > - .suspend = rtl8139_suspend, > - .resume = rtl8139_resume, > -#endif /* CONFIG_PM */ > + .driver.pm = &rtl8139_pm_ops, > }; > > > -- > 2.26.2 > _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1 1/2] realtek/8139too: Remove Legacy Power Management 2020-04-24 3:43 ` [Linux-kernel-mentees] [PATCH v1 1/2] realtek/8139too: " Bjorn Helgaas @ 2020-04-24 10:00 ` Vaibhav Gupta 0 siblings, 0 replies; 12+ messages in thread From: Vaibhav Gupta @ 2020-04-24 10:00 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: linux-kernel-mentees, Rafael J. Wysocki, Vaibhav Gupta On Fri, 24 Apr 2020 at 09:13, Bjorn Helgaas <helgaas@kernel.org> wrote: > > If you post more that one patch in a series, it's best if you include > a cover letter with the patches being responses to the cover letter. > In this case, it would be: > > [0/2] cover letter > -> [1/2] realtek/8139too: Remove ... > -> [2/2] realtek/8139cp: Remove ... Okay! I will do it. --Vaibhav Gupta > > On Thu, Apr 23, 2020 at 06:57:59PM +0530, Vaibhav Gupta wrote: > > Upgrade Power Management from legacy to generic using dev_pm_ops. > > > > Remove struct pci_driver.suspend and struct pci_driver.resume bindings, > > and add struct pci_driver.driver.pm . > > > > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com> > > --- > > drivers/net/ethernet/realtek/8139too.c | 26 +++++++------------------- > > 1 file changed, 7 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/net/ethernet/realtek/8139too.c b/drivers/net/ethernet/realtek/8139too.c > > index 5caeb8368eab..b7c98b165256 100644 > > --- a/drivers/net/ethernet/realtek/8139too.c > > +++ b/drivers/net/ethernet/realtek/8139too.c > > @@ -2603,17 +2603,13 @@ static void rtl8139_set_rx_mode (struct net_device *dev) > > spin_unlock_irqrestore (&tp->lock, flags); > > } > > > > -#ifdef CONFIG_PM > > - > > -static int rtl8139_suspend (struct pci_dev *pdev, pm_message_t state) > > +static int rtl8139_suspend(struct device *device) > > rtl8139_init_ring (dev); > > rtl8139_hw_start (dev); > > netif_device_attach (dev); > > return 0; > > } > > > > -#endif /* CONFIG_PM */ > > - > > +static SIMPLE_DEV_PM_OPS(rtl8139_pm_ops, rtl8139_suspend, rtl8139_resume); > > The common pattern here seems to be: > > #ifdef CONFIG_PM_SLEEP > static int *_suspend(struct device *device) { ... } > static int *_resume(struct device *device) { ... } > #endif > static SIMPLE_DEV_PM_OPS(*_pm_ops, *_suspend, *_resume); > > So your patch looks great except that I think you should keep the > #ifdef and convert it to CONFIG_PM_SLEEP. > _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-04-24 15:14 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-23 13:27 [Linux-kernel-mentees] [PATCH v1 1/2] realtek/8139too: Remove Legacy Power Management Vaibhav Gupta 2020-04-23 13:28 ` [Linux-kernel-mentees] [PATCH v1 2/2] realtek/8139cp: " Vaibhav Gupta 2020-04-24 3:47 ` Bjorn Helgaas 2020-04-24 10:31 ` Vaibhav Gupta 2020-04-24 12:44 ` Bjorn Helgaas 2020-04-24 13:00 ` Vaibhav Gupta 2020-04-24 13:59 ` Bjorn Helgaas 2020-04-24 14:04 ` Vaibhav Gupta 2020-04-24 15:01 ` Bjorn Helgaas 2020-04-24 15:13 ` Vaibhav Gupta 2020-04-24 3:43 ` [Linux-kernel-mentees] [PATCH v1 1/2] realtek/8139too: " Bjorn Helgaas 2020-04-24 10:00 ` Vaibhav Gupta
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).