linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] of: Don't allow __of_attached_node_sysfs() without CONFIG_SYSFS
@ 2021-08-20 14:47 Marc Zyngier
  2021-08-23  4:01 ` Frank Rowand
  2021-08-23 18:48 ` Rob Herring
  0 siblings, 2 replies; 8+ messages in thread
From: Marc Zyngier @ 2021-08-20 14:47 UTC (permalink / raw)
  To: linux-kernel, devicetree; +Cc: frowand.list, robh+dt, kernel-team

Trying to boot without SYSFS, but with OF_DYNAMIC quickly
results in a crash:

[    0.088460] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000070
[...]
[    0.103927] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.14.0-rc3 #4179
[    0.105810] Hardware name: linux,dummy-virt (DT)
[    0.107147] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
[    0.108876] pc : kernfs_find_and_get_ns+0x3c/0x7c
[    0.110244] lr : kernfs_find_and_get_ns+0x3c/0x7c
[...]
[    0.134087] Call trace:
[    0.134800]  kernfs_find_and_get_ns+0x3c/0x7c
[    0.136054]  safe_name+0x4c/0xd0
[    0.136994]  __of_attach_node_sysfs+0xf8/0x124
[    0.138287]  of_core_init+0x90/0xfc
[    0.139296]  driver_init+0x30/0x4c
[    0.140283]  kernel_init_freeable+0x160/0x1b8
[    0.141543]  kernel_init+0x30/0x140
[    0.142561]  ret_from_fork+0x10/0x18

While not having sysfs isn't a very common option these days,
it is still expected that such configuration would work.

Paper over it by bailing out from __of_attach_node_sysfs() if
CONFIG_SYSFS isn't enabled.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/of/kobj.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/kobj.c b/drivers/of/kobj.c
index a32e60b024b8..6675b5e56960 100644
--- a/drivers/of/kobj.c
+++ b/drivers/of/kobj.c
@@ -119,7 +119,7 @@ int __of_attach_node_sysfs(struct device_node *np)
 	struct property *pp;
 	int rc;
 
-	if (!of_kset)
+	if (!IS_ENABLED(CONFIG_SYSFS) || !of_kset)
 		return 0;
 
 	np->kobj.kset = of_kset;
-- 
2.30.2


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

* Re: [PATCH] of: Don't allow __of_attached_node_sysfs() without CONFIG_SYSFS
  2021-08-20 14:47 [PATCH] of: Don't allow __of_attached_node_sysfs() without CONFIG_SYSFS Marc Zyngier
@ 2021-08-23  4:01 ` Frank Rowand
  2021-08-23  8:04   ` Marc Zyngier
  2021-08-23 18:46   ` Rob Herring
  2021-08-23 18:48 ` Rob Herring
  1 sibling, 2 replies; 8+ messages in thread
From: Frank Rowand @ 2021-08-23  4:01 UTC (permalink / raw)
  To: Marc Zyngier, linux-kernel, devicetree; +Cc: robh+dt, kernel-team

Hi Marc,

On 8/20/21 9:47 AM, Marc Zyngier wrote:
> Trying to boot without SYSFS, but with OF_DYNAMIC quickly
> results in a crash:
> 
> [    0.088460] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000070
> [...]
> [    0.103927] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.14.0-rc3 #4179
> [    0.105810] Hardware name: linux,dummy-virt (DT)
> [    0.107147] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
> [    0.108876] pc : kernfs_find_and_get_ns+0x3c/0x7c
> [    0.110244] lr : kernfs_find_and_get_ns+0x3c/0x7c
> [...]
> [    0.134087] Call trace:
> [    0.134800]  kernfs_find_and_get_ns+0x3c/0x7c
> [    0.136054]  safe_name+0x4c/0xd0
> [    0.136994]  __of_attach_node_sysfs+0xf8/0x124
> [    0.138287]  of_core_init+0x90/0xfc
> [    0.139296]  driver_init+0x30/0x4c
> [    0.140283]  kernel_init_freeable+0x160/0x1b8
> [    0.141543]  kernel_init+0x30/0x140
> [    0.142561]  ret_from_fork+0x10/0x18
> 
> While not having sysfs isn't a very common option these days,
> it is still expected that such configuration would work.
> 
> Paper over it by bailing out from __of_attach_node_sysfs() if
> CONFIG_SYSFS isn't enabled.

CONFIG_SYSFS should be automatically selected when CONFIG_OF_DYNAMIC
is enabled, and it should not be possible to disable CONFIG_SYSFS
in this case.

Can you send your .config?
What ARCH did you build the kernel with?

Thanks,

Frank


> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  drivers/of/kobj.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/of/kobj.c b/drivers/of/kobj.c
> index a32e60b024b8..6675b5e56960 100644
> --- a/drivers/of/kobj.c
> +++ b/drivers/of/kobj.c
> @@ -119,7 +119,7 @@ int __of_attach_node_sysfs(struct device_node *np)
>  	struct property *pp;
>  	int rc;
>  
> -	if (!of_kset)
> +	if (!IS_ENABLED(CONFIG_SYSFS) || !of_kset)
>  		return 0;
>  
>  	np->kobj.kset = of_kset;
> 


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

* Re: [PATCH] of: Don't allow __of_attached_node_sysfs() without CONFIG_SYSFS
  2021-08-23  4:01 ` Frank Rowand
@ 2021-08-23  8:04   ` Marc Zyngier
  2021-08-23 18:46   ` Rob Herring
  1 sibling, 0 replies; 8+ messages in thread
From: Marc Zyngier @ 2021-08-23  8:04 UTC (permalink / raw)
  To: Frank Rowand; +Cc: linux-kernel, devicetree, robh+dt, kernel-team

Hi Frank,

On Mon, 23 Aug 2021 05:01:15 +0100,
Frank Rowand <frowand.list@gmail.com> wrote:
> 
> Hi Marc,
> 
> On 8/20/21 9:47 AM, Marc Zyngier wrote:
> > Trying to boot without SYSFS, but with OF_DYNAMIC quickly
> > results in a crash:
> > 
> > [    0.088460] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000070
> > [...]
> > [    0.103927] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.14.0-rc3 #4179
> > [    0.105810] Hardware name: linux,dummy-virt (DT)
> > [    0.107147] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
> > [    0.108876] pc : kernfs_find_and_get_ns+0x3c/0x7c
> > [    0.110244] lr : kernfs_find_and_get_ns+0x3c/0x7c
> > [...]
> > [    0.134087] Call trace:
> > [    0.134800]  kernfs_find_and_get_ns+0x3c/0x7c
> > [    0.136054]  safe_name+0x4c/0xd0
> > [    0.136994]  __of_attach_node_sysfs+0xf8/0x124
> > [    0.138287]  of_core_init+0x90/0xfc
> > [    0.139296]  driver_init+0x30/0x4c
> > [    0.140283]  kernel_init_freeable+0x160/0x1b8
> > [    0.141543]  kernel_init+0x30/0x140
> > [    0.142561]  ret_from_fork+0x10/0x18
> > 
> > While not having sysfs isn't a very common option these days,
> > it is still expected that such configuration would work.
> > 
> > Paper over it by bailing out from __of_attach_node_sysfs() if
> > CONFIG_SYSFS isn't enabled.
> 
> CONFIG_SYSFS should be automatically selected when CONFIG_OF_DYNAMIC
> is enabled, and it should not be possible to disable CONFIG_SYSFS
> in this case.

OF_DYNAMIC selects OF_KOBJ, which itself doesn't actually depends on
SYSFS. It merely has a "def_bool SYSFS". If a stronger dependency is
expected, that's probably where it should be expressed.

However, kobj.c already has a couple of

	if (!IS_ENABLED(CONFIG_SYSFS))
		return 0;

sprinkled over it, so there is already an assumption that it can be
build without SYSFS.

> 
> Can you send your .config?

https://pastebin.com/MmRWuxLr

> What ARCH did you build the kernel with?

That's an arm64 build.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

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

* Re: [PATCH] of: Don't allow __of_attached_node_sysfs() without CONFIG_SYSFS
  2021-08-23  4:01 ` Frank Rowand
  2021-08-23  8:04   ` Marc Zyngier
@ 2021-08-23 18:46   ` Rob Herring
  2021-08-23 21:27     ` Frank Rowand
  1 sibling, 1 reply; 8+ messages in thread
From: Rob Herring @ 2021-08-23 18:46 UTC (permalink / raw)
  To: Frank Rowand; +Cc: Marc Zyngier, linux-kernel, devicetree, kernel-team

On Sun, Aug 22, 2021 at 11:01:15PM -0500, Frank Rowand wrote:
> Hi Marc,
> 
> On 8/20/21 9:47 AM, Marc Zyngier wrote:
> > Trying to boot without SYSFS, but with OF_DYNAMIC quickly
> > results in a crash:
> > 
> > [    0.088460] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000070
> > [...]
> > [    0.103927] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.14.0-rc3 #4179
> > [    0.105810] Hardware name: linux,dummy-virt (DT)
> > [    0.107147] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
> > [    0.108876] pc : kernfs_find_and_get_ns+0x3c/0x7c
> > [    0.110244] lr : kernfs_find_and_get_ns+0x3c/0x7c
> > [...]
> > [    0.134087] Call trace:
> > [    0.134800]  kernfs_find_and_get_ns+0x3c/0x7c
> > [    0.136054]  safe_name+0x4c/0xd0
> > [    0.136994]  __of_attach_node_sysfs+0xf8/0x124
> > [    0.138287]  of_core_init+0x90/0xfc
> > [    0.139296]  driver_init+0x30/0x4c
> > [    0.140283]  kernel_init_freeable+0x160/0x1b8
> > [    0.141543]  kernel_init+0x30/0x140
> > [    0.142561]  ret_from_fork+0x10/0x18
> > 
> > While not having sysfs isn't a very common option these days,
> > it is still expected that such configuration would work.
> > 
> > Paper over it by bailing out from __of_attach_node_sysfs() if
> > CONFIG_SYSFS isn't enabled.
> 
> CONFIG_SYSFS should be automatically selected when CONFIG_OF_DYNAMIC
> is enabled, and it should not be possible to disable CONFIG_SYSFS
> in this case.

That used to be true, but isn't now.

Rob

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

* Re: [PATCH] of: Don't allow __of_attached_node_sysfs() without CONFIG_SYSFS
  2021-08-20 14:47 [PATCH] of: Don't allow __of_attached_node_sysfs() without CONFIG_SYSFS Marc Zyngier
  2021-08-23  4:01 ` Frank Rowand
@ 2021-08-23 18:48 ` Rob Herring
  1 sibling, 0 replies; 8+ messages in thread
From: Rob Herring @ 2021-08-23 18:48 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: linux-kernel, devicetree, frowand.list, robh+dt, kernel-team

On Fri, 20 Aug 2021 15:47:22 +0100, Marc Zyngier wrote:
> Trying to boot without SYSFS, but with OF_DYNAMIC quickly
> results in a crash:
> 
> [    0.088460] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000070
> [...]
> [    0.103927] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.14.0-rc3 #4179
> [    0.105810] Hardware name: linux,dummy-virt (DT)
> [    0.107147] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
> [    0.108876] pc : kernfs_find_and_get_ns+0x3c/0x7c
> [    0.110244] lr : kernfs_find_and_get_ns+0x3c/0x7c
> [...]
> [    0.134087] Call trace:
> [    0.134800]  kernfs_find_and_get_ns+0x3c/0x7c
> [    0.136054]  safe_name+0x4c/0xd0
> [    0.136994]  __of_attach_node_sysfs+0xf8/0x124
> [    0.138287]  of_core_init+0x90/0xfc
> [    0.139296]  driver_init+0x30/0x4c
> [    0.140283]  kernel_init_freeable+0x160/0x1b8
> [    0.141543]  kernel_init+0x30/0x140
> [    0.142561]  ret_from_fork+0x10/0x18
> 
> While not having sysfs isn't a very common option these days,
> it is still expected that such configuration would work.
> 
> Paper over it by bailing out from __of_attach_node_sysfs() if
> CONFIG_SYSFS isn't enabled.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  drivers/of/kobj.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Applied, thanks!

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

* Re: [PATCH] of: Don't allow __of_attached_node_sysfs() without CONFIG_SYSFS
  2021-08-23 18:46   ` Rob Herring
@ 2021-08-23 21:27     ` Frank Rowand
  2021-08-23 22:14       ` Rob Herring
  0 siblings, 1 reply; 8+ messages in thread
From: Frank Rowand @ 2021-08-23 21:27 UTC (permalink / raw)
  To: Rob Herring; +Cc: Marc Zyngier, linux-kernel, devicetree, kernel-team

On 8/23/21 1:46 PM, Rob Herring wrote:
> On Sun, Aug 22, 2021 at 11:01:15PM -0500, Frank Rowand wrote:
>> Hi Marc,
>>
>> On 8/20/21 9:47 AM, Marc Zyngier wrote:
>>> Trying to boot without SYSFS, but with OF_DYNAMIC quickly
>>> results in a crash:
>>>
>>> [    0.088460] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000070
>>> [...]
>>> [    0.103927] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.14.0-rc3 #4179
>>> [    0.105810] Hardware name: linux,dummy-virt (DT)
>>> [  0.107147] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
>>> [    0.108876] pc : kernfs_find_and_get_ns+0x3c/0x7c
>>> [    0.110244] lr : kernfs_find_and_get_ns+0x3c/0x7c
>>> [...]
>>> [    0.134087] Call trace:
>>> [    0.134800]  kernfs_find_and_get_ns+0x3c/0x7c
>>> [    0.136054]  safe_name+0x4c/0xd0
>>> [    0.136994]  __of_attach_node_sysfs+0xf8/0x124
>>> [    0.138287]  of_core_init+0x90/0xfc
>>> [    0.139296]  driver_init+0x30/0x4c
>>> [    0.140283]  kernel_init_freeable+0x160/0x1b8
>>> [    0.141543]  kernel_init+0x30/0x140
>>> [    0.142561]  ret_from_fork+0x10/0x18
>>>
>>> While not having sysfs isn't a very common option these days,
>>> it is still expected that such configuration would work.
>>>
>>> Paper over it by bailing out from __of_attach_node_sysfs() if
>>> CONFIG_SYSFS isn't enabled.
>>
>> CONFIG_SYSFS should be automatically selected when CONFIG_OF_DYNAMIC
>> is enabled, and it should not be possible to disable CONFIG_SYSFS
>> in this case.
> 
> That used to be true, but isn't now.

OK.  I agree with you, but when I investigated the original patch
email I came to a different conclusion because of the way that I
used make menuconfig to debug the situation.

It is true when I start with a .config created from 'make qcom_defconfig',
then select OF_UNITTEST, which is the only way I can see OF_DYNAMIC.  It
is the "if OF_UNITTEST" that means SYSFS can not be disabled.

If I start with the .config that Marc supplied, then make menuconfig
still does not show the OC_DYNAMIC option, but leaves it set since
it was already set.  In this case SYSFS remains disabled because
OF_UNITTEST is also disabled.

Using '/OF_DYNAMIC' from within make menuconfig, to get more info
about OF_DYNAMIC tells me that the prompt for OF_DYNAMIC is visible
if OF && OF_UNITTEST.  This is due to the "if OF_UNITTEST" in line 58
of drivers/of/Kconfig in the OF_DYNAMIC specification.

Thus I can't figure out how to use make menuconfig to set OF_DYNAMIC
without setting OF_UNITTEST.  I tried setting OF_UNITTEST, then setting
OF_DYNAMIC, saving the changes, then another make menuconfig to
disable OF_UNITTEST, which then has the side effect of unsetting
OF_DYNAMIC.

Would you accept a patch that removes the "if OF_UNITTEST" from
the Kconfig entry for OF_DYNAMIC?

-Frank

> 
> Rob
> 


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

* Re: [PATCH] of: Don't allow __of_attached_node_sysfs() without CONFIG_SYSFS
  2021-08-23 21:27     ` Frank Rowand
@ 2021-08-23 22:14       ` Rob Herring
  2021-08-23 22:21         ` Frank Rowand
  0 siblings, 1 reply; 8+ messages in thread
From: Rob Herring @ 2021-08-23 22:14 UTC (permalink / raw)
  To: Frank Rowand; +Cc: Marc Zyngier, linux-kernel, devicetree, Android Kernel Team

On Mon, Aug 23, 2021 at 4:27 PM Frank Rowand <frowand.list@gmail.com> wrote:
>
> On 8/23/21 1:46 PM, Rob Herring wrote:
> > On Sun, Aug 22, 2021 at 11:01:15PM -0500, Frank Rowand wrote:
> >> Hi Marc,
> >>
> >> On 8/20/21 9:47 AM, Marc Zyngier wrote:
> >>> Trying to boot without SYSFS, but with OF_DYNAMIC quickly
> >>> results in a crash:
> >>>
> >>> [    0.088460] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000070
> >>> [...]
> >>> [    0.103927] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.14.0-rc3 #4179
> >>> [    0.105810] Hardware name: linux,dummy-virt (DT)
> >>> [  0.107147] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
> >>> [    0.108876] pc : kernfs_find_and_get_ns+0x3c/0x7c
> >>> [    0.110244] lr : kernfs_find_and_get_ns+0x3c/0x7c
> >>> [...]
> >>> [    0.134087] Call trace:
> >>> [    0.134800]  kernfs_find_and_get_ns+0x3c/0x7c
> >>> [    0.136054]  safe_name+0x4c/0xd0
> >>> [    0.136994]  __of_attach_node_sysfs+0xf8/0x124
> >>> [    0.138287]  of_core_init+0x90/0xfc
> >>> [    0.139296]  driver_init+0x30/0x4c
> >>> [    0.140283]  kernel_init_freeable+0x160/0x1b8
> >>> [    0.141543]  kernel_init+0x30/0x140
> >>> [    0.142561]  ret_from_fork+0x10/0x18
> >>>
> >>> While not having sysfs isn't a very common option these days,
> >>> it is still expected that such configuration would work.
> >>>
> >>> Paper over it by bailing out from __of_attach_node_sysfs() if
> >>> CONFIG_SYSFS isn't enabled.
> >>
> >> CONFIG_SYSFS should be automatically selected when CONFIG_OF_DYNAMIC
> >> is enabled, and it should not be possible to disable CONFIG_SYSFS
> >> in this case.
> >
> > That used to be true, but isn't now.
>
> OK.  I agree with you, but when I investigated the original patch
> email I came to a different conclusion because of the way that I
> used make menuconfig to debug the situation.
>
> It is true when I start with a .config created from 'make qcom_defconfig',
> then select OF_UNITTEST, which is the only way I can see OF_DYNAMIC.  It
> is the "if OF_UNITTEST" that means SYSFS can not be disabled.

Not really. Disabling SYSFS has nothing to do with the DT code. It's
not super easy though. It required setting EXPERT and disabling
CONFIGFS_FS and things selecting it (PCIE endpoint and USB gadget).

> If I start with the .config that Marc supplied, then make menuconfig
> still does not show the OC_DYNAMIC option, but leaves it set since
> it was already set.  In this case SYSFS remains disabled because
> OF_UNITTEST is also disabled.

I don't see the relationship between SYSFS and OF_UNITTEST.

> Using '/OF_DYNAMIC' from within make menuconfig, to get more info
> about OF_DYNAMIC tells me that the prompt for OF_DYNAMIC is visible
> if OF && OF_UNITTEST.  This is due to the "if OF_UNITTEST" in line 58
> of drivers/of/Kconfig in the OF_DYNAMIC specification.
>
> Thus I can't figure out how to use make menuconfig to set OF_DYNAMIC
> without setting OF_UNITTEST.  I tried setting OF_UNITTEST, then setting
> OF_DYNAMIC, saving the changes, then another make menuconfig to
> disable OF_UNITTEST, which then has the side effect of unsetting
> OF_DYNAMIC.

Selecting OF_OVERLAY also.

> Would you accept a patch that removes the "if OF_UNITTEST" from
> the Kconfig entry for OF_DYNAMIC?

I guess. The purpose of making it visible was for compile testing, so
maybe make it 'if COMPILE_TEST' instead?


Looking at occurrences of CONFIG_OF_DYNAMIC, there's more than I'd
like. This for example is a common pattern:

drivers/spi/spi.c:      if (IS_ENABLED(CONFIG_OF_DYNAMIC))
drivers/spi/spi.c-
WARN_ON(of_reconfig_notifier_register(&spi_of_notifier));

Really, of_reconfig_notifier_register() should just return 0 if
!IS_ENABLED(CONFIG_OF_DYNAMIC).

Rob

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

* Re: [PATCH] of: Don't allow __of_attached_node_sysfs() without CONFIG_SYSFS
  2021-08-23 22:14       ` Rob Herring
@ 2021-08-23 22:21         ` Frank Rowand
  0 siblings, 0 replies; 8+ messages in thread
From: Frank Rowand @ 2021-08-23 22:21 UTC (permalink / raw)
  To: Rob Herring; +Cc: Marc Zyngier, linux-kernel, devicetree, Android Kernel Team

On 8/23/21 5:14 PM, Rob Herring wrote:
> On Mon, Aug 23, 2021 at 4:27 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>
>> On 8/23/21 1:46 PM, Rob Herring wrote:
>>> On Sun, Aug 22, 2021 at 11:01:15PM -0500, Frank Rowand wrote:
>>>> Hi Marc,
>>>>
>>>> On 8/20/21 9:47 AM, Marc Zyngier wrote:
>>>>> Trying to boot without SYSFS, but with OF_DYNAMIC quickly
>>>>> results in a crash:
>>>>>
>>>>> [    0.088460] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000070
>>>>> [...]
>>>>> [    0.103927] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.14.0-rc3 #4179
>>>>> [    0.105810] Hardware name: linux,dummy-virt (DT)
>>>>> [  0.107147] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
>>>>> [    0.108876] pc : kernfs_find_and_get_ns+0x3c/0x7c
>>>>> [    0.110244] lr : kernfs_find_and_get_ns+0x3c/0x7c
>>>>> [...]
>>>>> [    0.134087] Call trace:
>>>>> [    0.134800]  kernfs_find_and_get_ns+0x3c/0x7c
>>>>> [    0.136054]  safe_name+0x4c/0xd0
>>>>> [    0.136994]  __of_attach_node_sysfs+0xf8/0x124
>>>>> [    0.138287]  of_core_init+0x90/0xfc
>>>>> [    0.139296]  driver_init+0x30/0x4c
>>>>> [    0.140283]  kernel_init_freeable+0x160/0x1b8
>>>>> [    0.141543]  kernel_init+0x30/0x140
>>>>> [    0.142561]  ret_from_fork+0x10/0x18
>>>>>
>>>>> While not having sysfs isn't a very common option these days,
>>>>> it is still expected that such configuration would work.
>>>>>
>>>>> Paper over it by bailing out from __of_attach_node_sysfs() if
>>>>> CONFIG_SYSFS isn't enabled.
>>>>
>>>> CONFIG_SYSFS should be automatically selected when CONFIG_OF_DYNAMIC
>>>> is enabled, and it should not be possible to disable CONFIG_SYSFS
>>>> in this case.
>>>
>>> That used to be true, but isn't now.
>>
>> OK.  I agree with you, but when I investigated the original patch
>> email I came to a different conclusion because of the way that I
>> used make menuconfig to debug the situation.
>>
>> It is true when I start with a .config created from 'make qcom_defconfig',
>> then select OF_UNITTEST, which is the only way I can see OF_DYNAMIC.  It
>> is the "if OF_UNITTEST" that means SYSFS can not be disabled.
> 
> Not really. Disabling SYSFS has nothing to do with the DT code. It's
> not super easy though. It required setting EXPERT and disabling
> CONFIGFS_FS and things selecting it (PCIE endpoint and USB gadget).
> 
>> If I start with the .config that Marc supplied, then make menuconfig
>> still does not show the OC_DYNAMIC option, but leaves it set since
>> it was already set.  In this case SYSFS remains disabled because
>> OF_UNITTEST is also disabled.
> 
> I don't see the relationship between SYSFS and OF_UNITTEST.

I don't either.  Other than the results of experimenting showing
that if I enable OF_UNITTEST then I can no long disable SYSFS.

> 
>> Using '/OF_DYNAMIC' from within make menuconfig, to get more info
>> about OF_DYNAMIC tells me that the prompt for OF_DYNAMIC is visible
>> if OF && OF_UNITTEST.  This is due to the "if OF_UNITTEST" in line 58
>> of drivers/of/Kconfig in the OF_DYNAMIC specification.
>>
>> Thus I can't figure out how to use make menuconfig to set OF_DYNAMIC
>> without setting OF_UNITTEST.  I tried setting OF_UNITTEST, then setting
>> OF_DYNAMIC, saving the changes, then another make menuconfig to
>> disable OF_UNITTEST, which then has the side effect of unsetting
>> OF_DYNAMIC.
> 
> Selecting OF_OVERLAY also.
> 
>> Would you accept a patch that removes the "if OF_UNITTEST" from
>> the Kconfig entry for OF_DYNAMIC?
> 
> I guess. The purpose of making it visible was for compile testing, so
> maybe make it 'if COMPILE_TEST' instead?

After thinking a bit, I didn't like my original suggestion.
I'll think some more, but I think that I will not follow up
on my original suggestion.

> 
> 
> Looking at occurrences of CONFIG_OF_DYNAMIC, there's more than I'd
> like. This for example is a common pattern:
> 
> drivers/spi/spi.c:      if (IS_ENABLED(CONFIG_OF_DYNAMIC))
> drivers/spi/spi.c-
> WARN_ON(of_reconfig_notifier_register(&spi_of_notifier));
> 
> Really, of_reconfig_notifier_register() should just return 0 if
> !IS_ENABLED(CONFIG_OF_DYNAMIC).

I'll add researching use of CONFIG_OF_DYNAMIC to my todo list.

-Frank

> 
> Rob
> 


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

end of thread, other threads:[~2021-08-23 22:21 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-20 14:47 [PATCH] of: Don't allow __of_attached_node_sysfs() without CONFIG_SYSFS Marc Zyngier
2021-08-23  4:01 ` Frank Rowand
2021-08-23  8:04   ` Marc Zyngier
2021-08-23 18:46   ` Rob Herring
2021-08-23 21:27     ` Frank Rowand
2021-08-23 22:14       ` Rob Herring
2021-08-23 22:21         ` Frank Rowand
2021-08-23 18:48 ` Rob Herring

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).