[PATCH] modpost: make KBUILD_MODPOST_WARN also configurable for external modules

[PATCH] modpost: make KBUILD_MODPOST_WARN also configurable for external modules

[Wiebe, Wladislav (Nokia - DE/Ulm)]

Commit ea837f1c0503 ("kbuild: make modpost processing configurable")
was intended to give KBUILD_MODPOST_WARN flexibility to be configurable.
Right now KBUILD_MODPOST_WARN gets just ignored when KBUILD_EXTMOD is
set which happens per default when building modules out of the tree.

This change gives the opportunity to define module build behaving also
in case of out of tree builds and default will become exit on error.
Errors which can be detected by the build should be trapped out of the box
there, unless somebody wants to notice broken stuff later at runtime.

Signed-off-by: Wladislav Wiebe <wladislav.wiebe@nokia.com>
---
 scripts/Makefile.modpost | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index 6b7f354f189a..fec6ec2ffa47 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -78,7 +78,7 @@ modpost = scripts/mod/modpost                    \
  $(if $(KBUILD_EXTRA_SYMBOLS), $(patsubst %, -e %,$(KBUILD_EXTRA_SYMBOLS))) \
  $(if $(KBUILD_EXTMOD),-o $(modulesymfile))      \
  $(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),,-E)  \
- $(if $(KBUILD_EXTMOD)$(KBUILD_MODPOST_WARN),-w)
+ $(if $(KBUILD_MODPOST_WARN),-w)
 
 MODPOST_OPT=$(subst -i,-n,$(filter -i,$(MAKEFLAGS)))
 
-- 
2.19.2

Re: [PATCH] modpost: make KBUILD_MODPOST_WARN also configurable for external modules

[Masahiro Yamada]

(+CC Jonas Gorski)


On Tue, Mar 26, 2019 at 6:58 PM Wiebe, Wladislav (Nokia - DE/Ulm)
<wladislav.wiebe@nokia.com> wrote:
>
> Commit ea837f1c0503 ("kbuild: make modpost processing configurable")
> was intended to give KBUILD_MODPOST_WARN flexibility to be configurable.
> Right now KBUILD_MODPOST_WARN gets just ignored when KBUILD_EXTMOD is
> set which happens per default when building modules out of the tree.
>
> This change gives the opportunity to define module build behaving also
> in case of out of tree builds and default will become exit on error.
> Errors which can be detected by the build should be trapped out of the box
> there, unless somebody wants to notice broken stuff later at runtime.

If an external module refers to a symbol
provided by another external module,
this patch will turn the warning into the error by default,
which is probably a bad idea.


I suggested a desired change in the following.
https://patchwork.kernel.org/patch/9980691/

I am poking Jonas Gorski
if he plans to send v2.


> Signed-off-by: Wladislav Wiebe <wladislav.wiebe@nokia.com>
> ---
>  scripts/Makefile.modpost | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
> index 6b7f354f189a..fec6ec2ffa47 100644
> --- a/scripts/Makefile.modpost
> +++ b/scripts/Makefile.modpost
> @@ -78,7 +78,7 @@ modpost = scripts/mod/modpost                    \
>   $(if $(KBUILD_EXTRA_SYMBOLS), $(patsubst %, -e %,$(KBUILD_EXTRA_SYMBOLS))) \
>   $(if $(KBUILD_EXTMOD),-o $(modulesymfile))      \
>   $(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),,-E)  \
> - $(if $(KBUILD_EXTMOD)$(KBUILD_MODPOST_WARN),-w)
> + $(if $(KBUILD_MODPOST_WARN),-w)
>
>  MODPOST_OPT=$(subst -i,-n,$(filter -i,$(MAKEFLAGS)))
>
> --
> 2.19.2



-- 
Best Regards
Masahiro Yamada

Re: [PATCH] modpost: make KBUILD_MODPOST_WARN also configurable for external modules

[Wiebe, Wladislav (Nokia - DE/Ulm)]

Hi!

On 07.04.2019 11:04, Masahiro Yamada wrote:
> (+CC Jonas Gorski)
> 
> 
> On Tue, Mar 26, 2019 at 6:58 PM Wiebe, Wladislav (Nokia - DE/Ulm)
> <wladislav.wiebe@nokia.com> wrote:
>>
>> Commit ea837f1c0503 ("kbuild: make modpost processing configurable")
>> was intended to give KBUILD_MODPOST_WARN flexibility to be configurable.
>> Right now KBUILD_MODPOST_WARN gets just ignored when KBUILD_EXTMOD is
>> set which happens per default when building modules out of the tree.
>>
>> This change gives the opportunity to define module build behaving also
>> in case of out of tree builds and default will become exit on error.
>> Errors which can be detected by the build should be trapped out of the box
>> there, unless somebody wants to notice broken stuff later at runtime.
> 
> If an external module refers to a symbol
> provided by another external module,
> this patch will turn the warning into the error by default,
> which is probably a bad idea.

Indeed, exactly this should happen. You should fix your external module 
dependencies by providing their symbols. Please use e.g. 
KBUILD_EXTRA_SYMBOLS instead of converting errors to warning and hoping 
that things will work. Or how do you want to make sure module A still 
delivers all symbols needed by module B after an update/changes? 
Manually comparing the logs after an update or waiting until it turns 
out broken during run-time? I wouldn't say this is a good idea :-)


> I suggested a desired change in the following.
> https://patchwork.kernel.org/patch/9980691/
> 
> I am poking Jonas Gorski
> if he plans to send v2.

I wouldn't vote for v2 based on explanation above.

- Wladislav

> 
> 
>> Signed-off-by: Wladislav Wiebe <wladislav.wiebe@nokia.com>
>> ---
>>   scripts/Makefile.modpost | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
>> index 6b7f354f189a..fec6ec2ffa47 100644
>> --- a/scripts/Makefile.modpost
>> +++ b/scripts/Makefile.modpost
>> @@ -78,7 +78,7 @@ modpost = scripts/mod/modpost                    \
>>    $(if $(KBUILD_EXTRA_SYMBOLS), $(patsubst %, -e %,$(KBUILD_EXTRA_SYMBOLS))) \
>>    $(if $(KBUILD_EXTMOD),-o $(modulesymfile))      \
>>    $(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),,-E)  \
>> - $(if $(KBUILD_EXTMOD)$(KBUILD_MODPOST_WARN),-w)
>> + $(if $(KBUILD_MODPOST_WARN),-w)
>>
>>   MODPOST_OPT=$(subst -i,-n,$(filter -i,$(MAKEFLAGS)))
>>
>> --
>> 2.19.2
> 
> 
> 

Re: [PATCH] modpost: make KBUILD_MODPOST_WARN also configurable for external modules

[Masahiro Yamada]

Hi.

On Mon, Apr 8, 2019 at 5:03 PM Wiebe, Wladislav (Nokia - DE/Ulm)
<wladislav.wiebe@nokia.com> wrote:
>
> Hi!
>
> On 07.04.2019 11:04, Masahiro Yamada wrote:
> > (+CC Jonas Gorski)
> >
> >
> > On Tue, Mar 26, 2019 at 6:58 PM Wiebe, Wladislav (Nokia - DE/Ulm)
> > <wladislav.wiebe@nokia.com> wrote:
> >>
> >> Commit ea837f1c0503 ("kbuild: make modpost processing configurable")
> >> was intended to give KBUILD_MODPOST_WARN flexibility to be configurable.
> >> Right now KBUILD_MODPOST_WARN gets just ignored when KBUILD_EXTMOD is
> >> set which happens per default when building modules out of the tree.
> >>
> >> This change gives the opportunity to define module build behaving also
> >> in case of out of tree builds and default will become exit on error.
> >> Errors which can be detected by the build should be trapped out of the box
> >> there, unless somebody wants to notice broken stuff later at runtime.
> >
> > If an external module refers to a symbol
> > provided by another external module,
> > this patch will turn the warning into the error by default,
> > which is probably a bad idea.
>
> Indeed, exactly this should happen. You should fix your external module
> dependencies by providing their symbols. Please use e.g.
> KBUILD_EXTRA_SYMBOLS instead of converting errors to warning and hoping
> that things will work.

I know this option.
What I want to say is, this patch changes the behavior, and
may annoy some people.

> Or how do you want to make sure module A still
> delivers all symbols needed by module B after an update/changes?
> Manually comparing the logs after an update or waiting until it turns
> out broken during run-time? I wouldn't say this is a good idea :-)

OK, so the commit log should record both the behavior change
and workarounds.

- If an external module being built refers to symbols
  in another external module, Kbuild previously showed a warning,
  but going forward it will turn it into an error.

- To work around this, you should pass a symbol table via KBUILD_EXTRA_SYMBOLS,
  or KBUILD_EXTRA_SYMBOLS=1 to turn the error into a warning.


Thanks.

>
> > I suggested a desired change in the following.
> > https://patchwork.kernel.org/patch/9980691/
> >
> > I am poking Jonas Gorski
> > if he plans to send v2.
>
> I wouldn't vote for v2 based on explanation above.
>
> - Wladislav
>
> >
> >
> >> Signed-off-by: Wladislav Wiebe <wladislav.wiebe@nokia.com>
> >> ---
> >>   scripts/Makefile.modpost | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
> >> index 6b7f354f189a..fec6ec2ffa47 100644
> >> --- a/scripts/Makefile.modpost
> >> +++ b/scripts/Makefile.modpost
> >> @@ -78,7 +78,7 @@ modpost = scripts/mod/modpost                    \
> >>    $(if $(KBUILD_EXTRA_SYMBOLS), $(patsubst %, -e %,$(KBUILD_EXTRA_SYMBOLS))) \
> >>    $(if $(KBUILD_EXTMOD),-o $(modulesymfile))      \
> >>    $(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),,-E)  \
> >> - $(if $(KBUILD_EXTMOD)$(KBUILD_MODPOST_WARN),-w)
> >> + $(if $(KBUILD_MODPOST_WARN),-w)
> >>
> >>   MODPOST_OPT=$(subst -i,-n,$(filter -i,$(MAKEFLAGS)))
> >>
> >> --
> >> 2.19.2
> >
> >
> >


--
Best Regards
Masahiro Yamada

Re: [PATCH] modpost: make KBUILD_MODPOST_WARN also configurable for external modules

[Jonas Gorski]

Hi,

本当に申し訳ありません, I got sidetracked and completely forgot about it. I
actually still have my old tree with the suggested changes for v2.

On Tue, 9 Apr 2019 at 11:01, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> Hi.
>
> On Mon, Apr 8, 2019 at 5:03 PM Wiebe, Wladislav (Nokia - DE/Ulm)
> <wladislav.wiebe@nokia.com> wrote:
> >
> > Hi!
> >
> > On 07.04.2019 11:04, Masahiro Yamada wrote:
> > > (+CC Jonas Gorski)
> > >
> > >
> > > On Tue, Mar 26, 2019 at 6:58 PM Wiebe, Wladislav (Nokia - DE/Ulm)
> > > <wladislav.wiebe@nokia.com> wrote:
> > >>
> > >> Commit ea837f1c0503 ("kbuild: make modpost processing configurable")
> > >> was intended to give KBUILD_MODPOST_WARN flexibility to be configurable.
> > >> Right now KBUILD_MODPOST_WARN gets just ignored when KBUILD_EXTMOD is
> > >> set which happens per default when building modules out of the tree.
> > >>
> > >> This change gives the opportunity to define module build behaving also
> > >> in case of out of tree builds and default will become exit on error.
> > >> Errors which can be detected by the build should be trapped out of the box
> > >> there, unless somebody wants to notice broken stuff later at runtime.
> > >
> > > If an external module refers to a symbol
> > > provided by another external module,
> > > this patch will turn the warning into the error by default,
> > > which is probably a bad idea.
> >
> > Indeed, exactly this should happen. You should fix your external module
> > dependencies by providing their symbols. Please use e.g.
> > KBUILD_EXTRA_SYMBOLS instead of converting errors to warning and hoping
> > that things will work.
>
> I know this option.
> What I want to say is, this patch changes the behavior, and
> may annoy some people.
>
> > Or how do you want to make sure module A still
> > delivers all symbols needed by module B after an update/changes?
> > Manually comparing the logs after an update or waiting until it turns
> > out broken during run-time? I wouldn't say this is a good idea :-)
>
> OK, so the commit log should record both the behavior change
> and workarounds.
>
> - If an external module being built refers to symbols
>   in another external module, Kbuild previously showed a warning,
>   but going forward it will turn it into an error.
>
> - To work around this, you should pass a symbol table via KBUILD_EXTRA_SYMBOLS,
>   or KBUILD_EXTRA_SYMBOLS=1 to turn the error into a warning.

This sounds like a good middle ground. When you do the effort of
setting KBUILD_EXTRA_SYMBOLS, you are likely interested in proper
dependency resolution and being notified if it fails.

I would probably still use KBUILD_MODPOST_WARN=0 to force the warnings
as errors, to have it as the central switch for the behaviour. So the
behaviour would then become

- If KBUILD_MODPOST_WARN is set to any value, set -w accordingly
- else, set -w only when building for an external module and
KBUILD_EXTRA_SYMBOLS is empty

or

ifdef KBUILD_MODPOST_WARN
    KBUILD_MODPOST_WARN:=$(filter-out 0,$(KBUILD_MODPOST_WARN))
else
    KBUILD_MODPOST_WARN:=$(if $(KBUILD_EXTMOD),$(if $(KBUILD_EXTRA_SYMBOLS),,1))
endif

(completely untested)


Regards
Jonas

Re: [PATCH] modpost: make KBUILD_MODPOST_WARN also configurable for external modules

[Masahiro Yamada]

On Tue, Apr 9, 2019 at 7:31 PM Jonas Gorski <jonas.gorski@gmail.com> wrote:
>
> Hi,
>
> 本当に申し訳ありません, I got sidetracked and completely forgot about it. I
> actually still have my old tree with the suggested changes for v2.
>
> On Tue, 9 Apr 2019 at 11:01, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> >
> > Hi.
> >
> > On Mon, Apr 8, 2019 at 5:03 PM Wiebe, Wladislav (Nokia - DE/Ulm)
> > <wladislav.wiebe@nokia.com> wrote:
> > >
> > > Hi!
> > >
> > > On 07.04.2019 11:04, Masahiro Yamada wrote:
> > > > (+CC Jonas Gorski)
> > > >
> > > >
> > > > On Tue, Mar 26, 2019 at 6:58 PM Wiebe, Wladislav (Nokia - DE/Ulm)
> > > > <wladislav.wiebe@nokia.com> wrote:
> > > >>
> > > >> Commit ea837f1c0503 ("kbuild: make modpost processing configurable")
> > > >> was intended to give KBUILD_MODPOST_WARN flexibility to be configurable.
> > > >> Right now KBUILD_MODPOST_WARN gets just ignored when KBUILD_EXTMOD is
> > > >> set which happens per default when building modules out of the tree.
> > > >>
> > > >> This change gives the opportunity to define module build behaving also
> > > >> in case of out of tree builds and default will become exit on error.
> > > >> Errors which can be detected by the build should be trapped out of the box
> > > >> there, unless somebody wants to notice broken stuff later at runtime.
> > > >
> > > > If an external module refers to a symbol
> > > > provided by another external module,
> > > > this patch will turn the warning into the error by default,
> > > > which is probably a bad idea.
> > >
> > > Indeed, exactly this should happen. You should fix your external module
> > > dependencies by providing their symbols. Please use e.g.
> > > KBUILD_EXTRA_SYMBOLS instead of converting errors to warning and hoping
> > > that things will work.
> >
> > I know this option.
> > What I want to say is, this patch changes the behavior, and
> > may annoy some people.
> >
> > > Or how do you want to make sure module A still
> > > delivers all symbols needed by module B after an update/changes?
> > > Manually comparing the logs after an update or waiting until it turns
> > > out broken during run-time? I wouldn't say this is a good idea :-)
> >
> > OK, so the commit log should record both the behavior change
> > and workarounds.
> >
> > - If an external module being built refers to symbols
> >   in another external module, Kbuild previously showed a warning,
> >   but going forward it will turn it into an error.
> >
> > - To work around this, you should pass a symbol table via KBUILD_EXTRA_SYMBOLS,
> >   or KBUILD_EXTRA_SYMBOLS=1 to turn the error into a warning.
>
> This sounds like a good middle ground. When you do the effort of
> setting KBUILD_EXTRA_SYMBOLS, you are likely interested in proper
> dependency resolution and being notified if it fails.
>
> I would probably still use KBUILD_MODPOST_WARN=0 to force the warnings
> as errors, to have it as the central switch for the behaviour. So the
> behaviour would then become
>
> - If KBUILD_MODPOST_WARN is set to any value, set -w accordingly
> - else, set -w only when building for an external module and
> KBUILD_EXTRA_SYMBOLS is empty
>
> or
>
> ifdef KBUILD_MODPOST_WARN
>     KBUILD_MODPOST_WARN:=$(filter-out 0,$(KBUILD_MODPOST_WARN))
> else
>     KBUILD_MODPOST_WARN:=$(if $(KBUILD_EXTMOD),$(if $(KBUILD_EXTRA_SYMBOLS),,1))
> endif


Sorry, I think this is too complicated.

I applied this since it is simple.
https://patchwork.kernel.org/patch/10895465/



-- 
Best Regards
Masahiro Yamada

Re: [PATCH] modpost: make KBUILD_MODPOST_WARN also configurable for external modules

[Wiebe, Wladislav (Nokia - DE/Ulm)]

Hi,

On 09.04.2019 11:00, Masahiro Yamada wrote:
> Hi.
> 
> On Mon, Apr 8, 2019 at 5:03 PM Wiebe, Wladislav (Nokia - DE/Ulm)
> <wladislav.wiebe@nokia.com> wrote:
>>
>> Hi!
>>
>> On 07.04.2019 11:04, Masahiro Yamada wrote:
>>> (+CC Jonas Gorski)
>>>
>>>
>>> On Tue, Mar 26, 2019 at 6:58 PM Wiebe, Wladislav (Nokia - DE/Ulm)
>>> <wladislav.wiebe@nokia.com> wrote:
>>>>
>>>> Commit ea837f1c0503 ("kbuild: make modpost processing configurable")
>>>> was intended to give KBUILD_MODPOST_WARN flexibility to be configurable.
>>>> Right now KBUILD_MODPOST_WARN gets just ignored when KBUILD_EXTMOD is
>>>> set which happens per default when building modules out of the tree.
>>>>
>>>> This change gives the opportunity to define module build behaving also
>>>> in case of out of tree builds and default will become exit on error.
>>>> Errors which can be detected by the build should be trapped out of the box
>>>> there, unless somebody wants to notice broken stuff later at runtime.
>>>
>>> If an external module refers to a symbol
>>> provided by another external module,
>>> this patch will turn the warning into the error by default,
>>> which is probably a bad idea.
>>
>> Indeed, exactly this should happen. You should fix your external module
>> dependencies by providing their symbols. Please use e.g.
>> KBUILD_EXTRA_SYMBOLS instead of converting errors to warning and hoping
>> that things will work.
> 
> I know this option.
> What I want to say is, this patch changes the behavior, and
> may annoy some people.
> 
>> Or how do you want to make sure module A still
>> delivers all symbols needed by module B after an update/changes?
>> Manually comparing the logs after an update or waiting until it turns
>> out broken during run-time? I wouldn't say this is a good idea :-)
> 
> OK, so the commit log should record both the behavior change
> and workarounds.
> 
> - If an external module being built refers to symbols
>    in another external module, Kbuild previously showed a warning,
>    but going forward it will turn it into an error.
> 
> - To work around this, you should pass a symbol table via KBUILD_EXTRA_SYMBOLS,
>    or KBUILD_EXTRA_SYMBOLS=1 to turn the error into a warning.


Ok, I've posted v2:
https://lore.kernel.org/patchwork/patch/1059985/


Thanks!

- Wladislav



> Thanks.
> 
>>
>>> I suggested a desired change in the following.
>>> https://patchwork.kernel.org/patch/9980691/
>>>
>>> I am poking Jonas Gorski
>>> if he plans to send v2.
>>
>> I wouldn't vote for v2 based on explanation above.
>>
>> - Wladislav
>>
>>>
>>>
>>>> Signed-off-by: Wladislav Wiebe <wladislav.wiebe@nokia.com>
>>>> ---
>>>>    scripts/Makefile.modpost | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
>>>> index 6b7f354f189a..fec6ec2ffa47 100644
>>>> --- a/scripts/Makefile.modpost
>>>> +++ b/scripts/Makefile.modpost
>>>> @@ -78,7 +78,7 @@ modpost = scripts/mod/modpost                    \
>>>>     $(if $(KBUILD_EXTRA_SYMBOLS), $(patsubst %, -e %,$(KBUILD_EXTRA_SYMBOLS))) \
>>>>     $(if $(KBUILD_EXTMOD),-o $(modulesymfile))      \
>>>>     $(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),,-E)  \
>>>> - $(if $(KBUILD_EXTMOD)$(KBUILD_MODPOST_WARN),-w)
>>>> + $(if $(KBUILD_MODPOST_WARN),-w)
>>>>
>>>>    MODPOST_OPT=$(subst -i,-n,$(filter -i,$(MAKEFLAGS)))
>>>>
>>>> --
>>>> 2.19.2
>>>
>>>
>>>
> 
> 
> --
> Best Regards
> Masahiro Yamada
>