[PATCH -next] init/Kconfig: fix unmet direct dependencies

[PATCH -next] init/Kconfig: fix unmet direct dependencies

[Ren Zhijie]

Commit 3c07bfce92a5 ("proc: make config PROC_CHILDREN depend on PROC_FS") 
make config PROC_CHILDREN depend on PROC_FS.

When CONFIG_PROC_FS is not set and CONFIG_CHECKPOINT_RESTORE=y,
make menuconfig screams like this:

WARNING: unmet direct dependencies detected for PROC_CHILDREN
  Depends on [n]: PROC_FS [=n]
  Selected by [y]:
  - CHECKPOINT_RESTORE [=y]

So add PROC_FS to dependencies to fix this.
Fixes: 3c07bfce92a5 ("proc: make config PROC_CHILDREN depend on PROC_FS")
Signed-off-by: Ren Zhijie <renzhijie2@huawei.com>
---
 init/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/init/Kconfig b/init/Kconfig
index e11307310fc1..fc32b28fe93e 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1273,6 +1273,7 @@ endif # NAMESPACES
 
 config CHECKPOINT_RESTORE
 	bool "Checkpoint/restore support"
+	select PROC_FS
 	select PROC_CHILDREN
 	select KCMP
 	default n
-- 
2.17.1

Re: [PATCH -next] init/Kconfig: fix unmet direct dependencies

[Sebastian Andrzej Siewior]

On 2022-09-28 06:49:34 [+0000], Ren Zhijie wrote:
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1273,6 +1273,7 @@ endif # NAMESPACES
>  
>  config CHECKPOINT_RESTORE
>  	bool "Checkpoint/restore support"
> +	select PROC_FS

Couldn't this become a depends?

>  	select PROC_CHILDREN
>  	select KCMP
>  	default n

Sebastian

Re: [PATCH -next] init/Kconfig: fix unmet direct dependencies

[Lukas Bulwahn]

On Wed, Sep 28, 2022 at 8:53 AM Ren Zhijie <renzhijie2@huawei.com> wrote:
>
> Commit 3c07bfce92a5 ("proc: make config PROC_CHILDREN depend on PROC_FS")
> make config PROC_CHILDREN depend on PROC_FS.
>
> When CONFIG_PROC_FS is not set and CONFIG_CHECKPOINT_RESTORE=y,
> make menuconfig screams like this:
>
> WARNING: unmet direct dependencies detected for PROC_CHILDREN
>   Depends on [n]: PROC_FS [=n]
>   Selected by [y]:
>   - CHECKPOINT_RESTORE [=y]
>
> So add PROC_FS to dependencies to fix this.
> Fixes: 3c07bfce92a5 ("proc: make config PROC_CHILDREN depend on PROC_FS")
> Signed-off-by: Ren Zhijie <renzhijie2@huawei.com>

This patch here makes sense.

Just some minor addition:

The commit  3c07bfce92a5 ("proc: make config PROC_CHILDREN depend on
PROC_FS") is not really broken, but just made the implicit
dependencies much more explicit and hence now this warning from
menuconfig indicates that.

Before the commit 3c07bfce92a5 ("proc: make config PROC_CHILDREN
depend on PROC_FS"):

When CONFIG_PROC_FS is not set and CONFIG_CHECKPOINT_RESTORE=y, then
the feature of CONFIG_CHECKPOINT_RESTORE would just not worked (as
this would not be any proc fs, and no "proc children" functionality.)

After the commit 3c07bfce92a5 ("proc: make config PROC_CHILDREN depend
on PROC_FS"):

When CONFIG_PROC_FS is not set and CONFIG_CHECKPOINT_RESTORE=y, it
warns to add PROC_FS to assist in configuring something that makes the
feature of CONFIG_CHECKPOINT_RESTORE work.

So this patch is yet another improvement to what the commit
3c07bfce92a5 ("proc: make config PROC_CHILDREN depend on PROC_FS")
already partially improved. However, feel free to keep the Fixes-tag
in this patch, as this patch certainly improves handling of what the
commit 3c07bfce92a5 started improving.

Reviewed-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>

The recipient list is a bit "random" (coincidental). You could have
probably just sent this patch to Andrew Morton, me and linux-kernel
mailing list.

Lukas

> ---
>  init/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/init/Kconfig b/init/Kconfig
> index e11307310fc1..fc32b28fe93e 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1273,6 +1273,7 @@ endif # NAMESPACES
>
>  config CHECKPOINT_RESTORE
>         bool "Checkpoint/restore support"
> +       select PROC_FS
>         select PROC_CHILDREN
>         select KCMP
>         default n
> --
> 2.17.1
>

Re: [PATCH -next] init/Kconfig: fix unmet direct dependencies

[Lukas Bulwahn]

On Wed, Sep 28, 2022 at 9:01 AM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> On 2022-09-28 06:49:34 [+0000], Ren Zhijie wrote:
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -1273,6 +1273,7 @@ endif # NAMESPACES
> >
> >  config CHECKPOINT_RESTORE
> >       bool "Checkpoint/restore support"
> > +     select PROC_FS
>
> Couldn't this become a depends?
>

It could also be a depends (to resolve the warning).

It is just the question whether:

When PROC_FS is not set, should the CHECKPOINT_RESTORE still be
visible as a config option to add (and then automatically add
PROC_FS)? Then select is right here.

or:

When PROC_FS is not set, should the CHECKPOINT_RESTORE not be visible
as a config option to add? Instead the user first needs to add
PROC_FS, then CHECKPOINT_RESTORE becomes visible as an option to add,
and then the user can add it. Then depends would be right.

For me, both seem reasonable. So, I assume Ren considered select the
better choice.

But maybe Ren can confirm.

A kernel build configuration without PROC_FS is quite special
anyway... and then being interested in CHECKPOINT_ RESTORE for such a
system is really really special. I wonder if that user then really
knows what he or she is configuring at that point.


Lukas

Re: [PATCH -next] init/Kconfig: fix unmet direct dependencies

[Ren Zhijie]

在 2022/9/28 15:20, Lukas Bulwahn 写道:
> On Wed, Sep 28, 2022 at 9:01 AM Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
>> On 2022-09-28 06:49:34 [+0000], Ren Zhijie wrote:
>>> --- a/init/Kconfig
>>> +++ b/init/Kconfig
>>> @@ -1273,6 +1273,7 @@ endif # NAMESPACES
>>>
>>>   config CHECKPOINT_RESTORE
>>>        bool "Checkpoint/restore support"
>>> +     select PROC_FS
>> Couldn't this become a depends?
>>
> It could also be a depends (to resolve the warning).
>
> It is just the question whether:
>
> When PROC_FS is not set, should the CHECKPOINT_RESTORE still be
> visible as a config option to add (and then automatically add
> PROC_FS)? Then select is right here.
>
> or:
>
> When PROC_FS is not set, should the CHECKPOINT_RESTORE not be visible
> as a config option to add? Instead the user first needs to add
> PROC_FS, then CHECKPOINT_RESTORE becomes visible as an option to add,
> and then the user can add it. Then depends would be right.
>
> For me, both seem reasonable. So, I assume Ren considered select the
> better choice.
>
> But maybe Ren can confirm.

My consider is that if CHECKPOINT_RESTORE depends on PROC_FS , when 
PROC_FS is not set the user have no chance to set it on.

Thanks,

Ren

>
> A kernel build configuration without PROC_FS is quite special
> anyway... and then being interested in CHECKPOINT_ RESTORE for such a
> system is really really special. I wonder if that user then really
> knows what he or she is configuring at that point.
>
>
> Lukas
> .
.

Re: [PATCH -next] init/Kconfig: fix unmet direct dependencies

[Sebastian Andrzej Siewior]

On 2022-09-28 09:20:42 [+0200], Lukas Bulwahn wrote:
> > Couldn't this become a depends?
> It could also be a depends (to resolve the warning).
…
> It is just the question whether:
> 
> When PROC_FS is not set, should the CHECKPOINT_RESTORE still be
> visible as a config option to add (and then automatically add
> PROC_FS)? Then select is right here.

then CHECKPOINT_RESTORE is the only option selecting PROC_FS while
everyone else depends on it _or_ avoids using it in the absence of
PROC_FS.

Sebastian

Re: [PATCH -next] init/Kconfig: fix unmet direct dependencies

[Arnd Bergmann]

On Wed, Sep 28, 2022, at 11:14 AM, Sebastian Andrzej Siewior wrote:
> On 2022-09-28 09:20:42 [+0200], Lukas Bulwahn wrote:
>> > Couldn't this become a depends?
>> It could also be a depends (to resolve the warning).
> …
>> It is just the question whether:
>> 
>> When PROC_FS is not set, should the CHECKPOINT_RESTORE still be
>> visible as a config option to add (and then automatically add
>> PROC_FS)? Then select is right here.
>
> then CHECKPOINT_RESTORE is the only option selecting PROC_FS while
> everyone else depends on it _or_ avoids using it in the absence of
> PROC_FS.

Right, we should not mix 'select' and 'depends on' for the same
symbol, as that leads to circular dependencies and general
confusion.

If there is no way to use CHECKPOINT_RESTORE without procfs,
then the symbol should just not be visible (it will still show
up with the dependency when one searches in menuconfig).
Force-enabling a major subsystem like procfs from another
symbol is not a good solution.

    Arnd

Re: [PATCH -next] init/Kconfig: fix unmet direct dependencies

[Lukas Bulwahn]

On Wed, Sep 28, 2022 at 11:32 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wed, Sep 28, 2022, at 11:14 AM, Sebastian Andrzej Siewior wrote:
> > On 2022-09-28 09:20:42 [+0200], Lukas Bulwahn wrote:
> >> > Couldn't this become a depends?
> >> It could also be a depends (to resolve the warning).
> > …
> >> It is just the question whether:
> >>
> >> When PROC_FS is not set, should the CHECKPOINT_RESTORE still be
> >> visible as a config option to add (and then automatically add
> >> PROC_FS)? Then select is right here.
> >
> > then CHECKPOINT_RESTORE is the only option selecting PROC_FS while
> > everyone else depends on it _or_ avoids using it in the absence of
> > PROC_FS.
>
> Right, we should not mix 'select' and 'depends on' for the same
> symbol, as that leads to circular dependencies and general
> confusion.
>
> If there is no way to use CHECKPOINT_RESTORE without procfs,
> then the symbol should just not be visible (it will still show
> up with the dependency when one searches in menuconfig).
> Force-enabling a major subsystem like procfs from another
> symbol is not a good solution.
>

Agree. I retract my Reviewed-by.

The arguments are clear to make this depend on PROC_FS.

Lukas

Re: [PATCH -next] init/Kconfig: fix unmet direct dependencies

[Ren Zhijie]

在 2022/9/28 17:32, Arnd Bergmann 写道:
> On Wed, Sep 28, 2022, at 11:14 AM, Sebastian Andrzej Siewior wrote:
>> On 2022-09-28 09:20:42 [+0200], Lukas Bulwahn wrote:
>>>> Couldn't this become a depends?
>>> It could also be a depends (to resolve the warning).
>> …
>>> It is just the question whether:
>>>
>>> When PROC_FS is not set, should the CHECKPOINT_RESTORE still be
>>> visible as a config option to add (and then automatically add
>>> PROC_FS)? Then select is right here.
>> then CHECKPOINT_RESTORE is the only option selecting PROC_FS while
>> everyone else depends on it _or_ avoids using it in the absence of
>> PROC_FS.
> Right, we should not mix 'select' and 'depends on' for the same
> symbol, as that leads to circular dependencies and general
> confusion.
>
> If there is no way to use CHECKPOINT_RESTORE without procfs,
> then the symbol should just not be visible (it will still show
> up with the dependency when one searches in menuconfig).
> Force-enabling a major subsystem like procfs from another
> symbol is not a good solution.

Agree that,  i will fix it in patch v2.

Thanks,

Ren

>      Arnd
> .