LinuxLists.cc - [PATCH] irqdomain: remove debugfs_file from struct irq

[permalink] [raw]

Subject: Re: [PATCH] irqdomain: remove debugfs_file from struct irq_domain

On 2021-02-17 19:57, Michael Walle wrote:
> Hi Greg,
>
>> There's no need to keep around a dentry pointer to a simple file that
>> debugfs itself can look up when we need to remove it from the system.
>> So simplify the code by deleting the variable and cleaning up the
>> logic
>> around the debugfs file.
>
> This will generate the following oops on my board (arm64,
> freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts). In debugfs_lookup()
> debugfs_mount is NULL.

That's odd. I gave it a go yesterday, and nothing blew up.
Which makes me wonder whether I had the debug stuff enabled
the first place...

I've dropped the patch from -next for now until I figure it out
(probably tomorrow).

Thanks,

M.
--
Jazz is not dead. It just smells funny...

2021-02-17 21:23:14

[permalink] [raw]

Subject: Re: [PATCH] irqdomain: remove debugfs_file from struct irq_domain

Hi Greg,

> There's no need to keep around a dentry pointer to a simple file that
> debugfs itself can look up when we need to remove it from the system.
> So simplify the code by deleting the variable and cleaning up the logic
> around the debugfs file.

This will generate the following oops on my board (arm64,
freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts). In debugfs_lookup()
debugfs_mount is NULL.

[ 0.000000] Linux version 5.11.0-next-20210217-00014-g41e0e92a2d05 (mwalle@mwalle01) (aarch64-linux-gnu-gcc (Debian 8.3.0-2) 8.3.0, GNU ld (GNU Binutils for Debian) 2.31.1) #69 SMP PREEMPT Wed Feb 17 20:47:10 CET 2021
[..]
[ 0.000000] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
[ 0.000000] Mem abort info:
[ 0.000000] ESR = 0x96000004
[ 0.000000] EC = 0x25: DABT (current EL), IL = 32 bits
[ 0.000000] SET = 0, FnV = 0
[ 0.000000] EA = 0, S1PTW = 0
[ 0.000000] Data abort info:
[ 0.000000] ISV = 0, ISS = 0x00000004
[ 0.000000] CM = 0, WnR = 0
[ 0.000000] [0000000000000000] user address but active_mm is swapper
[ 0.000000] Internal error: Oops: 96000004 [#1] PREEMPT SMP
[ 0.000000] Modules linked in:
[ 0.000000] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.11.0-next-20210217-00013-g960bc30321c0-dirty #68
[ 0.000000] Hardware name: Kontron SMARC-sAL28 (Single PHY) on SMARC Eval 2.0 carrier (DT)
[ 0.000000] pstate: 60000085 (nZCv daIf -PAN -UAO -TCO BTYPE=--)
[ 0.000000] pc : debugfs_lookup+0x64/0xa0
[ 0.000000] lr : debugfs_lookup+0x5c/0xa0
[ 0.000000] sp : ffff800011933c90
[ 0.000000] x29: ffff800011933c90 x28: 0000000000000000
[ 0.000000] x27: 0000000000000000 x26: 0000000000000001
[ 0.000000] x25: ffff00200016f100 x24: ffff800010f19750
[ 0.000000] x23: ffff80001193d000 x22: ffff800011939948
[ 0.000000] x21: ffff800011bfaf98 x20: ffff00200016f200
[ 0.000000] x19: 0000000000000000 x18: 0000000000000010
[ 0.000000] x17: 0000000000007fff x16: 00000000ffffffff
[ 0.000000] x15: ffffffffffffffff x14: ffff800011939948
[ 0.000000] x13: ffff800091933997 x12: ffff8000119cb670
[ 0.000000] x11: 0000000000000003 x10: ffff8000119b3630
[ 0.000000] x9 : ffff800010102bc4 x8 : 0000000000017fe8
[ 0.000000] x7 : c0000000ffffefff x6 : 0000000000000001
[ 0.000000] x5 : 0000000000000000 x4 : 0000000000000000
[ 0.000000] x3 : 00000000ffffffff x2 : 0000000000000000
[ 0.000000] x1 : 0000000000000000 x0 : 0000000000000000
[ 0.000000] Call trace:
[ 0.000000] debugfs_lookup+0x64/0xa0
[ 0.000000] debugfs_remove_domain_dir+0x24/0x38
[ 0.000000] irq_domain_update_bus_token+0x6c/0xb8
[ 0.000000] gic_init_bases+0x19c/0x64c
[ 0.000000] gic_of_init+0x188/0x228
[ 0.000000] of_irq_init+0x1a8/0x350
[ 0.000000] irqchip_init+0x20/0x48
[ 0.000000] init_IRQ+0xd4/0x178
[ 0.000000] start_kernel+0x628/0x85c
[ 0.000000] 0x0
[ 0.000000] Code: 9128a000 9426cd08 b5000073 f94006a0 (f9400013)
[ 0.000000] random: get_random_bytes called from print_oops_end_marker+0x34/0x60 with crng_init=0
[ 0.000000] ---[ end trace 0000000000000000 ]---
[ 0.000000] Kernel panic - not syncing: Attempted to kill the idle task!
[ 0.000000] ---[ end Kernel panic - not syncing: Attempted to kill the idle task! ]---

If I revert this commit, the oops will go away.

-michael

2021-02-17 21:25:46

[permalink] [raw]

Subject: Re: [PATCH] irqdomain: remove debugfs_file from struct irq_domain

Am 2021-02-17 21:02, schrieb Marc Zyngier:
> On 2021-02-17 19:57, Michael Walle wrote:
>> Hi Greg,
>>
>>> There's no need to keep around a dentry pointer to a simple file that
>>> debugfs itself can look up when we need to remove it from the system.
>>> So simplify the code by deleting the variable and cleaning up the
>>> logic
>>> around the debugfs file.
>>
>> This will generate the following oops on my board (arm64,
>> freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts). In debugfs_lookup()
>> debugfs_mount is NULL.
>
> That's odd. I gave it a go yesterday, and nothing blew up.
> Which makes me wonder whether I had the debug stuff enabled
> the first place...
>
> I've dropped the patch from -next for now until I figure it out
> (probably tomorrow).

Mh, maybe its my .config, I've attached it. I also noticed that
the board boots just fine in our kernel-ci [1].

[1] https://lavalab.kontron.com/scheduler/job/8633

-michael

Attachments:

.config (170.54 kB)

2021-02-17 21:53:42

[permalink] [raw]

Subject: Re: [PATCH] irqdomain: remove debugfs_file from struct irq_domain

On Wed, 17 Feb 2021 20:10:50 +0000,
Michael Walle <[email protected]> wrote:
>
> Am 2021-02-17 21:02, schrieb Marc Zyngier:
> > On 2021-02-17 19:57, Michael Walle wrote:
> >> Hi Greg,
> >>
> >>> There's no need to keep around a dentry pointer to a simple file that
> >>> debugfs itself can look up when we need to remove it from the system.
> >>> So simplify the code by deleting the variable and cleaning up the
> >>> logic
> >>> around the debugfs file.
> >>
> >> This will generate the following oops on my board (arm64,
> >> freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts). In debugfs_lookup()
> >> debugfs_mount is NULL.
> >
> > That's odd. I gave it a go yesterday, and nothing blew up.
> > Which makes me wonder whether I had the debug stuff enabled
> > the first place...
> >
> > I've dropped the patch from -next for now until I figure it out
> > (probably tomorrow).
>
> Mh, maybe its my .config, I've attached it. I also noticed that
> the board boots just fine in our kernel-ci [1].

I reproduced here. I had disabled GENERIC_IRQ_DEBUGFS for obscure
reasons, and it caught fire as I re-enabled it.

Adding this fixes it for me:

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 367ff1c35f75..d8a14cf1a7b6 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -1904,7 +1904,8 @@ static void debugfs_add_domain_dir(struct irq_domain *d)

static void debugfs_remove_domain_dir(struct irq_domain *d)
{
- debugfs_remove(debugfs_lookup(d->name, domain_dir));
+ if (domain_dir)
+ debugfs_remove(debugfs_lookup(d->name, domain_dir));
}

void __init irq_domain_debugfs_init(struct dentry *root)

Could you please check whether it works for you?

Thanks,

M.

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

2021-02-17 22:38:03

[permalink] [raw]

Subject: Re: [PATCH] irqdomain: remove debugfs_file from struct irq_domain

Am 2021-02-17 22:50, schrieb Marc Zyngier:
> On Wed, 17 Feb 2021 20:10:50 +0000,
> Michael Walle <[email protected]> wrote:
>>
>> Am 2021-02-17 21:02, schrieb Marc Zyngier:
>> > On 2021-02-17 19:57, Michael Walle wrote:
>> >> Hi Greg,
>> >>
>> >>> There's no need to keep around a dentry pointer to a simple file that
>> >>> debugfs itself can look up when we need to remove it from the system.
>> >>> So simplify the code by deleting the variable and cleaning up the
>> >>> logic
>> >>> around the debugfs file.
>> >>
>> >> This will generate the following oops on my board (arm64,
>> >> freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts). In debugfs_lookup()
>> >> debugfs_mount is NULL.
>> >
>> > That's odd. I gave it a go yesterday, and nothing blew up.
>> > Which makes me wonder whether I had the debug stuff enabled
>> > the first place...
>> >
>> > I've dropped the patch from -next for now until I figure it out
>> > (probably tomorrow).
>>
>> Mh, maybe its my .config, I've attached it. I also noticed that
>> the board boots just fine in our kernel-ci [1].
>
> I reproduced here. I had disabled GENERIC_IRQ_DEBUGFS for obscure
> reasons, and it caught fire as I re-enabled it.
>
> Adding this fixes it for me:
>
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 367ff1c35f75..d8a14cf1a7b6 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -1904,7 +1904,8 @@ static void debugfs_add_domain_dir(struct
> irq_domain *d)
>
> static void debugfs_remove_domain_dir(struct irq_domain *d)
> {
> - debugfs_remove(debugfs_lookup(d->name, domain_dir));
> + if (domain_dir)
> + debugfs_remove(debugfs_lookup(d->name, domain_dir));
> }
>
> void __init irq_domain_debugfs_init(struct dentry *root)
>
>
> Could you please check whether it works for you?

Works for me, too. Thanks.

-michael

2021-02-18 08:15:08

[permalink] [raw]

Subject: Re: [PATCH] irqdomain: remove debugfs_file from struct irq_domain

On Wed, Feb 17, 2021 at 11:21:22PM +0100, Michael Walle wrote:
> Am 2021-02-17 22:50, schrieb Marc Zyngier:
> > On Wed, 17 Feb 2021 20:10:50 +0000,
> > Michael Walle <[email protected]> wrote:
> > >
> > > Am 2021-02-17 21:02, schrieb Marc Zyngier:
> > > > On 2021-02-17 19:57, Michael Walle wrote:
> > > >> Hi Greg,
> > > >>
> > > >>> There's no need to keep around a dentry pointer to a simple file that
> > > >>> debugfs itself can look up when we need to remove it from the system.
> > > >>> So simplify the code by deleting the variable and cleaning up the
> > > >>> logic
> > > >>> around the debugfs file.
> > > >>
> > > >> This will generate the following oops on my board (arm64,
> > > >> freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts). In debugfs_lookup()
> > > >> debugfs_mount is NULL.
> > > >
> > > > That's odd. I gave it a go yesterday, and nothing blew up.
> > > > Which makes me wonder whether I had the debug stuff enabled
> > > > the first place...
> > > >
> > > > I've dropped the patch from -next for now until I figure it out
> > > > (probably tomorrow).
> > >
> > > Mh, maybe its my .config, I've attached it. I also noticed that
> > > the board boots just fine in our kernel-ci [1].
> >
> > I reproduced here. I had disabled GENERIC_IRQ_DEBUGFS for obscure
> > reasons, and it caught fire as I re-enabled it.
> >
> > Adding this fixes it for me:
> >
> > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> > index 367ff1c35f75..d8a14cf1a7b6 100644
> > --- a/kernel/irq/irqdomain.c
> > +++ b/kernel/irq/irqdomain.c
> > @@ -1904,7 +1904,8 @@ static void debugfs_add_domain_dir(struct
> > irq_domain *d)
> >
> > static void debugfs_remove_domain_dir(struct irq_domain *d)
> > {
> > - debugfs_remove(debugfs_lookup(d->name, domain_dir));
> > + if (domain_dir)
> > + debugfs_remove(debugfs_lookup(d->name, domain_dir));
> > }
> >
> > void __init irq_domain_debugfs_init(struct dentry *root)
> >
> >
> > Could you please check whether it works for you?
>
> Works for me, too. Thanks.

Hm, odd, that shouldn't matter, the debugfs core should be able to
easily handle stuff like this, let me look into that before you apply
this...

thnaks,

greg k-h

2021-02-18 08:40:38

[permalink] [raw]

Subject: Re: [PATCH] irqdomain: remove debugfs_file from struct irq_domain

On Wed, Feb 17, 2021 at 09:50:38PM +0000, Marc Zyngier wrote:
> On Wed, 17 Feb 2021 20:10:50 +0000,
> Michael Walle <[email protected]> wrote:
> >
> > Am 2021-02-17 21:02, schrieb Marc Zyngier:
> > > On 2021-02-17 19:57, Michael Walle wrote:
> > >> Hi Greg,
> > >>
> > >>> There's no need to keep around a dentry pointer to a simple file that
> > >>> debugfs itself can look up when we need to remove it from the system.
> > >>> So simplify the code by deleting the variable and cleaning up the
> > >>> logic
> > >>> around the debugfs file.
> > >>
> > >> This will generate the following oops on my board (arm64,
> > >> freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts). In debugfs_lookup()
> > >> debugfs_mount is NULL.
> > >
> > > That's odd. I gave it a go yesterday, and nothing blew up.
> > > Which makes me wonder whether I had the debug stuff enabled
> > > the first place...
> > >
> > > I've dropped the patch from -next for now until I figure it out
> > > (probably tomorrow).
> >
> > Mh, maybe its my .config, I've attached it. I also noticed that
> > the board boots just fine in our kernel-ci [1].
>
> I reproduced here. I had disabled GENERIC_IRQ_DEBUGFS for obscure
> reasons, and it caught fire as I re-enabled it.
>
> Adding this fixes it for me:
>
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 367ff1c35f75..d8a14cf1a7b6 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -1904,7 +1904,8 @@ static void debugfs_add_domain_dir(struct irq_domain *d)
>
> static void debugfs_remove_domain_dir(struct irq_domain *d)
> {
> - debugfs_remove(debugfs_lookup(d->name, domain_dir));
> + if (domain_dir)
> + debugfs_remove(debugfs_lookup(d->name, domain_dir));
> }
>
> void __init irq_domain_debugfs_init(struct dentry *root)
>
>
> Could you please check whether it works for you?

Can you try this debugfs core change instead? Callers to debugfs should
not have to do the above type of checking as debugfs should be much more
robust than that.

thanks,

greg k-h

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 2fcf66473436..5aa798f52b6e 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -297,7 +297,7 @@ struct dentry *debugfs_lookup(const char *name, struct dentry *parent)
{
struct dentry *dentry;

- if (IS_ERR(parent))
+ if (IS_ERR_OR_NULL(name) || IS_ERR(parent))
return NULL;

if (!parent)

2021-02-18 10:01:10

[permalink] [raw]

Subject: Re: [PATCH] irqdomain: remove debugfs_file from struct irq_domain

Am 2021-02-18 08:31, schrieb Greg KH:
> On Wed, Feb 17, 2021 at 09:50:38PM +0000, Marc Zyngier wrote:
>> On Wed, 17 Feb 2021 20:10:50 +0000,
>> Michael Walle <[email protected]> wrote:
>> >
>> > Am 2021-02-17 21:02, schrieb Marc Zyngier:
>> > > On 2021-02-17 19:57, Michael Walle wrote:
>> > >> Hi Greg,
>> > >>
>> > >>> There's no need to keep around a dentry pointer to a simple file that
>> > >>> debugfs itself can look up when we need to remove it from the system.
>> > >>> So simplify the code by deleting the variable and cleaning up the
>> > >>> logic
>> > >>> around the debugfs file.
>> > >>
>> > >> This will generate the following oops on my board (arm64,
>> > >> freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts). In debugfs_lookup()
>> > >> debugfs_mount is NULL.
>> > >
>> > > That's odd. I gave it a go yesterday, and nothing blew up.
>> > > Which makes me wonder whether I had the debug stuff enabled
>> > > the first place...
>> > >
>> > > I've dropped the patch from -next for now until I figure it out
>> > > (probably tomorrow).
>> >
>> > Mh, maybe its my .config, I've attached it. I also noticed that
>> > the board boots just fine in our kernel-ci [1].
>>
>> I reproduced here. I had disabled GENERIC_IRQ_DEBUGFS for obscure
>> reasons, and it caught fire as I re-enabled it.
>>
>> Adding this fixes it for me:
>>
>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> index 367ff1c35f75..d8a14cf1a7b6 100644
>> --- a/kernel/irq/irqdomain.c
>> +++ b/kernel/irq/irqdomain.c
>> @@ -1904,7 +1904,8 @@ static void debugfs_add_domain_dir(struct
>> irq_domain *d)
>>
>> static void debugfs_remove_domain_dir(struct irq_domain *d)
>> {
>> - debugfs_remove(debugfs_lookup(d->name, domain_dir));
>> + if (domain_dir)
>> + debugfs_remove(debugfs_lookup(d->name, domain_dir));
>> }
>>
>> void __init irq_domain_debugfs_init(struct dentry *root)
>>
>>
>> Could you please check whether it works for you?
>
> Can you try this debugfs core change instead? Callers to debugfs
> should
> not have to do the above type of checking as debugfs should be much
> more
> robust than that.
>
> thanks,
>
> greg k-h
>
>
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 2fcf66473436..5aa798f52b6e 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -297,7 +297,7 @@ struct dentry *debugfs_lookup(const char *name,
> struct dentry *parent)
> {
> struct dentry *dentry;
>
> - if (IS_ERR(parent))
> + if (IS_ERR_OR_NULL(name) || IS_ERR(parent))
> return NULL;
>
> if (!parent)

This doesn't work. name is not NULL when it is called.

What has to happen before debugfs_lookup() can be called? Looks like
someone has to initialize the static debugfs_mount, first.

-michael

2021-02-18 10:01:19

[permalink] [raw]

Subject: Re: [PATCH] irqdomain: remove debugfs_file from struct irq_domain

Am 2021-02-18 09:40, schrieb Greg KH:
> On Wed, Feb 17, 2021 at 08:57:17PM +0100, Michael Walle wrote:
>> Hi Greg,
>>
>> > There's no need to keep around a dentry pointer to a simple file that
>> > debugfs itself can look up when we need to remove it from the system.
>> > So simplify the code by deleting the variable and cleaning up the logic
>> > around the debugfs file.
>>
>> This will generate the following oops on my board (arm64,
>> freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts). In debugfs_lookup()
>> debugfs_mount is NULL.
>>
>> [ 0.000000] Linux version 5.11.0-next-20210217-00014-g41e0e92a2d05
>> (mwalle@mwalle01) (aarch64-linux-gnu-gcc (Debian 8.3.0-2) 8.3.0, GNU
>> ld (GNU Binutils for Debian) 2.31.1) #69 SMP PREEMPT Wed Feb 17
>> 20:47:10 CET 2021
>> [..]
>> [ 0.000000] Unable to handle kernel NULL pointer dereference at
>> virtual address 0000000000000000
>> [ 0.000000] Mem abort info:
>> [ 0.000000] ESR = 0x96000004
>
> <snip>
>
> Back to this log, do you see:
> "Unable to pin filesystem for file "
> anywhere before this crash?

No.

Here is the complete log:

[ 0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd083]
[ 0.000000] Linux version
5.11.0-next-20210217-00013-g960bc30321c0-dirty (mwalle@mwalle01)
(aarch64-linux-gnu-gcc (Debian 8.3.0-2) 8.3.0, GNU ld (GNU Binutils for
Debian) 2.31.1) #73 SMP PREEMPT Thu Feb 18 09:19:21 CET 2021
[ 0.000000] Machine model: Kontron SMARC-sAL28 (Single PHY) on SMARC
Eval 2.0 carrier
[ 0.000000] efi: UEFI not found.
[ 0.000000] earlycon: ns16550a0 at MMIO 0x00000000021c0500 (options
'115200n8')
[ 0.000000] printk: bootconsole [ns16550a0] enabled
[ 0.000000] NUMA: No NUMA configuration found
[ 0.000000] NUMA: Faking a node at [mem
0x0000000080000000-0x00000020ffffffff]
[ 0.000000] NUMA: NODE_DATA [mem 0x20ff7d9200-0x20ff7dafff]
[ 0.000000] Zone ranges:
[ 0.000000] DMA [mem 0x0000000000000000-0x00000000ffffffff]
[ 0.000000] DMA32 empty
[ 0.000000] Normal [mem 0x0000000100000000-0x00000020ffffffff]
[ 0.000000] Movable zone start for each node
[ 0.000000] Early memory node ranges
[ 0.000000] node 0: [mem 0x0000000080000000-0x00000000ffffffff]
[ 0.000000] node 0: [mem 0x0000002080000000-0x00000020ffffffff]
[ 0.000000] Initmem setup node 0 [mem
0x0000000080000000-0x00000020ffffffff]
[ 0.000000] On node 0 totalpages: 1048576
[ 0.000000] DMA zone: 8192 pages used for memmap
[ 0.000000] DMA zone: 0 pages reserved
[ 0.000000] DMA zone: 524288 pages, LIFO batch:63
[ 0.000000] Normal zone: 8192 pages used for memmap
[ 0.000000] Normal zone: 524288 pages, LIFO batch:63
[ 0.000000] cma: Reserved 32 MiB at 0x00000000fcc00000
[ 0.000000] percpu: Embedded 31 pages/cpu s89752 r8192 d29032 u126976
[ 0.000000] pcpu-alloc: s89752 r8192 d29032 u126976 alloc=31*4096
[ 0.000000] pcpu-alloc: [0] 0 [0] 1
[ 0.000000] Detected PIPT I-cache on CPU0
[ 0.000000] CPU features: detected: GIC system register CPU interface
[ 0.000000] CPU features: detected: Spectre-v3a
[ 0.000000] CPU features: detected: Spectre-v2
[ 0.000000] CPU features: detected: Spectre-v4
[ 0.000000] CPU features: detected: ARM errata 1165522, 1319367, or
1530923
[ 0.000000] Built 1 zonelists, mobility grouping on. Total pages:
1032192
[ 0.000000] Policy zone: Normal
[ 0.000000] Kernel command line: debug root=/dev/mmcblk0p2 rootwait
earlycon
[ 0.000000] Dentry cache hash table entries: 524288 (order: 10,
4194304 bytes, linear)
[ 0.000000] Inode-cache hash table entries: 262144 (order: 9, 2097152
bytes, linear)
[ 0.000000] mem auto-init: stack:off, heap alloc:off, heap free:off
[ 0.000000] software IO TLB: mapped [mem
0x00000000f8c00000-0x00000000fcc00000] (64MB)
[ 0.000000] Memory: 3986440K/4194304K available (14912K kernel code,
2164K rwdata, 5876K rodata, 4864K init, 849K bss, 175096K reserved,
32768K cma-reserved)
[ 0.000000] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=2,
Nodes=1
[ 0.000000] ftrace: allocating 52190 entries in 204 pages
[ 0.000000] ftrace: allocated 204 pages with 4 groups
[ 0.000000] rcu: Preemptible hierarchical RCU implementation.
[ 0.000000] rcu: RCU restricting CPUs from NR_CPUS=256 to
nr_cpu_ids=2.
[ 0.000000] Trampoline variant of Tasks RCU enabled.
[ 0.000000] Rude variant of Tasks RCU enabled.
[ 0.000000] rcu: RCU calculated value of scheduler-enlistment delay
is 25 jiffies.
[ 0.000000] rcu: Adjusting geometry for rcu_fanout_leaf=16,
nr_cpu_ids=2
[ 0.000000] NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0
[ 0.000000] GICv3: GIC: Using split EOI/Deactivate mode
[ 0.000000] GICv3: 256 SPIs implemented
[ 0.000000] GICv3: 0 Extended SPIs implemented
[ 0.000000] Unable to handle kernel NULL pointer dereference at
virtual address 0000000000000000
[ 0.000000] Mem abort info:
[ 0.000000] ESR = 0x96000004
[ 0.000000] EC = 0x25: DABT (current EL), IL = 32 bits
[ 0.000000] SET = 0, FnV = 0
[ 0.000000] EA = 0, S1PTW = 0
[ 0.000000] Data abort info:
[ 0.000000] ISV = 0, ISS = 0x00000004
[ 0.000000] CM = 0, WnR = 0
[ 0.000000] [0000000000000000] user address but active_mm is swapper
[ 0.000000] Internal error: Oops: 96000004 [#1] PREEMPT SMP
[ 0.000000] Modules linked in:
[ 0.000000] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
5.11.0-next-20210217-00013-g960bc30321c0-dirty #73
[ 0.000000] Hardware name: Kontron SMARC-sAL28 (Single PHY) on SMARC
Eval 2.0 carrier (DT)
[ 0.000000] pstate: 00000085 (nzcv daIf -PAN -UAO -TCO BTYPE=--)
[ 0.000000] pc : debugfs_lookup+0x5c/0x94
[ 0.000000] lr : debugfs_lookup+0x3c/0x94
[ 0.000000] sp : ffff800011933ca0
[ 0.000000] x29: ffff800011933ca0 x28: 0000000000000000
[ 0.000000] x27: 0000000000000000 x26: 0000000000000001
[ 0.000000] x25: ffff00200016f100 x24: ffff800010f19750
[ 0.000000] x23: ffff80001193d000 x22: ffff800011939948
[ 0.000000] x21: ffff8000119cbcc8 x20: 0000000000000000
[ 0.000000] x19: ffff00200016f200 x18: 0000000000000010
[ 0.000000] x17: 0000000000007fff x16: 00000000ffffffff
[ 0.000000] x15: ffffffffffffffff x14: ffff800011939948
[ 0.000000] x13: ffff8000919339a7 x12: ffff8000119cb670
[ 0.000000] x11: 0000000000000003 x10: ffff8000119b3630
[ 0.000000] x9 : ffff800010102bc4 x8 : 0000000000017fe8
[ 0.000000] x7 : c0000000ffffefff x6 : 0000000000000001
[ 0.000000] x5 : 0000000000000000 x4 : 0000000000000000
[ 0.000000] x3 : 00000000ffffffff x2 : 0000000000000000
[ 0.000000] x1 : 0000000000000000 x0 : 0000000000000000
[ 0.000000] Call trace:
[ 0.000000] debugfs_lookup+0x5c/0x94
[ 0.000000] debugfs_remove_domain_dir+0x24/0x38
[ 0.000000] irq_domain_update_bus_token+0x6c/0xb8
[ 0.000000] gic_init_bases+0x19c/0x64c
[ 0.000000] gic_of_init+0x188/0x228
[ 0.000000] of_irq_init+0x1a8/0x350
[ 0.000000] irqchip_init+0x20/0x48
[ 0.000000] init_IRQ+0xd4/0x178
[ 0.000000] start_kernel+0x628/0x85c
[ 0.000000] 0x0
[ 0.000000] Code: 540001a8 b5000094 b0006ca0 f947d000 (f9400014)
[ 0.000000] random: get_random_bytes called from
print_oops_end_marker+0x34/0x60 with crng_init=0
[ 0.000000] ---[ end trace 0000000000000000 ]---
[ 0.000000] Kernel panic - not syncing: Attempted to kill the idle
task!
[ 0.000000] ---[ end Kernel panic - not syncing: Attempted to kill
the idle task! ]---

-michael

2021-02-18 10:01:47

[permalink] [raw]

Subject: Re: [PATCH] irqdomain: remove debugfs_file from struct irq_domain

On Thu, Feb 18, 2021 at 09:27:09AM +0100, Michael Walle wrote:
> Am 2021-02-18 08:31, schrieb Greg KH:
> > On Wed, Feb 17, 2021 at 09:50:38PM +0000, Marc Zyngier wrote:
> > > On Wed, 17 Feb 2021 20:10:50 +0000,
> > > Michael Walle <[email protected]> wrote:
> > > >
> > > > Am 2021-02-17 21:02, schrieb Marc Zyngier:
> > > > > On 2021-02-17 19:57, Michael Walle wrote:
> > > > >> Hi Greg,
> > > > >>
> > > > >>> There's no need to keep around a dentry pointer to a simple file that
> > > > >>> debugfs itself can look up when we need to remove it from the system.
> > > > >>> So simplify the code by deleting the variable and cleaning up the
> > > > >>> logic
> > > > >>> around the debugfs file.
> > > > >>
> > > > >> This will generate the following oops on my board (arm64,
> > > > >> freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts). In debugfs_lookup()
> > > > >> debugfs_mount is NULL.
> > > > >
> > > > > That's odd. I gave it a go yesterday, and nothing blew up.
> > > > > Which makes me wonder whether I had the debug stuff enabled
> > > > > the first place...
> > > > >
> > > > > I've dropped the patch from -next for now until I figure it out
> > > > > (probably tomorrow).
> > > >
> > > > Mh, maybe its my .config, I've attached it. I also noticed that
> > > > the board boots just fine in our kernel-ci [1].
> > >
> > > I reproduced here. I had disabled GENERIC_IRQ_DEBUGFS for obscure
> > > reasons, and it caught fire as I re-enabled it.
> > >
> > > Adding this fixes it for me:
> > >
> > > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> > > index 367ff1c35f75..d8a14cf1a7b6 100644
> > > --- a/kernel/irq/irqdomain.c
> > > +++ b/kernel/irq/irqdomain.c
> > > @@ -1904,7 +1904,8 @@ static void debugfs_add_domain_dir(struct
> > > irq_domain *d)
> > >
> > > static void debugfs_remove_domain_dir(struct irq_domain *d)
> > > {
> > > - debugfs_remove(debugfs_lookup(d->name, domain_dir));
> > > + if (domain_dir)
> > > + debugfs_remove(debugfs_lookup(d->name, domain_dir));
> > > }
> > >
> > > void __init irq_domain_debugfs_init(struct dentry *root)
> > >
> > >
> > > Could you please check whether it works for you?
> >
> > Can you try this debugfs core change instead? Callers to debugfs should
> > not have to do the above type of checking as debugfs should be much more
> > robust than that.
> >
> > thanks,
> >
> > greg k-h
> >
> >
> > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> > index 2fcf66473436..5aa798f52b6e 100644
> > --- a/fs/debugfs/inode.c
> > +++ b/fs/debugfs/inode.c
> > @@ -297,7 +297,7 @@ struct dentry *debugfs_lookup(const char *name,
> > struct dentry *parent)
> > {
> > struct dentry *dentry;
> >
> > - if (IS_ERR(parent))
> > + if (IS_ERR_OR_NULL(name) || IS_ERR(parent))
> > return NULL;
> >
> > if (!parent)
>
> This doesn't work. name is not NULL when it is called.
>
> What has to happen before debugfs_lookup() can be called? Looks like
> someone has to initialize the static debugfs_mount, first.

Ok, how about this:

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 2fcf66473436..86c7f0489620 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -297,7 +297,7 @@ struct dentry *debugfs_lookup(const char *name, struct dentry *parent)
{
struct dentry *dentry;

- if (IS_ERR(parent))
+ if (!debugfs_initialized() || IS_ERR_OR_NULL(name) || IS_ERR(parent))
return NULL;

if (!parent)
@@ -318,6 +318,9 @@ static struct dentry *start_creating(const char *name, struct dentry *parent)
if (!(debugfs_allow & DEBUGFS_ALLOW_API))
return ERR_PTR(-EPERM);

+ if (!debugfs_initialized())
+ return ERR_PTR(-ENOENT);
+
pr_debug("creating file '%s'\n", name);

if (IS_ERR(parent))

2021-02-18 10:01:58

[permalink] [raw]

Subject: Re: [PATCH] irqdomain: remove debugfs_file from struct irq_domain

On Thu, Feb 18, 2021 at 08:47:15AM +0000, Marc Zyngier wrote:
> On 2021-02-18 08:38, Greg KH wrote:
> > On Thu, Feb 18, 2021 at 09:27:09AM +0100, Michael Walle wrote:
> > > Am 2021-02-18 08:31, schrieb Greg KH:
> > > > On Wed, Feb 17, 2021 at 09:50:38PM +0000, Marc Zyngier wrote:
> > > > > On Wed, 17 Feb 2021 20:10:50 +0000,
> > > > > Michael Walle <[email protected]> wrote:
> > > > > >
> > > > > > Am 2021-02-17 21:02, schrieb Marc Zyngier:
> > > > > > > On 2021-02-17 19:57, Michael Walle wrote:
> > > > > > >> Hi Greg,
> > > > > > >>
> > > > > > >>> There's no need to keep around a dentry pointer to a simple file that
> > > > > > >>> debugfs itself can look up when we need to remove it from the system.
> > > > > > >>> So simplify the code by deleting the variable and cleaning up the
> > > > > > >>> logic
> > > > > > >>> around the debugfs file.
> > > > > > >>
> > > > > > >> This will generate the following oops on my board (arm64,
> > > > > > >> freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts). In debugfs_lookup()
> > > > > > >> debugfs_mount is NULL.
> > > > > > >
> > > > > > > That's odd. I gave it a go yesterday, and nothing blew up.
> > > > > > > Which makes me wonder whether I had the debug stuff enabled
> > > > > > > the first place...
> > > > > > >
> > > > > > > I've dropped the patch from -next for now until I figure it out
> > > > > > > (probably tomorrow).
> > > > > >
> > > > > > Mh, maybe its my .config, I've attached it. I also noticed that
> > > > > > the board boots just fine in our kernel-ci [1].
> > > > >
> > > > > I reproduced here. I had disabled GENERIC_IRQ_DEBUGFS for obscure
> > > > > reasons, and it caught fire as I re-enabled it.
> > > > >
> > > > > Adding this fixes it for me:
> > > > >
> > > > > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> > > > > index 367ff1c35f75..d8a14cf1a7b6 100644
> > > > > --- a/kernel/irq/irqdomain.c
> > > > > +++ b/kernel/irq/irqdomain.c
> > > > > @@ -1904,7 +1904,8 @@ static void debugfs_add_domain_dir(struct
> > > > > irq_domain *d)
> > > > >
> > > > > static void debugfs_remove_domain_dir(struct irq_domain *d)
> > > > > {
> > > > > - debugfs_remove(debugfs_lookup(d->name, domain_dir));
> > > > > + if (domain_dir)
> > > > > + debugfs_remove(debugfs_lookup(d->name, domain_dir));
> > > > > }
> > > > >
> > > > > void __init irq_domain_debugfs_init(struct dentry *root)
> > > > >
> > > > >
> > > > > Could you please check whether it works for you?
> > > >
> > > > Can you try this debugfs core change instead? Callers to debugfs should
> > > > not have to do the above type of checking as debugfs should be much more
> > > > robust than that.
> > > >
> > > > thanks,
> > > >
> > > > greg k-h
> > > >
> > > >
> > > > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> > > > index 2fcf66473436..5aa798f52b6e 100644
> > > > --- a/fs/debugfs/inode.c
> > > > +++ b/fs/debugfs/inode.c
> > > > @@ -297,7 +297,7 @@ struct dentry *debugfs_lookup(const char *name,
> > > > struct dentry *parent)
> > > > {
> > > > struct dentry *dentry;
> > > >
> > > > - if (IS_ERR(parent))
> > > > + if (IS_ERR_OR_NULL(name) || IS_ERR(parent))
> > > > return NULL;
> > > >
> > > > if (!parent)
> > >
> > > This doesn't work. name is not NULL when it is called.
> >
> > Ok, but it is a good check we need to make anyway, I'll add it to a
> > patch series :)
> >
> > > What has to happen before debugfs_lookup() can be called? Looks like
> > > someone has to initialize the static debugfs_mount, first.
> >
> > Wow, wait, you are removing a debugfs file _before_ debugfs is even
> > initialized? Didn't expect that, ok, let me go try this again...
>
> Yeah, that's a poor man's rename (file being deleted and re-created).

True, but that's not happening here, right? Some driver is being
initialized and creates a debugfs file, and then decides to unload so it
removes the debugfs file?

Why was it trying to create the file in the first place if it didn't
properly bind to the hardware?

odd...

greg k-h

2021-02-18 10:03:05

[permalink] [raw]

Subject: Re: [PATCH] irqdomain: remove debugfs_file from struct irq_domain

On 2021-02-18 08:38, Greg KH wrote:
> On Thu, Feb 18, 2021 at 09:27:09AM +0100, Michael Walle wrote:
>> Am 2021-02-18 08:31, schrieb Greg KH:
>> > On Wed, Feb 17, 2021 at 09:50:38PM +0000, Marc Zyngier wrote:
>> > > On Wed, 17 Feb 2021 20:10:50 +0000,
>> > > Michael Walle <[email protected]> wrote:
>> > > >
>> > > > Am 2021-02-17 21:02, schrieb Marc Zyngier:
>> > > > > On 2021-02-17 19:57, Michael Walle wrote:
>> > > > >> Hi Greg,
>> > > > >>
>> > > > >>> There's no need to keep around a dentry pointer to a simple file that
>> > > > >>> debugfs itself can look up when we need to remove it from the system.
>> > > > >>> So simplify the code by deleting the variable and cleaning up the
>> > > > >>> logic
>> > > > >>> around the debugfs file.
>> > > > >>
>> > > > >> This will generate the following oops on my board (arm64,
>> > > > >> freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts). In debugfs_lookup()
>> > > > >> debugfs_mount is NULL.
>> > > > >
>> > > > > That's odd. I gave it a go yesterday, and nothing blew up.
>> > > > > Which makes me wonder whether I had the debug stuff enabled
>> > > > > the first place...
>> > > > >
>> > > > > I've dropped the patch from -next for now until I figure it out
>> > > > > (probably tomorrow).
>> > > >
>> > > > Mh, maybe its my .config, I've attached it. I also noticed that
>> > > > the board boots just fine in our kernel-ci [1].
>> > >
>> > > I reproduced here. I had disabled GENERIC_IRQ_DEBUGFS for obscure
>> > > reasons, and it caught fire as I re-enabled it.
>> > >
>> > > Adding this fixes it for me:
>> > >
>> > > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> > > index 367ff1c35f75..d8a14cf1a7b6 100644
>> > > --- a/kernel/irq/irqdomain.c
>> > > +++ b/kernel/irq/irqdomain.c
>> > > @@ -1904,7 +1904,8 @@ static void debugfs_add_domain_dir(struct
>> > > irq_domain *d)
>> > >
>> > > static void debugfs_remove_domain_dir(struct irq_domain *d)
>> > > {
>> > > - debugfs_remove(debugfs_lookup(d->name, domain_dir));
>> > > + if (domain_dir)
>> > > + debugfs_remove(debugfs_lookup(d->name, domain_dir));
>> > > }
>> > >
>> > > void __init irq_domain_debugfs_init(struct dentry *root)
>> > >
>> > >
>> > > Could you please check whether it works for you?
>> >
>> > Can you try this debugfs core change instead? Callers to debugfs should
>> > not have to do the above type of checking as debugfs should be much more
>> > robust than that.
>> >
>> > thanks,
>> >
>> > greg k-h
>> >
>> >
>> > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
>> > index 2fcf66473436..5aa798f52b6e 100644
>> > --- a/fs/debugfs/inode.c
>> > +++ b/fs/debugfs/inode.c
>> > @@ -297,7 +297,7 @@ struct dentry *debugfs_lookup(const char *name,
>> > struct dentry *parent)
>> > {
>> > struct dentry *dentry;
>> >
>> > - if (IS_ERR(parent))
>> > + if (IS_ERR_OR_NULL(name) || IS_ERR(parent))
>> > return NULL;
>> >
>> > if (!parent)
>>
>> This doesn't work. name is not NULL when it is called.
>
> Ok, but it is a good check we need to make anyway, I'll add it to a
> patch series :)
>
>> What has to happen before debugfs_lookup() can be called? Looks like
>> someone has to initialize the static debugfs_mount, first.
>
> Wow, wait, you are removing a debugfs file _before_ debugfs is even
> initialized? Didn't expect that, ok, let me go try this again...

Yeah, that's a poor man's rename (file being deleted and re-created).

M.
--
Jazz is not dead. It just smells funny...

2021-02-18 10:14:22

[permalink] [raw]

Subject: Re: [PATCH] irqdomain: remove debugfs_file from struct irq_domain

On Thu, 18 Feb 2021 08:54:00 +0000,
Greg KH <[email protected]> wrote:

[...]

> > > Wow, wait, you are removing a debugfs file _before_ debugfs is even
> > > initialized? Didn't expect that, ok, let me go try this again...
> >
> > Yeah, that's a poor man's rename (file being deleted and re-created).
>
> True, but that's not happening here, right? Some driver is being
> initialized and creates a debugfs file, and then decides to unload so it
> removes the debugfs file?

No, that's not what is happening.

The irqchip driver starts, creates an irqdomain. File gets created, at
least in theory (it fails because debugfs isn't ready, but that's not
the issue).

It then changes an attribute to the domain (the so-called bus_token),
which gets reflected in the domain name to avoid aliasing.
Delete/create follows.

> Why was it trying to create the file in the first place if it didn't
> properly bind to the hardware?

See above. We encode properties of the domain in the filename, and
reflect the change of these properties as they happen.

M.

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

2021-02-18 10:19:48

[permalink] [raw]

Subject: Re: [PATCH] irqdomain: remove debugfs_file from struct irq_domain

On Thu, 18 Feb 2021 08:52:53 +0000,
Greg KH <[email protected]> wrote:

[...]

> Ok, how about this:
>
>
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 2fcf66473436..86c7f0489620 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -297,7 +297,7 @@ struct dentry *debugfs_lookup(const char *name, struct dentry *parent)
> {
> struct dentry *dentry;
>
> - if (IS_ERR(parent))
> + if (!debugfs_initialized() || IS_ERR_OR_NULL(name) || IS_ERR(parent))
> return NULL;
>
> if (!parent)
> @@ -318,6 +318,9 @@ static struct dentry *start_creating(const char *name, struct dentry *parent)
> if (!(debugfs_allow & DEBUGFS_ALLOW_API))
> return ERR_PTR(-EPERM);
>
> + if (!debugfs_initialized())
> + return ERR_PTR(-ENOENT);
> +
> pr_debug("creating file '%s'\n", name);
>
> if (IS_ERR(parent))
>

That one boots correctly in a guest.

Thanks,

M.

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

2021-02-18 10:23:45

[permalink] [raw]

Subject: Re: [PATCH] irqdomain: remove debugfs_file from struct irq_domain

On Wed, Feb 17, 2021 at 08:57:17PM +0100, Michael Walle wrote:
> Hi Greg,
>
> > There's no need to keep around a dentry pointer to a simple file that
> > debugfs itself can look up when we need to remove it from the system.
> > So simplify the code by deleting the variable and cleaning up the logic
> > around the debugfs file.
>
> This will generate the following oops on my board (arm64,
> freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts). In debugfs_lookup()
> debugfs_mount is NULL.
>
> [ 0.000000] Linux version 5.11.0-next-20210217-00014-g41e0e92a2d05 (mwalle@mwalle01) (aarch64-linux-gnu-gcc (Debian 8.3.0-2) 8.3.0, GNU ld (GNU Binutils for Debian) 2.31.1) #69 SMP PREEMPT Wed Feb 17 20:47:10 CET 2021
> [..]
> [ 0.000000] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> [ 0.000000] Mem abort info:
> [ 0.000000] ESR = 0x96000004

<snip>

Back to this log, do you see:
"Unable to pin filesystem for file "
anywhere before this crash?

thanks,

greg k-h

2021-02-18 10:36:46

[permalink] [raw]

Subject: Re: [PATCH] irqdomain: remove debugfs_file from struct irq_domain

Am 2021-02-18 09:52, schrieb Greg KH:
> On Thu, Feb 18, 2021 at 09:27:09AM +0100, Michael Walle wrote:
>> Am 2021-02-18 08:31, schrieb Greg KH:
>> > On Wed, Feb 17, 2021 at 09:50:38PM +0000, Marc Zyngier wrote:
>> > > On Wed, 17 Feb 2021 20:10:50 +0000,
>> > > Michael Walle <[email protected]> wrote:
>> > > >
>> > > > Am 2021-02-17 21:02, schrieb Marc Zyngier:
>> > > > > On 2021-02-17 19:57, Michael Walle wrote:
>> > > > >> Hi Greg,
>> > > > >>
>> > > > >>> There's no need to keep around a dentry pointer to a simple file that
>> > > > >>> debugfs itself can look up when we need to remove it from the system.
>> > > > >>> So simplify the code by deleting the variable and cleaning up the
>> > > > >>> logic
>> > > > >>> around the debugfs file.
>> > > > >>
>> > > > >> This will generate the following oops on my board (arm64,
>> > > > >> freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts). In debugfs_lookup()
>> > > > >> debugfs_mount is NULL.
>> > > > >
>> > > > > That's odd. I gave it a go yesterday, and nothing blew up.
>> > > > > Which makes me wonder whether I had the debug stuff enabled
>> > > > > the first place...
>> > > > >
>> > > > > I've dropped the patch from -next for now until I figure it out
>> > > > > (probably tomorrow).
>> > > >
>> > > > Mh, maybe its my .config, I've attached it. I also noticed that
>> > > > the board boots just fine in our kernel-ci [1].
>> > >
>> > > I reproduced here. I had disabled GENERIC_IRQ_DEBUGFS for obscure
>> > > reasons, and it caught fire as I re-enabled it.
>> > >
>> > > Adding this fixes it for me:
>> > >
>> > > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> > > index 367ff1c35f75..d8a14cf1a7b6 100644
>> > > --- a/kernel/irq/irqdomain.c
>> > > +++ b/kernel/irq/irqdomain.c
>> > > @@ -1904,7 +1904,8 @@ static void debugfs_add_domain_dir(struct
>> > > irq_domain *d)
>> > >
>> > > static void debugfs_remove_domain_dir(struct irq_domain *d)
>> > > {
>> > > - debugfs_remove(debugfs_lookup(d->name, domain_dir));
>> > > + if (domain_dir)
>> > > + debugfs_remove(debugfs_lookup(d->name, domain_dir));
>> > > }
>> > >
>> > > void __init irq_domain_debugfs_init(struct dentry *root)
>> > >
>> > >
>> > > Could you please check whether it works for you?
>> >
>> > Can you try this debugfs core change instead? Callers to debugfs should
>> > not have to do the above type of checking as debugfs should be much more
>> > robust than that.
>> >
>> > thanks,
>> >
>> > greg k-h
>> >
>> >
>> > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
>> > index 2fcf66473436..5aa798f52b6e 100644
>> > --- a/fs/debugfs/inode.c
>> > +++ b/fs/debugfs/inode.c
>> > @@ -297,7 +297,7 @@ struct dentry *debugfs_lookup(const char *name,
>> > struct dentry *parent)
>> > {
>> > struct dentry *dentry;
>> >
>> > - if (IS_ERR(parent))
>> > + if (IS_ERR_OR_NULL(name) || IS_ERR(parent))
>> > return NULL;
>> >
>> > if (!parent)
>>
>> This doesn't work. name is not NULL when it is called.
>>
>> What has to happen before debugfs_lookup() can be called? Looks like
>> someone has to initialize the static debugfs_mount, first.
>
> Ok, how about this:
>
>
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 2fcf66473436..86c7f0489620 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -297,7 +297,7 @@ struct dentry *debugfs_lookup(const char *name,
> struct dentry *parent)
> {
> struct dentry *dentry;
>
> - if (IS_ERR(parent))
> + if (!debugfs_initialized() || IS_ERR_OR_NULL(name) || IS_ERR(parent))
> return NULL;
>
> if (!parent)
> @@ -318,6 +318,9 @@ static struct dentry *start_creating(const char
> *name, struct dentry *parent)
> if (!(debugfs_allow & DEBUGFS_ALLOW_API))
> return ERR_PTR(-EPERM);
>
> + if (!debugfs_initialized())
> + return ERR_PTR(-ENOENT);
> +
> pr_debug("creating file '%s'\n", name);
>
> if (IS_ERR(parent))

That works.

-michael

2021-02-18 10:43:41

[permalink] [raw]

Subject: Re: [PATCH] irqdomain: remove debugfs_file from struct irq_domain

On Thu, Feb 18, 2021 at 09:27:09AM +0100, Michael Walle wrote:
> Am 2021-02-18 08:31, schrieb Greg KH:
> > On Wed, Feb 17, 2021 at 09:50:38PM +0000, Marc Zyngier wrote:
> > > On Wed, 17 Feb 2021 20:10:50 +0000,
> > > Michael Walle <[email protected]> wrote:
> > > >
> > > > Am 2021-02-17 21:02, schrieb Marc Zyngier:
> > > > > On 2021-02-17 19:57, Michael Walle wrote:
> > > > >> Hi Greg,
> > > > >>
> > > > >>> There's no need to keep around a dentry pointer to a simple file that
> > > > >>> debugfs itself can look up when we need to remove it from the system.
> > > > >>> So simplify the code by deleting the variable and cleaning up the
> > > > >>> logic
> > > > >>> around the debugfs file.
> > > > >>
> > > > >> This will generate the following oops on my board (arm64,
> > > > >> freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts). In debugfs_lookup()
> > > > >> debugfs_mount is NULL.
> > > > >
> > > > > That's odd. I gave it a go yesterday, and nothing blew up.
> > > > > Which makes me wonder whether I had the debug stuff enabled
> > > > > the first place...
> > > > >
> > > > > I've dropped the patch from -next for now until I figure it out
> > > > > (probably tomorrow).
> > > >
> > > > Mh, maybe its my .config, I've attached it. I also noticed that
> > > > the board boots just fine in our kernel-ci [1].
> > >
> > > I reproduced here. I had disabled GENERIC_IRQ_DEBUGFS for obscure
> > > reasons, and it caught fire as I re-enabled it.
> > >
> > > Adding this fixes it for me:
> > >
> > > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> > > index 367ff1c35f75..d8a14cf1a7b6 100644
> > > --- a/kernel/irq/irqdomain.c
> > > +++ b/kernel/irq/irqdomain.c
> > > @@ -1904,7 +1904,8 @@ static void debugfs_add_domain_dir(struct
> > > irq_domain *d)
> > >
> > > static void debugfs_remove_domain_dir(struct irq_domain *d)
> > > {
> > > - debugfs_remove(debugfs_lookup(d->name, domain_dir));
> > > + if (domain_dir)
> > > + debugfs_remove(debugfs_lookup(d->name, domain_dir));
> > > }
> > >
> > > void __init irq_domain_debugfs_init(struct dentry *root)
> > >
> > >
> > > Could you please check whether it works for you?
> >
> > Can you try this debugfs core change instead? Callers to debugfs should
> > not have to do the above type of checking as debugfs should be much more
> > robust than that.
> >
> > thanks,
> >
> > greg k-h
> >
> >
> > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> > index 2fcf66473436..5aa798f52b6e 100644
> > --- a/fs/debugfs/inode.c
> > +++ b/fs/debugfs/inode.c
> > @@ -297,7 +297,7 @@ struct dentry *debugfs_lookup(const char *name,
> > struct dentry *parent)
> > {
> > struct dentry *dentry;
> >
> > - if (IS_ERR(parent))
> > + if (IS_ERR_OR_NULL(name) || IS_ERR(parent))
> > return NULL;
> >
> > if (!parent)
>
> This doesn't work. name is not NULL when it is called.

Ok, but it is a good check we need to make anyway, I'll add it to a
patch series :)

> What has to happen before debugfs_lookup() can be called? Looks like
> someone has to initialize the static debugfs_mount, first.

Wow, wait, you are removing a debugfs file _before_ debugfs is even
initialized? Didn't expect that, ok, let me go try this again...

thanks,

greg k-h

2021-02-18 10:58:24

[permalink] [raw]

Subject: Re: [PATCH] irqdomain: remove debugfs_file from struct irq_domain

On Thu, Feb 18, 2021 at 09:04:42AM +0000, Marc Zyngier wrote:
> On Thu, 18 Feb 2021 08:54:00 +0000,
> Greg KH <[email protected]> wrote:
>
> [...]
>
> > > > Wow, wait, you are removing a debugfs file _before_ debugfs is even
> > > > initialized? Didn't expect that, ok, let me go try this again...
> > >
> > > Yeah, that's a poor man's rename (file being deleted and re-created).
> >
> > True, but that's not happening here, right? Some driver is being
> > initialized and creates a debugfs file, and then decides to unload so it
> > removes the debugfs file?
>
> No, that's not what is happening.
>
> The irqchip driver starts, creates an irqdomain. File gets created, at
> least in theory (it fails because debugfs isn't ready, but that's not
> the issue).
>
> It then changes an attribute to the domain (the so-called bus_token),
> which gets reflected in the domain name to avoid aliasing.
> Delete/create follows.
>
> > Why was it trying to create the file in the first place if it didn't
> > properly bind to the hardware?
>
> See above. We encode properties of the domain in the filename, and
> reflect the change of these properties as they happen.

Ah, ok, you really are doing delete/re-create. Crazy. And amazing it
was working previously without the checks I just added...

Funny that you all never were even noticing that the debugfs files are
not present in the system because they are tryign to be created before
debugfs is present? Is that an issue or has no one complained?

Anyway, I'll go turn this into a real patch and get it into 5.12-rc1 so
that the irqdomain patch I sent you will not blow anything up. Feel
free to also queue it up in your tree if you want to as well.

thanks,

greg k-h

2021-02-18 12:54:38