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] [day] [month] [year] [list]

Message-ID: <CAGETcx95ZMBJR0F-3Cr8++1_MwiMCPHiwfdx7m5q0XDAiGG75A@mail.gmail.com>
Date: Fri, 27 Sep 2024 15:27:26 -0700
From: Saravana Kannan <saravanak@...gle.com>
To: Marek Vasut <marex@...x.de>
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 Fri, Sep 27, 2024 at 2:55 PM Marek Vasut <marex@...x.de> wrote:
>
> 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.

I'll take your word for it.

>
> > 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.

Sure. If you can't use devm, I don't care too much about just cleaning
up "of_put()" error handling. Your call on whether you do a 3/3.

>
> [...]
>
> >> @@ -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 ?

No, I'm talking about the same function. I'm asking to change it to
return ERR_PTR(-EPROBE_DEFER) instead
of NULL if no soc device has been registered yet.

And you'll also go change all the drivers that use that API and are on
the IMX boards supported by this soc driver, to handle the
-EPROBE_DEFER correctly.

And this error will only get returned for boards that do async probing
and using a platform device to register the soc device. So it's
not going to break everyone if you do this change.

>
> > 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.

Good to know.

-Saravana

>
> [...]