diff mbox series

[8/8] net/iucv: Use the new device_to_pm() helper to access struct dev_pm_ops

Message ID 20200525182608.1823735-9-kw@linux.com (mailing list archive)
State Not Applicable, archived
Delegated to: Bjorn Helgaas
Headers show
Series Add helper for accessing Power Management callbacs | expand

Commit Message

Krzysztof Wilczyński May 25, 2020, 6:26 p.m. UTC
Use the new device_to_pm() helper to access Power Management callbacs
(struct dev_pm_ops) for a particular device (struct device_driver).

No functional change intended.

Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
---
 net/iucv/iucv.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

Comments

Greg KH May 26, 2020, 6:35 a.m. UTC | #1
On Mon, May 25, 2020 at 06:26:08PM +0000, Krzysztof Wilczyński wrote:
> Use the new device_to_pm() helper to access Power Management callbacs
> (struct dev_pm_ops) for a particular device (struct device_driver).
> 
> No functional change intended.
> 
> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> ---
>  net/iucv/iucv.c | 30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
> index 9a2d023842fe..1a3029ab7c1f 100644
> --- a/net/iucv/iucv.c
> +++ b/net/iucv/iucv.c
> @@ -1836,23 +1836,23 @@ static void iucv_external_interrupt(struct ext_code ext_code,
>  
>  static int iucv_pm_prepare(struct device *dev)
>  {
> -	int rc = 0;
> +	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>  
>  #ifdef CONFIG_PM_DEBUG
>  	printk(KERN_INFO "iucv_pm_prepare\n");
>  #endif
> -	if (dev->driver && dev->driver->pm && dev->driver->pm->prepare)
> -		rc = dev->driver->pm->prepare(dev);
> -	return rc;
> +	return pm && pm->prepare ? pm->prepare(dev) : 0;

No need for ? : here either, just use if () please.

It's "interesting" how using your new helper doesn't actually make the
code smaller.  Perhaps it isn't a good helper function?

thanks,

greg k-h
Ursula Braun May 26, 2020, 7:07 a.m. UTC | #2
On 5/25/20 8:26 PM, Krzysztof Wilczyński wrote:
> Use the new device_to_pm() helper to access Power Management callbacs
> (struct dev_pm_ops) for a particular device (struct device_driver).
> 
> No functional change intended.
> 
> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>

pm support is going to be removed (for s390 in general and) for
net/iucv/iucv.c with this net-next patch:

commit 4b32f86bf1673acb16441dd55d7b325609f54897
Author: Julian Wiedmann <jwi@linux.ibm.com>
Date:   Tue May 19 21:10:08 2020 +0200

    net/iucv: remove pm support
    
    commit 394216275c7d ("s390: remove broken hibernate / power management support")
    removed support for ARCH_HIBERNATION_POSSIBLE from s390.
    
    So drop the unused pm ops from the s390-only iucv bus driver.
    
    CC: Hendrik Brueckner <brueckner@linux.ibm.com>
    Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

> ---
>  net/iucv/iucv.c | 30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
> index 9a2d023842fe..1a3029ab7c1f 100644
> --- a/net/iucv/iucv.c
> +++ b/net/iucv/iucv.c

...
Krzysztof Wilczyński May 26, 2020, 2:57 p.m. UTC | #3
Hi Ursula,

On 20-05-26 09:07:27, Ursula Braun wrote:
> 
> 
> On 5/25/20 8:26 PM, Krzysztof Wilczyński wrote:
> > Use the new device_to_pm() helper to access Power Management callbacs
> > (struct dev_pm_ops) for a particular device (struct device_driver).
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> 
> pm support is going to be removed (for s390 in general and) for
> net/iucv/iucv.c with this net-next patch:
[...]

Good to know!  Thank you for letting me know.  I appreciate that.

Krzysztof
Krzysztof Wilczyński May 26, 2020, 3:07 p.m. UTC | #4
Hello Greg,

[...]
> It's "interesting" how using your new helper doesn't actually make the
> code smaller.  Perhaps it isn't a good helper function?

The idea for the helper was inspired by the comment Dan made to Bjorn
about Bjorn's change, as per:

  https://lore.kernel.org/driverdev-devel/20191016135002.GA24678@kadam/

It looked like a good idea to try to reduce the following:

  dev->driver && dev->driver->pm && dev->driver->pm->prepare

Into something more succinct.  Albeit, given the feedback from yourself
and Rafael, I gather that this helper is not really a good addition.

Thank you everyone and sorry for the commotion!

Krzysztof
Rafael J. Wysocki May 26, 2020, 3:19 p.m. UTC | #5
On Tue, May 26, 2020 at 5:07 PM Krzysztof Wilczyński <kw@linux.com> wrote:
>
> Hello Greg,
>
> [...]
> > It's "interesting" how using your new helper doesn't actually make the
> > code smaller.  Perhaps it isn't a good helper function?
>
> The idea for the helper was inspired by the comment Dan made to Bjorn
> about Bjorn's change, as per:
>
>   https://lore.kernel.org/driverdev-devel/20191016135002.GA24678@kadam/
>
> It looked like a good idea to try to reduce the following:
>
>   dev->driver && dev->driver->pm && dev->driver->pm->prepare
>
> Into something more succinct.  Albeit, given the feedback from yourself
> and Rafael, I gather that this helper is not really a good addition.

IMO it could be used for reducing code duplication like you did in the
PCI code, but not necessarily in the other places where the code in
question is not exactly duplicated.

Thanks!
Alan Stern May 26, 2020, 3:28 p.m. UTC | #6
On Tue, May 26, 2020 at 05:19:07PM +0200, Rafael J. Wysocki wrote:
> On Tue, May 26, 2020 at 5:07 PM Krzysztof Wilczyński <kw@linux.com> wrote:
> >
> > Hello Greg,
> >
> > [...]
> > > It's "interesting" how using your new helper doesn't actually make the
> > > code smaller.  Perhaps it isn't a good helper function?
> >
> > The idea for the helper was inspired by the comment Dan made to Bjorn
> > about Bjorn's change, as per:
> >
> >   https://lore.kernel.org/driverdev-devel/20191016135002.GA24678@kadam/
> >
> > It looked like a good idea to try to reduce the following:
> >
> >   dev->driver && dev->driver->pm && dev->driver->pm->prepare
> >
> > Into something more succinct.  Albeit, given the feedback from yourself
> > and Rafael, I gather that this helper is not really a good addition.
> 
> IMO it could be used for reducing code duplication like you did in the
> PCI code, but not necessarily in the other places where the code in
> question is not exactly duplicated.

The code could be a little more succinct, although it wouldn't fit every 
usage.  For example,

#define pm_do_callback(dev, method) \
	(dev->driver && dev->driver->pm && dev->driver->pm->callback ? \
	dev->driver->pm->callback(dev) : 0)

Then the usage is something like:

	ret = pm_do_callback(dev, prepare);

Would this be an overall improvement?

Alan Stern
Rafael J. Wysocki May 26, 2020, 4:06 p.m. UTC | #7
On Tue, May 26, 2020 at 5:28 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Tue, May 26, 2020 at 05:19:07PM +0200, Rafael J. Wysocki wrote:
> > On Tue, May 26, 2020 at 5:07 PM Krzysztof Wilczyński <kw@linux.com> wrote:
> > >
> > > Hello Greg,
> > >
> > > [...]
> > > > It's "interesting" how using your new helper doesn't actually make the
> > > > code smaller.  Perhaps it isn't a good helper function?
> > >
> > > The idea for the helper was inspired by the comment Dan made to Bjorn
> > > about Bjorn's change, as per:
> > >
> > >   https://lore.kernel.org/driverdev-devel/20191016135002.GA24678@kadam/
> > >
> > > It looked like a good idea to try to reduce the following:
> > >
> > >   dev->driver && dev->driver->pm && dev->driver->pm->prepare
> > >
> > > Into something more succinct.  Albeit, given the feedback from yourself
> > > and Rafael, I gather that this helper is not really a good addition.
> >
> > IMO it could be used for reducing code duplication like you did in the
> > PCI code, but not necessarily in the other places where the code in
> > question is not exactly duplicated.
>
> The code could be a little more succinct, although it wouldn't fit every
> usage.  For example,
>
> #define pm_do_callback(dev, method) \
>         (dev->driver && dev->driver->pm && dev->driver->pm->callback ? \
>         dev->driver->pm->callback(dev) : 0)
>
> Then the usage is something like:
>
>         ret = pm_do_callback(dev, prepare);
>
> Would this be an overall improvement?

It wouldn't cover all of the use cases.

For example, PCI does other things in addition to running a callback
when it is present.

Something like this might be enough though:

#define pm_driver_callback_is_present(dev, method) \
        (dev->driver && dev->driver->pm && dev->driver->pm->method)

#define pm_run_driver_callback(dev, method) \
        (pm_driver_callback_is_present(dev, method) ?
dev->driver->pm->method(dev) : 0)

#define pm_get_driver_callback(dev, method) \
        (pm_driver_callback_is_present(dev, method) ?
dev->driver->pm->method : NULL)

so whoever needs the callback pointer can use pm_get_driver_callback()
and whoever only needs to run the callback can use
pm_run_driver_callback().

Cheers!
Alex Elder May 26, 2020, 4:48 p.m. UTC | #8
On 5/26/20 10:07 AM, Krzysztof Wilczyński wrote:
> Hello Greg,
> 
> [...]
>> It's "interesting" how using your new helper doesn't actually make the
>> code smaller.  Perhaps it isn't a good helper function?

Helper functions often improve code readability, which is
beneficial even if it doesn't reduce code size or efficiency.

But I won't argue for or against this particular change.
It's OK with me either way.

					-Alex

> The idea for the helper was inspired by the comment Dan made to Bjorn
> about Bjorn's change, as per:
> 
>    https://lore.kernel.org/driverdev-devel/20191016135002.GA24678@kadam/
> 
> It looked like a good idea to try to reduce the following:
> 
>    dev->driver && dev->driver->pm && dev->driver->pm->prepare
> 
> Into something more succinct.  Albeit, given the feedback from yourself
> and Rafael, I gather that this helper is not really a good addition.
> 
> Thank you everyone and sorry for the commotion!
> 
> Krzysztof
> _______________________________________________
> greybus-dev mailing list
> greybus-dev@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/greybus-dev
>
diff mbox series

Patch

diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
index 9a2d023842fe..1a3029ab7c1f 100644
--- a/net/iucv/iucv.c
+++ b/net/iucv/iucv.c
@@ -1836,23 +1836,23 @@  static void iucv_external_interrupt(struct ext_code ext_code,
 
 static int iucv_pm_prepare(struct device *dev)
 {
-	int rc = 0;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 #ifdef CONFIG_PM_DEBUG
 	printk(KERN_INFO "iucv_pm_prepare\n");
 #endif
-	if (dev->driver && dev->driver->pm && dev->driver->pm->prepare)
-		rc = dev->driver->pm->prepare(dev);
-	return rc;
+	return pm && pm->prepare ? pm->prepare(dev) : 0;
 }
 
 static void iucv_pm_complete(struct device *dev)
 {
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
+
 #ifdef CONFIG_PM_DEBUG
 	printk(KERN_INFO "iucv_pm_complete\n");
 #endif
-	if (dev->driver && dev->driver->pm && dev->driver->pm->complete)
-		dev->driver->pm->complete(dev);
+	if (pm && pm->complete)
+		pm->complete(dev);
 }
 
 /**
@@ -1883,6 +1883,7 @@  static int iucv_path_table_empty(void)
 static int iucv_pm_freeze(struct device *dev)
 {
 	int cpu;
+	const struct dev_pm_ops *pm;
 	struct iucv_irq_list *p, *n;
 	int rc = 0;
 
@@ -1902,8 +1903,9 @@  static int iucv_pm_freeze(struct device *dev)
 		}
 	}
 	iucv_pm_state = IUCV_PM_FREEZING;
-	if (dev->driver && dev->driver->pm && dev->driver->pm->freeze)
-		rc = dev->driver->pm->freeze(dev);
+	pm = driver_to_pm(dev->driver);
+	if (pm && pm->freeze)
+		rc = pm->freeze(dev);
 	if (iucv_path_table_empty())
 		iucv_disable();
 	return rc;
@@ -1919,6 +1921,7 @@  static int iucv_pm_freeze(struct device *dev)
  */
 static int iucv_pm_thaw(struct device *dev)
 {
+	const struct dev_pm_ops *pm;
 	int rc = 0;
 
 #ifdef CONFIG_PM_DEBUG
@@ -1938,8 +1941,9 @@  static int iucv_pm_thaw(struct device *dev)
 			/* enable interrupts on all cpus */
 			iucv_setmask_mp();
 	}
-	if (dev->driver && dev->driver->pm && dev->driver->pm->thaw)
-		rc = dev->driver->pm->thaw(dev);
+	pm = driver_to_pm(dev->driver);
+	if (pm && pm->thaw)
+		rc = pm->thaw(dev);
 out:
 	return rc;
 }
@@ -1954,6 +1958,7 @@  static int iucv_pm_thaw(struct device *dev)
  */
 static int iucv_pm_restore(struct device *dev)
 {
+	const struct dev_pm_ops *pm;
 	int rc = 0;
 
 #ifdef CONFIG_PM_DEBUG
@@ -1968,8 +1973,9 @@  static int iucv_pm_restore(struct device *dev)
 		if (rc)
 			goto out;
 	}
-	if (dev->driver && dev->driver->pm && dev->driver->pm->restore)
-		rc = dev->driver->pm->restore(dev);
+	pm = driver_to_pm(dev->driver);
+	if (pm && pm->restore)
+		rc = pm->restore(dev);
 out:
 	return rc;
 }