linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m
       [not found] <201912181000.82Z7czbN%lkp@intel.com>
@ 2019-12-18  8:38 ` Uwe Kleine-König
  2019-12-18  9:06   ` Johan Hovold
  2019-12-18 17:23   ` kbuild obj-m directory descend (was: Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m) Johan Hovold
  0 siblings, 2 replies; 11+ messages in thread
From: Uwe Kleine-König @ 2019-12-18  8:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby, Rob Herring
  Cc: linux-kernel, kernel, kbuild test robot

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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m
  2019-12-18  8:38 ` [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m Uwe Kleine-König
@ 2019-12-18  9:06   ` Johan Hovold
  2019-12-18  9:29     ` Uwe Kleine-König
  2019-12-18 17:23   ` kbuild obj-m directory descend (was: Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m) Johan Hovold
  1 sibling, 1 reply; 11+ messages in thread
From: Johan Hovold @ 2019-12-18  9:06 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Greg Kroah-Hartman, Jiri Slaby, Rob Herring, linux-kernel,
	kernel, kbuild test robot

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m
  2019-12-18  9:06   ` Johan Hovold
@ 2019-12-18  9:29     ` Uwe Kleine-König
  2019-12-18  9:38       ` Johan Hovold
  0 siblings, 1 reply; 11+ messages in thread
From: Uwe Kleine-König @ 2019-12-18  9:29 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Greg Kroah-Hartman, Jiri Slaby, Rob Herring, linux-kernel,
	kernel, kbuild test robot

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/ |

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m
  2019-12-18  9:29     ` Uwe Kleine-König
@ 2019-12-18  9:38       ` Johan Hovold
  2019-12-18 10:39         ` Uwe Kleine-König
  0 siblings, 1 reply; 11+ messages in thread
From: Johan Hovold @ 2019-12-18  9:38 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Johan Hovold, Greg Kroah-Hartman, Jiri Slaby, Rob Herring,
	linux-kernel, kernel, kbuild test robot

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m
  2019-12-18  9:38       ` Johan Hovold
@ 2019-12-18 10:39         ` Uwe Kleine-König
  2019-12-18 12:51           ` Johan Hovold
  0 siblings, 1 reply; 11+ messages in thread
From: Uwe Kleine-König @ 2019-12-18 10:39 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Rob Herring, kbuild test robot, Greg Kroah-Hartman, linux-kernel,
	kernel, Jiri Slaby

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/ |

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m
  2019-12-18 10:39         ` Uwe Kleine-König
@ 2019-12-18 12:51           ` Johan Hovold
  0 siblings, 0 replies; 11+ messages in thread
From: Johan Hovold @ 2019-12-18 12:51 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Johan Hovold, Rob Herring, kbuild test robot, Greg Kroah-Hartman,
	linux-kernel, kernel, Jiri Slaby

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* kbuild obj-m directory descend (was: Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m)
  2019-12-18  8:38 ` [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m Uwe Kleine-König
  2019-12-18  9:06   ` Johan Hovold
@ 2019-12-18 17:23   ` Johan Hovold
  2019-12-19  2:00     ` Masahiro Yamada
  1 sibling, 1 reply; 11+ messages in thread
From: Johan Hovold @ 2019-12-18 17:23 UTC (permalink / raw)
  To: Uwe Kleine-König, Masahiro Yamada, Michal Marek
  Cc: Greg Kroah-Hartman, Jiri Slaby, Rob Herring, linux-kernel,
	kernel, kbuild test robot, linux-kbuild

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: kbuild obj-m directory descend (was: Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m)
  2019-12-18 17:23   ` kbuild obj-m directory descend (was: Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m) Johan Hovold
@ 2019-12-19  2:00     ` Masahiro Yamada
  2019-12-19  8:18       ` Johan Hovold
  0 siblings, 1 reply; 11+ messages in thread
From: Masahiro Yamada @ 2019-12-19  2:00 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Uwe Kleine-König, Michal Marek, Greg Kroah-Hartman,
	Jiri Slaby, Rob Herring, Linux Kernel Mailing List, Sascha Hauer,
	kbuild test robot, Linux Kbuild mailing list

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: kbuild obj-m directory descend (was: Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m)
  2019-12-19  2:00     ` Masahiro Yamada
@ 2019-12-19  8:18       ` Johan Hovold
  2019-12-19  8:55         ` Masahiro Yamada
  0 siblings, 1 reply; 11+ messages in thread
From: Johan Hovold @ 2019-12-19  8:18 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Johan Hovold, Uwe Kleine-König, Michal Marek,
	Greg Kroah-Hartman, Jiri Slaby, Rob Herring,
	Linux Kernel Mailing List, Sascha Hauer, kbuild test robot,
	Linux Kbuild mailing list

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: kbuild obj-m directory descend (was: Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m)
  2019-12-19  8:18       ` Johan Hovold
@ 2019-12-19  8:55         ` Masahiro Yamada
  2019-12-19  9:49           ` Johan Hovold
  0 siblings, 1 reply; 11+ messages in thread
From: Masahiro Yamada @ 2019-12-19  8:55 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Uwe Kleine-König, Michal Marek, Greg Kroah-Hartman,
	Jiri Slaby, Rob Herring, Linux Kernel Mailing List, Sascha Hauer,
	kbuild test robot, Linux Kbuild mailing list

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: kbuild obj-m directory descend (was: Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m)
  2019-12-19  8:55         ` Masahiro Yamada
@ 2019-12-19  9:49           ` Johan Hovold
  0 siblings, 0 replies; 11+ messages in thread
From: Johan Hovold @ 2019-12-19  9:49 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Johan Hovold, Uwe Kleine-König, Michal Marek,
	Greg Kroah-Hartman, Jiri Slaby, Rob Herring,
	Linux Kernel Mailing List, Sascha Hauer, kbuild test robot,
	Linux Kbuild mailing list

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-12-19  9:49 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <201912181000.82Z7czbN%lkp@intel.com>
2019-12-18  8:38 ` [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m Uwe Kleine-König
2019-12-18  9:06   ` Johan Hovold
2019-12-18  9:29     ` Uwe Kleine-König
2019-12-18  9:38       ` Johan Hovold
2019-12-18 10:39         ` Uwe Kleine-König
2019-12-18 12:51           ` Johan Hovold
2019-12-18 17:23   ` kbuild obj-m directory descend (was: Re: [PATCH] serdev: fix builds with CONFIG_SERIAL_DEV_BUS=m) Johan Hovold
2019-12-19  2:00     ` Masahiro Yamada
2019-12-19  8:18       ` Johan Hovold
2019-12-19  8:55         ` Masahiro Yamada
2019-12-19  9:49           ` Johan Hovold

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).