linux-kernel - Re: [PATCH v3 1/2] 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 for Android: free password hash cracker in your pocket

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

Message-ID: <e32ed329-b012-44f2-854b-80a8b0efc4e6@denx.de>
Date: Fri, 27 Sep 2024 23:42:41 +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 v3 1/2] soc: imx8m: Probe the SoC driver as platform
 driver

On 9/27/24 1:39 AM, Saravana Kannan wrote:

[...]

>> +static int imx8mq_soc_revision(u32 *socrev)
>>   {
>>          struct device_node *np;
>>          void __iomem *ocotp_base;
>>          u32 magic;
>>          u32 rev;
>>          struct clk *clk;
>> +       int ret;
>>
>>          np = of_find_compatible_node(NULL, NULL, "fsl,imx8mq-ocotp");
>>          if (!np)
>> -               return 0;
>> +               return -EINVAL;
>>
>>          ocotp_base = of_iomap(np, 0);
> 
> Using devm_of_iomap() and scoped "whatever it's called" might help
> simplify the error handling.
> 
> So something like this for np:
> struct device_node *np __free(device_node) = np =
> of_find_compatible_node(NULL, NULL, "fsl,imx8mq-ocotp");
> 
> And this for ocotp_base:
> ocotp_base = devm_of_iomap(dev, np, 0);

This would fail if OCOTP driver probes first and claims the memory area 
with request_mem_region() (or devm_request_mem_region(), used in 
__devm_ioremap_resource() which is called from devm_of_iomap()). I ran 
into this with ANATOP, which is the other iomap()d device here. The 
of_iomap() does not use request_mem_region(), so it can map the area.

> Would mean you can delete all the error handling parts

All right, let's do this in separate 3/3 patch , because the amount of 
changes in this one patch are growing to be too much and difficult to 
review.

[...]

>> @@ -212,8 +240,11 @@ static int __init imx8_soc_init(void)
>>          data = id->data;
>>          if (data) {
>>                  soc_dev_attr->soc_id = data->name;
>> -               if (data->soc_revision)
>> -                       soc_rev = data->soc_revision();
>> +               if (data->soc_revision) {
>> +                       ret = data->soc_revision(&soc_rev);
>> +                       if (ret)
>> +                               goto free_soc;
>> +               }
>>          }
> 
> I'm glad it's working for you, but I think there might still be a race
> that you are just lucky enough to not hit. I think you still need to
> fix up drivers/base/soc.c to return -EPROBE_DEFER when
> soc_device_match() is called but soc_bus_type has no devices
> registered. That way any drivers that try to use that API will defer
> probe until this device gets to probe.

soc_device_match() returns a pointer to soc_device_attribute or NULL, do 
you have some other function in mind ?

> And then you'll have to look at all the callers of that API for the
> boards this driver is meant for and make sure they don't ignore the
> error return value. Just add a WARN() on the API to figure out all the
> callers in your board.
> 
> Also, you might want to check that your list of probed devices doesn't
> change without any async probing or this patch vs with async probing
> and this patch. Quick way to get list of successfully probed devices
> is:
> # find /sys/devices -name driver

It seems OK.

[...]