[PATCH] firmware: arm_scmi: Resolve dependency with TEE subsystem

[PATCH] firmware: arm_scmi: Resolve dependency with TEE subsystem

[Sumit Garg]

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.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 drivers/firmware/arm_scmi/driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index f818d00bb2c6..f43e52541da4 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2667,7 +2667,7 @@ static int __init scmi_driver_init(void)
 
 	return platform_driver_register(&scmi_driver);
 }
-subsys_initcall(scmi_driver_init);
+subsys_initcall_sync(scmi_driver_init);
 
 static void __exit scmi_driver_exit(void)
 {
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: arm_scmi: Resolve dependency with TEE subsystem

[Sudeep Holla]

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

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.

@Ludvig, ?

Regards,
Sudeep

--
diff --git i/drivers/tee/optee/smc_abi.c w/drivers/tee/optee/smc_abi.c
index a1c1fa1a9c28..839feca0def4 100644
--- i/drivers/tee/optee/smc_abi.c
+++ w/drivers/tee/optee/smc_abi.c
@@ -1534,7 +1534,9 @@ static int optee_probe(struct platform_device *pdev)
                goto err_disable_shm_cache;

        pr_info("initialized driver\n");
-       return 0;
+
+       /* Populate any dependent child node(if any) */
+       return devm_of_platform_populate(&pdev->dev);

 err_disable_shm_cache:
        if (!optee->rpc_param_count)


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: arm_scmi: Resolve dependency with TEE subsystem

[Ludvig Pärsson]

On Fri, 2022-11-11 at 14:38 +0000, Sudeep Holla 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. 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.
> 
> 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.
> 
> @Ludvig, ?
> 
> Regards,
> Sudeep
> 
> --
> diff --git i/drivers/tee/optee/smc_abi.c
> w/drivers/tee/optee/smc_abi.c
> index a1c1fa1a9c28..839feca0def4 100644
> --- i/drivers/tee/optee/smc_abi.c
> +++ w/drivers/tee/optee/smc_abi.c
> @@ -1534,7 +1534,9 @@ static int optee_probe(struct platform_device
> *pdev)
>                 goto err_disable_shm_cache;
> 
>         pr_info("initialized driver\n");
> -       return 0;
> +
> +       /* Populate any dependent child node(if any) */
> +       return devm_of_platform_populate(&pdev->dev);
> 
>  err_disable_shm_cache:
>         if (!optee->rpc_param_count)
> 
I have answered something similar in my submit [1]. Maybe I should have
CCed you, or atleast sent you this link when I told you I made the
submission.

[1] https://lkml.org/lkml/2022/11/9/803

BR,
Ludvig
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: arm_scmi: Resolve dependency with TEE subsystem

[Sudeep Holla]

On Fri, Nov 11, 2022 at 03:00:29PM +0000, Ludvig Pärsson wrote:
> On Fri, 2022-11-11 at 14:38 +0000, Sudeep Holla 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. 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.
> > 
> > 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.
> > 
> > @Ludvig, ?
> > 
> > Regards,
> > Sudeep
> > 
> > --
> > diff --git i/drivers/tee/optee/smc_abi.c
> > w/drivers/tee/optee/smc_abi.c
> > index a1c1fa1a9c28..839feca0def4 100644
> > --- i/drivers/tee/optee/smc_abi.c
> > +++ w/drivers/tee/optee/smc_abi.c
> > @@ -1534,7 +1534,9 @@ static int optee_probe(struct platform_device
> > *pdev)
> >                 goto err_disable_shm_cache;
> > 
> >         pr_info("initialized driver\n");
> > -       return 0;
> > +
> > +       /* Populate any dependent child node(if any) */
> > +       return devm_of_platform_populate(&pdev->dev);
> > 
> >  err_disable_shm_cache:
> >         if (!optee->rpc_param_count)
> > 
> I have answered something similar in my submit [1]. Maybe I should have
> CCed you, or atleast sent you this link when I told you I made the
> submission.
> 
> [1] https://lkml.org/lkml/2022/11/9/803
>

Thanks for the reference.

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: arm_scmi: Resolve dependency with TEE subsystem

[Sumit Garg]

Hi Sudeep,

On Fri, 11 Nov 2022 at 20:08, Sudeep Holla <sudeep.holla@arm.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

 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.

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.

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.

-Sumit

>
> @Ludvig, ?
>
> Regards,
> Sudeep
>
> --
> diff --git i/drivers/tee/optee/smc_abi.c w/drivers/tee/optee/smc_abi.c
> index a1c1fa1a9c28..839feca0def4 100644
> --- i/drivers/tee/optee/smc_abi.c
> +++ w/drivers/tee/optee/smc_abi.c
> @@ -1534,7 +1534,9 @@ static int optee_probe(struct platform_device *pdev)
>                 goto err_disable_shm_cache;
>
>         pr_info("initialized driver\n");
> -       return 0;
> +
> +       /* Populate any dependent child node(if any) */
> +       return devm_of_platform_populate(&pdev->dev);
>
>  err_disable_shm_cache:
>         if (!optee->rpc_param_count)
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: arm_scmi: Resolve dependency with TEE subsystem

[Sudeep Holla]

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

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

> 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: arm_scmi: Resolve dependency with TEE subsystem

[Etienne Carriere]

Hello all,

On Mon, 14 Nov 2022 at 11:26, Sudeep Holla <sudeep.holla@arm.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@arm.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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: arm_scmi: Resolve dependency with TEE subsystem

[Ludvig Pärsson]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: arm_scmi: Resolve dependency with TEE subsystem

[Cristian Marussi]

On Mon, Nov 14, 2022 at 01:47:25PM +0000, Ludvig Pärsson wrote:
> On Mon, 2022-11-14 at 12:29 +0100, Etienne Carriere wrote:
> > Hello all,
> > 

Hi Ludvig,

following up on the issues raised by this thread and a few proposals that
were flying around (online and offline), in the past days I took the chance
to have a go at a substantial rework of the init/probe sequences in the SCMI
core to address the issue you faced with SCMI TEE transport while trying to
untangle a bit the SCMI core startup sequences (... while also possibly not
breaking it all :P...)

In a nutshell, building on an idea from an offline chat with Etienne ad
Sudeep, now the SCMI bus initialization is split on its own and initialized at
subsys_initcall level, while the SCMI core stack, including the the SCMI TEE
transport layer, is moved at module_init layer together with the SCMI
driver users.

This *should* theoretically solve your issue ... (and it seems like all the
rest it's still working :P) ... so I was wondering if you can give a go
at the following pachset on your setup:

https://gitlab.arm.com/linux-arm/linux-cm/-/commits/scmi_rework_stack_init_draft/

... note that this is just a draft at the moment, which has undergone a
reasonable amount of testing on mailbox/virtio transports only in both a
SCMI builtin and/or modules scenario, but is no where ready for review.

The top three patches are really what you need BUT these are probably
tightly bound to that bunch of early fixes you can see in the
branch...so in other words better if you pick the whole branch for
testing :D

Once you've confirmed me that this solves your issues I'll start the
final cleanup for posting in the next cycle.

Thanks,
Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: arm_scmi: Resolve dependency with TEE subsystem

[Ludvig Pärsson]

On Tue, 2022-11-22 at 17:48 +0000, Cristian Marussi wrote:
> On Mon, Nov 14, 2022 at 01:47:25PM +0000, Ludvig Pärsson wrote:
> > On Mon, 2022-11-14 at 12:29 +0100, Etienne Carriere wrote:
> > > Hello all,
> > > 
> 
> Hi Ludvig,
> 
> following up on the issues raised by this thread and a few proposals
> that
> were flying around (online and offline), in the past days I took the
> chance
> to have a go at a substantial rework of the init/probe sequences in
> the SCMI
> core to address the issue you faced with SCMI TEE transport while
> trying to
> untangle a bit the SCMI core startup sequences (... while also
> possibly not
> breaking it all :P...)
> 
> In a nutshell, building on an idea from an offline chat with Etienne
> ad
> Sudeep, now the SCMI bus initialization is split on its own and
> initialized at
> subsys_initcall level, while the SCMI core stack, including the the
> SCMI TEE
> transport layer, is moved at module_init layer together with the SCMI
> driver users.
> 
> This *should* theoretically solve your issue ... (and it seems like
> all the
> rest it's still working :P) ... so I was wondering if you can give a
> go
> at the following pachset on your setup:
> 
> https://gitlab.arm.com/linux-arm/linux-cm/-/commits/scmi_rework_stack_init_draft/
> 
> ... note that this is just a draft at the moment, which has undergone
> a
> reasonable amount of testing on mailbox/virtio transports only in
> both a
> SCMI builtin and/or modules scenario, but is no where ready for
> review.
> 
> The top three patches are really what you need BUT these are probably
> tightly bound to that bunch of early fixes you can see in the
> branch...so in other words better if you pick the whole branch for
> testing :D
> 
> Once you've confirmed me that this solves your issues I'll start the
> final cleanup for posting in the next cycle.
> 
> Thanks,
> Cristian

Hi Cristian,

I tried my best to get the patchset to work somehow on my version of
the kernel, and it seems to be working great. I played around with some
things, for example changing order of some drivers that were on the
same init levels, and it still worked. Only tested with voltage domain
protocol and optee transport.

Thanks for your great work!

BR,
Ludvig
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: arm_scmi: Resolve dependency with TEE subsystem

[Cristian Marussi]

On Tue, Nov 29, 2022 at 10:49:10AM +0000, Ludvig Pärsson wrote:
> On Tue, 2022-11-22 at 17:48 +0000, Cristian Marussi wrote:
> > On Mon, Nov 14, 2022 at 01:47:25PM +0000, Ludvig Pärsson wrote:
> > > On Mon, 2022-11-14 at 12:29 +0100, Etienne Carriere wrote:
> > > > Hello all,
> > > > 
> > 
> > Hi Ludvig,
> > 
> > following up on the issues raised by this thread and a few proposals
> > that
> > were flying around (online and offline), in the past days I took the
> > chance
> > to have a go at a substantial rework of the init/probe sequences in
> > the SCMI
> > core to address the issue you faced with SCMI TEE transport while
> > trying to
> > untangle a bit the SCMI core startup sequences (... while also
> > possibly not
> > breaking it all :P...)
> > 
> > In a nutshell, building on an idea from an offline chat with Etienne
> > ad
> > Sudeep, now the SCMI bus initialization is split on its own and
> > initialized at
> > subsys_initcall level, while the SCMI core stack, including the the
> > SCMI TEE
> > transport layer, is moved at module_init layer together with the SCMI
> > driver users.
> > 
> > This *should* theoretically solve your issue ... (and it seems like
> > all the
> > rest it's still working :P) ... so I was wondering if you can give a
> > go
> > at the following pachset on your setup:
> > 
> > https://gitlab.arm.com/linux-arm/linux-cm/-/commits/scmi_rework_stack_init_draft/
> > 
> > ... note that this is just a draft at the moment, which has undergone
> > a
> > reasonable amount of testing on mailbox/virtio transports only in
> > both a
> > SCMI builtin and/or modules scenario, but is no where ready for
> > review.
> > 
> > The top three patches are really what you need BUT these are probably
> > tightly bound to that bunch of early fixes you can see in the
> > branch...so in other words better if you pick the whole branch for
> > testing :D
> > 
> > Once you've confirmed me that this solves your issues I'll start the
> > final cleanup for posting in the next cycle.
> > 
> > Thanks,
> > Cristian
> 
> Hi Cristian,

Hi,

> 
> I tried my best to get the patchset to work somehow on my version of
> the kernel, and it seems to be working great. I played around with some
> things, for example changing order of some drivers that were on the
> same init levels, and it still worked. Only tested with voltage domain
> protocol and optee transport.
> 
> Thanks for your great work!
> 

Great, thanks for testing it.
I'll post shortly a cleaned up series aiming at the next release cycle.

Thanks,
Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: arm_scmi: Resolve dependency with TEE subsystem

[Etienne Carriere]

Hi Cristian, Ludvig,

For info, I've tested your patches Cristian on my setup.
they did the job for probing optee transport at module initcall level.

br,
etienne

On Tue, 29 Nov 2022 at 13:15, Cristian Marussi <cristian.marussi@arm.com> wrote:
>
> On Tue, Nov 29, 2022 at 10:49:10AM +0000, Ludvig Pärsson wrote:
> > On Tue, 2022-11-22 at 17:48 +0000, Cristian Marussi wrote:
> > > On Mon, Nov 14, 2022 at 01:47:25PM +0000, Ludvig Pärsson wrote:
> > > > On Mon, 2022-11-14 at 12:29 +0100, Etienne Carriere wrote:
> > > > > Hello all,
> > > > >
> > >
> > > Hi Ludvig,
> > >
> > > following up on the issues raised by this thread and a few proposals
> > > that
> > > were flying around (online and offline), in the past days I took the
> > > chance
> > > to have a go at a substantial rework of the init/probe sequences in
> > > the SCMI
> > > core to address the issue you faced with SCMI TEE transport while
> > > trying to
> > > untangle a bit the SCMI core startup sequences (... while also
> > > possibly not
> > > breaking it all :P...)
> > >
> > > In a nutshell, building on an idea from an offline chat with Etienne
> > > ad
> > > Sudeep, now the SCMI bus initialization is split on its own and
> > > initialized at
> > > subsys_initcall level, while the SCMI core stack, including the the
> > > SCMI TEE
> > > transport layer, is moved at module_init layer together with the SCMI
> > > driver users.
> > >
> > > This *should* theoretically solve your issue ... (and it seems like
> > > all the
> > > rest it's still working :P) ... so I was wondering if you can give a
> > > go
> > > at the following pachset on your setup:
> > >
> > > https://gitlab.arm.com/linux-arm/linux-cm/-/commits/scmi_rework_stack_init_draft/
> > >
> > > ... note that this is just a draft at the moment, which has undergone
> > > a
> > > reasonable amount of testing on mailbox/virtio transports only in
> > > both a
> > > SCMI builtin and/or modules scenario, but is no where ready for
> > > review.
> > >
> > > The top three patches are really what you need BUT these are probably
> > > tightly bound to that bunch of early fixes you can see in the
> > > branch...so in other words better if you pick the whole branch for
> > > testing :D
> > >
> > > Once you've confirmed me that this solves your issues I'll start the
> > > final cleanup for posting in the next cycle.
> > >
> > > Thanks,
> > > Cristian
> >
> > Hi Cristian,
>
> Hi,
>
> >
> > I tried my best to get the patchset to work somehow on my version of
> > the kernel, and it seems to be working great. I played around with some
> > things, for example changing order of some drivers that were on the
> > same init levels, and it still worked. Only tested with voltage domain
> > protocol and optee transport.
> >
> > Thanks for your great work!
> >
>
> Great, thanks for testing it.
> I'll post shortly a cleaned up series aiming at the next release cycle.
>
> Thanks,
> Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: arm_scmi: Resolve dependency with TEE subsystem

[Cristian Marussi]

On Tue, Nov 29, 2022 at 03:57:50PM +0100, Etienne Carriere wrote:
> Hi Cristian, Ludvig,
> 

Hi Etienne,

> For info, I've tested your patches Cristian on my setup.
> they did the job for probing optee transport at module initcall level.
> 

Thanks for testing, I'll keep you in CC when posting the final series.
Cristian


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: arm_scmi: Resolve dependency with TEE subsystem

[Sumit Garg]

On Mon, 14 Nov 2022 at 17:00, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Hello all,
>
> On Mon, 14 Nov 2022 at 11:26, Sudeep Holla <sudeep.holla@arm.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@arm.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.
>

So it's the DT legacy we want to maintain for the SCMI subsystem even
if the underlying transport is discoverable? This simply undermines
the benefits of a discoverable TEE bus over the non-discoverable
platform bus. Also, the reluctance to carry forward SCMI subsystem DT
legacy has resulted in misrepresentation of SCMI OP-TEE transport as
follows:

    First represented as a platform device via DT (compatible =
"linaro,scmi-optee";) and then
    Migrated to being a TEE bus device (UUID: 0xa8cfe406, 0xd4f5,
0x4a2e, 0x9f, 0x8d, 0xa2, 0x5d, 0xc7, 0x54, 0xc0, 0x99)

This misrepresentation is the reason for the indirect DT hack that
Sudeep suggested to fix the error message while registering a driver
on TEE bus.

Is it not possible for SCMI subsystem to evolve and support underlying
transport on a discoverable TEE bus?

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

Why? The SCMI OP-TEE transport driver will still be registered on TEE
bus via subsys_initcall() prior to TEE bus being registered via
subsys_initcall().

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

No it's not the correct representation. Devices on the TEE bus have
nothing to do with DT. The OP-TEE node in DT represents a particular
TEE implementation whereas there are other ways to represent OP-TEE
implementation as a device on FF-A bus.

Your suggested change only works due to misrepresentation of SCMI
OP-TEE transport as highlighted above while it won't fix the case with
OP-TEE device on FF-A bus.

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

As I mentioned in the patch description, it's an inter-subsystem
dependency. It has to be solved via initcall levels. I am unsure which
extra dependency you have in mind but the one mentioned below doesn't
fall into that category. Have you looked at other places within the
kernel for usage of subsys_initcall_sync()?

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

There isn't a three level dependency here. The only dependency we have
to solve is that the SCMI OP-TEE transport shouldn't register on TEE
bus prior to registration of TEE bus. And switching SCMI subsystem to
subsys_initcall_sync() fixes that dependency even with OP-TEE FF-A
ABI.

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

I guess here you are confusing the TEE subsystem driver invoked at
subsys_initcall() versus OP-TEE driver invoked at module_init(). The
TEE bus is registered by the TEE subsystem driver rather than the
OP-TEE driver.

So there is *no* dependency among SCMI firmware driver and OP-TEE
driver but rather the dependency is with TEE subsystem driver instead.

> Device discovery from OP-TEE bus will always need to wait for the
> OP-TEE bus to be ready.

It isn't an OP-TEE bus but rather a TEE bus with underlying TEE
implementations like OP-TEE etc. registering their corresponding
devices.

-Sumit

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: arm_scmi: Resolve dependency with TEE subsystem

[Sudeep Holla]

On Thu, Nov 17, 2022 at 03:11:32PM +0530, Sumit Garg wrote:
>
> So it's the DT legacy we want to maintain for the SCMI subsystem even
> if the underlying transport is discoverable?

Not sure what exactly you mean here by "DT legacy". The SCMI consumers
needing SCMI info from DT is not legacy for sure. The creation of SCMI
platform device from DT for TEE can be considered as legacy.

> This simply undermines the benefits of a discoverable TEE bus over the
> non-discoverable platform bus. Also, the reluctance to carry forward SCMI
> subsystem DT legacy has resulted in misrepresentation of SCMI OP-TEE
> transport as follows:
>

Agreed on using DT for creation of SCMI platform device.

>     First represented as a platform device via DT (compatible =
> "linaro,scmi-optee";) and then
>     Migrated to being a TEE bus device (UUID: 0xa8cfe406, 0xd4f5,
> 0x4a2e, 0x9f, 0x8d, 0xa2, 0x5d, 0xc7, 0x54, 0xc0, 0x99)
>

I am fine with SCMI platform device not created by TEE/OPTEE by
of_populate_device(). It can create one by matching the service(IIUC).

> This misrepresentation is the reason for the indirect DT hack that
> Sudeep suggested to fix the error message while registering a driver
> on TEE bus.
>

Again, sorry. We are mixing up things here I think. The DT representation
of SCMI node as child of TEE/OPTEE is *not* hack. I agree creating the
platform device from DT w/o checking the service is hack. Let me know
if we are on the same page here.

> Is it not possible for SCMI subsystem to evolve and support underlying
> transport on a discoverable TEE bus?
>

It must, and as I mentioned we need to create SCMI platform device only
once the SCMI service is discovered. For that reason, the SCMI node in
that case must not be in /firmware but inside /firmware/optee.

Now it is up to OPTEE or even FF-A to create the devices for the service
it provides when ready.

> 
> Why? The SCMI OP-TEE transport driver will still be registered on TEE
> bus via subsys_initcall() prior to TEE bus being registered via
> subsys_initcall().
>

Not if SCMI platform device is not created before TEE has discovered SCMI
service.

> > > 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
> 
> No it's not the correct representation. Devices on the TEE bus have
> nothing to do with DT.

I disagree, the whole child/parent hierarchy in DT is for such a dependency.
That said, I am not asking to create SCMI device via of_populate_devices()
from TEE. It can choose to do in whatever is the right ways especially if
it can be discovered.

> The OP-TEE node in DT represents a particular
> TEE implementation whereas there are other ways to represent OP-TEE
> implementation as a device on FF-A bus.
>

Agreed again.

> Your suggested change only works due to misrepresentation of SCMI
> OP-TEE transport as highlighted above while it won't fix the case with
> OP-TEE device on FF-A bus.
>

Incorrect. As I mentioned before I don't care how we create SCMI device,
whether it has to be SCMI platform device or it can be SCMI TEE device with
all these managed within probe.

> > > 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.
> > >
> 
> As I mentioned in the patch description, it's an inter-subsystem
> dependency. It has to be solved via initcall levels.

A big NACK again. What you think will solve problem on this problem may cause
issue on some other platform or some other config. We definitely need a
solution which is not fiddling with initcall levels. I am sure others agree
with that as we have had enough issues with that in the past and the whole
probe deferral is result of that. But in this case it is not working and 
we can't go back to initcall for sure.

> I am unsure which
> extra dependency you have in mind but the one mentioned below doesn't
> fall into that category. Have you looked at other places within the
> kernel for usage of subsys_initcall_sync()?
>

Yes, but still I don't agree with that. For me it is hack just to solve
the issue at hand instead of thinking what is wrong with the design. The
whole point of all these discoverable buses was to get rid of such initcall
and deferred probe dependency and we must not use them to solve the issue
here.

> > > 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.
> 
> There isn't a three level dependency here. The only dependency we have
> to solve is that the SCMI OP-TEE transport shouldn't register on TEE
> bus prior to registration of TEE bus. And switching SCMI subsystem to
> subsys_initcall_sync() fixes that dependency even with OP-TEE FF-A
> ABI.
>

NACK again. I won't accept that unless we have explore right options
and get agreement with Greg or others that this is the only way.

> >
> > 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.
> >
> 
> I guess here you are confusing the TEE subsystem driver invoked at
> subsys_initcall() versus OP-TEE driver invoked at module_init(). The
> TEE bus is registered by the TEE subsystem driver rather than the
> OP-TEE driver.
>

I agree. One option I could think of as I read this is to have SCMI optee
to be proper driver at appropriate level and the probe can add the platform
SCMI device needed to prove SCMI driver from scmi_optee_service_probe.
That should work IMO. I will try to hack up something and share.

> So there is *no* dependency among SCMI firmware driver and OP-TEE
> driver but rather the dependency is with TEE subsystem driver instead.
>

Agreed.

> > Device discovery from OP-TEE bus will always need to wait for the
> > OP-TEE bus to be ready.
> 
> It isn't an OP-TEE bus but rather a TEE bus with underlying TEE
> implementations like OP-TEE etc. registering their corresponding
> devices.
> 

+1

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel