Date:   Mon, 14 Nov 2022 12:29:50 +0100
From:   Etienne Carriere <etienne.carriere@...aro.org>
To:     Sudeep Holla <sudeep.holla@....com>
Cc:     Sumit Garg <sumit.garg@...aro.org>,
        linux-arm-kernel@...ts.infradead.org, cristian.marussi@....com,
        Ludvig.Parsson@...s.com, jens.wiklander@...aro.org,
        vincent.guittot@...aro.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] firmware: arm_scmi: Resolve dependency with TEE subsystem

Hello all,

On Mon, 14 Nov 2022 at 11:26, Sudeep Holla <sudeep.holla@....com> wrote:
>
> On Mon, Nov 14, 2022 at 12:01:32PM +0530, Sumit Garg wrote:
> > Hi Sudeep,
> >
> > On Fri, 11 Nov 2022 at 20:08, Sudeep Holla <sudeep.holla@....com> wrote:
> > >
> > > On Fri, Nov 11, 2022 at 03:23:13PM +0530, Sumit Garg wrote:
> > > > The OP-TEE SCMI transport channel is dependent on TEE subsystem to be
> > > > initialized first. But currently the Arm SCMI subsystem and TEE
> > > > subsystem are invoked on the same initcall level as subsystem_init().
> > > >
> > > > It is observed that the SCMI subsystem initcall is invoked prior to TEE
> > > > subsystem initcall. This leads to unwanted error messages regarding TEE
> > > > bus is not present yet. Although, -EPROBE_DEFER tries to workaround that
> > > > problem.
> > > >
> > > > Lets try to resolve inter subsystem dependency problem via shifting Arm
> > > > SCMI subsystem to subsystem_init_sync() initcall level.
> > > >
> > >
> > > I would avoid doing that. We already have some implicit dependency with
> > > subsys_initcall because this driver creates/registers bus and need to be
> > > done early.
> >
> > Yeah but that should work fine with subsystem_init_sync() too unless
> > you have drivers being registered on the bus at subsystem_init_sync()
> > initcall which doesn't seem to be the case in mainline. Have a look at
> > usage of subsystem_init_sync() elsewhere to see its similar usage to
> > resolve dependencies among different subsystems.
> >
> > However, if you are too skeptical regarding this change then we can
> > limit it to OP-TEE transport only as follows:
> >
> > diff --git a/drivers/firmware/arm_scmi/driver.c
> > b/drivers/firmware/arm_scmi/driver.c
> > index f43e52541da4..19c1222b3dfc 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -2667,7 +2667,11 @@ static int __init scmi_driver_init(void)
> >
> >         return platform_driver_register(&scmi_driver);
> >  }
> > +#ifdef CONFIG_ARM_SCMI_TRANSPORT_OPTEE
> >  subsys_initcall_sync(scmi_driver_init);
> > +#else
> > +subsys_initcall(scmi_driver_init);
> > +#endif
> >
>
> If this is the only way to solve, I would prefer to keep it unconditional.
>
> >  static void __exit scmi_driver_exit(void)
> >  {
> >
> > > Now in order to solve the dependency between SCMI and TEE,
> > > both of which creates/registers bus and are at same subsys_initcall,
> > > we are relying on subsys_initcall_sync.
> >
> > True.
> >
> > >
> > > Me and Ludvig discussed this in private and I suggested him to do something
> > > like below patch snippet. He mentioned he did post a patch on the list but
> > > I couldn't find it. For this the scmi node must be child node of OPTEE as
> > > it is providing the transport.
> >
> > TBH, the first thought that came to mind after looking at SCMI OP-TEE
> > DT node was that why do we need it when those properties can be probed
> > from SCMI pseudo TA at runtime? Maybe I am missing something as I
> > wasn't involved in its review process.
> >
>
> I don't have internal details OPTEE and may be it could be probed. Etienne
> can comment on that. But we need SCMI node in general as the consumers of
> the SCMI are in the DT and they need to link to the provider.

Indeed the SCMI OP-TEE service is currently designed to be discovered
by Linux but it does not allow Linux to discover which resources are
related to the exposed SCMI channels. As Sudeep said, these
information are provided by the DT. Moreover, consumer devices of the
SCMI services in Linux are using DT to reference the SCMI resource
used, as phandles on SCMI clock provider, SCMI regulator provider
etc... For the consumers, we need these DT descriptions.


>
> > The whole idea of TEE bus evolved from the idea that if the firmware
> > bits can be probed at runtime then we shouldn't use DT as a dumping
> > ground for those. I hope you remember discussions around discoverable
> > FF-A bus too.
> >
>
> Exactly this is what I thought of initially when I proposed the solution.
> And yes we won't even have DT node for TEE in that case, so it shouldn't
> be a problem. When both SCMI and TEE are present in DT and SCMI used OPTEE
> as a transport I see it is correct to represent them as child and parent
> and it can be utilised here to solved the problem with respect to the driver
> model without having to play around with the initcall levels which is always
> going to bite us back with one extra dependency.
>
> And with FF-A, TEE and SCMI all in the mix we might have that dependency
> already, so I really want to avoid playing with initcall levels just to solve
> this problem.

Even with FFA, the optee driver still registers from module_init level
(== device_init level initcall), as when using legacy OP-TE SMC ABI.
SCMI firmware driver is initialized from subsys_init level hence
before optee driver. So I think SCMI optee transport relies on the
same dependencies whatever OP-TEE is using FFA ABI or its legacy SCM
ABI.

Device discovery from OP-TEE bus will always need to wait for the
OP-TEE bus to be ready.
This is currently archived for scmi/optee by returning -EPROBE_DEFER
from  scmi_optee_link_supplier() (scmi_transport_ops::link handler
from scmi/optee).
@Luvig, your initial issue is that driver_register() prints an error
trace when one registers a driver for a bus device that is not setup,
not an issue with dependencies, right?

Regards,
Etienne

>
> > However, the change below is simply an incorrect way to fix the actual
> > inter subsystem dependency problem via DT. How would this fix the
> > problem in the case OP-TEE driver registers on FF-A bus? There won't
> > be any DT node for OP-TEE in that case.
> >
>
> Agreed and this is why I thought it in the first place. As I mention in this
> case there are no DT nodes and hence we can't use this at all. I am suggesting
> this only when both DT nodes are present and SCMI depends on OPTEE transport
> which fits the child/parent hierarchy irrespective of this solution. This
> is just ensuring any dependent DT nodes are populated only after OPTEE is
> ready. I don't see this to be an issue or see this as incorrect.
>
>
> Also I am not sure this initcall juggling will help if there are 3 or more
> at the same level, we need to rely on driver model and/or proper hierarchy
> in the DT node. The whole links between devices is modelled on that and
> I don't see that as any issue and we are not dumping any more information
> that it is already in DT. We are just using the correct hierarchical
> representation here IMO.
>
> --
> Regards,
> Sudeep

Powered by blists - more mailing lists