| Message ID | 20200525182608.1823735-9-kw@linux.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Add helper for accessing Power Management callbacs | expand |
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
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 ...
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
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
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!
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
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!
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 --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; }
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(-)