linux-kernel - Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV

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]

Date:   Wed, 18 Dec 2019 13:51:44 +0100
From:   Johan Hovold <johan@...nel.org>
To:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
Cc:     Johan Hovold <johan@...nel.org>, Rob Herring <robh@...nel.org>,
        kbuild test robot <lkp@...el.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org, kernel@...gutronix.de,
        Jiri Slaby <jslaby@...e.com>
Subject: Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m

On Wed, Dec 18, 2019 at 11:39:31AM +0100, Uwe Kleine-König wrote:
> Hello Johan,
> 
> On Wed, Dec 18, 2019 at 10:38:06AM +0100, Johan Hovold wrote:
> > On Wed, Dec 18, 2019 at 10:29:58AM +0100, Uwe Kleine-König wrote:
> > > On Wed, Dec 18, 2019 at 10:06:06AM +0100, Johan Hovold wrote:
> > > > On Wed, Dec 18, 2019 at 09:38:42AM +0100, Uwe Kleine-König wrote:
> > > > > Commit 54edb425346a ("serdev: simplify Makefile") broke builds with
> > > > > serdev configured as module. I don't understand it completely yet, but
> > > > > it seems that
> > > > > 
> > > > > 	obj-$(CONFIG_SERIAL_DEV_BUS) += serdev/
> > > > > 
> > > > > in drivers/tty/Makefile with CONFIG_SERIAL_DEV_BUS=m doesn't result in
> > > > > code that is added using obj-y in drivers/tty/serdev/Makefile being
> > > > > compiled. So instead of dropping $(CONFIG_SERIAL_DEV_BUS) in serdev's
> > > > > Makefile, drop it in drivers/tty/Makefile.
> > > > 
> > > > Why not simply revert the offending patch? There are some dependencies
> > > > here related to how the tty layer is built. If you're still not certain
> > > > on why things broke, I suggest just reverting for now.
> > > 
> > > I see that it is not easy to define what obj-y should do in a Makefile
> > > that is included via obj-m. Now it is the other way round and that
> > > should be safe. This construct is used in several places, so I'd say the
> > > patch is fine unless you have more concrete concerns.
> > 
> > No, and I don't have time to look into this right now.
> > 
> > It's more about the general principle that a patch should do one thing;
> 
> IMHO it does one thing: It does what 54edb425346a intended to do in the
> right way.

No, your original patch was just broken. The commit message only claimed
that

	drivers/tty/Makefile has:
        	obj-$(CONFIG_SERIAL_DEV_BUS)    += serdev/

	so in drivers/tty/serdev/Makefile CONFIG_SERIAL_DEV_BUS is
	always =y.

which is clearly false since it can be =m (as the build-bot reported).

You now suggest to change:

	obj-$(CONFIG_SERIAL_DEV_BUS)   += serdev/
	+obj-y                          += serdev/

in drivers/tty/Makefile so that make always recurse into serdev, even
when serdev is not enabled, which likewise makes no sense. And in any
case that chunk is not fixing your original patch, but introducing a
different slight regression.

> But I don't feel strong here. If you prefer to revert, that's ok, too.

Yes, I think reverting is the right thing to do here.

> Not sure I will find the motivation then, to reimplement the
> cleanup, though.

That's ok, I don't think that "cleanup" is something we want, and either
way it needs to be reviewed separately.

Johan