The purpose of this patch series is to remove legacy power management callbacks
from realtek ethernet drivers.
The callbacks performing suspend() and resume() operations are still calling
pci_save_state(), pci_set_power_state(), etc. and handling the powermanagement
themselves, which is not recommended.
The conversion requires the removal of the those function calls and change the
callback definition accordingly.
Vaibhav Gupta (2):
realtek/8139too: Remove Legacy Power Management
realtek/8139cp: Remove Legacy Power Management
drivers/net/ethernet/realtek/8139cp.c | 25 +++++++------------------
drivers/net/ethernet/realtek/8139too.c | 26 +++++++-------------------
2 files changed, 14 insertions(+), 37 deletions(-)
--
2.26.2
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" .
Add "__maybe_unused" attribute to resume() and susend() callbacks
definition to suppress compiler warnings.
Generic callback requires an argument of type "struct device*". Hence,
convert it to "struct net_device*" using "dev_get_drvdata()" to use
it in the callback.
Signed-off-by: Vaibhav Gupta <[email protected]>
---
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..227139d42227 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 __maybe_unused 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 __maybe_unused 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
Upgrade power management from legacy to generic using dev_pm_ops.
Add "__maybe_unused" attribute to resume() and susend() callbacks
definition to suppress compiler warnings.
Generic callback requires an argument of type "struct device*". Hence,
convert it to "struct net_device*" using "dev_get_drv_data()" to use
it in the callback.
Most of the cleaning part is to remove pci_save_state(),
pci_set_power_state(), etc power management function calls.
Signed-off-by: Vaibhav Gupta <[email protected]>
---
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..4f2fb1393966 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 __maybe_unused 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 __maybe_unused 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
Uncapitalize "legacy power management" in subject. I'd say "convert",
not "remove", to make it clear that the driver will still do power
management afterwards.
I think your to: and cc: list came from the get_maintainer.pl script,
but you can trim it a bit by omitting people who have just made
occasional random fixups. These drivers are really unmaintained, so
Dave M, netdev, Rafael, linux-pm, linux-pci, and maybe LKML are
probably enough.
On Tue, Apr 28, 2020 at 08:13:13PM +0530, Vaibhav Gupta wrote:
> Upgrade power management from legacy to generic using dev_pm_ops.
Instead of the paragraphs below, which cover the stuff that's fairly
obvious, I think it would be more useful to include hints about where
the things you removed will be done now. That helps reviewers verify
that this doesn't break anything. E.g.,
In the legacy PM model, drivers save and restore PCI state and set
the device power state directly. In the generic model, this is all
done by the PCI core in .suspend_noirq() (pci_pm_suspend_noirq())
and .resume_noirq() (pci_pm_resume_noirq()).
This sort of thing could go in each commit log. The cover letter
doesn't normally go in the commit log, so you have to assume it will
be lost.
> Remove "struct pci_driver.suspend" and "struct pci_driver.resume"
> bindings, and add "struct pci_driver.driver.pm" .
>
> Add "__maybe_unused" attribute to resume() and susend() callbacks
> definition to suppress compiler warnings.
>
> Generic callback requires an argument of type "struct device*". Hence,
> convert it to "struct net_device*" using "dev_get_drvdata()" to use
> it in the callback.
>
> Signed-off-by: Vaibhav Gupta <[email protected]>
Acked-by: Bjorn Helgaas <[email protected]>
Thanks a lot for doing this!
> ---
> 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..227139d42227 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 __maybe_unused 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 __maybe_unused 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
>
On Tue, Apr 28, 2020 at 08:13:14PM +0530, Vaibhav Gupta wrote:
> Upgrade power management from legacy to generic using dev_pm_ops.
>
> Add "__maybe_unused" attribute to resume() and susend() callbacks
> definition to suppress compiler warnings.
>
> Generic callback requires an argument of type "struct device*". Hence,
> convert it to "struct net_device*" using "dev_get_drv_data()" to use
> it in the callback.
>
> Most of the cleaning part is to remove pci_save_state(),
> pci_set_power_state(), etc power management function calls.
>
> Signed-off-by: Vaibhav Gupta <[email protected]>
> ---
> 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..4f2fb1393966 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 __maybe_unused 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);
This one is a little more interesting because it relies on the driver
state (cp->wol_enabled). IIUC, the corresponding pci_enable_wake() in
the generic path is in pci_prepare_to_sleep() (called from
pci_pm_suspend_noirq()).
But of course the generic path doesn't look at cp->wol_enabled. It
looks at device_may_wakeup(), but I don't know whether there's a
connection between that and 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 __maybe_unused 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
>
On 28.04.2020 16:43, Vaibhav Gupta wrote:
> The purpose of this patch series is to remove legacy power management callbacks
> from realtek ethernet drivers.
>
> The callbacks performing suspend() and resume() operations are still calling
> pci_save_state(), pci_set_power_state(), etc. and handling the powermanagement
> themselves, which is not recommended.
>
Did you test any of the changes? If not, then mention this at least.
A typical comment in the commit message would be "compile-tested only".
In addition the following should be changed.
[Linux-kernel-mentees] [PATCH v2 0/2]
Use
[PATCH net-next v2 0/2]
instead.
> The conversion requires the removal of the those function calls and change the
> callback definition accordingly.
>
> Vaibhav Gupta (2):
> realtek/8139too: Remove Legacy Power Management
> realtek/8139cp: Remove Legacy Power Management
>
> drivers/net/ethernet/realtek/8139cp.c | 25 +++++++------------------
> drivers/net/ethernet/realtek/8139too.c | 26 +++++++-------------------
> 2 files changed, 14 insertions(+), 37 deletions(-)
>
On Tue, 28 Apr 2020 at 23:24, Heiner Kallweit <[email protected]> wrote:
>
> On 28.04.2020 16:43, Vaibhav Gupta wrote:
> > The purpose of this patch series is to remove legacy power management callbacks
> > from realtek ethernet drivers.
> >
> > The callbacks performing suspend() and resume() operations are still calling
> > pci_save_state(), pci_set_power_state(), etc. and handling the powermanagement
> > themselves, which is not recommended.
> >
> Did you test any of the changes? If not, then mention this at least.
> A typical comment in the commit message would be "compile-tested only".
Yeah its compile-tested only. I should have mention that. Thanks for
pointing it out.
>
> In addition the following should be changed.
> [Linux-kernel-mentees] [PATCH v2 0/2]
> Use
> [PATCH net-next v2 0/2]
> instead.
Sure!
--Vaibhav Gupta
>
> > The conversion requires the removal of the those function calls and change the
> > callback definition accordingly.
> >
> > Vaibhav Gupta (2):
> > realtek/8139too: Remove Legacy Power Management
> > realtek/8139cp: Remove Legacy Power Management
> >
> > drivers/net/ethernet/realtek/8139cp.c | 25 +++++++------------------
> > drivers/net/ethernet/realtek/8139too.c | 26 +++++++-------------------
> > 2 files changed, 14 insertions(+), 37 deletions(-)
> >
>
On Tue, 28 Apr 2020 at 22:59, Bjorn Helgaas <[email protected]> wrote:
>
> On Tue, Apr 28, 2020 at 08:13:14PM +0530, Vaibhav Gupta wrote:
> > Upgrade power management from legacy to generic using dev_pm_ops.
> >
> > Add "__maybe_unused" attribute to resume() and susend() callbacks
> > definition to suppress compiler warnings.
> >
> > Generic callback requires an argument of type "struct device*". Hence,
> > convert it to "struct net_device*" using "dev_get_drv_data()" to use
> > it in the callback.
> >
> > Most of the cleaning part is to remove pci_save_state(),
> > pci_set_power_state(), etc power management function calls.
> >
> > Signed-off-by: Vaibhav Gupta <[email protected]>
> > ---
> > 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..4f2fb1393966 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 __maybe_unused 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);
>
> This one is a little more interesting because it relies on the driver
> state (cp->wol_enabled). IIUC, the corresponding pci_enable_wake() in
> the generic path is in pci_prepare_to_sleep() (called from
> pci_pm_suspend_noirq()).
>
> But of course the generic path doesn't look at cp->wol_enabled. It
> looks at device_may_wakeup(), but I don't know whether there's a
> connection between that and cp->wol_enabled.
I have tested it by just compiling it. I will try to dig a bit more deep into
this and check if it is affecting something (on the basis of code).
The final test has to done with hardware.
-- Vaibhav Gupta
>
> > - pci_set_power_state(pdev, pci_choose_state(pdev, state));
> > -
> > return 0;
> > }
> >
> > -static int cp_resume (struct pci_dev *pdev)
> > +static int __maybe_unused 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
> >
> >
> > Uncapitalize "legacy power management" in subject. I'd say "convert",
> > not "remove", to make it clear that the driver will still do power
> > management afterwards.
Sure!
> >
> > I think your to: and cc: list came from the get_maintainer.pl script,
yeah.
> > but you can trim it a bit by omitting people who have just made
> > occasional random fixups. These drivers are really unmaintained, so
> > Dave M, netdev, Rafael, linux-pm, linux-pci, and maybe LKML are
> > probably enough.
I will keep this in mind next time.
> >
> > On Tue, Apr 28, 2020 at 08:13:13PM +0530, Vaibhav Gupta wrote:
> > > Upgrade power management from legacy to generic using dev_pm_ops.
> >
> > Instead of the paragraphs below, which cover the stuff that's fairly
> > obvious, I think it would be more useful to include hints about where
> > the things you removed will be done now. That helps reviewers verify
> > that this doesn't break anything. E.g.,
> >
> > In the legacy PM model, drivers save and restore PCI state and set
> > the device power state directly. In the generic model, this is all
> > done by the PCI core in .suspend_noirq() (pci_pm_suspend_noirq())
> > and .resume_noirq() (pci_pm_resume_noirq()).
> >
> > This sort of thing could go in each commit log. The cover letter
> > doesn't normally go in the commit log, so you have to assume it will
> > be lost.
Okay. I will send v3 patch-series with changes. Thanks for
acknowledging :)
--Vaibhav Gupta
> >
> > > Remove "struct pci_driver.suspend" and "struct pci_driver.resume"
> > > bindings, and add "struct pci_driver.driver.pm" .
> > >