Date:   Mon, 14 Nov 2022 13:47:25 +0000
From:   Ludvig Pärsson <Ludvig.Parsson@...s.com>
To:     "etienne.carriere@...aro.org" <etienne.carriere@...aro.org>,
        "sudeep.holla@....com" <sudeep.holla@....com>
CC:     "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "jens.wiklander@...aro.org" <jens.wiklander@...aro.org>,
        "sumit.garg@...aro.org" <sumit.garg@...aro.org>,
        Ludvig Pärsson <Ludvig.Parsson@...s.com>,
        "vincent.guittot@...aro.org" <vincent.guittot@...aro.org>,
        "cristian.marussi@....com" <cristian.marussi@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] firmware: arm_scmi: Resolve dependency with TEE subsystem

On Mon, 2022-11-14 at 12:29 +0100, Etienne Carriere wrote:
> 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
> 
Yes, exactly. We don't want to call driver_register() before the bus is
initialized. I guess you can say that there should be a dependency
here, but there isn't one.

BR,
Ludvig
> > 
> > > 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