lkml.org 
[lkml]   [2022]   [Aug]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH] ARM: 9220/1: amba: Add sanity check for dev->periphid in amba_probe()
From
Date


On 2022/8/30 5:00, Linus Walleij wrote:
> On Sat, Aug 27, 2022 at 12:22 PM Zhen Lei <thunder.leizhen@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@huawei.com>
>
> For the record, this makes Vexpress QEMU boot again for me, so
> Tested-by: Linus Walleij <linus.walleij@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:

Also on QEMU. But your conclusion may be wrong, The problem on QEMU is really
because of pl031 is not ready. With my patch, pl031_probe() is called, so that
its ID is implemented on QEMU.

+++ b/drivers/base/driver.c
@@ -217,6 +217,9 @@ int driver_register(struct device_driver *drv)
{
int ret;
struct device_driver *other;
+ static int seq;
+
+ printk("%d: drv=%s, bus=%s\n", ++seq, drv->name, drv->bus->name);

Add the above print, we can see:
50: drv=rtc-pl031, bus=amba
51: drv=vexpress-reset, bus=platform

I didn't go to further verify. I think the driver vexpress-reset makes the
clock available. After that, rtc-pl031's periphid can be read successfully.

>
>>From 561e9c5a80183223613a2cfe225a900dd28c1cd5 Mon Sep 17 00:00:00 2001
> From: Linus Walleij <linus.walleij@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@google.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: qemu-devel@nongnu.org
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Linus Walleij <linus.walleij@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);
>

--
Regards,
Zhen Lei

\
 
 \ /
  Last update: 2022-08-30 05:07    [W:0.051 / U:0.920 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site