v5.13-rcX regression - NULL pointer dereference - MFD and software node API

v5.13-rcX regression - NULL pointer dereference - MFD and software node API

[Dominik Brodowski]

Over a month ago, Andy Shevchenko reported and fixed a NULL pointer
dereference issue introduced by commit
	42e59982917a ("mfd: core: Add support for software nodes")
in v5.13-rc1:
	https://lore.kernel.org/lkml/20210510141552.57045-1-andriy.shevchenko@linux.intel.com/

A bisect shows that it is indeed commit 42e59982917a which causes boot to
fail due to a NULL pointer dereference on my work laptop, where "intel-lpss"
is bound to
	00:15.0	Signal processing controller: Intel Corporation Sunrise Point-LP Serial IO I2C Controller #0 (rev 21)
and fails to bind to INT3446:

[    6.048087] intel-lpss 0000:00:15.0: enabling device (0000 -> 0002)
[    6.050625] idma64 idma64.0: Found Intel integrated DMA 64-bit
[    6.109112] intel-lpss 0000:00:15.1: enabling device (0000 -> 0002)
[    6.111348] idma64 idma64.1: Found Intel integrated DMA 64-bit
[    6.172229] intel-lpss 0000:00:15.2: enabling device (0000 -> 0002)
[    6.174353] idma64 idma64.2: Found Intel integrated DMA 64-bit
[    6.231865] intel-lpss 0000:00:15.3: enabling device (0000 -> 0002)
[    6.233845] idma64 idma64.3: Found Intel integrated DMA 64-bit
[    6.287492] ACPI Warning: SystemMemory range 0x00000000FE028000-0x00000000FE0281FF conflicts with OpRegion 0x00000000FE028000-0x00000000FE028207 (\_SB.PCI0.GEXP.BAR0) (20210331/utaddress-204)
[    6.287704] ACPI: OSL: Resource conflict; ACPI support missing from driver?
[    6.289760] intel-lpss: probe of INT3446:00 failed with error -16

Unfortunately, the patch by Andy Shevchenko (applied on top of Linus' tree)
does not fix the issue. A complete revert, however, does fix the issue, and
allows my laptop to boot again.

In my opinion, it is unfortunate that although it has been known for over a
month that commit 42e59982917a is broken, the bugfix (though probably not 
far-reaching enough) has not yet progressed upstream.

Thanks,
	Dominik


[PATCH] Revert "mfd: core: Add support for software nodes"

This reverts commit 42e59982917a25ad254b74e6e8decee5e684763d.

Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 6f02b8022c6d..7a052e61280e 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -226,12 +226,6 @@ static int mfd_add_device(struct device *parent, int id,
 			goto fail_of_entry;
 	}
 
-	if (cell->swnode) {
-		ret = device_add_software_node(&pdev->dev, cell->swnode);
-		if (ret)
-			goto fail_of_entry;
-	}
-
 	for (r = 0; r < cell->num_resources; r++) {
 		res[r].name = cell->resources[r].name;
 		res[r].flags = cell->resources[r].flags;
@@ -292,7 +286,6 @@ static int mfd_add_device(struct device *parent, int id,
 			list_del(&of_entry->list);
 			kfree(of_entry);
 		}
-	device_remove_software_node(&pdev->dev);
 fail_alias:
 	regulator_bulk_unregister_supply_alias(&pdev->dev,
 					       cell->parent_supplies,
@@ -361,8 +354,6 @@ static int mfd_remove_devices_fn(struct device *dev, void *data)
 	regulator_bulk_unregister_supply_alias(dev, cell->parent_supplies,
 					       cell->num_parent_supplies);
 
-	device_remove_software_node(&pdev->dev);
-
 	platform_device_unregister(pdev);
 	return 0;
 }

Re: v5.13-rcX regression - NULL pointer dereference - MFD and software node API

[Andy Shevchenko]

On Sun, Jun 20, 2021 at 11:36 AM Dominik Brodowski
<linux@dominikbrodowski.net> wrote:
>
> Over a month ago, Andy Shevchenko reported and fixed a NULL pointer
> dereference issue introduced by commit
>         42e59982917a ("mfd: core: Add support for software nodes")
> in v5.13-rc1:
>         https://lore.kernel.org/lkml/20210510141552.57045-1-andriy.shevchenko@linux.intel.com/
>
> A bisect shows that it is indeed commit 42e59982917a which causes boot to
> fail due to a NULL pointer dereference on my work laptop,

Can you, please, be more specific? E.g. where may I find the ACPI dump
of your laptop, along with other information?
What you may prepare is (all run under root user)
1. `acpidump -o laptop-$MODEL.dat` (the *.dat file)
2. `grep -H 15 /sys/bus/acpi/devices/*/status`
3. `dmesg`
4. `cat /proc/iomem /proc/ioport`
5. `lspci -nk -vv`

(#2 and #3 are interesting to have in working and non-working cases)

Perhaps a bug on the kernel bugzilla would be a good container for all these.

Also it's not clear what exactly an Oops you have (I don't believe
it's the same).

> where "intel-lpss"
> is bound to
>         00:15.0 Signal processing controller: Intel Corporation Sunrise Point-LP Serial IO I2C Controller #0 (rev 21)
> and fails to bind to INT3446:

Yeah, this is confusing (see above for additional information needed).

> [    6.048087] intel-lpss 0000:00:15.0: enabling device (0000 -> 0002)
> [    6.050625] idma64 idma64.0: Found Intel integrated DMA 64-bit
> [    6.109112] intel-lpss 0000:00:15.1: enabling device (0000 -> 0002)
> [    6.111348] idma64 idma64.1: Found Intel integrated DMA 64-bit
> [    6.172229] intel-lpss 0000:00:15.2: enabling device (0000 -> 0002)
> [    6.174353] idma64 idma64.2: Found Intel integrated DMA 64-bit
> [    6.231865] intel-lpss 0000:00:15.3: enabling device (0000 -> 0002)
> [    6.233845] idma64 idma64.3: Found Intel integrated DMA 64-bit
> [    6.287492] ACPI Warning: SystemMemory range 0x00000000FE028000-0x00000000FE0281FF conflicts with OpRegion 0x00000000FE028000-0x00000000FE028207 (\_SB.PCI0.GEXP.BAR0) (20210331/utaddress-204)
> [    6.287704] ACPI: OSL: Resource conflict; ACPI support missing from driver?
> [    6.289760] intel-lpss: probe of INT3446:00 failed with error -16
>
> Unfortunately, the patch by Andy Shevchenko (applied on top of Linus' tree)
> does not fix the issue. A complete revert, however, does fix the issue, and
> allows my laptop to boot again.

The problem my patch fixed (besides logical issues) was to work around
_buggy_ ACPI table. If anything, I guess the firmware is to blame for
this, but let's see the actual data before judging and getting the
right course of action.

> In my opinion, it is unfortunate that although it has been known for over a
> month that commit 42e59982917a is broken, the bugfix (though probably not
> far-reaching enough) has not yet progressed upstream.

Which sounds like a narrow scope of the issue and supports the theory
of buggy tables. It may also be possible that some driver

-- 
With Best Regards,
Andy Shevchenko

Re: v5.13-rcX regression - NULL pointer dereference - MFD and software node API

[Dominik Brodowski]

Am Sun, Jun 20, 2021 at 02:19:44PM +0300 schrieb Andy Shevchenko:
> On Sun, Jun 20, 2021 at 11:36 AM Dominik Brodowski
> <linux@dominikbrodowski.net> wrote:
> >
> > Over a month ago, Andy Shevchenko reported and fixed a NULL pointer
> > dereference issue introduced by commit
> >         42e59982917a ("mfd: core: Add support for software nodes")
> > in v5.13-rc1:
> >         https://lore.kernel.org/lkml/20210510141552.57045-1-andriy.shevchenko@linux.intel.com/
> >
> > A bisect shows that it is indeed commit 42e59982917a which causes boot to
> > fail due to a NULL pointer dereference on my work laptop,
> 
> Can you, please, be more specific? E.g. where may I find the ACPI dump
> of your laptop, along with other information?
> What you may prepare is (all run under root user)
> 1. `acpidump -o laptop-$MODEL.dat` (the *.dat file)
> 2. `grep -H 15 /sys/bus/acpi/devices/*/status`
> 3. `dmesg`
> 4. `cat /proc/iomem /proc/ioport`
> 5. `lspci -nk -vv`
> 
> (#2 and #3 are interesting to have in working and non-working cases)
> 
> Perhaps a bug on the kernel bugzilla would be a good container for all these.
> 
> Also it's not clear what exactly an Oops you have (I don't believe
> it's the same).

Thanks for taking a look at this issue. As it's actually a panic during
boot which triggers before initramfs is ready, I can only provide the data
for the "working case", i.e. with the patch causing the regression already
reverted:

	https://bugzilla.kernel.org/show_bug.cgi?id=213511

With commit 42e59982917a reverted, the system works just fine.

> > In my opinion, it is unfortunate that although it has been known for over a
> > month that commit 42e59982917a is broken, the bugfix (though probably not
> > far-reaching enough) has not yet progressed upstream.
> 
> Which sounds like a narrow scope of the issue and supports the theory
> of buggy tables. It may also be possible that some driver

... still, it's a regression, with a clear "git bisect" result :-(

Thanks,
	Dominik

Re: v5.13-rcX regression - NULL pointer dereference - MFD and software node API

[Andy Shevchenko]

On Sun, Jun 20, 2021 at 2:50 PM Dominik Brodowski
<linux@dominikbrodowski.net> wrote:
>
> Am Sun, Jun 20, 2021 at 02:19:44PM +0300 schrieb Andy Shevchenko:
> > On Sun, Jun 20, 2021 at 11:36 AM Dominik Brodowski
> > <linux@dominikbrodowski.net> wrote:
> > >
> > > Over a month ago, Andy Shevchenko reported and fixed a NULL pointer
> > > dereference issue introduced by commit
> > >         42e59982917a ("mfd: core: Add support for software nodes")
> > > in v5.13-rc1:
> > >         https://lore.kernel.org/lkml/20210510141552.57045-1-andriy.shevchenko@linux.intel.com/
> > >
> > > A bisect shows that it is indeed commit 42e59982917a which causes boot to
> > > fail due to a NULL pointer dereference on my work laptop,
> >
> > Can you, please, be more specific? E.g. where may I find the ACPI dump
> > of your laptop, along with other information?
> > What you may prepare is (all run under root user)
> > 1. `acpidump -o laptop-$MODEL.dat` (the *.dat file)
> > 2. `grep -H 15 /sys/bus/acpi/devices/*/status`
> > 3. `dmesg`
> > 4. `cat /proc/iomem /proc/ioport`
> > 5. `lspci -nk -vv`
> >
> > (#2 and #3 are interesting to have in working and non-working cases)
> >
> > Perhaps a bug on the kernel bugzilla would be a good container for all these.
> >
> > Also it's not clear what exactly an Oops you have (I don't believe
> > it's the same).
>
> Thanks for taking a look at this issue. As it's actually a panic during
> boot which triggers before initramfs is ready, I can only provide the data
> for the "working case", i.e. with the patch causing the regression already
> reverted:
>
>         https://bugzilla.kernel.org/show_bug.cgi?id=213511
>
> With commit 42e59982917a reverted, the system works just fine.

Is it possible to have any traces of the panic to be logged / photoed / etc?

> > > In my opinion, it is unfortunate that although it has been known for over a
> > > month that commit 42e59982917a is broken, the bugfix (though probably not
> > > far-reaching enough) has not yet progressed upstream.
> >
> > Which sounds like a narrow scope of the issue and supports the theory
> > of buggy tables. It may also be possible that some driver
>
> ... still, it's a regression, with a clear "git bisect" result :-(

I understand and I'm looking at this...

-- 
With Best Regards,
Andy Shevchenko

Re: v5.13-rcX regression - NULL pointer dereference - MFD and software node API

[Andy Shevchenko]

On Sun, Jun 20, 2021 at 7:36 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sun, Jun 20, 2021 at 2:50 PM Dominik Brodowski
> <linux@dominikbrodowski.net> wrote:
> > Am Sun, Jun 20, 2021 at 02:19:44PM +0300 schrieb Andy Shevchenko:
> > > On Sun, Jun 20, 2021 at 11:36 AM Dominik Brodowski
> > > <linux@dominikbrodowski.net> wrote:
> > > >
> > > > Over a month ago, Andy Shevchenko reported and fixed a NULL pointer
> > > > dereference issue introduced by commit
> > > >         42e59982917a ("mfd: core: Add support for software nodes")
> > > > in v5.13-rc1:
> > > >         https://lore.kernel.org/lkml/20210510141552.57045-1-andriy.shevchenko@linux.intel.com/
> > > >
> > > > A bisect shows that it is indeed commit 42e59982917a which causes boot to
> > > > fail due to a NULL pointer dereference on my work laptop,
> > >
> > > Can you, please, be more specific? E.g. where may I find the ACPI dump
> > > of your laptop, along with other information?
> > > What you may prepare is (all run under root user)
> > > 1. `acpidump -o laptop-$MODEL.dat` (the *.dat file)
> > > 2. `grep -H 15 /sys/bus/acpi/devices/*/status`
> > > 3. `dmesg`
> > > 4. `cat /proc/iomem /proc/ioport`
> > > 5. `lspci -nk -vv`
> > >
> > > (#2 and #3 are interesting to have in working and non-working cases)
> > >
> > > Perhaps a bug on the kernel bugzilla would be a good container for all these.
> > >
> > > Also it's not clear what exactly an Oops you have (I don't believe
> > > it's the same).
> >
> > Thanks for taking a look at this issue. As it's actually a panic during
> > boot which triggers before initramfs is ready, I can only provide the data
> > for the "working case", i.e. with the patch causing the regression already
> > reverted:
> >
> >         https://bugzilla.kernel.org/show_bug.cgi?id=213511
> >
> > With commit 42e59982917a reverted, the system works just fine.
>
> Is it possible to have any traces of the panic to be logged / photoed / etc?

I'm just wondering (based on the idea behind my fix) if the commenting
out INT3446 ID entry in the intel-lpss-acpi.c will help boot this? If
so, you may try to bind it later to the driver and see the Oops.


-- 
With Best Regards,
Andy Shevchenko

Re: v5.13-rcX regression - NULL pointer dereference - MFD and software node API

[Dominik Brodowski]

Am Sun, Jun 20, 2021 at 09:27:34PM +0300 schrieb Andy Shevchenko:
> On Sun, Jun 20, 2021 at 7:36 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sun, Jun 20, 2021 at 2:50 PM Dominik Brodowski
> > <linux@dominikbrodowski.net> wrote:
> > > Am Sun, Jun 20, 2021 at 02:19:44PM +0300 schrieb Andy Shevchenko:
> > > > On Sun, Jun 20, 2021 at 11:36 AM Dominik Brodowski
> > > > <linux@dominikbrodowski.net> wrote:
> > > > >
> > > > > Over a month ago, Andy Shevchenko reported and fixed a NULL pointer
> > > > > dereference issue introduced by commit
> > > > >         42e59982917a ("mfd: core: Add support for software nodes")
> > > > > in v5.13-rc1:
> > > > >         https://lore.kernel.org/lkml/20210510141552.57045-1-andriy.shevchenko@linux.intel.com/
> > > > >
> > > > > A bisect shows that it is indeed commit 42e59982917a which causes boot to
> > > > > fail due to a NULL pointer dereference on my work laptop,
> > > >
> > > > Can you, please, be more specific? E.g. where may I find the ACPI dump
> > > > of your laptop, along with other information?
> > > > What you may prepare is (all run under root user)
> > > > 1. `acpidump -o laptop-$MODEL.dat` (the *.dat file)
> > > > 2. `grep -H 15 /sys/bus/acpi/devices/*/status`
> > > > 3. `dmesg`
> > > > 4. `cat /proc/iomem /proc/ioport`
> > > > 5. `lspci -nk -vv`
> > > >
> > > > (#2 and #3 are interesting to have in working and non-working cases)
> > > >
> > > > Perhaps a bug on the kernel bugzilla would be a good container for all these.
> > > >
> > > > Also it's not clear what exactly an Oops you have (I don't believe
> > > > it's the same).
> > >
> > > Thanks for taking a look at this issue. As it's actually a panic during
> > > boot which triggers before initramfs is ready, I can only provide the data
> > > for the "working case", i.e. with the patch causing the regression already
> > > reverted:
> > >
> > >         https://bugzilla.kernel.org/show_bug.cgi?id=213511
> > >
> > > With commit 42e59982917a reverted, the system works just fine.
> >
> > Is it possible to have any traces of the panic to be logged / photoed / etc?
> 
> I'm just wondering (based on the idea behind my fix) if the commenting
> out INT3446 ID entry in the intel-lpss-acpi.c will help boot this?

Indeed, that allows booting to proceed as expected.

> If so, you may try to bind it later to the driver and see the Oops.

Binding alone doesn't work, as the driver does not allow for dynamic IDs.
But building just the acpi part as a module allowed me to log the oops:

[   87.695362] intel_lpss_acpi: module verification failed: signature and/or required key missing - tainting kernel
[   87.697932] intel-lpss INT3446:00: Failed to create debugfs entries
[   87.698438] ACPI Warning: SystemMemory range 0x00000000FE028000-0x00000000FE0281FF conflicts with OpRegion 0x00000000FE028000-0x00000000FE028207 (\_SB.PCI0.GEXP.BAR0) (20210331/utaddress-204)
[   87.698452] ACPI: OSL: Resource conflict; ACPI support missing from driver?
[   87.698456] BUG: kernel NULL pointer dereference, address: 0000000000000000
[   87.698459] #PF: supervisor read access in kernel mode
[   87.698460] #PF: error_code(0x0000) - not-present page
[   87.698462] PGD 0 P4D 0 
[   87.698465] Oops: 0000 [#1] PREEMPT SMP PTI
[   87.698467] CPU: 4 PID: 2249 Comm: insmod Tainted: G            E   T 5.13.0-rc7 #1
[   87.698470] Hardware name: Dell Inc. Latitude 7390/09386V, BIOS 1.9.2 04/03/2019
[   87.698471] RIP: 0010:strlen+0x0/0x20
[   87.698477] Code: 48 89 f9 74 09 48 83 c1 01 80 39 00 75 f7 31 d2 44 0f b6 04 16 44 88 04 11 48 83 c2 01 45 84 c0 75 ee c3 0f 1f 80 00 00 00 00 <80> 3f 00 74 10 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 c3 31
[   87.698479] RSP: 0018:ffffa463c359f990 EFLAGS: 00010246
[   87.698481] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000001
[   87.698483] RDX: 00000000ffffffff RSI: 0000000000000000 RDI: 0000000000000000
[   87.698485] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000001
[   87.698486] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000000
[   87.698487] R13: 0000000000000000 R14: 0000000000000000 R15: dead000000000100
[   87.698489] FS:  0000701d893a7740(0000) GS:ffff96026d600000(0000) knlGS:0000000000000000
[   87.698491] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   87.698493] CR2: 0000000000000000 CR3: 000000015555a006 CR4: 00000000003706e0
[   87.698494] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   87.698496] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   87.698497] Call Trace:
[   87.698502]  kernfs_name_hash+0x12/0x80
[   87.698509]  kernfs_find_ns+0x51/0x110
[   87.698512]  kernfs_remove_by_name_ns+0x34/0x80
[   87.698515]  software_node_notify+0x7d/0x110
[   87.698519]  device_remove_software_node+0x5b/0x70
[   87.698523]  mfd_add_device+0x3a2/0x520
[   87.698527]  ? dev_printk_emit+0x4e/0x65
[   87.698531]  ? __dev_printk+0x2d/0x69
[   87.698534]  mfd_add_devices+0x6c/0xd0
[   87.698539]  intel_lpss_probe+0x3db/0x510
[   87.698543]  ? preempt_count_sub+0x9b/0xd0
[   87.698546]  platform_probe+0x3f/0x90
[   87.698550]  really_probe+0xed/0x360
[   87.698553]  driver_probe_device+0x56/0xb0
[   87.698556]  device_driver_attach+0xa8/0xb0
[   87.698560]  __driver_attach+0x55/0xc0
[   87.698563]  ? device_driver_attach+0xb0/0xb0
[   87.698565]  bus_for_each_dev+0x7b/0xc0
[   87.698568]  bus_add_driver+0x106/0x1b0
[   87.698571]  driver_register+0x8f/0xe0
[   87.698574]  ? 0xffffffffc0252000
[   87.698577]  do_one_initcall+0x5b/0x2f0
[   87.698580]  ? rcu_read_lock_sched_held+0x3f/0x70
[   87.698583]  ? trace_kmalloc+0xa6/0xd0
[   87.698587]  ? kmem_cache_alloc_trace+0x22a/0x360
[   87.698590]  do_init_module+0x7a/0x5c0
[   87.698595]  __do_sys_finit_module+0xa5/0xe0
[   87.698600]  do_syscall_64+0x6b/0x80
[   87.698604]  ? do_syscall_64+0x11/0x80
[   87.698607]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[   87.698611] RIP: 0033:0x701d894cd18d
[   87.698613] Code: b4 0c 00 0f 05 eb a9 66 0f 1f 44 00 00 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d b3 6c 0c 00 f7 d8 64 89 01 48
[   87.698615] RSP: 002b:00007fff19800878 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
[   87.698617] RAX: ffffffffffffffda RBX: 0000603fadd124b0 RCX: 0000701d894cd18d
[   87.698619] RDX: 0000000000000000 RSI: 0000603fac9b8270 RDI: 0000000000000003
[   87.698620] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000701d89598380
[   87.698621] R10: 0000000000000003 R11: 0000000000000246 R12: 0000603fac9b8270
[   87.698623] R13: 0000000000000000 R14: 0000603fadd12480 R15: 0000000000000000
[   87.698626] Modules linked in: intel_lpss_acpi(E+)
[   87.698629] CR2: 0000000000000000
[   87.698631] ---[ end trace c7ab8e7eeffb100e ]---
[   87.698633] RIP: 0010:strlen+0x0/0x20
[   87.698636] Code: 48 89 f9 74 09 48 83 c1 01 80 39 00 75 f7 31 d2 44 0f b6 04 16 44 88 04 11 48 83 c2 01 45 84 c0 75 ee c3 0f 1f 80 00 00 00 00 <80> 3f 00 74 10 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 c3 31
[   87.698638] RSP: 0018:ffffa463c359f990 EFLAGS: 00010246
[   87.698640] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000001
[   87.698641] RDX: 00000000ffffffff RSI: 0000000000000000 RDI: 0000000000000000
[   87.698643] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000001
[   87.698644] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000000
[   87.698645] R13: 0000000000000000 R14: 0000000000000000 R15: dead000000000100
[   87.698647] FS:  0000701d893a7740(0000) GS:ffff96026d600000(0000) knlGS:0000000000000000
[   87.698649] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   87.698650] CR2: 0000000000000000 CR3: 000000015555a006 CR4: 00000000003706e0
[   87.698652] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   87.698653] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   87.698655] BUG: sleeping function called from invalid context at include/linux/percpu-rwsem.h:49
[   87.698656] in_atomic(): 0, irqs_disabled(): 1, non_block: 0, pid: 2249, name: insmod
[   87.698658] INFO: lockdep is turned off.
[   87.698659] irq event stamp: 12200
[   87.698660] hardirqs last  enabled at (12199): [<ffffffff8a027c8e>] console_unlock+0x4de/0x5f0
[   87.698664] hardirqs last disabled at (12200): [<ffffffff8b18d904>] exc_page_fault+0x34/0x8a0
[   87.698667] softirqs last  enabled at (11666): [<ffffffff8b400349>] __do_softirq+0x349/0x4b1
[   87.698671] softirqs last disabled at (11659): [<ffffffff89faf73f>] irq_exit_rcu+0xff/0x140
[   87.698675] CPU: 4 PID: 2249 Comm: insmod Tainted: G      D     E   T 5.13.0-rc7 #1
[   87.698677] Hardware name: Dell Inc. Latitude 7390/09386V, BIOS 1.9.2 04/03/2019
[   87.698678] Call Trace:
[   87.698680]  dump_stack+0x7f/0xad
[   87.698684]  ___might_sleep.cold+0xf5/0x109
[   87.698688]  exit_signals+0x1c/0x330
[   87.698691]  do_exit+0xcd/0xb50
[   87.698695]  rewind_stack_do_exit+0x17/0x17
[   87.698698] RIP: 0033:0x701d894cd18d
[   87.698700] Code: b4 0c 00 0f 05 eb a9 66 0f 1f 44 00 00 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d b3 6c 0c 00 f7 d8 64 89 01 48
[   87.698702] RSP: 002b:00007fff19800878 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
[   87.698704] RAX: ffffffffffffffda RBX: 0000603fadd124b0 RCX: 0000701d894cd18d
[   87.698705] RDX: 0000000000000000 RSI: 0000603fac9b8270 RDI: 0000000000000003
[   87.698707] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000701d89598380
[   87.698708] R10: 0000000000000003 R11: 0000000000000246 R12: 0000603fac9b8270
[   87.698709] R13: 0000000000000000 R14: 0000603fadd12480 R15: 0000000000000000

Hope this helps,
	Dominik

Re: v5.13-rcX regression - NULL pointer dereference - MFD and software node API

[Andy Shevchenko]

On Mon, Jun 21, 2021 at 10:59:08AM +0200, Dominik Brodowski wrote:
> Am Sun, Jun 20, 2021 at 09:27:34PM +0300 schrieb Andy Shevchenko:
> > On Sun, Jun 20, 2021 at 7:36 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Sun, Jun 20, 2021 at 2:50 PM Dominik Brodowski
> > > <linux@dominikbrodowski.net> wrote:
> > > > Am Sun, Jun 20, 2021 at 02:19:44PM +0300 schrieb Andy Shevchenko:
> > > > > On Sun, Jun 20, 2021 at 11:36 AM Dominik Brodowski
> > > > > <linux@dominikbrodowski.net> wrote:
> > > > > >
> > > > > > Over a month ago, Andy Shevchenko reported and fixed a NULL pointer
> > > > > > dereference issue introduced by commit
> > > > > >         42e59982917a ("mfd: core: Add support for software nodes")
> > > > > > in v5.13-rc1:
> > > > > >         https://lore.kernel.org/lkml/20210510141552.57045-1-andriy.shevchenko@linux.intel.com/
> > > > > >
> > > > > > A bisect shows that it is indeed commit 42e59982917a which causes boot to
> > > > > > fail due to a NULL pointer dereference on my work laptop,
> > > > >
> > > > > Can you, please, be more specific? E.g. where may I find the ACPI dump
> > > > > of your laptop, along with other information?
> > > > > What you may prepare is (all run under root user)
> > > > > 1. `acpidump -o laptop-$MODEL.dat` (the *.dat file)
> > > > > 2. `grep -H 15 /sys/bus/acpi/devices/*/status`
> > > > > 3. `dmesg`
> > > > > 4. `cat /proc/iomem /proc/ioport`
> > > > > 5. `lspci -nk -vv`
> > > > >
> > > > > (#2 and #3 are interesting to have in working and non-working cases)
> > > > >
> > > > > Perhaps a bug on the kernel bugzilla would be a good container for all these.
> > > > >
> > > > > Also it's not clear what exactly an Oops you have (I don't believe
> > > > > it's the same).
> > > >
> > > > Thanks for taking a look at this issue. As it's actually a panic during
> > > > boot which triggers before initramfs is ready, I can only provide the data
> > > > for the "working case", i.e. with the patch causing the regression already
> > > > reverted:
> > > >
> > > >         https://bugzilla.kernel.org/show_bug.cgi?id=213511
> > > >
> > > > With commit 42e59982917a reverted, the system works just fine.
> > >
> > > Is it possible to have any traces of the panic to be logged / photoed / etc?
> > 
> > I'm just wondering (based on the idea behind my fix) if the commenting
> > out INT3446 ID entry in the intel-lpss-acpi.c will help boot this?
> 
> Indeed, that allows booting to proceed as expected.
> 
> > If so, you may try to bind it later to the driver and see the Oops.
> 
> Binding alone doesn't work, as the driver does not allow for dynamic IDs.
> But building just the acpi part as a module allowed me to log the oops:
> 
> [   87.695362] intel_lpss_acpi: module verification failed: signature and/or required key missing - tainting kernel
> [   87.697932] intel-lpss INT3446:00: Failed to create debugfs entries
> [   87.698438] ACPI Warning: SystemMemory range 0x00000000FE028000-0x00000000FE0281FF conflicts with OpRegion 0x00000000FE028000-0x00000000FE028207 (\_SB.PCI0.GEXP.BAR0) (20210331/utaddress-204)
> [   87.698452] ACPI: OSL: Resource conflict; ACPI support missing from driver?
> [   87.698456] BUG: kernel NULL pointer dereference, address: 0000000000000000
> [   87.698459] #PF: supervisor read access in kernel mode
> [   87.698460] #PF: error_code(0x0000) - not-present page
> [   87.698462] PGD 0 P4D 0 
> [   87.698465] Oops: 0000 [#1] PREEMPT SMP PTI
> [   87.698467] CPU: 4 PID: 2249 Comm: insmod Tainted: G            E   T 5.13.0-rc7 #1
> [   87.698470] Hardware name: Dell Inc. Latitude 7390/09386V, BIOS 1.9.2 04/03/2019
> [   87.698471] RIP: 0010:strlen+0x0/0x20
> [   87.698477] Code: 48 89 f9 74 09 48 83 c1 01 80 39 00 75 f7 31 d2 44 0f b6 04 16 44 88 04 11 48 83 c2 01 45 84 c0 75 ee c3 0f 1f 80 00 00 00 00 <80> 3f 00 74 10 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 c3 31
> [   87.698479] RSP: 0018:ffffa463c359f990 EFLAGS: 00010246
> [   87.698481] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000001
> [   87.698483] RDX: 00000000ffffffff RSI: 0000000000000000 RDI: 0000000000000000
> [   87.698485] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000001
> [   87.698486] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000000
> [   87.698487] R13: 0000000000000000 R14: 0000000000000000 R15: dead000000000100
> [   87.698489] FS:  0000701d893a7740(0000) GS:ffff96026d600000(0000) knlGS:0000000000000000
> [   87.698491] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   87.698493] CR2: 0000000000000000 CR3: 000000015555a006 CR4: 00000000003706e0
> [   87.698494] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [   87.698496] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [   87.698497] Call Trace:
> [   87.698502]  kernfs_name_hash+0x12/0x80
> [   87.698509]  kernfs_find_ns+0x51/0x110
> [   87.698512]  kernfs_remove_by_name_ns+0x34/0x80
> [   87.698515]  software_node_notify+0x7d/0x110
> [   87.698519]  device_remove_software_node+0x5b/0x70
> [   87.698523]  mfd_add_device+0x3a2/0x520
> [   87.698527]  ? dev_printk_emit+0x4e/0x65
> [   87.698531]  ? __dev_printk+0x2d/0x69
> [   87.698534]  mfd_add_devices+0x6c/0xd0
> [   87.698539]  intel_lpss_probe+0x3db/0x510
> [   87.698543]  ? preempt_count_sub+0x9b/0xd0
> [   87.698546]  platform_probe+0x3f/0x90
> [   87.698550]  really_probe+0xed/0x360
> [   87.698553]  driver_probe_device+0x56/0xb0
> [   87.698556]  device_driver_attach+0xa8/0xb0
> [   87.698560]  __driver_attach+0x55/0xc0
> [   87.698563]  ? device_driver_attach+0xb0/0xb0
> [   87.698565]  bus_for_each_dev+0x7b/0xc0
> [   87.698568]  bus_add_driver+0x106/0x1b0
> [   87.698571]  driver_register+0x8f/0xe0
> [   87.698574]  ? 0xffffffffc0252000
> [   87.698577]  do_one_initcall+0x5b/0x2f0
> [   87.698580]  ? rcu_read_lock_sched_held+0x3f/0x70
> [   87.698583]  ? trace_kmalloc+0xa6/0xd0
> [   87.698587]  ? kmem_cache_alloc_trace+0x22a/0x360
> [   87.698590]  do_init_module+0x7a/0x5c0
> [   87.698595]  __do_sys_finit_module+0xa5/0xe0
> [   87.698600]  do_syscall_64+0x6b/0x80
> [   87.698604]  ? do_syscall_64+0x11/0x80
> [   87.698607]  entry_SYSCALL_64_after_hwframe+0x44/0xae
> [   87.698611] RIP: 0033:0x701d894cd18d
> [   87.698613] Code: b4 0c 00 0f 05 eb a9 66 0f 1f 44 00 00 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d b3 6c 0c 00 f7 d8 64 89 01 48
> [   87.698615] RSP: 002b:00007fff19800878 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
> [   87.698617] RAX: ffffffffffffffda RBX: 0000603fadd124b0 RCX: 0000701d894cd18d
> [   87.698619] RDX: 0000000000000000 RSI: 0000603fac9b8270 RDI: 0000000000000003
> [   87.698620] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000701d89598380
> [   87.698621] R10: 0000000000000003 R11: 0000000000000246 R12: 0000603fac9b8270
> [   87.698623] R13: 0000000000000000 R14: 0000603fadd12480 R15: 0000000000000000
> [   87.698626] Modules linked in: intel_lpss_acpi(E+)
> [   87.698629] CR2: 0000000000000000
> [   87.698631] ---[ end trace c7ab8e7eeffb100e ]---
> [   87.698633] RIP: 0010:strlen+0x0/0x20
> [   87.698636] Code: 48 89 f9 74 09 48 83 c1 01 80 39 00 75 f7 31 d2 44 0f b6 04 16 44 88 04 11 48 83 c2 01 45 84 c0 75 ee c3 0f 1f 80 00 00 00 00 <80> 3f 00 74 10 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 c3 31
> [   87.698638] RSP: 0018:ffffa463c359f990 EFLAGS: 00010246
> [   87.698640] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000001
> [   87.698641] RDX: 00000000ffffffff RSI: 0000000000000000 RDI: 0000000000000000
> [   87.698643] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000001
> [   87.698644] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000000
> [   87.698645] R13: 0000000000000000 R14: 0000000000000000 R15: dead000000000100
> [   87.698647] FS:  0000701d893a7740(0000) GS:ffff96026d600000(0000) knlGS:0000000000000000
> [   87.698649] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   87.698650] CR2: 0000000000000000 CR3: 000000015555a006 CR4: 00000000003706e0
> [   87.698652] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [   87.698653] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [   87.698655] BUG: sleeping function called from invalid context at include/linux/percpu-rwsem.h:49
> [   87.698656] in_atomic(): 0, irqs_disabled(): 1, non_block: 0, pid: 2249, name: insmod
> [   87.698658] INFO: lockdep is turned off.
> [   87.698659] irq event stamp: 12200
> [   87.698660] hardirqs last  enabled at (12199): [<ffffffff8a027c8e>] console_unlock+0x4de/0x5f0
> [   87.698664] hardirqs last disabled at (12200): [<ffffffff8b18d904>] exc_page_fault+0x34/0x8a0
> [   87.698667] softirqs last  enabled at (11666): [<ffffffff8b400349>] __do_softirq+0x349/0x4b1
> [   87.698671] softirqs last disabled at (11659): [<ffffffff89faf73f>] irq_exit_rcu+0xff/0x140
> [   87.698675] CPU: 4 PID: 2249 Comm: insmod Tainted: G      D     E   T 5.13.0-rc7 #1
> [   87.698677] Hardware name: Dell Inc. Latitude 7390/09386V, BIOS 1.9.2 04/03/2019
> [   87.698678] Call Trace:
> [   87.698680]  dump_stack+0x7f/0xad
> [   87.698684]  ___might_sleep.cold+0xf5/0x109
> [   87.698688]  exit_signals+0x1c/0x330
> [   87.698691]  do_exit+0xcd/0xb50
> [   87.698695]  rewind_stack_do_exit+0x17/0x17
> [   87.698698] RIP: 0033:0x701d894cd18d
> [   87.698700] Code: b4 0c 00 0f 05 eb a9 66 0f 1f 44 00 00 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d b3 6c 0c 00 f7 d8 64 89 01 48
> [   87.698702] RSP: 002b:00007fff19800878 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
> [   87.698704] RAX: ffffffffffffffda RBX: 0000603fadd124b0 RCX: 0000701d894cd18d
> [   87.698705] RDX: 0000000000000000 RSI: 0000603fac9b8270 RDI: 0000000000000003
> [   87.698707] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000701d89598380
> [   87.698708] R10: 0000000000000003 R11: 0000000000000246 R12: 0000603fac9b8270
> [   87.698709] R13: 0000000000000000 R14: 0000603fadd12480 R15: 0000000000000000

Can you, please, attach this to the bug report?

Long story here is that the device creation fails but we already have added
swnode to it. Meanwhile, device itself is not completely instantiated (yet)
and dev_name(dev) is NULL. The software_node_notify() is called with such
device and Oopses in the following line

	sysfs_remove_link(&swnode->kobj, dev_name(dev));

My patch fixes another issue that might happen before this and in the code
that retrieves swnode itself in the device_remove_software_node().

Of course my patch won't fix this issue.

I have heard that Heikki is looking how to fix the issue in your case and
potentially in any other cases where device_add_software_node() is called
against not formed object instance.

-- 
With Best Regards,
Andy Shevchenko

Re: v5.13-rcX regression - NULL pointer dereference - MFD and software node API

[Heikki Krogerus]

[-- Attachment #1: Type: text/plain, Size: 937 bytes --]

On Mon, Jun 21, 2021 at 01:00:06PM +0300, Andy Shevchenko wrote:
> Can you, please, attach this to the bug report?
> 
> Long story here is that the device creation fails but we already have added
> swnode to it. Meanwhile, device itself is not completely instantiated (yet)
> and dev_name(dev) is NULL. The software_node_notify() is called with such
> device and Oopses in the following line
> 
> 	sysfs_remove_link(&swnode->kobj, dev_name(dev));
> 
> My patch fixes another issue that might happen before this and in the code
> that retrieves swnode itself in the device_remove_software_node().
> 
> Of course my patch won't fix this issue.
> 
> I have heard that Heikki is looking how to fix the issue in your case and
> potentially in any other cases where device_add_software_node() is called
> against not formed object instance.

Dominik, can you test the attached patch to confirm if this really is
the case.

thanks,

-- 
heikki

[-- Attachment #2: 0001-software-node-Handle-software-node-injection-to-an-e.patch --]
[-- Type: text/plain, Size: 1759 bytes --]

From 0cddd29f5d5d41d1b7fa38b0f927f4e755a1bcd0 Mon Sep 17 00:00:00 2001
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Date: Mon, 21 Jun 2021 13:31:51 +0300
Subject: [PATCH] software node: Handle software node injection to an existing
 device properly

Interim, work-in-progress.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/swnode.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 3cc11b813f28c..33c8f31dbab4f 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -1045,7 +1045,15 @@ int device_add_software_node(struct device *dev, const struct software_node *nod
 	}
 
 	set_secondary_fwnode(dev, &swnode->fwnode);
-	software_node_notify(dev, KOBJ_ADD);
+
+	/*
+	 * In some special cases the software node has to be injected to an
+	 * already existing device. In these cases software_node_node() has to
+	 * be called separate from here. Using the device name here to check was
+	 * the device already added or not.
+	 */
+	if (dev_name(dev))
+		software_node_notify(dev, KOBJ_ADD);
 
 	return 0;
 }
@@ -1065,7 +1073,8 @@ void device_remove_software_node(struct device *dev)
 	if (!swnode)
 		return;
 
-	software_node_notify(dev, KOBJ_REMOVE);
+	if (dev_name(dev))
+		software_node_notify(dev, KOBJ_REMOVE);
 	set_secondary_fwnode(dev, NULL);
 	kobject_put(&swnode->kobj);
 }
@@ -1119,8 +1128,7 @@ int software_node_notify(struct device *dev, unsigned long action)
 
 	switch (action) {
 	case KOBJ_ADD:
-		ret = sysfs_create_link_nowarn(&dev->kobj, &swnode->kobj,
-					       "software_node");
+		ret = sysfs_create_link(&dev->kobj, &swnode->kobj, "software_node");
 		if (ret)
 			break;
 
-- 
2.30.2

Re: v5.13-rcX regression - NULL pointer dereference - MFD and software node API

[Dominik Brodowski]

Am Mon, Jun 21, 2021 at 01:37:59PM +0300 schrieb Heikki Krogerus:
> On Mon, Jun 21, 2021 at 01:00:06PM +0300, Andy Shevchenko wrote:
> > Can you, please, attach this to the bug report?
> > 
> > Long story here is that the device creation fails but we already have added
> > swnode to it. Meanwhile, device itself is not completely instantiated (yet)
> > and dev_name(dev) is NULL. The software_node_notify() is called with such
> > device and Oopses in the following line
> > 
> > 	sysfs_remove_link(&swnode->kobj, dev_name(dev));
> > 
> > My patch fixes another issue that might happen before this and in the code
> > that retrieves swnode itself in the device_remove_software_node().
> > 
> > Of course my patch won't fix this issue.
> > 
> > I have heard that Heikki is looking how to fix the issue in your case and
> > potentially in any other cases where device_add_software_node() is called
> > against not formed object instance.
> 
> Dominik, can you test the attached patch to confirm if this really is
> the case.

With this patch applied, the panic disappears.

Many thanks,
	Dominik

Re: v5.13-rcX regression - NULL pointer dereference - MFD and software node API

[Heikki Krogerus]

On Mon, Jun 21, 2021 at 05:31:50PM +0200, Dominik Brodowski wrote:
> Am Mon, Jun 21, 2021 at 01:37:59PM +0300 schrieb Heikki Krogerus:
> > On Mon, Jun 21, 2021 at 01:00:06PM +0300, Andy Shevchenko wrote:
> > > Can you, please, attach this to the bug report?
> > > 
> > > Long story here is that the device creation fails but we already have added
> > > swnode to it. Meanwhile, device itself is not completely instantiated (yet)
> > > and dev_name(dev) is NULL. The software_node_notify() is called with such
> > > device and Oopses in the following line
> > > 
> > > 	sysfs_remove_link(&swnode->kobj, dev_name(dev));
> > > 
> > > My patch fixes another issue that might happen before this and in the code
> > > that retrieves swnode itself in the device_remove_software_node().
> > > 
> > > Of course my patch won't fix this issue.
> > > 
> > > I have heard that Heikki is looking how to fix the issue in your case and
> > > potentially in any other cases where device_add_software_node() is called
> > > against not formed object instance.
> > 
> > Dominik, can you test the attached patch to confirm if this really is
> > the case.
> 
> With this patch applied, the panic disappears.

Thanks Dominik. I'll clean it and send it out today.

cheers,

-- 
heikki

Re: v5.13-rcX regression - NULL pointer dereference - MFD and software node API

[Heikki Krogerus]

[-- Attachment #1: Type: text/plain, Size: 1608 bytes --]

On Tue, Jun 22, 2021 at 11:09:58AM +0300, Heikki Krogerus wrote:
> On Mon, Jun 21, 2021 at 05:31:50PM +0200, Dominik Brodowski wrote:
> > Am Mon, Jun 21, 2021 at 01:37:59PM +0300 schrieb Heikki Krogerus:
> > > On Mon, Jun 21, 2021 at 01:00:06PM +0300, Andy Shevchenko wrote:
> > > > Can you, please, attach this to the bug report?
> > > > 
> > > > Long story here is that the device creation fails but we already have added
> > > > swnode to it. Meanwhile, device itself is not completely instantiated (yet)
> > > > and dev_name(dev) is NULL. The software_node_notify() is called with such
> > > > device and Oopses in the following line
> > > > 
> > > > 	sysfs_remove_link(&swnode->kobj, dev_name(dev));
> > > > 
> > > > My patch fixes another issue that might happen before this and in the code
> > > > that retrieves swnode itself in the device_remove_software_node().
> > > > 
> > > > Of course my patch won't fix this issue.
> > > > 
> > > > I have heard that Heikki is looking how to fix the issue in your case and
> > > > potentially in any other cases where device_add_software_node() is called
> > > > against not formed object instance.
> > > 
> > > Dominik, can you test the attached patch to confirm if this really is
> > > the case.
> > 
> > With this patch applied, the panic disappears.
> 
> Thanks Dominik. I'll clean it and send it out today.

Before I send the patch to Rafael and Greg, can you confirm that the
appropriate API (device_is_registered()) also works? I'm attaching
patch that should be the final version (if it works).

I'm sorry to bother you with this.

thanks,

-- 
heikki

[-- Attachment #2: 0001-software-node-Handle-software-node-injection-to-an-e.patch --]
[-- Type: text/plain, Size: 2242 bytes --]

From 9dcfc8e6bae658288fa6f112efc18246285f0f27 Mon Sep 17 00:00:00 2001
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Date: Mon, 21 Jun 2021 13:31:51 +0300
Subject: [PATCH] software node: Handle software node injection to an existing
 device properly

The function software_node_notify(), which creates and
removes the symlinks between the software node and the
device, must be called conditionally. In normal case
software_node_notify() is called automatically when the
device that the software node is assigned to is registered,
and only in the special cases where the software node has to
be added to an already existing device it needs to be called
separately.

This fixes NULL pointer dereference that happenes if
device_remove_software_node() is called with device that
was never registered.

Fixes: b622b24519f5 ("software node: Allow node addition to already existing device")
Reported-by: Dominik Brodowski <linux@dominikbrodowski.net>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/swnode.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 3cc11b813f28c..042eef31b182a 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -1045,7 +1045,16 @@ int device_add_software_node(struct device *dev, const struct software_node *nod
 	}
 
 	set_secondary_fwnode(dev, &swnode->fwnode);
-	software_node_notify(dev, KOBJ_ADD);
+
+	/*
+	 * Software nodes are also allowed to be added to already existing
+	 * devices. If the device has been fully registered by the time this
+	 * function is called, software_node_notify() must be called separately
+	 * so that the symlinks get created and the reference count of the node
+	 * is kept in balance.
+	 */
+	if (device_is_registered(dev))
+		software_node_notify(dev, KOBJ_ADD);
 
 	return 0;
 }
@@ -1065,7 +1074,8 @@ void device_remove_software_node(struct device *dev)
 	if (!swnode)
 		return;
 
-	software_node_notify(dev, KOBJ_REMOVE);
+	if (device_is_registered(dev))
+		software_node_notify(dev, KOBJ_REMOVE);
 	set_secondary_fwnode(dev, NULL);
 	kobject_put(&swnode->kobj);
 }
-- 
2.30.2

Re: v5.13-rcX regression - NULL pointer dereference - MFD and software node API

[Dominik Brodowski]

Am Tue, Jun 22, 2021 at 05:10:44PM +0300 schrieb Heikki Krogerus:
> On Tue, Jun 22, 2021 at 11:09:58AM +0300, Heikki Krogerus wrote:
> > On Mon, Jun 21, 2021 at 05:31:50PM +0200, Dominik Brodowski wrote:
> > > Am Mon, Jun 21, 2021 at 01:37:59PM +0300 schrieb Heikki Krogerus:
> > > > On Mon, Jun 21, 2021 at 01:00:06PM +0300, Andy Shevchenko wrote:
> > > > > Can you, please, attach this to the bug report?
> > > > > 
> > > > > Long story here is that the device creation fails but we already have added
> > > > > swnode to it. Meanwhile, device itself is not completely instantiated (yet)
> > > > > and dev_name(dev) is NULL. The software_node_notify() is called with such
> > > > > device and Oopses in the following line
> > > > > 
> > > > > 	sysfs_remove_link(&swnode->kobj, dev_name(dev));
> > > > > 
> > > > > My patch fixes another issue that might happen before this and in the code
> > > > > that retrieves swnode itself in the device_remove_software_node().
> > > > > 
> > > > > Of course my patch won't fix this issue.
> > > > > 
> > > > > I have heard that Heikki is looking how to fix the issue in your case and
> > > > > potentially in any other cases where device_add_software_node() is called
> > > > > against not formed object instance.
> > > > 
> > > > Dominik, can you test the attached patch to confirm if this really is
> > > > the case.
> > > 
> > > With this patch applied, the panic disappears.
> > 
> > Thanks Dominik. I'll clean it and send it out today.
> 
> Before I send the patch to Rafael and Greg, can you confirm that the
> appropriate API (device_is_registered()) also works? I'm attaching
> patch that should be the final version (if it works).

Heikki, that works perfectly. Many thanks, especially for the speedy
development of the bugfix!

Best,
	Dominik

Re: v5.13-rcX regression - NULL pointer dereference - MFD and software node API

[Andy Shevchenko]

On Tue, Jun 22, 2021 at 5:37 PM Dominik Brodowski
<linux@dominikbrodowski.net> wrote:
> Am Tue, Jun 22, 2021 at 05:10:44PM +0300 schrieb Heikki Krogerus:
> > On Tue, Jun 22, 2021 at 11:09:58AM +0300, Heikki Krogerus wrote:

...

> > Before I send the patch to Rafael and Greg, can you confirm that the
> > appropriate API (device_is_registered()) also works? I'm attaching
> > patch that should be the final version (if it works).
>
> Heikki, that works perfectly. Many thanks, especially for the speedy
> development of the bugfix!

May we transform this to the Tested-by tag?

-- 
With Best Regards,
Andy Shevchenko

Re: v5.13-rcX regression - NULL pointer dereference - MFD and software node API

[Dominik Brodowski]

Am Tue, Jun 22, 2021 at 06:28:41PM +0300 schrieb Andy Shevchenko:
> On Tue, Jun 22, 2021 at 5:37 PM Dominik Brodowski
> <linux@dominikbrodowski.net> wrote:
> > Am Tue, Jun 22, 2021 at 05:10:44PM +0300 schrieb Heikki Krogerus:
> > > On Tue, Jun 22, 2021 at 11:09:58AM +0300, Heikki Krogerus wrote:
> 
> ...
> 
> > > Before I send the patch to Rafael and Greg, can you confirm that the
> > > appropriate API (device_is_registered()) also works? I'm attaching
> > > patch that should be the final version (if it works).
> >
> > Heikki, that works perfectly. Many thanks, especially for the speedy
> > development of the bugfix!
> 
> May we transform this to the Tested-by tag?

Indeed:

	Reported-and-tested-by: Dominik Brodowski <linux@dominikbrodowski.net>

Thanks,
	Dominik

Re: v5.13-rcX regression - NULL pointer dereference - MFD and software node API

[Andy Shevchenko]

On Tue, Jun 22, 2021 at 5:10 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
> On Tue, Jun 22, 2021 at 11:09:58AM +0300, Heikki Krogerus wrote:

...

> Before I send the patch to Rafael and Greg, can you confirm that the
> appropriate API (device_is_registered()) also works? I'm attaching
> patch that should be the final version (if it works).
>
> I'm sorry to bother you with this.

In the commit message: happenes -> happens.

And you may replace my Cc to the Reviewed-by tag.

Thanks for your help!

-- 
With Best Regards,
Andy Shevchenko

Re: v5.13-rcX regression - NULL pointer dereference - MFD and software node API

[Andy Shevchenko]

On Tue, Jun 22, 2021 at 6:28 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Jun 22, 2021 at 5:10 PM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> > On Tue, Jun 22, 2021 at 11:09:58AM +0300, Heikki Krogerus wrote:
>
> ...
>
> > Before I send the patch to Rafael and Greg, can you confirm that the
> > appropriate API (device_is_registered()) also works? I'm attaching
> > patch that should be the final version (if it works).
> >
> > I'm sorry to bother you with this.
>
> In the commit message: happenes -> happens.
>
> And you may replace my Cc to the Reviewed-by tag.
>
> Thanks for your help!

One more detail, shouldn't we drop _nowarn as well?

-- 
With Best Regards,
Andy Shevchenko

Re: v5.13-rcX regression - NULL pointer dereference - MFD and software node API

[Heikki Krogerus]

On Tue, Jun 22, 2021 at 06:30:19PM +0300, Andy Shevchenko wrote:
> On Tue, Jun 22, 2021 at 6:28 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Tue, Jun 22, 2021 at 5:10 PM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > > On Tue, Jun 22, 2021 at 11:09:58AM +0300, Heikki Krogerus wrote:
> >
> > ...
> >
> > > Before I send the patch to Rafael and Greg, can you confirm that the
> > > appropriate API (device_is_registered()) also works? I'm attaching
> > > patch that should be the final version (if it works).
> > >
> > > I'm sorry to bother you with this.
> >
> > In the commit message: happenes -> happens.
> >
> > And you may replace my Cc to the Reviewed-by tag.
> >
> > Thanks for your help!
> 
> One more detail, shouldn't we drop _nowarn as well?

OK. I'll fix those and send the patch to Greg and Rafael.

thanks,,

-- 
heikki

Re: v5.13-rcX regression - NULL pointer dereference - MFD and software node API

[Andy Shevchenko]

-Lee, Hekki
+Hans de Goede, Rafael, Laurent, Sakari

Below bug report again reminds me about the "interesting" design of
some ACPI tables in the wild. See below for more.

On Sun, Jun 20, 2021 at 11:36 AM Dominik Brodowski
<linux@dominikbrodowski.net> wrote:
>
> Over a month ago, Andy Shevchenko reported and fixed a NULL pointer
> dereference issue introduced by commit
>         42e59982917a ("mfd: core: Add support for software nodes")
> in v5.13-rc1:
>         https://lore.kernel.org/lkml/20210510141552.57045-1-andriy.shevchenko@linux.intel.com/
>
> A bisect shows that it is indeed commit 42e59982917a which causes boot to
> fail due to a NULL pointer dereference on my work laptop, where "intel-lpss"
> is bound to
>         00:15.0 Signal processing controller: Intel Corporation Sunrise Point-LP Serial IO I2C Controller #0 (rev 21)
> and fails to bind to INT3446:
>
> [    6.048087] intel-lpss 0000:00:15.0: enabling device (0000 -> 0002)
> [    6.050625] idma64 idma64.0: Found Intel integrated DMA 64-bit
> [    6.109112] intel-lpss 0000:00:15.1: enabling device (0000 -> 0002)
> [    6.111348] idma64 idma64.1: Found Intel integrated DMA 64-bit
> [    6.172229] intel-lpss 0000:00:15.2: enabling device (0000 -> 0002)
> [    6.174353] idma64 idma64.2: Found Intel integrated DMA 64-bit
> [    6.231865] intel-lpss 0000:00:15.3: enabling device (0000 -> 0002)
> [    6.233845] idma64 idma64.3: Found Intel integrated DMA 64-bit

> [    6.287492] ACPI Warning: SystemMemory range 0x00000000FE028000-0x00000000FE0281FF conflicts with OpRegion 0x00000000FE028000-0x00000000FE028207 (\_SB.PCI0.GEXP.BAR0) (20210331/utaddress-204)
> [    6.287704] ACPI: OSL: Resource conflict; ACPI support missing from driver?
> [    6.289760] intel-lpss: probe of INT3446:00 failed with error -16

Above lines appear due to the following:
1. In hardware we have an I²C bus with GPIO I/O expander (PCA953x
compatible in all cases I saw so far).
2. The ACPI table has a "real" driver for that expander written in ASL (sic!).
3. Due to the above we have I²C controller resources and that driver
in a conflict, because the driver uses I²C controller registers
directly (sic!).

The questions for brainstorming the ideas here are:
1. Is it possible to blacklist the Device Node from being evaluated /
parsed based on the ACPI DSDT path (the "real driver", of course,
doesn't have any other means to be enumerated properly)?
2. Can we create the ACPI driver in the OS which will take the ACPI
path as ID for enumeration / instantiation?
3. Is it possible to hook up on the methods, so we will know what to
do when a certain method is called (like setting pin direction or so)?
4. Does above make any sense or we simply mark that hardware as broken
(i.e. Windoze-only bad desing) and that's it?

> Unfortunately, the patch by Andy Shevchenko (applied on top of Linus' tree)
> does not fix the issue. A complete revert, however, does fix the issue, and
> allows my laptop to boot again.
>
> In my opinion, it is unfortunate that although it has been known for over a
> month that commit 42e59982917a is broken, the bugfix (though probably not
> far-reaching enough) has not yet progressed upstream.



-- 
With Best Regards,
Andy Shevchenko

Re: v5.13-rcX regression - NULL pointer dereference - MFD and software node API

[Laurent Pinchart]

Hi Andy,

On Tue, Jun 22, 2021 at 12:35:46PM +0300, Andy Shevchenko wrote:
> -Lee, Hekki
> +Hans de Goede, Rafael, Laurent, Sakari
> 
> Below bug report again reminds me about the "interesting" design of
> some ACPI tables in the wild. See below for more.
> 
> On Sun, Jun 20, 2021 at 11:36 AM Dominik Brodowski wrote:
> >
> > Over a month ago, Andy Shevchenko reported and fixed a NULL pointer
> > dereference issue introduced by commit
> >         42e59982917a ("mfd: core: Add support for software nodes")
> > in v5.13-rc1:
> >         https://lore.kernel.org/lkml/20210510141552.57045-1-andriy.shevchenko@linux.intel.com/
> >
> > A bisect shows that it is indeed commit 42e59982917a which causes boot to
> > fail due to a NULL pointer dereference on my work laptop, where "intel-lpss"
> > is bound to
> >         00:15.0 Signal processing controller: Intel Corporation Sunrise Point-LP Serial IO I2C Controller #0 (rev 21)
> > and fails to bind to INT3446:
> >
> > [    6.048087] intel-lpss 0000:00:15.0: enabling device (0000 -> 0002)
> > [    6.050625] idma64 idma64.0: Found Intel integrated DMA 64-bit
> > [    6.109112] intel-lpss 0000:00:15.1: enabling device (0000 -> 0002)
> > [    6.111348] idma64 idma64.1: Found Intel integrated DMA 64-bit
> > [    6.172229] intel-lpss 0000:00:15.2: enabling device (0000 -> 0002)
> > [    6.174353] idma64 idma64.2: Found Intel integrated DMA 64-bit
> > [    6.231865] intel-lpss 0000:00:15.3: enabling device (0000 -> 0002)
> > [    6.233845] idma64 idma64.3: Found Intel integrated DMA 64-bit
> 
> > [    6.287492] ACPI Warning: SystemMemory range 0x00000000FE028000-0x00000000FE0281FF conflicts with OpRegion 0x00000000FE028000-0x00000000FE028207 (\_SB.PCI0.GEXP.BAR0) (20210331/utaddress-204)
> > [    6.287704] ACPI: OSL: Resource conflict; ACPI support missing from driver?
> > [    6.289760] intel-lpss: probe of INT3446:00 failed with error -16

For the people who haven't followed this, https://lore.kernel.org/linux-acpi/20210112113538.GF4077@smile.fi.intel.com/

> Above lines appear due to the following:
> 1. In hardware we have an I²C bus with GPIO I/O expander (PCA953x
> compatible in all cases I saw so far).
> 2. The ACPI table has a "real" driver for that expander written in ASL (sic!).
> 3. Due to the above we have I²C controller resources and that driver
> in a conflict, because the driver uses I²C controller registers
> directly (sic!).
> 
> The questions for brainstorming the ideas here are:
> 1. Is it possible to blacklist the Device Node from being evaluated /
> parsed based on the ACPI DSDT path (the "real driver", of course,
> doesn't have any other means to be enumerated properly)?
> 2. Can we create the ACPI driver in the OS which will take the ACPI
> path as ID for enumeration / instantiation?
> 3. Is it possible to hook up on the methods, so we will know what to
> do when a certain method is called (like setting pin direction or so)?
> 4. Does above make any sense or we simply mark that hardware as broken
> (i.e. Windoze-only bad desing) and that's it?

The first question we need to answer is whether there's an actual
hardware conflict or not. Can the PCA953x be accessed by the ASL driver
at any point, or do those accesses only occur at early boot time or at
reboot time for instance ? If there's an actual risk of conflict at
runtime, we need to find out how this was designed to work on Windows.

> > Unfortunately, the patch by Andy Shevchenko (applied on top of Linus' tree)
> > does not fix the issue. A complete revert, however, does fix the issue, and
> > allows my laptop to boot again.
> >
> > In my opinion, it is unfortunate that although it has been known for over a
> > month that commit 42e59982917a is broken, the bugfix (though probably not
> > far-reaching enough) has not yet progressed upstream.

-- 
Regards,

Laurent Pinchart

Re: v5.13-rcX regression - NULL pointer dereference - MFD and software node API

[Andy Shevchenko]

On Tue, Jun 22, 2021 at 12:48:18PM +0300, Laurent Pinchart wrote:
> On Tue, Jun 22, 2021 at 12:35:46PM +0300, Andy Shevchenko wrote:

> > Below bug report again reminds me about the "interesting" design of
> > some ACPI tables in the wild. See below for more.
> > 
> > On Sun, Jun 20, 2021 at 11:36 AM Dominik Brodowski wrote:
> > >
> > > Over a month ago, Andy Shevchenko reported and fixed a NULL pointer
> > > dereference issue introduced by commit
> > >         42e59982917a ("mfd: core: Add support for software nodes")
> > > in v5.13-rc1:
> > >         https://lore.kernel.org/lkml/20210510141552.57045-1-andriy.shevchenko@linux.intel.com/
> > >
> > > A bisect shows that it is indeed commit 42e59982917a which causes boot to
> > > fail due to a NULL pointer dereference on my work laptop, where "intel-lpss"
> > > is bound to
> > >         00:15.0 Signal processing controller: Intel Corporation Sunrise Point-LP Serial IO I2C Controller #0 (rev 21)
> > > and fails to bind to INT3446:
> > >
> > > [    6.048087] intel-lpss 0000:00:15.0: enabling device (0000 -> 0002)
> > > [    6.050625] idma64 idma64.0: Found Intel integrated DMA 64-bit
> > > [    6.109112] intel-lpss 0000:00:15.1: enabling device (0000 -> 0002)
> > > [    6.111348] idma64 idma64.1: Found Intel integrated DMA 64-bit
> > > [    6.172229] intel-lpss 0000:00:15.2: enabling device (0000 -> 0002)
> > > [    6.174353] idma64 idma64.2: Found Intel integrated DMA 64-bit
> > > [    6.231865] intel-lpss 0000:00:15.3: enabling device (0000 -> 0002)
> > > [    6.233845] idma64 idma64.3: Found Intel integrated DMA 64-bit
> > 
> > > [    6.287492] ACPI Warning: SystemMemory range 0x00000000FE028000-0x00000000FE0281FF conflicts with OpRegion 0x00000000FE028000-0x00000000FE028207 (\_SB.PCI0.GEXP.BAR0) (20210331/utaddress-204)
> > > [    6.287704] ACPI: OSL: Resource conflict; ACPI support missing from driver?
> > > [    6.289760] intel-lpss: probe of INT3446:00 failed with error -16
> 
> For the people who haven't followed this, https://lore.kernel.org/linux-acpi/20210112113538.GF4077@smile.fi.intel.com/

Thanks!

> > Above lines appear due to the following:
> > 1. In hardware we have an I²C bus with GPIO I/O expander (PCA953x
> > compatible in all cases I saw so far).
> > 2. The ACPI table has a "real" driver for that expander written in ASL (sic!).
> > 3. Due to the above we have I²C controller resources and that driver
> > in a conflict, because the driver uses I²C controller registers
> > directly (sic!).
> > 
> > The questions for brainstorming the ideas here are:
> > 1. Is it possible to blacklist the Device Node from being evaluated /
> > parsed based on the ACPI DSDT path (the "real driver", of course,
> > doesn't have any other means to be enumerated properly)?
> > 2. Can we create the ACPI driver in the OS which will take the ACPI
> > path as ID for enumeration / instantiation?
> > 3. Is it possible to hook up on the methods, so we will know what to
> > do when a certain method is called (like setting pin direction or so)?
> > 4. Does above make any sense or we simply mark that hardware as broken
> > (i.e. Windoze-only bad desing) and that's it?
> 
> The first question we need to answer is whether there's an actual
> hardware conflict or not. Can the PCA953x be accessed by the ASL driver
> at any point, or do those accesses only occur at early boot time or at
> reboot time for instance ? If there's an actual risk of conflict at
> runtime, we need to find out how this was designed to work on Windows.

As long as there the only I²C slave is connected (AFAIK not always the case),
there is no conflict and we simply may hide the entire controller from the OS.

Otherwise it is easy to answer, yes, and this is one of the ugliest conflicts,
because the driver in ASL touches I²C controller registers directly (messing up
completely with what OS thinks the state of the registers is at any point of
time of the certain I²C controller).

> > > Unfortunately, the patch by Andy Shevchenko (applied on top of Linus' tree)
> > > does not fix the issue. A complete revert, however, does fix the issue, and
> > > allows my laptop to boot again.
> > >
> > > In my opinion, it is unfortunate that although it has been known for over a
> > > month that commit 42e59982917a is broken, the bugfix (though probably not
> > > far-reaching enough) has not yet progressed upstream.

-- 
With Best Regards,
Andy Shevchenko

Re: v5.13-rcX regression - NULL pointer dereference - MFD and software node API

[Laurent Pinchart]

On Tue, Jun 22, 2021 at 01:23:26PM +0300, Andy Shevchenko wrote:
> On Tue, Jun 22, 2021 at 12:48:18PM +0300, Laurent Pinchart wrote:
> > On Tue, Jun 22, 2021 at 12:35:46PM +0300, Andy Shevchenko wrote:
> 
> > > Below bug report again reminds me about the "interesting" design of
> > > some ACPI tables in the wild. See below for more.
> > > 
> > > On Sun, Jun 20, 2021 at 11:36 AM Dominik Brodowski wrote:
> > > >
> > > > Over a month ago, Andy Shevchenko reported and fixed a NULL pointer
> > > > dereference issue introduced by commit
> > > >         42e59982917a ("mfd: core: Add support for software nodes")
> > > > in v5.13-rc1:
> > > >         https://lore.kernel.org/lkml/20210510141552.57045-1-andriy.shevchenko@linux.intel.com/
> > > >
> > > > A bisect shows that it is indeed commit 42e59982917a which causes boot to
> > > > fail due to a NULL pointer dereference on my work laptop, where "intel-lpss"
> > > > is bound to
> > > >         00:15.0 Signal processing controller: Intel Corporation Sunrise Point-LP Serial IO I2C Controller #0 (rev 21)
> > > > and fails to bind to INT3446:
> > > >
> > > > [    6.048087] intel-lpss 0000:00:15.0: enabling device (0000 -> 0002)
> > > > [    6.050625] idma64 idma64.0: Found Intel integrated DMA 64-bit
> > > > [    6.109112] intel-lpss 0000:00:15.1: enabling device (0000 -> 0002)
> > > > [    6.111348] idma64 idma64.1: Found Intel integrated DMA 64-bit
> > > > [    6.172229] intel-lpss 0000:00:15.2: enabling device (0000 -> 0002)
> > > > [    6.174353] idma64 idma64.2: Found Intel integrated DMA 64-bit
> > > > [    6.231865] intel-lpss 0000:00:15.3: enabling device (0000 -> 0002)
> > > > [    6.233845] idma64 idma64.3: Found Intel integrated DMA 64-bit
> > > 
> > > > [    6.287492] ACPI Warning: SystemMemory range 0x00000000FE028000-0x00000000FE0281FF conflicts with OpRegion 0x00000000FE028000-0x00000000FE028207 (\_SB.PCI0.GEXP.BAR0) (20210331/utaddress-204)
> > > > [    6.287704] ACPI: OSL: Resource conflict; ACPI support missing from driver?
> > > > [    6.289760] intel-lpss: probe of INT3446:00 failed with error -16
> > 
> > For the people who haven't followed this, https://lore.kernel.org/linux-acpi/20210112113538.GF4077@smile.fi.intel.com/
> 
> Thanks!
> 
> > > Above lines appear due to the following:
> > > 1. In hardware we have an I²C bus with GPIO I/O expander (PCA953x
> > > compatible in all cases I saw so far).
> > > 2. The ACPI table has a "real" driver for that expander written in ASL (sic!).
> > > 3. Due to the above we have I²C controller resources and that driver
> > > in a conflict, because the driver uses I²C controller registers
> > > directly (sic!).
> > > 
> > > The questions for brainstorming the ideas here are:
> > > 1. Is it possible to blacklist the Device Node from being evaluated /
> > > parsed based on the ACPI DSDT path (the "real driver", of course,
> > > doesn't have any other means to be enumerated properly)?
> > > 2. Can we create the ACPI driver in the OS which will take the ACPI
> > > path as ID for enumeration / instantiation?
> > > 3. Is it possible to hook up on the methods, so we will know what to
> > > do when a certain method is called (like setting pin direction or so)?
> > > 4. Does above make any sense or we simply mark that hardware as broken
> > > (i.e. Windoze-only bad desing) and that's it?
> > 
> > The first question we need to answer is whether there's an actual
> > hardware conflict or not. Can the PCA953x be accessed by the ASL driver
> > at any point, or do those accesses only occur at early boot time or at
> > reboot time for instance ? If there's an actual risk of conflict at
> > runtime, we need to find out how this was designed to work on Windows.
> 
> As long as there the only I²C slave is connected (AFAIK not always the case),
> there is no conflict and we simply may hide the entire controller from the OS.

Many machines have other I2C devices on the same bus. This is the case
of the Skylake- and Kabylake-based machines with CSI-2 camera sensors
designed for Windows (The Surface Go 2 is an example). We need to ensure
there won't be a regression for users of those machines (they currently
set acpi_enforce_resources=lax).

> > > > Unfortunately, the patch by Andy Shevchenko (applied on top of Linus' tree)
> 
> Otherwise it is easy to answer, yes, and this is one of the ugliest conflicts,
> because the driver in ASL touches I²C controller registers directly (messing up
> completely with what OS thinks the state of the registers is at any point of
> time of the certain I²C controller).

Sure, but this depends on whether the conflict can actually occur in
practice. If the ASL I2C driver is only used at early boot time or
reboot time, at times when no I2C traffic can be initiated from Linux,
then we're safe.

If a conflict can actually occur, we have to figure out how this was
designed to work. One option I've been thinking about is to trap
accesses to the I2C controller by the ASL driver (by remapping the BAR
?) and emulate the I2C controller behaviour with proper arbitration.
It's ugly, will require lots of work, please feel free to blame whoever
designed thus at Intel :-)

> > > > does not fix the issue. A complete revert, however, does fix the issue, and
> > > > allows my laptop to boot again.
> > > >
> > > > In my opinion, it is unfortunate that although it has been known for over a
> > > > month that commit 42e59982917a is broken, the bugfix (though probably not
> > > > far-reaching enough) has not yet progressed upstream.

-- 
Regards,

Laurent Pinchart