[RFC/PATCH 0/3] pm: Make SET_*_PM_OPS() macros more smart

[RFC/PATCH 0/3] pm: Make SET_*_PM_OPS() macros more smart

[David Cohen]

Hi,

These patches are proposal to extend the lack of #ifdef checks on PM callback
to its implementation too.

Currently SET_*_PM_OPS() macros make #ifdefs checks not necessary when setting
the callback to PM ops, but the callbacks implementation don't see same
benefit.

This RFC Solves a problem reported by Santosh on xhci-plat.c driver due to
wrong #ifdef checks:

drivers/usb/host/xhci-plat.c:201:12: warning: ‘xhci_plat_suspend’ defined but not used [-Wunused-function]
drivers/usb/host/xhci-plat.c:209:12: warning: ‘xhci_plat_resume’ defined but not used [-Wunused-function]

But instead of fixing the #ifdefs, we remove the need for it :)

Br, David Cohen

---
David Cohen (2):
  pm: make PM macros more smart
  usb/xhci: implement proper static inline stubs when !CONFIG_PM

Santosh Shilimkar (1):
  usb/xhci-plat: remove unnecessary #ifdef checks for CONFIG_PM_SLEEP

 drivers/usb/host/xhci-plat.c |  7 +------
 drivers/usb/host/xhci.h      |  6 ++++--
 include/linux/pm.h           | 11 +++++++++--
 3 files changed, 14 insertions(+), 10 deletions(-)

-- 
1.8.4.2

[RFC/PATCH 1/3] pm: make PM macros more smart

[David Cohen]

This patch makes SET_SYSTEM_SLEEP_PM_OPS() and SET_RUNTIME_PM_OPS() more
smart.

Despite those macros check for '#ifdef CONFIG_PM_SLEEP/RUNTIME' to avoid
setting the callbacks when such #ifdef's aren't defined, they don't
handle compiler to avoid messages like that:

drivers/usb/host/xhci-plat.c:200:12: warning: ‘xhci_plat_suspend’ defined but not used [-Wunused-function]
drivers/usb/host/xhci-plat.c:208:12: warning: ‘xhci_plat_resume’ defined but not used [-Wunused-function]

As result, those macros get rid of #ifdef's when setting callbacks but
not when implementing them.

With this patch, drivers using SET_*_PM_OPS() macros don't need to #ifdef
the callbacks implementation as well.

Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
---
 include/linux/pm.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/linux/pm.h b/include/linux/pm.h
index a224c7f5c377..41a0f3b42209 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -299,6 +299,8 @@ struct dev_pm_ops {
 	int (*runtime_idle)(struct device *dev);
 };
 
+#define MAKE_ME_NULL(fn) ((void *)((unsigned long)(fn) - (unsigned long)(fn)))
+
 #ifdef CONFIG_PM_SLEEP
 #define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
 	.suspend = suspend_fn, \
@@ -308,7 +310,9 @@ struct dev_pm_ops {
 	.poweroff = suspend_fn, \
 	.restore = resume_fn,
 #else
-#define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
+#define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
+	.suspend = MAKE_ME_NULL(suspend_fn), \
+	.resume = MAKE_ME_NULL(resume_fn),
 #endif
 
 #ifdef CONFIG_PM_RUNTIME
@@ -317,7 +321,10 @@ struct dev_pm_ops {
 	.runtime_resume = resume_fn, \
 	.runtime_idle = idle_fn,
 #else
-#define SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn)
+#define SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn) \
+	.runtime_suspend = MAKE_ME_NULL(suspend_fn), \
+	.runtime_resume = MAKE_ME_NULL(resume_fn), \
+	.runtime_idle = MAKE_ME_NULL(idle_fn),
 #endif
 
 /*
-- 
1.8.4.2

[RFC/PATCH 2/3] usb/xhci: implement proper static inline stubs when !CONFIG_PM

[David Cohen]

Current xhci_suspend() and xhci_resume() implementation in case of
CONFIG_PM not defined is buggy. If we try to use them we get the
following error:

drivers/usb/host/xhci-plat.c: In function ‘xhci_plat_suspend’:
drivers/usb/host/xhci-plat.c:205:21: error: called object ‘0u’ is not a function
drivers/usb/host/xhci-plat.c: In function ‘xhci_plat_resume’:
drivers/usb/host/xhci-plat.c:213:20: error: called object ‘0u’ is not a function

It happens because the function names are replaced by NULL but the
brackets stay: NULL()

This patch implements proper static inline stubs.

Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
---
 drivers/usb/host/xhci.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 941d5f59e4dc..6a5e7a98de7e 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1771,8 +1771,10 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks);
 int xhci_suspend(struct xhci_hcd *xhci);
 int xhci_resume(struct xhci_hcd *xhci, bool hibernated);
 #else
-#define	xhci_suspend	NULL
-#define	xhci_resume	NULL
+static inline int
+xhci_suspend(struct xhci_hcd *xhci) { return 0; }
+static inline int
+xhci_resume(struct xhci_hcd *xhci, bool hibernated) { return 0; }
 #endif
 
 int xhci_get_frame(struct usb_hcd *hcd);
-- 
1.8.4.2

[RFC/PATCH 3/3] usb/xhci-plat: remove unnecessary #ifdef checks for CONFIG_PM_SLEEP

[David Cohen]

From: Santosh Shilimkar <santosh.shilimkar@ti.com>

Drivers using SET_*_PM_OPS() no longer need to #ifdef for CONFIG_PM_*
So, let's remove the unnecessary #ifdef's.

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
---
 drivers/usb/host/xhci-plat.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index d9c169f470d3..b1d93c344e04 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -197,7 +197,6 @@ static int xhci_plat_remove(struct platform_device *dev)
 	return 0;
 }
 
-#ifdef CONFIG_PM
 static int xhci_plat_suspend(struct device *dev)
 {
 	struct usb_hcd	*hcd = dev_get_drvdata(dev);
@@ -217,10 +216,6 @@ static int xhci_plat_resume(struct device *dev)
 static const struct dev_pm_ops xhci_plat_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(xhci_plat_suspend, xhci_plat_resume)
 };
-#define DEV_PM_OPS	(&xhci_plat_pm_ops)
-#else
-#define DEV_PM_OPS	NULL
-#endif /* CONFIG_PM */
 
 #ifdef CONFIG_OF
 static const struct of_device_id usb_xhci_of_match[] = {
@@ -235,7 +230,7 @@ static struct platform_driver usb_xhci_driver = {
 	.remove	= xhci_plat_remove,
 	.driver	= {
 		.name = "xhci-hcd",
-		.pm = DEV_PM_OPS,
+		.pm = &xhci_plat_pm_ops,
 		.of_match_table = of_match_ptr(usb_xhci_of_match),
 	},
 };
-- 
1.8.4.2

Re: [RFC/PATCH 3/3] usb/xhci-plat: remove unnecessary #ifdef checks for CONFIG_PM_SLEEP

[Ulf Hansson]

On 13 December 2013 06:18, David Cohen <david.a.cohen@linux.intel.com> wrote:
> From: Santosh Shilimkar <santosh.shilimkar@ti.com>
>
> Drivers using SET_*_PM_OPS() no longer need to #ifdef for CONFIG_PM_*
> So, let's remove the unnecessary #ifdef's.
>
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> ---
>  drivers/usb/host/xhci-plat.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index d9c169f470d3..b1d93c344e04 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -197,7 +197,6 @@ static int xhci_plat_remove(struct platform_device *dev)
>         return 0;
>  }
>
> -#ifdef CONFIG_PM

I think you can solve this in another way.

As a start, I would suggest to change above to CONFIG_PM_SLEEP

>  static int xhci_plat_suspend(struct device *dev)
>  {
>         struct usb_hcd  *hcd = dev_get_drvdata(dev);
> @@ -217,10 +216,6 @@ static int xhci_plat_resume(struct device *dev)
>  static const struct dev_pm_ops xhci_plat_pm_ops = {
>         SET_SYSTEM_SLEEP_PM_OPS(xhci_plat_suspend, xhci_plat_resume)
>  };
> -#define DEV_PM_OPS     (&xhci_plat_pm_ops)
> -#else
> -#define DEV_PM_OPS     NULL
> -#endif /* CONFIG_PM */

Remove the use of DEV_PM_OPS define. Instead use below macro and
outside #ifdef CONFIG_PM_SLEEP.

"static SIMPLE_DEV_PM_OPS(xhci_plat_pm_ops, xhci_plat_suspend,
xhci_plat_resume)"

That should do the trick I believe, without the need for changing the
existing SET_SYSTEM_SLEEP_PM_OPS macro in patch1.

Kind regards
Ulf Hansson

>
>  #ifdef CONFIG_OF
>  static const struct of_device_id usb_xhci_of_match[] = {
> @@ -235,7 +230,7 @@ static struct platform_driver usb_xhci_driver = {
>         .remove = xhci_plat_remove,
>         .driver = {
>                 .name = "xhci-hcd",
> -               .pm = DEV_PM_OPS,
> +               .pm = &xhci_plat_pm_ops,
>                 .of_match_table = of_match_ptr(usb_xhci_of_match),
>         },
>  };
> --
> 1.8.4.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC/PATCH 3/3] usb/xhci-plat: remove unnecessary #ifdef checks for CONFIG_PM_SLEEP

[David Cohen]

On Fri, Dec 13, 2013 at 09:19:34AM +0100, Ulf Hansson wrote:
> On 13 December 2013 06:18, David Cohen <david.a.cohen@linux.intel.com> wrote:
> > From: Santosh Shilimkar <santosh.shilimkar@ti.com>
> >
> > Drivers using SET_*_PM_OPS() no longer need to #ifdef for CONFIG_PM_*
> > So, let's remove the unnecessary #ifdef's.
> >
> > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> > ---
> >  drivers/usb/host/xhci-plat.c | 7 +------
> >  1 file changed, 1 insertion(+), 6 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index d9c169f470d3..b1d93c344e04 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -197,7 +197,6 @@ static int xhci_plat_remove(struct platform_device *dev)
> >         return 0;
> >  }
> >
> > -#ifdef CONFIG_PM
> 
> I think you can solve this in another way.
> 
> As a start, I would suggest to change above to CONFIG_PM_SLEEP
> 
> >  static int xhci_plat_suspend(struct device *dev)
> >  {
> >         struct usb_hcd  *hcd = dev_get_drvdata(dev);
> > @@ -217,10 +216,6 @@ static int xhci_plat_resume(struct device *dev)
> >  static const struct dev_pm_ops xhci_plat_pm_ops = {
> >         SET_SYSTEM_SLEEP_PM_OPS(xhci_plat_suspend, xhci_plat_resume)
> >  };
> > -#define DEV_PM_OPS     (&xhci_plat_pm_ops)
> > -#else
> > -#define DEV_PM_OPS     NULL
> > -#endif /* CONFIG_PM */
> 
> Remove the use of DEV_PM_OPS define. Instead use below macro and
> outside #ifdef CONFIG_PM_SLEEP.
> 
> "static SIMPLE_DEV_PM_OPS(xhci_plat_pm_ops, xhci_plat_suspend,
> xhci_plat_resume)"
> 
> That should do the trick I believe, without the need for changing the
> existing SET_SYSTEM_SLEEP_PM_OPS macro in patch1.

Yes, it does. That matches my suggestion in this e-mail:
http://marc.info/?l=linux-kernel&m=138690490016503&w=2

But I personally don't like #ifdef's. In this case, SET_*_PM_OPS()
macros handle #ifdef's when setting the callbacks. My intention is to
extend it to callbacks' implementation too making the code cleaner

Br, David Cohen

> 
> Kind regards
> Ulf Hansson
> 
> >
> >  #ifdef CONFIG_OF
> >  static const struct of_device_id usb_xhci_of_match[] = {
> > @@ -235,7 +230,7 @@ static struct platform_driver usb_xhci_driver = {
> >         .remove = xhci_plat_remove,
> >         .driver = {
> >                 .name = "xhci-hcd",
> > -               .pm = DEV_PM_OPS,
> > +               .pm = &xhci_plat_pm_ops,
> >                 .of_match_table = of_match_ptr(usb_xhci_of_match),
> >         },
> >  };
> > --
> > 1.8.4.2
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC/PATCH 1/3] pm: make PM macros more smart

[Pavel Machek]

On Thu 2013-12-12 21:18:23, David Cohen wrote:
> This patch makes SET_SYSTEM_SLEEP_PM_OPS() and SET_RUNTIME_PM_OPS() more
> smart.
> 
> Despite those macros check for '#ifdef CONFIG_PM_SLEEP/RUNTIME' to avoid
> setting the callbacks when such #ifdef's aren't defined, they don't
> handle compiler to avoid messages like that:
> 
> drivers/usb/host/xhci-plat.c:200:12: warning: ???xhci_plat_suspend??? defined but not used [-Wunused-function]
> drivers/usb/host/xhci-plat.c:208:12: warning: ???xhci_plat_resume??? defined but not used [-Wunused-function]
> 
> As result, those macros get rid of #ifdef's when setting callbacks but
> not when implementing them.
> 
> With this patch, drivers using SET_*_PM_OPS() macros don't need to #ifdef
> the callbacks implementation as well.

Well... Interesting trickery, but it means that resulting kernel
will be bigge due to the dead functions no?

That may be acceptable tradeoff, but I guess its worth discussing...
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC/PATCH 1/3] pm: make PM macros more smart

[David Cohen]

On Sun, Dec 15, 2013 at 06:51:12PM +0100, Pavel Machek wrote:
> On Thu 2013-12-12 21:18:23, David Cohen wrote:
> > This patch makes SET_SYSTEM_SLEEP_PM_OPS() and SET_RUNTIME_PM_OPS() more
> > smart.
> > 
> > Despite those macros check for '#ifdef CONFIG_PM_SLEEP/RUNTIME' to avoid
> > setting the callbacks when such #ifdef's aren't defined, they don't
> > handle compiler to avoid messages like that:
> > 
> > drivers/usb/host/xhci-plat.c:200:12: warning: ???xhci_plat_suspend??? defined but not used [-Wunused-function]
> > drivers/usb/host/xhci-plat.c:208:12: warning: ???xhci_plat_resume??? defined but not used [-Wunused-function]
> > 
> > As result, those macros get rid of #ifdef's when setting callbacks but
> > not when implementing them.
> > 
> > With this patch, drivers using SET_*_PM_OPS() macros don't need to #ifdef
> > the callbacks implementation as well.
> 
> Well... Interesting trickery, but it means that resulting kernel
> will be bigge due to the dead functions no?

Actually, it doesn't get bigger. Before sending the patch I did this
dummy test app:

--------------------------------------------------------------------------------
#include <stdio.h>

#define USE_IT_OR_LOOSE_IT(fn)  ((void *)((unsigned long)(fn) - (unsigned long)(fn)))

#ifdef MAKE_ME_NULL
static int func1(int a)
{
        printf("Hey!!\n");
        return 0;
}
#endif

struct global_data {
        int (*func)(int);
};

static struct global_data gd = {
#ifdef MAKE_ME_NULL
        .func = USE_IT_OR_LOOSE_IT(func1),
#endif
};

int main(void)
{
#ifdef MAKE_ME_NULL
        printf("MAKE_ME_NULL is defined\n");
#else
        printf("MAKE_ME_NULL is NOT defined\n");
#endif
        printf("%p\n", gd.func);
        return 0;
}
--------------------------------------------------------------------------------

Then I compiled 2 .S files:
$ gcc -DMAKE_ME_NULL test1.c -O2 -S -o test_makemenull.S
$ gcc test1.c -O2 -S -o test_no_makemenull.S

This is the diff between both:

--------------------------------------------------------------------------------
--- test_makemenull.S	2013-12-15 11:07:02.635992492 -0800
+++ test_no_makemenull.S	2013-12-15 11:07:10.639992359 -0800
@@ -1,7 +1,7 @@
 	.file	"test1.c"
 	.section	.rodata.str1.1,"aMS",@progbits,1
 .LC0:
-	.string	"MAKE_ME_NULL is defined"
+	.string	"MAKE_ME_NULL is NOT defined"
 .LC1:
 	.string	"%p\n"
 	.section	.text.startup,"ax",@progbits
@@ -9,7 +9,7 @@
 	.globl	main
 	.type	main, @function
 main:
-.LFB12:
+.LFB11:
 	.cfi_startproc
 	subq	$8, %rsp
 	.cfi_def_cfa_offset 16
@@ -24,7 +24,7 @@
 	.cfi_def_cfa_offset 8
 	ret
 	.cfi_endproc
-.LFE12:
+.LFE11:
 	.size	main, .-main
 	.ident	"GCC: (Debian 4.8.2-1) 4.8.2"
 	.section	.note.GNU-stack,"",@progbits
--------------------------------------------------------------------------------

My conclusion is gcc's -O2 handles this situation pretty well, which
makes my patch to have not much actual side effects on kernel binary.

Br, David Cohen

> 
> That may be acceptable tradeoff, but I guess its worth discussing...
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC/PATCH 0/3] pm: Make SET_*_PM_OPS() macros more smart

[David Cohen]

On Thu, Dec 12, 2013 at 09:18:22PM -0800, David Cohen wrote:
> Hi,
> 
> These patches are proposal to extend the lack of #ifdef checks on PM callback
> to its implementation too.
> 
> Currently SET_*_PM_OPS() macros make #ifdefs checks not necessary when setting
> the callback to PM ops, but the callbacks implementation don't see same
> benefit.
> 
> This RFC Solves a problem reported by Santosh on xhci-plat.c driver due to
> wrong #ifdef checks:
> 
> drivers/usb/host/xhci-plat.c:201:12: warning: ‘xhci_plat_suspend’ defined but not used [-Wunused-function]
> drivers/usb/host/xhci-plat.c:209:12: warning: ‘xhci_plat_resume’ defined but not used [-Wunused-function]
> 
> But instead of fixing the #ifdefs, we remove the need for it :)

Ping. Comments here? :)

Br, David

> 
> Br, David Cohen
> 
> ---
> David Cohen (2):
>   pm: make PM macros more smart
>   usb/xhci: implement proper static inline stubs when !CONFIG_PM
> 
> Santosh Shilimkar (1):
>   usb/xhci-plat: remove unnecessary #ifdef checks for CONFIG_PM_SLEEP
> 
>  drivers/usb/host/xhci-plat.c |  7 +------
>  drivers/usb/host/xhci.h      |  6 ++++--
>  include/linux/pm.h           | 11 +++++++++--
>  3 files changed, 14 insertions(+), 10 deletions(-)
> 
> -- 
> 1.8.4.2

Re: [RFC/PATCH 0/3] pm: Make SET_*_PM_OPS() macros more smart

[David Cohen]

On Thu, Dec 12, 2013 at 09:18:22PM -0800, David Cohen wrote:
> Hi,
> 
> These patches are proposal to extend the lack of #ifdef checks on PM callback
> to its implementation too.
> 
> Currently SET_*_PM_OPS() macros make #ifdefs checks not necessary when setting
> the callback to PM ops, but the callbacks implementation don't see same
> benefit.
> 
> This RFC Solves a problem reported by Santosh on xhci-plat.c driver due to
> wrong #ifdef checks:
> 
> drivers/usb/host/xhci-plat.c:201:12: warning: ‘xhci_plat_suspend’ defined but not used [-Wunused-function]
> drivers/usb/host/xhci-plat.c:209:12: warning: ‘xhci_plat_resume’ defined but not used [-Wunused-function]
> 
> But instead of fixing the #ifdefs, we remove the need for it :)

Ping. Comments here? :)

Br, David

> 
> Br, David Cohen
> 
> ---
> David Cohen (2):
>   pm: make PM macros more smart
>   usb/xhci: implement proper static inline stubs when !CONFIG_PM
> 
> Santosh Shilimkar (1):
>   usb/xhci-plat: remove unnecessary #ifdef checks for CONFIG_PM_SLEEP
> 
>  drivers/usb/host/xhci-plat.c |  7 +------
>  drivers/usb/host/xhci.h      |  6 ++++--
>  include/linux/pm.h           | 11 +++++++++--
>  3 files changed, 14 insertions(+), 10 deletions(-)
> 
> -- 
> 1.8.4.2
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH 1/3] pm: make PM macros more smart

[Pavel Machek]

On Sun 2013-12-15 11:25:08, David Cohen wrote:
> On Sun, Dec 15, 2013 at 06:51:12PM +0100, Pavel Machek wrote:
> > On Thu 2013-12-12 21:18:23, David Cohen wrote:
> > > This patch makes SET_SYSTEM_SLEEP_PM_OPS() and SET_RUNTIME_PM_OPS() more
> > > smart.
> > > 
> > > Despite those macros check for '#ifdef CONFIG_PM_SLEEP/RUNTIME' to avoid
> > > setting the callbacks when such #ifdef's aren't defined, they don't
> > > handle compiler to avoid messages like that:
> > > 
> > > drivers/usb/host/xhci-plat.c:200:12: warning: ???xhci_plat_suspend??? defined but not used [-Wunused-function]
> > > drivers/usb/host/xhci-plat.c:208:12: warning: ???xhci_plat_resume??? defined but not used [-Wunused-function]
> > > 
> > > As result, those macros get rid of #ifdef's when setting callbacks but
> > > not when implementing them.
> > > 
> > > With this patch, drivers using SET_*_PM_OPS() macros don't need to #ifdef
> > > the callbacks implementation as well.
> > 
> > Well... Interesting trickery, but it means that resulting kernel
> > will be bigge due to the dead functions no?
> 
> Actually, it doesn't get bigger. Before sending the patch I did this
> dummy test app:
> 
> --------------------------------------------------------------------------------
> #include <stdio.h>
> 
> #define USE_IT_OR_LOOSE_IT(fn)  ((void *)((unsigned long)(fn) - (unsigned long)(fn)))
> 
> #ifdef MAKE_ME_NULL
> static int func1(int a)
> {
>         printf("Hey!!\n");
>         return 0;
> }
> #endif

I thought that point of this patch series was getting rid of the
#ifdefs around the function...? Now I'm confused.

> struct global_data {
>         int (*func)(int);
> };
> 
> static struct global_data gd = {
> #ifdef MAKE_ME_NULL
>         .func = USE_IT_OR_LOOSE_IT(func1),

If you have ifdef around the function, why do you need magic here? Why
not

	.func = func1

?

Basically the warning tells you that you want the ifdef around the
function, too... (Otherwise you waste space). That seems like good
warning.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC/PATCH 1/3] pm: make PM macros more smart

[David Cohen]

On Fri, Dec 20, 2013 at 08:55:27PM +0100, Pavel Machek wrote:
> On Sun 2013-12-15 11:25:08, David Cohen wrote:
> > On Sun, Dec 15, 2013 at 06:51:12PM +0100, Pavel Machek wrote:
> > > On Thu 2013-12-12 21:18:23, David Cohen wrote:
> > > > This patch makes SET_SYSTEM_SLEEP_PM_OPS() and SET_RUNTIME_PM_OPS() more
> > > > smart.
> > > > 
> > > > Despite those macros check for '#ifdef CONFIG_PM_SLEEP/RUNTIME' to avoid
> > > > setting the callbacks when such #ifdef's aren't defined, they don't
> > > > handle compiler to avoid messages like that:
> > > > 
> > > > drivers/usb/host/xhci-plat.c:200:12: warning: ???xhci_plat_suspend??? defined but not used [-Wunused-function]
> > > > drivers/usb/host/xhci-plat.c:208:12: warning: ???xhci_plat_resume??? defined but not used [-Wunused-function]
> > > > 
> > > > As result, those macros get rid of #ifdef's when setting callbacks but
> > > > not when implementing them.
> > > > 
> > > > With this patch, drivers using SET_*_PM_OPS() macros don't need to #ifdef
> > > > the callbacks implementation as well.
> > > 
> > > Well... Interesting trickery, but it means that resulting kernel
> > > will be bigge due to the dead functions no?
> > 
> > Actually, it doesn't get bigger. Before sending the patch I did this
> > dummy test app:
> > 
> > --------------------------------------------------------------------------------
> > #include <stdio.h>
> > 
> > #define USE_IT_OR_LOOSE_IT(fn)  ((void *)((unsigned long)(fn) - (unsigned long)(fn)))
> > 
> > #ifdef MAKE_ME_NULL
> > static int func1(int a)
> > {
> >         printf("Hey!!\n");
> >         return 0;
> > }
> > #endif
> 
> I thought that point of this patch series was getting rid of the
> #ifdefs around the function...? Now I'm confused.

Maybe you're misinterpreting the test :)

This #ifdef is used to make this same test code to replicate both
scenarios according to -DMAKE_ME_NULL (just pay attention to actual
resulting code after #ifdef's are tested. the #ifdef here is nor related
to actual #ifdef on kernel). Here are both scenarios:

(1) Not using my trickery (which needs the function to not be present).
(2) Using my trickery (which needs to function to stay).

With -DMAKE_ME_NULL we replicate (2), then the function *is* there but
gcc gets rid of it on resulting binary without warnings if used with -O2.

Without -DMAKE_ME_NULL we replicate (1). The #ifdef will fail and then
remove the function which is an obvious scenario the function won't be
part of resulting binary.

If we use -S option to have human readable resulting assembly code
(which is kind of 1:1 for resulting binary), we can compare the result
of (1) and (2) and check they are pretty similar.
This proves gcc behaves as expected with my patch: do not need #ifdef
and do not generate dead codes to resulting binary.

> 
> > struct global_data {
> >         int (*func)(int);
> > };
> > 
> > static struct global_data gd = {
> > #ifdef MAKE_ME_NULL
> >         .func = USE_IT_OR_LOOSE_IT(func1),
> 
> If you have ifdef around the function, why do you need magic here? Why
> not

This #ifdef is necessary to prevent the function to be used when it
doesn't exist due to above #ifdef. But once again: don't misinterpret
the #ifdefs in this test app with the ones in kernel. They are not
related at all. If it's still confusing you just make 2 test apps
without #ifdeds out of this one where one keeps the code inside #ifdefs
and the other doesn't.

> 
> 	.func = func1
> 
> ?
> 
> Basically the warning tells you that you want the ifdef around the
> function, too... (Otherwise you waste space). That seems like good
> warning.

Just check my first explanation.

Br, David Cohen

> 
> 									Pavel
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC/PATCH 1/3] pm: make PM macros more smart

[David Cohen]

On Fri, Dec 20, 2013 at 12:23:36PM -0800, David Cohen wrote:
> On Fri, Dec 20, 2013 at 08:55:27PM +0100, Pavel Machek wrote:
> > On Sun 2013-12-15 11:25:08, David Cohen wrote:
> > > On Sun, Dec 15, 2013 at 06:51:12PM +0100, Pavel Machek wrote:
> > > > On Thu 2013-12-12 21:18:23, David Cohen wrote:
> > > > > This patch makes SET_SYSTEM_SLEEP_PM_OPS() and SET_RUNTIME_PM_OPS() more
> > > > > smart.
> > > > > 
> > > > > Despite those macros check for '#ifdef CONFIG_PM_SLEEP/RUNTIME' to avoid
> > > > > setting the callbacks when such #ifdef's aren't defined, they don't
> > > > > handle compiler to avoid messages like that:
> > > > > 
> > > > > drivers/usb/host/xhci-plat.c:200:12: warning: ???xhci_plat_suspend??? defined but not used [-Wunused-function]
> > > > > drivers/usb/host/xhci-plat.c:208:12: warning: ???xhci_plat_resume??? defined but not used [-Wunused-function]
> > > > > 
> > > > > As result, those macros get rid of #ifdef's when setting callbacks but
> > > > > not when implementing them.
> > > > > 
> > > > > With this patch, drivers using SET_*_PM_OPS() macros don't need to #ifdef
> > > > > the callbacks implementation as well.
> > > > 
> > > > Well... Interesting trickery, but it means that resulting kernel
> > > > will be bigge due to the dead functions no?
> > > 
> > > Actually, it doesn't get bigger. Before sending the patch I did this
> > > dummy test app:
> > > 
> > > --------------------------------------------------------------------------------
> > > #include <stdio.h>
> > > 
> > > #define USE_IT_OR_LOOSE_IT(fn)  ((void *)((unsigned long)(fn) - (unsigned long)(fn)))
> > > 
> > > #ifdef MAKE_ME_NULL
> > > static int func1(int a)
> > > {
> > >         printf("Hey!!\n");
> > >         return 0;
> > > }
> > > #endif
> > 
> > I thought that point of this patch series was getting rid of the
> > #ifdefs around the function...? Now I'm confused.
> 
> Maybe you're misinterpreting the test :)
> 
> This #ifdef is used to make this same test code to replicate both
> scenarios according to -DMAKE_ME_NULL (just pay attention to actual
> resulting code after #ifdef's are tested. the #ifdef here is nor related
> to actual #ifdef on kernel). Here are both scenarios:
> 
> (1) Not using my trickery (which needs the function to not be present).
> (2) Using my trickery (which needs to function to stay).
> 
> With -DMAKE_ME_NULL we replicate (2), then the function *is* there but
> gcc gets rid of it on resulting binary without warnings if used with -O2.
> 
> Without -DMAKE_ME_NULL we replicate (1). The #ifdef will fail and then
> remove the function which is an obvious scenario the function won't be
> part of resulting binary.
> 
> If we use -S option to have human readable resulting assembly code
> (which is kind of 1:1 for resulting binary), we can compare the result
> of (1) and (2) and check they are pretty similar.
> This proves gcc behaves as expected with my patch: do not need #ifdef
> and do not generate dead codes to resulting binary.
> 
> > 
> > > struct global_data {
> > >         int (*func)(int);
> > > };
> > > 
> > > static struct global_data gd = {
> > > #ifdef MAKE_ME_NULL
> > >         .func = USE_IT_OR_LOOSE_IT(func1),
> > 
> > If you have ifdef around the function, why do you need magic here? Why
> > not
> 
> This #ifdef is necessary to prevent the function to be used when it
> doesn't exist due to above #ifdef. But once again: don't misinterpret
> the #ifdefs in this test app with the ones in kernel. They are not
> related at all. If it's still confusing you just make 2 test apps
> without #ifdeds out of this one where one keeps the code inside #ifdefs
> and the other doesn't.
> 
> > 
> > 	.func = func1
> > 
> > ?
> > 
> > Basically the warning tells you that you want the ifdef around the
> > function, too... (Otherwise you waste space). That seems like good
> > warning.
> 
> Just check my first explanation.

Ping :)

Comments here?

Br, David Cohen

Re: [RFC/PATCH 1/3] pm: make PM macros more smart

[David Cohen]

On Tue, Jan 14, 2014 at 02:42:11PM -0800, David Cohen wrote:
> On Fri, Dec 20, 2013 at 12:23:36PM -0800, David Cohen wrote:
> > On Fri, Dec 20, 2013 at 08:55:27PM +0100, Pavel Machek wrote:
> > > On Sun 2013-12-15 11:25:08, David Cohen wrote:
> > > > On Sun, Dec 15, 2013 at 06:51:12PM +0100, Pavel Machek wrote:
> > > > > On Thu 2013-12-12 21:18:23, David Cohen wrote:
> > > > > > This patch makes SET_SYSTEM_SLEEP_PM_OPS() and SET_RUNTIME_PM_OPS() more
> > > > > > smart.
> > > > > > 
> > > > > > Despite those macros check for '#ifdef CONFIG_PM_SLEEP/RUNTIME' to avoid
> > > > > > setting the callbacks when such #ifdef's aren't defined, they don't
> > > > > > handle compiler to avoid messages like that:
> > > > > > 
> > > > > > drivers/usb/host/xhci-plat.c:200:12: warning: ???xhci_plat_suspend??? defined but not used [-Wunused-function]
> > > > > > drivers/usb/host/xhci-plat.c:208:12: warning: ???xhci_plat_resume??? defined but not used [-Wunused-function]
> > > > > > 
> > > > > > As result, those macros get rid of #ifdef's when setting callbacks but
> > > > > > not when implementing them.
> > > > > > 
> > > > > > With this patch, drivers using SET_*_PM_OPS() macros don't need to #ifdef
> > > > > > the callbacks implementation as well.
> > > > > 
> > > > > Well... Interesting trickery, but it means that resulting kernel
> > > > > will be bigge due to the dead functions no?
> > > > 
> > > > Actually, it doesn't get bigger. Before sending the patch I did this
> > > > dummy test app:
> > > > 
> > > > --------------------------------------------------------------------------------
> > > > #include <stdio.h>
> > > > 
> > > > #define USE_IT_OR_LOOSE_IT(fn)  ((void *)((unsigned long)(fn) - (unsigned long)(fn)))
> > > > 
> > > > #ifdef MAKE_ME_NULL
> > > > static int func1(int a)
> > > > {
> > > >         printf("Hey!!\n");
> > > >         return 0;
> > > > }
> > > > #endif
> > > 
> > > I thought that point of this patch series was getting rid of the
> > > #ifdefs around the function...? Now I'm confused.
> > 
> > Maybe you're misinterpreting the test :)
> > 
> > This #ifdef is used to make this same test code to replicate both
> > scenarios according to -DMAKE_ME_NULL (just pay attention to actual
> > resulting code after #ifdef's are tested. the #ifdef here is nor related
> > to actual #ifdef on kernel). Here are both scenarios:
> > 
> > (1) Not using my trickery (which needs the function to not be present).
> > (2) Using my trickery (which needs to function to stay).
> > 
> > With -DMAKE_ME_NULL we replicate (2), then the function *is* there but
> > gcc gets rid of it on resulting binary without warnings if used with -O2.
> > 
> > Without -DMAKE_ME_NULL we replicate (1). The #ifdef will fail and then
> > remove the function which is an obvious scenario the function won't be
> > part of resulting binary.
> > 
> > If we use -S option to have human readable resulting assembly code
> > (which is kind of 1:1 for resulting binary), we can compare the result
> > of (1) and (2) and check they are pretty similar.
> > This proves gcc behaves as expected with my patch: do not need #ifdef
> > and do not generate dead codes to resulting binary.
> > 
> > > 
> > > > struct global_data {
> > > >         int (*func)(int);
> > > > };
> > > > 
> > > > static struct global_data gd = {
> > > > #ifdef MAKE_ME_NULL
> > > >         .func = USE_IT_OR_LOOSE_IT(func1),
> > > 
> > > If you have ifdef around the function, why do you need magic here? Why
> > > not
> > 
> > This #ifdef is necessary to prevent the function to be used when it
> > doesn't exist due to above #ifdef. But once again: don't misinterpret
> > the #ifdefs in this test app with the ones in kernel. They are not
> > related at all. If it's still confusing you just make 2 test apps
> > without #ifdeds out of this one where one keeps the code inside #ifdefs
> > and the other doesn't.
> > 
> > > 
> > > 	.func = func1
> > > 
> > > ?
> > > 
> > > Basically the warning tells you that you want the ifdef around the
> > > function, too... (Otherwise you waste space). That seems like good
> > > warning.
> > 
> > Just check my first explanation.
> 
> Ping :)
> 
> Comments here?

I found few problems to be fixed prior to be possible this optimization.

Many drivers are calling SET_*_PM_OPS() functions and passing
non-defined symbols as argument. It's not triggering compilation issue
currently because the drivers synchronize when the symbols are not
defined with SET_*_PM_OPS() not using them. But IMHO it's a violation of
scopes: all drivers calling SET_*_PM_OPS() should give valid symbols,
since it creates an unwanted cross-dependence between those PM functions
and their users (why SET_*_PM_OPS() can't use symbols given to them as
argument?).

I can work on fixing SET_*_PM_OPS() users beforehand in case my proposal
here is accepted.

Br, David Cohen

> 
> Br, David Cohen