[PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m

[PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m

[Uwe Kleine-König]

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.

Fixes: 54edb425346a ("serdev: simplify Makefile")
Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

as Greg already added the offending patch to tty-next I assume it is
frozen and cannot be dropped any more, so here is an incremental fix.

Best regards
Uwe

 drivers/tty/Makefile        | 2 +-
 drivers/tty/serdev/Makefile | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
index 020b1cd9294f..6b2a21d4e0bb 100644
--- a/drivers/tty/Makefile
+++ b/drivers/tty/Makefile
@@ -16,7 +16,7 @@ obj-$(CONFIG_R3964)		+= n_r3964.o
 obj-y				+= vt/
 obj-$(CONFIG_HVC_DRIVER)	+= hvc/
 obj-y				+= serial/
-obj-$(CONFIG_SERIAL_DEV_BUS)	+= serdev/
+obj-y				+= serdev/
 
 # tty drivers
 obj-$(CONFIG_AMIGA_BUILTIN_SERIAL) += amiserial.o
diff --git a/drivers/tty/serdev/Makefile b/drivers/tty/serdev/Makefile
index f71bb931735b..078417e5b068 100644
--- a/drivers/tty/serdev/Makefile
+++ b/drivers/tty/serdev/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 serdev-objs := core.o
 
-obj-y += serdev.o
+obj-$(CONFIG_SERIAL_DEV_BUS) += serdev.o
 
 obj-$(CONFIG_SERIAL_DEV_CTRL_TTYPORT) += serdev-ttyport.o
-- 
2.24.0

Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m

[Johan Hovold]

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.

You can send a follow-up clean up if you think that's warranted instead.

> Fixes: 54edb425346a ("serdev: simplify Makefile")
> Reported-by: kbuild test robot <lkp@intel.com>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> as Greg already added the offending patch to tty-next I assume it is
> frozen and cannot be dropped any more, so here is an incremental fix.

Johan

Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m

[Uwe Kleine-König]

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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m

[Johan Hovold]

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;
in this case unbreak the build. If you want to do cleanups as well, you
do that separately (and argue for why its needed).

Johan

Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m

[Uwe Kleine-König]

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.

But I don't feel strong here. If you prefer to revert, that's ok, too.
Not sure I will find the motivation then, to reimplement the
cleanup, though.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m

[Johan Hovold]

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

kbuild obj-m directory descend (was: Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m)

[Johan Hovold]

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.

I was just looking into this when you commented on this off-list.

The offending patch is broken since it effectively makes
CONFIG_SERIAL_DEV_BUS bool (built-in or disabled), but for some reason
those symbols do not end up in vmlinux (despite being compiled) when you
add a built-in object goal under a directory that was entered using
obj-m.

That seems like a bug to me and contradicts the kbuild documentation
(3.6):

	Example:

	#fs/Makefile
	obj-$(CONFIG_EXT2_FS) += ext2/

	If CONFIG_EXT2_FS is set to either ‘y’ (built-in) or ‘m’
	(modular) the corresponding obj- variable will be set, and
	kbuild will descend down in the ext2 directory. Kbuild only uses
	this information to decide that it needs to visit the directory,
	it is the Makefile in the subdirectory that specifies what is
	modular and what is built-in.

I tried adding other targets to obj-y directly and they are also are not
included, seemingly since the directory was entered using obj-m.

Masahiro or Michal, can you shed some light?

Johan

Re: kbuild obj-m directory descend (was: Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m)

[Masahiro Yamada]

Hi.

On Thu, Dec 19, 2019 at 2:23 AM Johan Hovold <johan@kernel.org> 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.
>
> I was just looking into this when you commented on this off-list.
>
> The offending patch is broken since it effectively makes
> CONFIG_SERIAL_DEV_BUS bool (built-in or disabled), but for some reason
> those symbols do not end up in vmlinux (despite being compiled) when you
> add a built-in object goal under a directory that was entered using
> obj-m.
>
> That seems like a bug to me and contradicts the kbuild documentation
> (3.6):
>
>         Example:
>
>         #fs/Makefile
>         obj-$(CONFIG_EXT2_FS) += ext2/
>
>         If CONFIG_EXT2_FS is set to either ‘y’ (built-in) or ‘m’
>         (modular) the corresponding obj- variable will be set, and
>         kbuild will descend down in the ext2 directory. Kbuild only uses
>         this information to decide that it needs to visit the directory,
>         it is the Makefile in the subdirectory that specifies what is
>         modular and what is built-in.
>
> I tried adding other targets to obj-y directly and they are also are not
> included, seemingly since the directory was entered using obj-m.
>
> Masahiro or Michal, can you shed some light?
>
> Johan


I saw similar questions in ML in the past.
Yes, this is how Kbuild works;
Kbuild only links objects in Makefiles visited by obj-y.

If you use
    obj-m += serdev/
all objects in serdev/ are considered as modular.

I think it is better to make the document
clarify this.


-- 
Best Regards
Masahiro Yamada

Re: kbuild obj-m directory descend (was: Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m)

[Johan Hovold]

On Thu, Dec 19, 2019 at 11:00:15AM +0900, Masahiro Yamada wrote:
> Hi.
> 
> On Thu, Dec 19, 2019 at 2:23 AM Johan Hovold <johan@kernel.org> wrote:

> > The offending patch is broken since it effectively makes
> > CONFIG_SERIAL_DEV_BUS bool (built-in or disabled), but for some reason
> > those symbols do not end up in vmlinux (despite being compiled) when you
> > add a built-in object goal under a directory that was entered using
> > obj-m.
> >
> > That seems like a bug to me and contradicts the kbuild documentation
> > (3.6):
> >
> >         Example:
> >
> >         #fs/Makefile
> >         obj-$(CONFIG_EXT2_FS) += ext2/
> >
> >         If CONFIG_EXT2_FS is set to either ‘y’ (built-in) or ‘m’
> >         (modular) the corresponding obj- variable will be set, and
> >         kbuild will descend down in the ext2 directory. Kbuild only uses
> >         this information to decide that it needs to visit the directory,
> >         it is the Makefile in the subdirectory that specifies what is
> >         modular and what is built-in.
> >
> > I tried adding other targets to obj-y directly and they are also are not
> > included, seemingly since the directory was entered using obj-m.
> >
> > Masahiro or Michal, can you shed some light?

> I saw similar questions in ML in the past.
> Yes, this is how Kbuild works;
> Kbuild only links objects in Makefiles visited by obj-y.
> 
> If you use
>     obj-m += serdev/
> all objects in serdev/ are considered as modular.

Well, any objects in serdev/ specified in obj-y would currently be built
but never included in either built-in.a or any module.

> I think it is better to make the document
> clarify this.

Yeah, I don't have a use case for this, but the documentation would need
to be updated to not have more people look into this.

And at least this behaviour allowed us to catch this bug, but only
because we had dependent modules that failed to build. I see now that
you posted a patch adding a general warning about obj-y under obj-m last
fall, but that was apparently never merged.

But when was this behaviour changed? And was that done on purpose?

Johan

Re: kbuild obj-m directory descend (was: Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m)

[Masahiro Yamada]

On Thu, Dec 19, 2019 at 5:18 PM Johan Hovold <johan@kernel.org> wrote:
>
> On Thu, Dec 19, 2019 at 11:00:15AM +0900, Masahiro Yamada wrote:
> > Hi.
> >
> > On Thu, Dec 19, 2019 at 2:23 AM Johan Hovold <johan@kernel.org> wrote:
>
> > > The offending patch is broken since it effectively makes
> > > CONFIG_SERIAL_DEV_BUS bool (built-in or disabled), but for some reason
> > > those symbols do not end up in vmlinux (despite being compiled) when you
> > > add a built-in object goal under a directory that was entered using
> > > obj-m.
> > >
> > > That seems like a bug to me and contradicts the kbuild documentation
> > > (3.6):
> > >
> > >         Example:
> > >
> > >         #fs/Makefile
> > >         obj-$(CONFIG_EXT2_FS) += ext2/
> > >
> > >         If CONFIG_EXT2_FS is set to either ‘y’ (built-in) or ‘m’
> > >         (modular) the corresponding obj- variable will be set, and
> > >         kbuild will descend down in the ext2 directory. Kbuild only uses
> > >         this information to decide that it needs to visit the directory,
> > >         it is the Makefile in the subdirectory that specifies what is
> > >         modular and what is built-in.
> > >
> > > I tried adding other targets to obj-y directly and they are also are not
> > > included, seemingly since the directory was entered using obj-m.
> > >
> > > Masahiro or Michal, can you shed some light?
>
> > I saw similar questions in ML in the past.
> > Yes, this is how Kbuild works;
> > Kbuild only links objects in Makefiles visited by obj-y.
> >
> > If you use
> >     obj-m += serdev/
> > all objects in serdev/ are considered as modular.
>
> Well, any objects in serdev/ specified in obj-y would currently be built
> but never included in either built-in.a or any module.

Right.

>
> > I think it is better to make the document
> > clarify this.
>
> Yeah, I don't have a use case for this, but the documentation would need
> to be updated to not have more people look into this.
>
> And at least this behaviour allowed us to catch this bug, but only
> because we had dependent modules that failed to build. I see now that
> you posted a patch adding a general warning about obj-y under obj-m last
> fall, but that was apparently never merged.

This one?

https://lore.kernel.org/patchwork/patch/1126959/

This patch detected several Makefile/Kconfig bugs,
and it is good.

But, it also turned out to cause false positive warnings
on some architectures.
I thought it was not so easy to fix it.
So, I decided to postpone it.


> But when was this behaviour changed? And was that done on purpose?

No. Not changed at all.

In my understanding, the current behavior has been kept
from the beginning.



> Johan



-- 
Best Regards
Masahiro Yamada

Re: kbuild obj-m directory descend (was: Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m)

[Johan Hovold]

On Thu, Dec 19, 2019 at 05:55:54PM +0900, Masahiro Yamada wrote:
> On Thu, Dec 19, 2019 at 5:18 PM Johan Hovold <johan@kernel.org> wrote:
> >
> > On Thu, Dec 19, 2019 at 11:00:15AM +0900, Masahiro Yamada wrote:
> > > Hi.
> > >
> > > On Thu, Dec 19, 2019 at 2:23 AM Johan Hovold <johan@kernel.org> wrote:
> >
> > > > The offending patch is broken since it effectively makes
> > > > CONFIG_SERIAL_DEV_BUS bool (built-in or disabled), but for some reason
> > > > those symbols do not end up in vmlinux (despite being compiled) when you
> > > > add a built-in object goal under a directory that was entered using
> > > > obj-m.
> > > >
> > > > That seems like a bug to me and contradicts the kbuild documentation
> > > > (3.6):
> > > >
> > > >         Example:
> > > >
> > > >         #fs/Makefile
> > > >         obj-$(CONFIG_EXT2_FS) += ext2/
> > > >
> > > >         If CONFIG_EXT2_FS is set to either ‘y’ (built-in) or ‘m’
> > > >         (modular) the corresponding obj- variable will be set, and
> > > >         kbuild will descend down in the ext2 directory. Kbuild only uses
> > > >         this information to decide that it needs to visit the directory,
> > > >         it is the Makefile in the subdirectory that specifies what is
> > > >         modular and what is built-in.
> > > >
> > > > I tried adding other targets to obj-y directly and they are also are not
> > > > included, seemingly since the directory was entered using obj-m.
> > > >
> > > > Masahiro or Michal, can you shed some light?
> >
> > > I saw similar questions in ML in the past.
> > > Yes, this is how Kbuild works;
> > > Kbuild only links objects in Makefiles visited by obj-y.
> > >
> > > If you use
> > >     obj-m += serdev/
> > > all objects in serdev/ are considered as modular.
> >
> > Well, any objects in serdev/ specified in obj-y would currently be built
> > but never included in either built-in.a or any module.
> 
> Right.
> 
> > > I think it is better to make the document
> > > clarify this.
> >
> > Yeah, I don't have a use case for this, but the documentation would need
> > to be updated to not have more people look into this.
> >
> > And at least this behaviour allowed us to catch this bug, but only
> > because we had dependent modules that failed to build. I see now that
> > you posted a patch adding a general warning about obj-y under obj-m last
> > fall, but that was apparently never merged.
> 
> This one?
> 
> https://lore.kernel.org/patchwork/patch/1126959/

Yep, that's the one I meant.
 
> This patch detected several Makefile/Kconfig bugs,
> and it is good.
> 
> But, it also turned out to cause false positive warnings
> on some architectures.
> I thought it was not so easy to fix it.
> So, I decided to postpone it.

Ok.

> > But when was this behaviour changed? And was that done on purpose?
> 
> No. Not changed at all.
> 
> In my understanding, the current behavior has been kept
> from the beginning.

Ok, thanks for confirming. The contradicting documentation I refer to
above has been there since before git at least however.

Here's a random example of someone being bit by this five years ago:

	https://www.spinics.net/lists/linux-kbuild/msg10887.html

Would you mind taking a stab at updating the docs?

Johan