The next-20080314 tree build fails
drivers/serial/serial_core.c: In function `uart_add_one_port':
drivers/serial/serial_core.c:2359: error: invalid lvalue in assignment
make[2]: *** [drivers/serial/serial_core.o] Error 1
The config # CONFIG_PM was not set.The code which is causing the
build failure is
device_can_wakeup(tty_dev) = 1;
when the CONFIG_PM is set the macro is preprocessed as
#define device_can_wakeup(dev) \
((dev)->power.can_wakeup)
and when not set, it becomes 0 = 1
#define device_can_wakeup(dev) 0
--
Thanks & Regards,
Kamalesh Babulal,
Linux Technology Center,
IBM, ISTL.
On Fri, 14 Mar 2008 13:27:10 +0530
Kamalesh Babulal <[email protected]> wrote:
> The next-20080314 tree build fails
>
> drivers/serial/serial_core.c: In function `uart_add_one_port':
> drivers/serial/serial_core.c:2359: error: invalid lvalue in assignment
> make[2]: *** [drivers/serial/serial_core.o] Error 1
>
> The config # CONFIG_PM was not set.The code which is causing the
> build failure is
>
> device_can_wakeup(tty_dev) = 1;
>
> when the CONFIG_PM is set the macro is preprocessed as
>
> #define device_can_wakeup(dev) \
> ((dev)->power.can_wakeup)
>
> and when not set, it becomes 0 = 1
>
> #define device_can_wakeup(dev) 0
>
Caused by Alan's "PM: make wakeup flags available whenever CONFIG_PM is
set" which I assume Len merged.
Sorry, but that code should be dragged out and shot. Doing this:
> device_can_wakeup(tty_dev) = 1;
is just gross and stupid. It looks daft, it isn't C and it *requires* that
device_can_wakeup() be implemented as a macro, which totally busts any
concept of abstraction.
The code previously had quite reasonable accessors:
#define device_set_wakeup_enable(dev,val) \
((dev)->power.should_wakeup = !!(val))
#define device_may_wakeup(dev) \
(device_can_wakeup(dev) && (dev)->power.should_wakeup)
can we please go back to that scheme? Please also convert all these
magical macros into inlined C functions. One reason is that this:
+#define device_init_wakeup(dev,val) \
+ do { \
+ device_can_wakeup(dev) = !!(val); \
+ device_set_wakeup_enable(dev,val); \
+ } while(0)
is buggy. It evaluates its arguments multiple times and will cause
failures when passed expressions which have side-effects.
Alan, please also pass all future patches through checkpatch - there is no
need to be adding trivial coding-style errors in this day and age.
Thanks.
On Friday, 14 of March 2008, Andrew Morton wrote:
> On Fri, 14 Mar 2008 13:27:10 +0530
> Kamalesh Babulal <[email protected]> wrote:
>
> > The next-20080314 tree build fails
> >
> > drivers/serial/serial_core.c: In function `uart_add_one_port':
> > drivers/serial/serial_core.c:2359: error: invalid lvalue in assignment
> > make[2]: *** [drivers/serial/serial_core.o] Error 1
> >
> > The config # CONFIG_PM was not set.The code which is causing the
> > build failure is
> >
> > device_can_wakeup(tty_dev) = 1;
> >
> > when the CONFIG_PM is set the macro is preprocessed as
> >
> > #define device_can_wakeup(dev) \
> > ((dev)->power.can_wakeup)
> >
> > and when not set, it becomes 0 = 1
> >
> > #define device_can_wakeup(dev) 0
Kamalesh, thanks for reporting the problem.
> Caused by Alan's "PM: make wakeup flags available whenever CONFIG_PM is
> set" which I assume Len merged.
It's in the Greg's tree actually.
> Sorry, but that code should be dragged out and shot. Doing this:
>
> > device_can_wakeup(tty_dev) = 1;
>
> is just gross and stupid. It looks daft, it isn't C and it *requires* that
> device_can_wakeup() be implemented as a macro, which totally busts any
> concept of abstraction.
>
>
> The code previously had quite reasonable accessors:
>
> #define device_set_wakeup_enable(dev,val) \
> ((dev)->power.should_wakeup = !!(val))
> #define device_may_wakeup(dev) \
> (device_can_wakeup(dev) && (dev)->power.should_wakeup)
>
> can we please go back to that scheme? Please also convert all these
> magical macros into inlined C functions. One reason is that this:
>
> +#define device_init_wakeup(dev,val) \
> + do { \
> + device_can_wakeup(dev) = !!(val); \
> + device_set_wakeup_enable(dev,val); \
> + } while(0)
>
> is buggy. It evaluates its arguments multiple times and will cause
> failures when passed expressions which have side-effects.
>
> Alan, please also pass all future patches through checkpatch - there is no
> need to be adding trivial coding-style errors in this day and age.
>
> Thanks.
Well, Greg please drop the "PM: make wakeup flags available whenever CONFIG_PM
is set" and Alan please resubmit the patch with the problems pointed out by
Andrew fixed.
In fact I'd even prefer it to be two patches, one that moves should_wakeup and
the related macros out of the CONFIG_PM_SLEEP #ifdef, because that's what fixes
the problem described in the changelog, and one that makes the remaining
changes with a separate changelog.
Thanks,
Rafael
On Fri, 14 Mar 2008, Rafael J. Wysocki wrote:
> On Friday, 14 of March 2008, Andrew Morton wrote:
> > Sorry, but that code should be dragged out and shot. Doing this:
> >
> > > device_can_wakeup(tty_dev) = 1;
> >
> > is just gross and stupid. It looks daft, it isn't C and it *requires* that
> > device_can_wakeup() be implemented as a macro, which totally busts any
> > concept of abstraction.
It seems to have been a one-time occurrence. This patch series fixes
it.
> > The code previously had quite reasonable accessors:
> >
> > #define device_set_wakeup_enable(dev,val) \
> > ((dev)->power.should_wakeup = !!(val))
> > #define device_may_wakeup(dev) \
> > (device_can_wakeup(dev) && (dev)->power.should_wakeup)
> >
> > can we please go back to that scheme? Please also convert all these
> > magical macros into inlined C functions. One reason is that this:
> >
> > +#define device_init_wakeup(dev,val) \
> > + do { \
> > + device_can_wakeup(dev) = !!(val); \
> > + device_set_wakeup_enable(dev,val); \
> > + } while(0)
> >
> > is buggy. It evaluates its arguments multiple times and will cause
> > failures when passed expressions which have side-effects.
This series converts the macros to inline functions.
> > Alan, please also pass all future patches through checkpatch - there is no
> > need to be adding trivial coding-style errors in this day and age.
Rest assurred that checkpath is now an integral part of my normal
workflow. All the patches in this series pass with flying colors.
> Well, Greg please drop the "PM: make wakeup flags available whenever CONFIG_PM
> is set" and Alan please resubmit the patch with the problems pointed out by
> Andrew fixed.
>
> In fact I'd even prefer it to be two patches, one that moves should_wakeup and
> the related macros out of the CONFIG_PM_SLEEP #ifdef, because that's what fixes
> the problem described in the changelog, and one that makes the remaining
> changes with a separate changelog.
Done.
The 1/3 patch fixes the unfortunate code in the serial-core driver.
The 2/3 patch moves the macros out from under CONFIG_PM_SLEEP.
The 3/3 patch converts the macros to inline functions and creates a new
pm_wakeup.h file for them. They can't remain in pm.h, because they
depend on the definition of struct device -- and the compiler hasn't
seen that yet when pm.h is read.
It's not clear why the can_wakeup accessors are written to work even
when CONFIG_PM isn't set. Nevertheless, the patches retain that
behavior.
Alan Stern
This patch (as1059) fixes a mistake in the way the serial core
initializes a device's wakeup settings. It should use the accessor
routine instead of relying on a macro producing an lvalue.
Signed-off-by: Alan Stern <[email protected]>
---
Index: usb-2.6/drivers/serial/serial_core.c
===================================================================
--- usb-2.6.orig/drivers/serial/serial_core.c
+++ usb-2.6/drivers/serial/serial_core.c
@@ -2356,7 +2356,7 @@ int uart_add_one_port(struct uart_driver
*/
tty_dev = tty_register_device(drv->tty_driver, port->line, port->dev);
if (likely(!IS_ERR(tty_dev))) {
- device_can_wakeup(tty_dev) = 1;
+ device_init_wakeup(tty_dev, 1);
device_set_wakeup_enable(tty_dev, 0);
} else
printk(KERN_ERR "Cannot register tty device on line %d\n",
The various wakeup flags and their accessor macros in struct
dev_pm_info should be available whenever CONFIG_PM is enabled, not
just when CONFIG_PM_SLEEP is on. Otherwise remote wakeup won't always
be configurable for runtime power management. This patch (as1056b)
fixes the oversight.
Signed-off-by: Alan Stern <[email protected]>
---
Index: usb-2.6/include/linux/pm.h
===================================================================
--- usb-2.6.orig/include/linux/pm.h
+++ usb-2.6/include/linux/pm.h
@@ -183,8 +183,8 @@ typedef struct pm_message {
struct dev_pm_info {
pm_message_t power_state;
unsigned can_wakeup:1;
-#ifdef CONFIG_PM_SLEEP
unsigned should_wakeup:1;
+#ifdef CONFIG_PM_SLEEP
struct list_head entry;
#endif
};
@@ -197,11 +197,6 @@ extern void device_resume(void);
extern int device_suspend(pm_message_t state);
extern int device_prepare_suspend(pm_message_t state);
-#define device_set_wakeup_enable(dev,val) \
- ((dev)->power.should_wakeup = !!(val))
-#define device_may_wakeup(dev) \
- (device_can_wakeup(dev) && (dev)->power.should_wakeup)
-
extern void __suspend_report_result(const char *function, void *fn, int ret);
#define suspend_report_result(fn, ret) \
@@ -209,6 +204,24 @@ extern void __suspend_report_result(cons
__suspend_report_result(__FUNCTION__, fn, ret); \
} while (0)
+#else /* !CONFIG_PM_SLEEP */
+
+static inline int device_suspend(pm_message_t state)
+{
+ return 0;
+}
+
+#define suspend_report_result(fn, ret) do { } while (0)
+
+#endif /* !CONFIG_PM_SLEEP */
+
+#ifdef CONFIG_PM
+
+#define device_set_wakeup_enable(dev,val) \
+ ((dev)->power.should_wakeup = !!(val))
+#define device_may_wakeup(dev) \
+ (device_can_wakeup(dev) && (dev)->power.should_wakeup)
+
/*
* Platform hook to activate device wakeup capability, if that's not already
* handled by enable_irq_wake() etc.
@@ -223,24 +236,17 @@ static inline int call_platform_enable_w
return 0;
}
-#else /* !CONFIG_PM_SLEEP */
-
-static inline int device_suspend(pm_message_t state)
-{
- return 0;
-}
+#else /* !CONFIG_PM */
#define device_set_wakeup_enable(dev,val) do{}while(0)
#define device_may_wakeup(dev) (0)
-#define suspend_report_result(fn, ret) do { } while (0)
-
static inline int call_platform_enable_wakeup(struct device *dev, int is_on)
{
return 0;
}
-#endif /* !CONFIG_PM_SLEEP */
+#endif /* !CONFIG_PM */
/* changes to device_may_wakeup take effect on the next pm state change.
* by default, devices should wakeup if they can.
Index: usb-2.6/drivers/base/power/main.c
===================================================================
--- usb-2.6.orig/drivers/base/power/main.c
+++ usb-2.6/drivers/base/power/main.c
@@ -56,8 +56,6 @@ static DEFINE_MUTEX(dpm_list_mtx);
static DECLARE_RWSEM(pm_sleep_rwsem);
-int (*platform_enable_wakeup)(struct device *dev, int is_on);
-
/**
* device_pm_add - add a device to the list of active devices
* @dev: Device to be added to the list
Index: usb-2.6/drivers/base/power/sysfs.c
===================================================================
--- usb-2.6.orig/drivers/base/power/sysfs.c
+++ usb-2.6/drivers/base/power/sysfs.c
@@ -6,6 +6,8 @@
#include <linux/string.h>
#include "power.h"
+int (*platform_enable_wakeup)(struct device *dev, int is_on);
+
/*
* wakeup - Report/change current wakeup option for device
This patch (as1058) improves the wakeup macros in include/linux/pm.h.
All but the trivial ones are converted to inline routines, which
requires moving them to a separate header file since they depend on
the definition of struct device.
Signed-off-by: Alan Stern <[email protected]>
---
Index: usb-2.6/include/linux/pm.h
===================================================================
--- usb-2.6.orig/include/linux/pm.h
+++ usb-2.6/include/linux/pm.h
@@ -211,54 +211,10 @@ static inline int device_suspend(pm_mess
return 0;
}
-#define suspend_report_result(fn, ret) do { } while (0)
+#define suspend_report_result(fn, ret) do {} while (0)
#endif /* !CONFIG_PM_SLEEP */
-#ifdef CONFIG_PM
-
-#define device_set_wakeup_enable(dev,val) \
- ((dev)->power.should_wakeup = !!(val))
-#define device_may_wakeup(dev) \
- (device_can_wakeup(dev) && (dev)->power.should_wakeup)
-
-/*
- * Platform hook to activate device wakeup capability, if that's not already
- * handled by enable_irq_wake() etc.
- * Returns zero on success, else negative errno
- */
-extern int (*platform_enable_wakeup)(struct device *dev, int is_on);
-
-static inline int call_platform_enable_wakeup(struct device *dev, int is_on)
-{
- if (platform_enable_wakeup)
- return (*platform_enable_wakeup)(dev, is_on);
- return 0;
-}
-
-#else /* !CONFIG_PM */
-
-#define device_set_wakeup_enable(dev,val) do{}while(0)
-#define device_may_wakeup(dev) (0)
-
-static inline int call_platform_enable_wakeup(struct device *dev, int is_on)
-{
- return 0;
-}
-
-#endif /* !CONFIG_PM */
-
-/* changes to device_may_wakeup take effect on the next pm state change.
- * by default, devices should wakeup if they can.
- */
-#define device_can_wakeup(dev) \
- ((dev)->power.can_wakeup)
-#define device_init_wakeup(dev,val) \
- do { \
- device_can_wakeup(dev) = !!(val); \
- device_set_wakeup_enable(dev,val); \
- } while(0)
-
/*
* Global Power Management flags
* Used to keep APM and ACPI from both being active
Index: usb-2.6/include/linux/device.h
===================================================================
--- usb-2.6.orig/include/linux/device.h
+++ usb-2.6/include/linux/device.h
@@ -475,6 +475,9 @@ struct device {
void (*release)(struct device *dev);
};
+/* Get the wakeup routines, which depend on struct device */
+#include <linux/pm_wakeup.h>
+
#ifdef CONFIG_NUMA
static inline int dev_to_node(struct device *dev)
{
Index: usb-2.6/include/linux/pm_wakeup.h
===================================================================
--- /dev/null
+++ usb-2.6/include/linux/pm_wakeup.h
@@ -0,0 +1,90 @@
+/*
+ * pm_wakeup.h - Power management wakeup interface
+ *
+ * Copyright (C) 2008 Alan Stern
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#ifndef _LINUX_PM_WAKEUP_H
+#define _LINUX_PM_WAKEUP_H
+
+#ifndef _DEVICE_H_
+# error "please don't include this file directly"
+#endif
+
+#ifdef CONFIG_PM
+
+/* changes to device_may_wakeup take effect on the next pm state change.
+ * by default, devices should wakeup if they can.
+ */
+static inline void device_init_wakeup(struct device *dev, int val)
+{
+ dev->power.can_wakeup = dev->power.should_wakeup = !!val;
+}
+
+static inline int device_can_wakeup(struct device *dev)
+{
+ return dev->power.can_wakeup;
+}
+
+static inline void device_set_wakeup_enable(struct device *dev, int val)
+{
+ dev->power.should_wakeup = !!val;
+}
+
+static inline int device_may_wakeup(struct device *dev)
+{
+ return dev->power.can_wakeup & dev->power.should_wakeup;
+}
+
+/*
+ * Platform hook to activate device wakeup capability, if that's not already
+ * handled by enable_irq_wake() etc.
+ * Returns zero on success, else negative errno
+ */
+extern int (*platform_enable_wakeup)(struct device *dev, int is_on);
+
+static inline int call_platform_enable_wakeup(struct device *dev, int is_on)
+{
+ if (platform_enable_wakeup)
+ return (*platform_enable_wakeup)(dev, is_on);
+ return 0;
+}
+
+#else /* !CONFIG_PM */
+
+/* For some reason the next two routines work even without CONFIG_PM */
+static inline void device_init_wakeup(struct device *dev, int val)
+{
+ dev->power.can_wakeup = !!val;
+}
+
+static inline int device_can_wakeup(struct device *dev)
+{
+ return dev->power.can_wakeup;
+}
+
+#define device_set_wakeup_enable(dev, val) do {} while (0)
+#define device_may_wakeup(dev) 0
+
+static inline int call_platform_enable_wakeup(struct device *dev, int is_on)
+{
+ return 0;
+}
+
+#endif /* !CONFIG_PM */
+
+#endif /* _LINUX_PM_WAKEUP_H */