| Message ID | 20221215181848.129326-1-helgaas@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | PM: runtime: Simplify __rpm_get_callback() | expand |
On Thu, Dec 15, 2022 at 7:19 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > From: Bjorn Helgaas <bhelgaas@google.com> > > Simplify __rpm_get_callback() slightly by returning as soon as the return > value is known. No functional change intended. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/base/power/runtime.c | 15 +++++---------- > 1 file changed, 5 insertions(+), 10 deletions(-) > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > index 50e726b6c2cf..7171ed0668f3 100644 > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -20,8 +20,7 @@ typedef int (*pm_callback_t)(struct device *); > > static pm_callback_t __rpm_get_callback(struct device *dev, size_t cb_offset) > { > - pm_callback_t cb; > - const struct dev_pm_ops *ops; > + const struct dev_pm_ops *ops = NULL; > > if (dev->pm_domain) > ops = &dev->pm_domain->ops; > @@ -31,18 +30,14 @@ static pm_callback_t __rpm_get_callback(struct device *dev, size_t cb_offset) > ops = dev->class->pm; > else if (dev->bus && dev->bus->pm) > ops = dev->bus->pm; > - else > - ops = NULL; > > if (ops) > - cb = *(pm_callback_t *)((void *)ops + cb_offset); > - else > - cb = NULL; > + return *(pm_callback_t *)((void *)ops + cb_offset); > > - if (!cb && dev->driver && dev->driver->pm) > - cb = *(pm_callback_t *)((void *)dev->driver->pm + cb_offset); > + if (dev->driver && dev->driver->pm) > + return *(pm_callback_t *)((void *)dev->driver->pm + cb_offset); > > - return cb; > + return NULL; > } > > #define RPM_GET_CALLBACK(dev, callback) \ > -- Applied as 6.3 material, thanks!
Hi Bjorn, On Thu, Dec 15, 2022 at 7:23 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > From: Bjorn Helgaas <bhelgaas@google.com> > > Simplify __rpm_get_callback() slightly by returning as soon as the return > value is known. No functional change intended. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Thanks for your patch, which is now commit 650bdddb6b311705 ("PM: runtime: Simplify __rpm_get_callback()") in pm/linux-next. > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -20,8 +20,7 @@ typedef int (*pm_callback_t)(struct device *); > > static pm_callback_t __rpm_get_callback(struct device *dev, size_t cb_offset) > { > - pm_callback_t cb; > - const struct dev_pm_ops *ops; > + const struct dev_pm_ops *ops = NULL; > > if (dev->pm_domain) > ops = &dev->pm_domain->ops; > @@ -31,18 +30,14 @@ static pm_callback_t __rpm_get_callback(struct device *dev, size_t cb_offset) > ops = dev->class->pm; > else if (dev->bus && dev->bus->pm) > ops = dev->bus->pm; > - else > - ops = NULL; > > if (ops) > - cb = *(pm_callback_t *)((void *)ops + cb_offset); > - else > - cb = NULL; > + return *(pm_callback_t *)((void *)ops + cb_offset); This is a change in behavior in case the callback turns out to be NULL: - before, it would fall back to the driver-specific callback below, - after, it always returns NULL. > > - if (!cb && dev->driver && dev->driver->pm) > - cb = *(pm_callback_t *)((void *)dev->driver->pm + cb_offset); > + if (dev->driver && dev->driver->pm) > + return *(pm_callback_t *)((void *)dev->driver->pm + cb_offset); > > - return cb; > + return NULL; > } Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Tue, Jan 24, 2023 at 12:20 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Bjorn, > > On Thu, Dec 15, 2022 at 7:23 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > From: Bjorn Helgaas <bhelgaas@google.com> > > > > Simplify __rpm_get_callback() slightly by returning as soon as the return > > value is known. No functional change intended. > > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > Thanks for your patch, which is now commit 650bdddb6b311705 ("PM: > runtime: Simplify __rpm_get_callback()") in pm/linux-next. > > > --- a/drivers/base/power/runtime.c > > +++ b/drivers/base/power/runtime.c > > @@ -20,8 +20,7 @@ typedef int (*pm_callback_t)(struct device *); > > > > static pm_callback_t __rpm_get_callback(struct device *dev, size_t cb_offset) > > { > > - pm_callback_t cb; > > - const struct dev_pm_ops *ops; > > + const struct dev_pm_ops *ops = NULL; > > > > if (dev->pm_domain) > > ops = &dev->pm_domain->ops; > > @@ -31,18 +30,14 @@ static pm_callback_t __rpm_get_callback(struct device *dev, size_t cb_offset) > > ops = dev->class->pm; > > else if (dev->bus && dev->bus->pm) > > ops = dev->bus->pm; > > - else > > - ops = NULL; > > > > if (ops) > > - cb = *(pm_callback_t *)((void *)ops + cb_offset); > > - else > > - cb = NULL; > > + return *(pm_callback_t *)((void *)ops + cb_offset); > > This is a change in behavior in case the callback turns out to be NULL: > - before, it would fall back to the driver-specific callback below, > - after, it always returns NULL. Good point and sorry for missing this! > > > > - if (!cb && dev->driver && dev->driver->pm) > > - cb = *(pm_callback_t *)((void *)dev->driver->pm + cb_offset); > > + if (dev->driver && dev->driver->pm) > > + return *(pm_callback_t *)((void *)dev->driver->pm + cb_offset); > > > > - return cb; > > + return NULL; > > } > Something like the patch below (modulo gmail-induced whitespace breakage) should restore the previous behavior if I'm not mistaken: --- drivers/base/power/runtime.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) Index: linux-pm/drivers/base/power/runtime.c =================================================================== --- linux-pm.orig/drivers/base/power/runtime.c +++ linux-pm/drivers/base/power/runtime.c @@ -31,8 +31,13 @@ static pm_callback_t __rpm_get_callback( else if (dev->bus && dev->bus->pm) ops = dev->bus->pm; - if (ops) - return *(pm_callback_t *)((void *)ops + cb_offset); + if (ops) { + pm_callback_t cb; + + cb = *(pm_callback_t *)((void *)ops + cb_offset); + if (cb) + return cb; + } if (dev->driver && dev->driver->pm) return *(pm_callback_t *)((void *)dev->driver->pm + cb_offset);
Hi Rafael, On Tue, Jan 24, 2023 at 3:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > On Tue, Jan 24, 2023 at 12:20 PM Geert Uytterhoeven > <geert@linux-m68k.org> wrote: > > > > Hi Bjorn, > > > > On Thu, Dec 15, 2022 at 7:23 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > From: Bjorn Helgaas <bhelgaas@google.com> > > > > > > Simplify __rpm_get_callback() slightly by returning as soon as the return > > > value is known. No functional change intended. > > > > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > > > Thanks for your patch, which is now commit 650bdddb6b311705 ("PM: > > runtime: Simplify __rpm_get_callback()") in pm/linux-next. > > > > > --- a/drivers/base/power/runtime.c > > > +++ b/drivers/base/power/runtime.c > > > @@ -20,8 +20,7 @@ typedef int (*pm_callback_t)(struct device *); > > > > > > static pm_callback_t __rpm_get_callback(struct device *dev, size_t cb_offset) > > > { > > > - pm_callback_t cb; > > > - const struct dev_pm_ops *ops; > > > + const struct dev_pm_ops *ops = NULL; > > > > > > if (dev->pm_domain) > > > ops = &dev->pm_domain->ops; > > > @@ -31,18 +30,14 @@ static pm_callback_t __rpm_get_callback(struct device *dev, size_t cb_offset) > > > ops = dev->class->pm; > > > else if (dev->bus && dev->bus->pm) > > > ops = dev->bus->pm; > > > - else > > > - ops = NULL; > > > > > > if (ops) > > > - cb = *(pm_callback_t *)((void *)ops + cb_offset); > > > - else > > > - cb = NULL; > > > + return *(pm_callback_t *)((void *)ops + cb_offset); > > > > This is a change in behavior in case the callback turns out to be NULL: > > - before, it would fall back to the driver-specific callback below, > > - after, it always returns NULL. > > Good point and sorry for missing this! > > > > > > > - if (!cb && dev->driver && dev->driver->pm) > > > - cb = *(pm_callback_t *)((void *)dev->driver->pm + cb_offset); > > > + if (dev->driver && dev->driver->pm) > > > + return *(pm_callback_t *)((void *)dev->driver->pm + cb_offset); > > > > > > - return cb; > > > + return NULL; > > > } > > > > Something like the patch below (modulo gmail-induced whitespace > breakage) should restore the previous behavior if I'm not mistaken: > > --- > drivers/base/power/runtime.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > Index: linux-pm/drivers/base/power/runtime.c > =================================================================== > --- linux-pm.orig/drivers/base/power/runtime.c > +++ linux-pm/drivers/base/power/runtime.c > @@ -31,8 +31,13 @@ static pm_callback_t __rpm_get_callback( > else if (dev->bus && dev->bus->pm) > ops = dev->bus->pm; > > - if (ops) > - return *(pm_callback_t *)((void *)ops + cb_offset); > + if (ops) { > + pm_callback_t cb; > + > + cb = *(pm_callback_t *)((void *)ops + cb_offset); > + if (cb) > + return cb; > + } > > if (dev->driver && dev->driver->pm) > return *(pm_callback_t *)((void *)dev->driver->pm + cb_offset); Which is now more complex than the original? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Tue, Jan 24, 2023 at 3:37 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Rafael, > > On Tue, Jan 24, 2023 at 3:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Tue, Jan 24, 2023 at 12:20 PM Geert Uytterhoeven > > <geert@linux-m68k.org> wrote: > > > > > > Hi Bjorn, > > > > > > On Thu, Dec 15, 2022 at 7:23 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > From: Bjorn Helgaas <bhelgaas@google.com> > > > > > > > > Simplify __rpm_get_callback() slightly by returning as soon as the return > > > > value is known. No functional change intended. > > > > > > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > > > > > Thanks for your patch, which is now commit 650bdddb6b311705 ("PM: > > > runtime: Simplify __rpm_get_callback()") in pm/linux-next. > > > > > > > --- a/drivers/base/power/runtime.c > > > > +++ b/drivers/base/power/runtime.c > > > > @@ -20,8 +20,7 @@ typedef int (*pm_callback_t)(struct device *); > > > > > > > > static pm_callback_t __rpm_get_callback(struct device *dev, size_t cb_offset) > > > > { > > > > - pm_callback_t cb; > > > > - const struct dev_pm_ops *ops; > > > > + const struct dev_pm_ops *ops = NULL; > > > > > > > > if (dev->pm_domain) > > > > ops = &dev->pm_domain->ops; > > > > @@ -31,18 +30,14 @@ static pm_callback_t __rpm_get_callback(struct device *dev, size_t cb_offset) > > > > ops = dev->class->pm; > > > > else if (dev->bus && dev->bus->pm) > > > > ops = dev->bus->pm; > > > > - else > > > > - ops = NULL; > > > > > > > > if (ops) > > > > - cb = *(pm_callback_t *)((void *)ops + cb_offset); > > > > - else > > > > - cb = NULL; > > > > + return *(pm_callback_t *)((void *)ops + cb_offset); > > > > > > This is a change in behavior in case the callback turns out to be NULL: > > > - before, it would fall back to the driver-specific callback below, > > > - after, it always returns NULL. > > > > Good point and sorry for missing this! > > > > > > > > > > - if (!cb && dev->driver && dev->driver->pm) > > > > - cb = *(pm_callback_t *)((void *)dev->driver->pm + cb_offset); > > > > + if (dev->driver && dev->driver->pm) > > > > + return *(pm_callback_t *)((void *)dev->driver->pm + cb_offset); > > > > > > > > - return cb; > > > > + return NULL; > > > > } > > > > > > > Something like the patch below (modulo gmail-induced whitespace > > breakage) should restore the previous behavior if I'm not mistaken: > > > > --- > > drivers/base/power/runtime.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > Index: linux-pm/drivers/base/power/runtime.c > > =================================================================== > > --- linux-pm.orig/drivers/base/power/runtime.c > > +++ linux-pm/drivers/base/power/runtime.c > > @@ -31,8 +31,13 @@ static pm_callback_t __rpm_get_callback( > > else if (dev->bus && dev->bus->pm) > > ops = dev->bus->pm; > > > > - if (ops) > > - return *(pm_callback_t *)((void *)ops + cb_offset); > > + if (ops) { > > + pm_callback_t cb; > > + > > + cb = *(pm_callback_t *)((void *)ops + cb_offset); > > + if (cb) > > + return cb; > > + } > > > > if (dev->driver && dev->driver->pm) > > return *(pm_callback_t *)((void *)dev->driver->pm + cb_offset); > > Which is now more complex than the original? Arguably so. OK, I'll drop the commit in question then, sorry Bjorn.
On Tue, Jan 24, 2023 at 03:45:50PM +0100, Rafael J. Wysocki wrote: > On Tue, Jan 24, 2023 at 3:37 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Tue, Jan 24, 2023 at 3:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > On Tue, Jan 24, 2023 at 12:20 PM Geert Uytterhoeven > > > <geert@linux-m68k.org> wrote: > > > > On Thu, Dec 15, 2022 at 7:23 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > > From: Bjorn Helgaas <bhelgaas@google.com> > > > > > > > > > > Simplify __rpm_get_callback() slightly by returning as soon as the return > > > > > value is known. No functional change intended. > > > > > > > > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > > > > > > > Thanks for your patch, which is now commit 650bdddb6b311705 ("PM: > > > > runtime: Simplify __rpm_get_callback()") in pm/linux-next. > > > > > > > > > --- a/drivers/base/power/runtime.c > > > > > +++ b/drivers/base/power/runtime.c > > > > > @@ -20,8 +20,7 @@ typedef int (*pm_callback_t)(struct device *); > > > > > > > > > > static pm_callback_t __rpm_get_callback(struct device *dev, size_t cb_offset) > > > > > { > > > > > - pm_callback_t cb; > > > > > - const struct dev_pm_ops *ops; > > > > > + const struct dev_pm_ops *ops = NULL; > > > > > > > > > > if (dev->pm_domain) > > > > > ops = &dev->pm_domain->ops; > > > > > @@ -31,18 +30,14 @@ static pm_callback_t __rpm_get_callback(struct device *dev, size_t cb_offset) > > > > > ops = dev->class->pm; > > > > > else if (dev->bus && dev->bus->pm) > > > > > ops = dev->bus->pm; > > > > > - else > > > > > - ops = NULL; > > > > > > > > > > if (ops) > > > > > - cb = *(pm_callback_t *)((void *)ops + cb_offset); > > > > > - else > > > > > - cb = NULL; > > > > > + return *(pm_callback_t *)((void *)ops + cb_offset); > > > > > > > > This is a change in behavior in case the callback turns out to be NULL: > > > > - before, it would fall back to the driver-specific callback below, > > > > - after, it always returns NULL. > > > > > > Good point and sorry for missing this! > > > > > > > > > > > > > - if (!cb && dev->driver && dev->driver->pm) > > > > > - cb = *(pm_callback_t *)((void *)dev->driver->pm + cb_offset); > > > > > + if (dev->driver && dev->driver->pm) > > > > > + return *(pm_callback_t *)((void *)dev->driver->pm + cb_offset); > > > > > > > > > > - return cb; > > > > > + return NULL; > > > > > } > > > > > > > > > > Something like the patch below (modulo gmail-induced whitespace > > > breakage) should restore the previous behavior if I'm not mistaken: > > > > > > --- > > > drivers/base/power/runtime.c | 9 +++++++-- > > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > > > Index: linux-pm/drivers/base/power/runtime.c > > > =================================================================== > > > --- linux-pm.orig/drivers/base/power/runtime.c > > > +++ linux-pm/drivers/base/power/runtime.c > > > @@ -31,8 +31,13 @@ static pm_callback_t __rpm_get_callback( > > > else if (dev->bus && dev->bus->pm) > > > ops = dev->bus->pm; > > > > > > - if (ops) > > > - return *(pm_callback_t *)((void *)ops + cb_offset); > > > + if (ops) { > > > + pm_callback_t cb; > > > + > > > + cb = *(pm_callback_t *)((void *)ops + cb_offset); > > > + if (cb) > > > + return cb; > > > + } > > > > > > if (dev->driver && dev->driver->pm) > > > return *(pm_callback_t *)((void *)dev->driver->pm + cb_offset); > > > > Which is now more complex than the original? > > Arguably so. > > OK, I'll drop the commit in question then, sorry Bjorn. Really sorry about this. Think I'm all out of brown paper bags :( Thanks for catching this, Geert, and sorry for all the time you had to spend debugging it. Bjorn
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index 50e726b6c2cf..7171ed0668f3 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -20,8 +20,7 @@ typedef int (*pm_callback_t)(struct device *); static pm_callback_t __rpm_get_callback(struct device *dev, size_t cb_offset) { - pm_callback_t cb; - const struct dev_pm_ops *ops; + const struct dev_pm_ops *ops = NULL; if (dev->pm_domain) ops = &dev->pm_domain->ops; @@ -31,18 +30,14 @@ static pm_callback_t __rpm_get_callback(struct device *dev, size_t cb_offset) ops = dev->class->pm; else if (dev->bus && dev->bus->pm) ops = dev->bus->pm; - else - ops = NULL; if (ops) - cb = *(pm_callback_t *)((void *)ops + cb_offset); - else - cb = NULL; + return *(pm_callback_t *)((void *)ops + cb_offset); - if (!cb && dev->driver && dev->driver->pm) - cb = *(pm_callback_t *)((void *)dev->driver->pm + cb_offset); + if (dev->driver && dev->driver->pm) + return *(pm_callback_t *)((void *)dev->driver->pm + cb_offset); - return cb; + return NULL; } #define RPM_GET_CALLBACK(dev, callback) \