[PATCH] ARM: 9220/1: amba: Add sanity check for dev->periphid in amba_probe()

[Linus Walleij]

On Sat, Aug 27, 2022 at 12:22 PM Zhen Lei <thunder.leizhen at huawei.com> wrote:

> Commit f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
> forcibly invokes device_add() even if dev->periphid is not ready. Although
> it will be remedied in amba_match(): dev->periphid will be initialized
> if everything is in place; Otherwise, return -EPROBE_DEFER to block
> __driver_attach() from further execution. But not all drivers have .match
> hook, such as pl031, the dev->bus->probe will be called directly in
> __driver_attach(). Unfortunately, if dev->periphid is still not
> initialized, the following exception will be triggered.
>
> 8<--- cut here ---
> Unable to handle kernel NULL pointer dereference at virtual address 00000008
> [00000008] *pgd=00000000
> Internal error: Oops: 5 [#1] SMP ARM
> Modules linked in:
> CPU: 1 PID: 1 Comm: swapper/0 Not tainted 6.0.0-rc2+ #7
> Hardware name: ARM-Versatile Express
> PC is at pl031_probe+0x8/0x208
> LR is at amba_probe+0xf0/0x160
> pc : 80698df8  lr : 8050eb54  psr: 80000013
> sp : c0825df8  ip : 00000000  fp : 811fda38
> r10: 00000000  r9 : 80d72470  r8 : fffffdfb
> r7 : 811fd800  r6 : be7eb330  r5 : 00000000  r4 : 811fd900
> r3 : 80698df0  r2 : 37000000  r1 : 00000000  r0 : 811fd800
> Flags: Nzcv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> Control: 10c5387d  Table: 6000406a  DAC: 00000051
> ... ...
>  pl031_probe from amba_probe+0xf0/0x160
>  amba_probe from really_probe+0x118/0x290
>  really_probe from __driver_probe_device+0x84/0xe4
>  __driver_probe_device from driver_probe_device+0x30/0xd0
>  driver_probe_device from __driver_attach+0x8c/0xfc
>  __driver_attach from bus_for_each_dev+0x70/0xb0
>  bus_for_each_dev from bus_add_driver+0x168/0x1f4
>  bus_add_driver from driver_register+0x7c/0x118
>  driver_register from do_one_initcall+0x44/0x1ec
>  do_one_initcall from kernel_init_freeable+0x238/0x288
>  kernel_init_freeable from kernel_init+0x18/0x12c
>  kernel_init from ret_from_fork+0x14/0x2c
> ... ...
> ---[ end trace 0000000000000000 ]---
>
> Therefore, take the same action as in amba_match(): return -EPROBE_DEFER
> if dev->periphid is not ready in amba_probe().
>
> Fixes: f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
> Signed-off-by: Zhen Lei <thunder.leizhen at huawei.com>

For the record, this makes Vexpress QEMU boot again for me, so
Tested-by: Linus Walleij <linus.walleij at linaro.org>

The problem on QEMU is however not that the device is "not ready"
the device is simply not there, the implementation in QEMU
is incomplete. What platform are you getting this on? If it's QEMU
you probably need to alter the commit message. I made this similar patch:

>From 561e9c5a80183223613a2cfe225a900dd28c1cd5 Mon Sep 17 00:00:00 2001
From: Linus Walleij <linus.walleij at linaro.org>
Date: Mon, 29 Aug 2022 22:26:11 +0200
Subject: [PATCH] amba: Don't probe devices without valid match ID

After the recent changes to the AMBA bus core, QEMU Versatile
Express (and probably other machines) refuse to boot properly
in the v6.0-rc1 thru -rc3 kernels.

After enabling earlydebug this kind of stuff comes out:

Unable to handle kernel NULL pointer dereference at virtual address 00000008
[00000008] *pgd=00000000
Internal error: Oops: 5 [#1] SMP ARM
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.0.0-rc1+ #474
Hardware name: ARM-Versatile Express
PC is at pl031_probe+0x18/0x224
LR is at amba_probe+0xf0/0x174

This is because the AMBA driver probe call is called with an id
argument that is NULL while drivers often directly dereference
this ID to get to the match data.

This happens when an AMBA device exist in the device tree, but
it is lacking any PrimceCell ID in hardware so there is no match
data.

This happens most typically in QEMU which tries to mimic e.g.
Versatile Express, but several PrimeCells that exist in the
physical platform are not implemented or incomplete in the
QEMU model so only zeroes are returned when reading the PrimeCell
ID and thus there is no match data.

Fix this by not probing the device if there is no ID.

This print from QEMU after applying the patch shows clearly
what blocks are missing from QEMU:

1c0f0000.watchdog: no ID for device, skipping probe
  (QEMU incomplete device?)
2b0a0000.memory-controller: no ID for device, skipping probe
  (QEMU incomplete device?)
7ffd0000.memory-controller: no ID for device, skipping probe
  (QEMU incomplete device?)
7ffb0000.dma: no ID for device, skipping probe
  (QEMU incomplete device?)

Fixes: f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
Cc: Saravana Kannan <saravanak at google.com>
Cc: Sudeep Holla <sudeep.holla at arm.com>
Cc: qemu-devel at nongnu.org
Cc: Peter Maydell <peter.maydell at linaro.org>
Signed-off-by: Linus Walleij <linus.walleij at linaro.org>
---
 drivers/amba/bus.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 32b0e0b930c1..6a1bffc60169 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -300,9 +300,14 @@ static int amba_probe(struct device *dev)
         pm_runtime_set_active(dev);
         pm_runtime_enable(dev);

-        ret = pcdrv->probe(pcdev, id);
-        if (ret == 0)
-            break;
+        if (!id) {
+            pr_err("%s: no ID for device, skipping probe (QEMU
incomplete device?)\n",
+                   dev_name(dev));
+        } else {
+            ret = pcdrv->probe(pcdev, id);
+            if (ret == 0)
+                break;
+        }

         pm_runtime_disable(dev);
         pm_runtime_set_suspended(dev);
-- 
2.37.2

Yours,
Linus Walleij