[Linux-kernel-mentees] [PATCH v1 1/2] realtek/8139too: Remove Legacy Power Management

[Linux-kernel-mentees] [PATCH v1 1/2] realtek/8139too: Remove Legacy Power Management

[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

[Linux-kernel-mentees] [PATCH v1 2/2] realtek/8139cp: Remove Legacy Power Management

[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

Re: [Linux-kernel-mentees] [PATCH v1 1/2] realtek/8139too: Remove Legacy Power Management

[Bjorn Helgaas]

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

Re: [Linux-kernel-mentees] [PATCH v1 2/2] realtek/8139cp: Remove Legacy Power Management

[Bjorn Helgaas]

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

Re: [Linux-kernel-mentees] [PATCH v1 1/2] realtek/8139too: Remove Legacy Power Management

[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

Re: [Linux-kernel-mentees] [PATCH v1 2/2] realtek/8139cp: Remove Legacy Power Management

[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

Re: [Linux-kernel-mentees] [PATCH v1 2/2] realtek/8139cp: Remove Legacy Power Management

[Bjorn Helgaas]

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

Re: [Linux-kernel-mentees] [PATCH v1 2/2] realtek/8139cp: Remove Legacy Power Management

[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

Re: [Linux-kernel-mentees] [PATCH v1 2/2] realtek/8139cp: Remove Legacy Power Management

[Bjorn Helgaas]

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

Re: [Linux-kernel-mentees] [PATCH v1 2/2] realtek/8139cp: Remove Legacy Power Management

[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

Re: [Linux-kernel-mentees] [PATCH v1 2/2] realtek/8139cp: Remove Legacy Power Management

[Bjorn Helgaas]

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

Re: [Linux-kernel-mentees] [PATCH v1 2/2] realtek/8139cp: Remove Legacy Power Management

[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