Message-ID: <98beaa03-6eb5-c6e3-411d-7e76fb8f0936@huawei.com>
Date:   Tue, 30 Aug 2022 11:06:03 +0800
From:   "Leizhen (ThunderTown)" <thunder.leizhen@...wei.com>
To:     Linus Walleij <linus.walleij@...aro.org>
CC:     Russell King <linux@...linux.org.uk>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <patches@...linux.org.uk>,
        Saravana Kannan <saravanak@...gle.com>,
        "Kefeng Wang" <wangkefeng.wang@...wei.com>
Subject: Re: [PATCH] ARM: 9220/1: amba: Add sanity check for dev->periphid in
 amba_probe()



On 2022/8/30 5:00, Linus Walleij wrote:
> On Sat, Aug 27, 2022 at 12:22 PM Zhen Lei <thunder.leizhen@...wei.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@...wei.com>
> 
> For the record, this makes Vexpress QEMU boot again for me, so
> Tested-by: Linus Walleij <linus.walleij@...aro.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@...aro.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@...gle.com>
> Cc: Sudeep Holla <sudeep.holla@....com>
> Cc: qemu-devel@...gnu.org
> Cc: Peter Maydell <peter.maydell@...aro.org>
> Signed-off-by: Linus Walleij <linus.walleij@...aro.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

Powered by blists - more mailing lists