linux-kernel - Re: [PATCH] tee: optee: Populate child nodes in probe function

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

Date:   Fri, 11 Nov 2022 13:16:41 +0000
From:   Ludvig Pärsson <Ludvig.Parsson@...s.com>
To:     "sumit.garg@...aro.org" <sumit.garg@...aro.org>,
        Ludvig Pärsson <Ludvig.Parsson@...s.com>
CC:     "jens.wiklander@...aro.org" <jens.wiklander@...aro.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        kernel <kernel@...s.com>,
        "op-tee@...ts.trustedfirmware.org" <op-tee@...ts.trustedfirmware.org>
Subject: Re: [PATCH] tee: optee: Populate child nodes in probe function

On Fri, 2022-11-11 at 15:27 +0530, Sumit Garg wrote:
> On Thu, 10 Nov 2022 at 17:39, Ludvig Pärsson
> <Ludvig.Parsson@...s.com> wrote:
> > 
> > On Thu, 2022-11-10 at 16:23 +0530, Sumit Garg wrote:
> > > On Wed, 9 Nov 2022 at 21:37, Ludvig Pärsson < 
> > > ludvig.parsson@...s.com>
> > > wrote:
> > > > 
> > > > Currently there is no dependency between the "linaro,scmi-
> > > > optee"
> > > > driver
> > > > and the tee_core. If the scmi-optee driver gets probed before
> > > > the
> > > > tee_bus_type is initialized, then we will get an unwanted error
> > > > print.
> > > > 
> > > 
> > > What error print do you observe? I suppose this case is already
> > > handled by scmi optee driver via -EPROBE_DEFER.
> > > 
> > > -Sumit
> > > 
> > Hi Sumit,
> > 
> > The error print is in driver_register().
> > 
> > This is kind of what happens:
> > scmi_driver_init()
> > scmi_probe()
> >     scmi_optee_link_supplier()
> >         scmi_optee_init()
> >             driver_register() <--- pr_err() if tee_bus_type is not
> > initialized
> > tee_init() <--- tee_bus_type gets initialized here
> > 
> > The scmi_optee_link_supplier() will always return -EPROBE_DEFER the
> > first time because scmi_optee_private is initialized in
> > scmi_optee_service_probe, which is only called after the driver is
> > registered in scmi_optee_init. Right now the driver_register fails
> > because tee_bus_type is not initialized which is printing the
> > unwanted
> > error print. Another side effect of this is that
> > scmi_optee_link_supplier() will return -EPROBE_DEFER a second time,
> > and
> > scmi_probe will be successful the third time instead of the second
> > time.
> 
> Thanks for the report. It rather looks like an inter subsystem
> dependency problem. Check if this [1] patch resolves your problem?
> 
> [1] https://lkml.org/lkml/2022/11/11/329
> 
> -Sumit
> 
Yes, this solves the problem, but I don't think the maintainer for the
SCMI subsystem likes the solution. Earlier this week I discussed this
problem with Sudeep, Cristian and Etienne that are CCed in the
submission you linked. Changing the initcall level was one of my
suggestions which got instantly rejected. We agreed that the solution
in the patch i submitted was probably the best one, but maybe they will
change their minds.

BR,
Ludvig

> > 
> > BR,
> > Ludvig
> > 
> > > > This patch enables putting scmi-optee nodes as children to the
> > > > optee
> > > > node in devicetree, which indirectly creates the missing
> > > > dependency.
> > > > 
> > > > Signed-off-by: Ludvig Pärsson <ludvig.parsson@...s.com>
> > > > ---
> > > >  drivers/tee/optee/smc_abi.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/drivers/tee/optee/smc_abi.c
> > > > b/drivers/tee/optee/smc_abi.c
> > > > index a1c1fa1a9c28..be6f02fd5a7f 100644
> > > > --- a/drivers/tee/optee/smc_abi.c
> > > > +++ b/drivers/tee/optee/smc_abi.c
> > > > @@ -1533,6 +1533,11 @@ static int optee_probe(struct
> > > > platform_device *pdev)
> > > >         if (rc)
> > > >                 goto err_disable_shm_cache;
> > > > 
> > > > +       /* Populate any dependent child node (if any) */
> > > > +       rc = devm_of_platform_populate(&pdev->dev);
> > > > +       if (rc)
> > > > +               goto err_disable_shm_cache;
> > > > +
> > > >         pr_info("initialized driver\n");
> > > >         return 0;
> > > > 
> > > > --
> > > > 2.30.2
> > > > 
> >