diff mbox series

tee: optee: Populate child nodes in probe function

Message ID	20221109160708.507481-1-ludvig.parsson@axis.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; From: =?utf-8?q?Ludvig_P=C3=A4rsson?= <ludvig.parsson@axis.com> To: Jens Wiklander <jens.wiklander@linaro.org> CC: <kernel@axis.com>, =?utf-8?q?Ludvig_P=C3=A4rsson?= <ludvig.parsson@axis.com>, Sumit Garg <sumit.garg@linaro.org>, <op-tee@lists.trustedfirmware.org>, <linux-kernel@vger.kernel.org> Subject: [PATCH] tee: optee: Populate child nodes in probe function Date: Wed, 9 Nov 2022 17:07:08 +0100 Message-ID: <20221109160708.507481-1-ludvig.parsson@axis.com> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit Precedence: bulk
Series	tee: optee: Populate child nodes in probe function \| tee: optee: Populate child nodes in probe function

Commit Message

Ludvig Pärsson Nov. 9, 2022, 4:07 p.m. UTC

  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.

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@axis.com>
---
 drivers/tee/optee/smc_abi.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Sumit Garg Nov. 10, 2022, 10:53 a.m. UTC | #1

On Wed, 9 Nov 2022 at 21:37, Ludvig Pärsson <ludvig.parsson@axis.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

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

Ludvig Pärsson Nov. 10, 2022, 12:09 p.m. UTC | #2

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

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

Sumit Garg Nov. 11, 2022, 9:57 a.m. UTC | #3

On Thu, 10 Nov 2022 at 17:39, Ludvig Pärsson <Ludvig.Parsson@axis.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@axis.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

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

Ludvig Pärsson Nov. 11, 2022, 1:16 p.m. UTC | #4

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

Sudeep Holla Nov. 11, 2022, 4:13 p.m. UTC | #5

On Wed, Nov 09, 2022 at 05:07:08PM +0100, Ludvig Pärsson 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.
> 
> 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@axis.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;
> +

While I agree with idea of populating dependent child nodes from here,
I am not sure if it is OK to give error if that fails or to just continue
as there may be other users(like the user-space) for the OPTEE in general.

Sumit Garg Nov. 14, 2022, 6:38 a.m. UTC | #6

On Fri, 11 Nov 2022 at 21:43, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Wed, Nov 09, 2022 at 05:07:08PM +0100, Ludvig Pärsson 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.
> >
> > 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@axis.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;
> > +
>
> While I agree with idea of populating dependent child nodes from here,
> I am not sure if it is OK to give error if that fails or to just continue
> as there may be other users(like the user-space) for the OPTEE in general.
>

This uncertainty is simply because the inter subsystem dependency
issue can't be resolved by this. See my comment regarding FF-A ABI on
the other thread [1]

[1] https://lkml.org/lkml/2022/11/14/29

-Sumit

> --
> Regards,
> Sudeep

diff mbox series

Patch

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;