linux-kernel - Re: [PATCH v2] soc: imx8m: Probe the SoC driver as platform driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <3aaa4265-dfd5-47e3-9cfc-ca3fa649f425@denx.de>
Date: Thu, 26 Sep 2024 23:35:54 +0200
From: Marek Vasut <marex@...x.de>
To: Saravana Kannan <saravanak@...gle.com>
Cc: linux-arm-kernel@...ts.infradead.org, kernel@...electronics.com,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 Arnd Bergmann <arnd@...db.de>, Fabio Estevam <festevam@...il.com>,
 Jeff Johnson <quic_jjohnson@...cinc.com>,
 Neil Armstrong <neil.armstrong@...aro.org>,
 Pengutronix Kernel Team <kernel@...gutronix.de>,
 Sascha Hauer <s.hauer@...gutronix.de>, Shawn Guo <shawnguo@...nel.org>,
 imx@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] soc: imx8m: Probe the SoC driver as platform driver

On 9/26/24 1:03 AM, Saravana Kannan wrote:
> On Wed, Sep 25, 2024 at 3:06 PM Marek Vasut <marex@...x.de> wrote:
>>
>> With driver_async_probe=* on kernel command line, the following trace is
>> produced because on i.MX8M Plus hardware because the soc-imx8m.c driver
>> calls of_clk_get_by_name() which returns -EPROBE_DEFER because the clock
>> driver is not yet probed. This was not detected during regular testing
>> without driver_async_probe.
>>
>> Convert the SoC code to platform driver and instantiate a platform device
>> in its current device_initcall() to probe the platform driver. Rework
>> .soc_revision callback to always return valid error code and return SoC
>> revision via parameter. This way, if anything in the .soc_revision callback
>> return -EPROBE_DEFER, it gets propagated to .probe and the .probe will get
>> retried later.
> 
> I'm assuming you tested this patch and it works?

I tested it both with and without driver_async_probe=*

> Did you see any other
> issues with driver_async_probe=* ?

Nope, just this one.

> Does this improve your probe/boot time? Some stats on that would be nice.

It does improve the boot time from 4.5 to 2.9 seconds (measured by 
looking at the kernel log, so imprecise, but noticeable). With 'quiet' 
on kernel command line, boot time drops from 2.99s to 2.34s .

>> "
>> ------------[ cut here ]------------
>> WARNING: CPU: 1 PID: 1 at drivers/soc/imx/soc-imx8m.c:115 imx8mm_soc_revision+0xdc/0x180
>> CPU: 1 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.11.0-next-20240924-00002-g2062bb554dea #603
>> Hardware name: DH electronics i.MX8M Plus DHCOM Premium Developer Kit (3) (DT)
>> pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>> pc : imx8mm_soc_revision+0xdc/0x180
>> lr : imx8mm_soc_revision+0xd0/0x180
>> sp : ffff8000821fbcc0
>> x29: ffff8000821fbce0 x28: 0000000000000000 x27: ffff800081810120
>> x26: ffff8000818a9970 x25: 0000000000000006 x24: 0000000000824311
>> x23: ffff8000817f42c8 x22: ffff0000df8be210 x21: fffffffffffffdfb
>> x20: ffff800082780000 x19: 0000000000000001 x18: ffffffffffffffff
>> x17: ffff800081fff418 x16: ffff8000823e1000 x15: ffff0000c03b65e8
>> x14: ffff0000c00051b0 x13: ffff800082790000 x12: 0000000000000801
>> x11: ffff80008278ffff x10: ffff80008209d3a6 x9 : ffff80008062e95c
>> x8 : ffff8000821fb9a0 x7 : 0000000000000000 x6 : 00000000000080e3
>> x5 : ffff0000df8c03d8 x4 : 0000000000000000 x3 : 0000000000000000
>> x2 : 0000000000000000 x1 : fffffffffffffdfb x0 : fffffffffffffdfb
>> Call trace:
>>   imx8mm_soc_revision+0xdc/0x180
>>   imx8_soc_init+0xb0/0x1e0
>>   do_one_initcall+0x94/0x1a8
>>   kernel_init_freeable+0x240/0x2a8
>>   kernel_init+0x28/0x140
>>   ret_from_fork+0x10/0x20
>> ---[ end trace 0000000000000000 ]---
>> SoC: i.MX8MP revision 1.1
>> "

[...]

>> +static struct platform_driver imx8m_soc_driver = {
>> +       .probe = imx8m_soc_probe,
>> +       .driver = {
>> +               .name = "imx8m-soc",
>> +       },
>> +};
>> +module_platform_driver(imx8m_soc_driver);
> 
> This just translates to a module_init() when compiled as a module.
> 
>>
>> +
>> +static int __init imx8_soc_init(void)
>> +{
>> +       struct platform_device *pdev;
>> +
>> +       pdev = platform_device_register_simple("imx8m-soc", -1, NULL, 0);
>> +
>> +       return IS_ERR(pdev) ? PTR_ERR(pdev) : 0;
>> +}
>>   device_initcall(imx8_soc_init);
> 
> This also translates to a module_init() when compiled as a module.
> 
> I've never seen a module with two module_init()s and I'm pretty sure
> that doesn't work. I'm guessing this driver doesn't support tristate
> in its current state. But with this patch, it should work as a module
> too. Why not add support for that too?

I am not entirely sure whether there are no dependencies on this soc 
driver, so let's start with builtin .

> Why not just do both of these things in one initcall?
> platform_create_bundle() doesn't work with deferred probing though. So
> just do one initcall that adds the device and registers the platform
> driver.

Fixed in V3.

[...]