linux-kernel - Re: linux-next20080314 build fails with !CONFIG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <200803142337.12605.rjw@sisk.pl>
Date:	Fri, 14 Mar 2008 23:37:11 +0100
From:	"Rafael J. Wysocki" <rjw@...k.pl>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Kamalesh Babulal <kamalesh@...ux.vnet.ibm.com>,
	linux-next@...r.kernel.org, sfr@...b.auug.org.au,
	linux-kernel@...r.kernel.org, apw@...dowen.org,
	Alan Stern <stern@...land.harvard.edu>,
	Len Brown <lenb@...nel.org>, Greg KH <greg@...ah.com>
Subject: Re: linux-next20080314 build fails with !CONFIG_PM

On Friday, 14 of March 2008, Andrew Morton wrote:
> On Fri, 14 Mar 2008 13:27:10 +0530
> Kamalesh Babulal <kamalesh@...ux.vnet.ibm.com> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/