[PATCH] firmware: arm_scmi: optee: Drop the support for the OPTEE shared dynamic buffer

[PATCH] firmware: arm_scmi: optee: Drop the support for the OPTEE shared dynamic buffer

[Sudeep Holla]

The shared memory buffer allocated by the optee driver is normal cached
memory and can't be used with IOMEM APIs used in shmem_*.

We currently support only IO memory for shared memory and supporting
normal cached memory needs more changes and needs to be thought through
properly. So for now, let us drop the support for this OPTEE shared buffer.

Cc: Cristian Marussi <cristian.marussi@arm.com>
Cc: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/optee.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
index 901737c9f5f8..175b39bcd470 100644
--- a/drivers/firmware/arm_scmi/optee.c
+++ b/drivers/firmware/arm_scmi/optee.c
@@ -282,23 +282,6 @@ static void scmi_optee_clear_channel(struct scmi_chan_info *cinfo)
 	shmem_clear_channel(channel->shmem);
 }
 
-static int setup_dynamic_shmem(struct device *dev, struct scmi_optee_channel *channel)
-{
-	const size_t msg_size = SCMI_OPTEE_MAX_MSG_SIZE;
-
-	channel->tee_shm = tee_shm_alloc_kernel_buf(scmi_optee_private->tee_ctx, msg_size);
-	if (IS_ERR(channel->tee_shm)) {
-		dev_err(channel->cinfo->dev, "shmem allocation failed\n");
-		return -ENOMEM;
-	}
-
-	channel->shmem = (void *)tee_shm_get_va(channel->tee_shm, 0);
-	memset(channel->shmem, 0, msg_size);
-	shmem_clear_channel(channel->shmem);
-
-	return 0;
-}
-
 static int setup_static_shmem(struct device *dev, struct scmi_chan_info *cinfo,
 			      struct scmi_optee_channel *channel)
 {
@@ -342,7 +325,7 @@ static int setup_shmem(struct device *dev, struct scmi_chan_info *cinfo,
 	if (of_find_property(cinfo->dev->of_node, "shmem", NULL))
 		return setup_static_shmem(dev, cinfo, channel);
 	else
-		return setup_dynamic_shmem(dev, channel);
+		return -ENOMEM;
 }
 
 static int scmi_optee_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, bool tx)
-- 
2.25.1

Re: [PATCH] firmware: arm_scmi: optee: Drop the support for the OPTEE shared dynamic buffer

[Cristian Marussi]

On Thu, Nov 25, 2021 at 03:07:30PM +0000, Sudeep Holla wrote:
> The shared memory buffer allocated by the optee driver is normal cached
> memory and can't be used with IOMEM APIs used in shmem_*.
> 
> We currently support only IO memory for shared memory and supporting
> normal cached memory needs more changes and needs to be thought through
> properly. So for now, let us drop the support for this OPTEE shared buffer.
> 
> Cc: Cristian Marussi <cristian.marussi@arm.com>
> Cc: Etienne Carriere <etienne.carriere@linaro.org>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---

Hi,


>  drivers/firmware/arm_scmi/optee.c | 19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
> index 901737c9f5f8..175b39bcd470 100644
> --- a/drivers/firmware/arm_scmi/optee.c
> +++ b/drivers/firmware/arm_scmi/optee.c
> @@ -282,23 +282,6 @@ static void scmi_optee_clear_channel(struct scmi_chan_info *cinfo)
>  	shmem_clear_channel(channel->shmem);
>  }
>  
> -static int setup_dynamic_shmem(struct device *dev, struct scmi_optee_channel *channel)
> -{
> -	const size_t msg_size = SCMI_OPTEE_MAX_MSG_SIZE;
> -
> -	channel->tee_shm = tee_shm_alloc_kernel_buf(scmi_optee_private->tee_ctx, msg_size);
> -	if (IS_ERR(channel->tee_shm)) {
> -		dev_err(channel->cinfo->dev, "shmem allocation failed\n");
> -		return -ENOMEM;
> -	}
> -
> -	channel->shmem = (void *)tee_shm_get_va(channel->tee_shm, 0);
> -	memset(channel->shmem, 0, msg_size);
> -	shmem_clear_channel(channel->shmem);
> -
> -	return 0;
> -}
> -
>  static int setup_static_shmem(struct device *dev, struct scmi_chan_info *cinfo,
>  			      struct scmi_optee_channel *channel)
>  {
> @@ -342,7 +325,7 @@ static int setup_shmem(struct device *dev, struct scmi_chan_info *cinfo,
>  	if (of_find_property(cinfo->dev->of_node, "shmem", NULL))
>  		return setup_static_shmem(dev, cinfo, channel);
>  	else
> -		return setup_dynamic_shmem(dev, channel);
> +		return -ENOMEM;
>  }
>  

LGTM.

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian

Re: [PATCH] firmware: arm_scmi: optee: Drop the support for the OPTEE shared dynamic buffer

[Etienne Carriere]

Hello Sudeep,

On Thu, 25 Nov 2021 at 19:25, Cristian Marussi <cristian.marussi@arm.com> wrote:
>
> On Thu, Nov 25, 2021 at 03:07:30PM +0000, Sudeep Holla wrote:
> > The shared memory buffer allocated by the optee driver is normal cached
> > memory and can't be used with IOMEM APIs used in shmem_*.
> >
> > We currently support only IO memory for shared memory and supporting
> > normal cached memory needs more changes and needs to be thought through
> > properly. So for now, let us drop the support for this OPTEE shared buffer.
> >
> > Cc: Cristian Marussi <cristian.marussi@arm.com>
> > Cc: Etienne Carriere <etienne.carriere@linaro.org>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
>
> Hi,
>
>
> >  drivers/firmware/arm_scmi/optee.c | 19 +------------------
> >  1 file changed, 1 insertion(+), 18 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
> > index 901737c9f5f8..175b39bcd470 100644
> > --- a/drivers/firmware/arm_scmi/optee.c
> > +++ b/drivers/firmware/arm_scmi/optee.c
> > @@ -282,23 +282,6 @@ static void scmi_optee_clear_channel(struct scmi_chan_info *cinfo)
> >       shmem_clear_channel(channel->shmem);
> >  }
> >
> > -static int setup_dynamic_shmem(struct device *dev, struct scmi_optee_channel *channel)
> > -{
> > -     const size_t msg_size = SCMI_OPTEE_MAX_MSG_SIZE;
> > -
> > -     channel->tee_shm = tee_shm_alloc_kernel_buf(scmi_optee_private->tee_ctx, msg_size);
> > -     if (IS_ERR(channel->tee_shm)) {
> > -             dev_err(channel->cinfo->dev, "shmem allocation failed\n");
> > -             return -ENOMEM;
> > -     }
> > -
> > -     channel->shmem = (void *)tee_shm_get_va(channel->tee_shm, 0);
> > -     memset(channel->shmem, 0, msg_size);
> > -     shmem_clear_channel(channel->shmem);
> > -
> > -     return 0;
> > -}
> > -
> >  static int setup_static_shmem(struct device *dev, struct scmi_chan_info *cinfo,
> >                             struct scmi_optee_channel *channel)
> >  {
> > @@ -342,7 +325,7 @@ static int setup_shmem(struct device *dev, struct scmi_chan_info *cinfo,
> >       if (of_find_property(cinfo->dev->of_node, "shmem", NULL))
> >               return setup_static_shmem(dev, cinfo, channel);
> >       else
> > -             return setup_dynamic_shmem(dev, channel);
> > +             return -ENOMEM;
> >  }
> >

I would rather find an alternate way to support tee shared memory.
I think OP-TEE could use msg.c format when handling tee memory. Linux
and OP-TEE Scmi transport discovery negotiate the channel type and
support for msg format could allow OP-TEE to use its shm management. I
will prepare an implementation but if you prefer the current remove
support and later introduce back tee shm support, I'm fine.

Best regards,
Etienne

>
> LGTM.
>
> Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
>
> Thanks,
> Cristian

Re: [PATCH] firmware: arm_scmi: optee: Drop the support for the OPTEE shared dynamic buffer

[Sudeep Holla]

On Fri, Nov 26, 2021 at 08:59:45AM +0100, Etienne Carriere wrote:
> Hello Sudeep,
> 
> On Thu, 25 Nov 2021 at 19:25, Cristian Marussi <cristian.marussi@arm.com> wrote:
> >
> > On Thu, Nov 25, 2021 at 03:07:30PM +0000, Sudeep Holla wrote:
> > > The shared memory buffer allocated by the optee driver is normal cached
> > > memory and can't be used with IOMEM APIs used in shmem_*.
> > >
> > > We currently support only IO memory for shared memory and supporting
> > > normal cached memory needs more changes and needs to be thought through
> > > properly. So for now, let us drop the support for this OPTEE shared buffer.
> > >
> > > Cc: Cristian Marussi <cristian.marussi@arm.com>
> > > Cc: Etienne Carriere <etienne.carriere@linaro.org>
> > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > > ---
> >
> > Hi,
> >
> >
> > >  drivers/firmware/arm_scmi/optee.c | 19 +------------------
> > >  1 file changed, 1 insertion(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
> > > index 901737c9f5f8..175b39bcd470 100644
> > > --- a/drivers/firmware/arm_scmi/optee.c
> > > +++ b/drivers/firmware/arm_scmi/optee.c
> > > @@ -282,23 +282,6 @@ static void scmi_optee_clear_channel(struct scmi_chan_info *cinfo)
> > >       shmem_clear_channel(channel->shmem);
> > >  }
> > >
> > > -static int setup_dynamic_shmem(struct device *dev, struct scmi_optee_channel *channel)
> > > -{
> > > -     const size_t msg_size = SCMI_OPTEE_MAX_MSG_SIZE;
> > > -
> > > -     channel->tee_shm = tee_shm_alloc_kernel_buf(scmi_optee_private->tee_ctx, msg_size);
> > > -     if (IS_ERR(channel->tee_shm)) {
> > > -             dev_err(channel->cinfo->dev, "shmem allocation failed\n");
> > > -             return -ENOMEM;
> > > -     }
> > > -
> > > -     channel->shmem = (void *)tee_shm_get_va(channel->tee_shm, 0);
> > > -     memset(channel->shmem, 0, msg_size);
> > > -     shmem_clear_channel(channel->shmem);
> > > -
> > > -     return 0;
> > > -}
> > > -
> > >  static int setup_static_shmem(struct device *dev, struct scmi_chan_info *cinfo,
> > >                             struct scmi_optee_channel *channel)
> > >  {
> > > @@ -342,7 +325,7 @@ static int setup_shmem(struct device *dev, struct scmi_chan_info *cinfo,
> > >       if (of_find_property(cinfo->dev->of_node, "shmem", NULL))
> > >               return setup_static_shmem(dev, cinfo, channel);
> > >       else
> > > -             return setup_dynamic_shmem(dev, channel);
> > > +             return -ENOMEM;
> > >  }
> > >
>
> I would rather find an alternate way to support tee shared memory.

Sure

> I think OP-TEE could use msg.c format when handling tee memory.

Okay

> Linux and OP-TEE Scmi transport discovery negotiate the channel type and
> support for msg format could allow OP-TEE to use its shm management.

I am fine with that, just that what we have in for-next/scmi is broken and
I want to remove the support just because it is buggy and not because I
disagree with the requirement.

> I will prepare an implementation but if you prefer the current remove
> support and later introduce back tee shm support, I'm fine.
>

Sure, we may need to support this in a generic way. I mean in a way, other
transport can also use them if they need it. I remember someone else had
asked this in the past.

So yes, I am happy to merge the support for tee shm when that is ready.
What we have now is buggy and needs to be dropped. Sorry for not identifying
it early.

--
Regards,
Sudeep

Re: [PATCH] firmware: arm_scmi: optee: Drop the support for the OPTEE shared dynamic buffer

[Etienne Carriere]

On Fri, 26 Nov 2021 at 15:28, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Fri, Nov 26, 2021 at 08:59:45AM +0100, Etienne Carriere wrote:
> > Hello Sudeep,
> >
> > On Thu, 25 Nov 2021 at 19:25, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > >
> > > On Thu, Nov 25, 2021 at 03:07:30PM +0000, Sudeep Holla wrote:
> > > > The shared memory buffer allocated by the optee driver is normal cached
> > > > memory and can't be used with IOMEM APIs used in shmem_*.
> > > >
> > > > We currently support only IO memory for shared memory and supporting
> > > > normal cached memory needs more changes and needs to be thought through
> > > > properly. So for now, let us drop the support for this OPTEE shared buffer.
> > > >
> > > > Cc: Cristian Marussi <cristian.marussi@arm.com>
> > > > Cc: Etienne Carriere <etienne.carriere@linaro.org>
> > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > > > ---
> > >
> > > Hi,
> > >
> > >
> > > >  drivers/firmware/arm_scmi/optee.c | 19 +------------------
> > > >  1 file changed, 1 insertion(+), 18 deletions(-)
> > > >
> > > > diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
> > > > index 901737c9f5f8..175b39bcd470 100644
> > > > --- a/drivers/firmware/arm_scmi/optee.c
> > > > +++ b/drivers/firmware/arm_scmi/optee.c
> > > > @@ -282,23 +282,6 @@ static void scmi_optee_clear_channel(struct scmi_chan_info *cinfo)
> > > >       shmem_clear_channel(channel->shmem);
> > > >  }
> > > >
> > > > -static int setup_dynamic_shmem(struct device *dev, struct scmi_optee_channel *channel)
> > > > -{
> > > > -     const size_t msg_size = SCMI_OPTEE_MAX_MSG_SIZE;
> > > > -
> > > > -     channel->tee_shm = tee_shm_alloc_kernel_buf(scmi_optee_private->tee_ctx, msg_size);
> > > > -     if (IS_ERR(channel->tee_shm)) {
> > > > -             dev_err(channel->cinfo->dev, "shmem allocation failed\n");
> > > > -             return -ENOMEM;
> > > > -     }
> > > > -
> > > > -     channel->shmem = (void *)tee_shm_get_va(channel->tee_shm, 0);
> > > > -     memset(channel->shmem, 0, msg_size);
> > > > -     shmem_clear_channel(channel->shmem);
> > > > -
> > > > -     return 0;
> > > > -}
> > > > -
> > > >  static int setup_static_shmem(struct device *dev, struct scmi_chan_info *cinfo,
> > > >                             struct scmi_optee_channel *channel)
> > > >  {
> > > > @@ -342,7 +325,7 @@ static int setup_shmem(struct device *dev, struct scmi_chan_info *cinfo,
> > > >       if (of_find_property(cinfo->dev->of_node, "shmem", NULL))
> > > >               return setup_static_shmem(dev, cinfo, channel);
> > > >       else
> > > > -             return setup_dynamic_shmem(dev, channel);
> > > > +             return -ENOMEM;
> > > >  }
> > > >
> >
> > I would rather find an alternate way to support tee shared memory.
>
> Sure
>
> > I think OP-TEE could use msg.c format when handling tee memory.
>
> Okay
>
> > Linux and OP-TEE Scmi transport discovery negotiate the channel type and
> > support for msg format could allow OP-TEE to use its shm management.
>
> I am fine with that, just that what we have in for-next/scmi is broken and
> I want to remove the support just because it is buggy and not because I
> disagree with the requirement.
>
> > I will prepare an implementation but if you prefer the current remove
> > support and later introduce back tee shm support, I'm fine.
> >
>
> Sure, we may need to support this in a generic way. I mean in a way, other
> transport can also use them if they need it. I remember someone else had
> asked this in the past.
>
> So yes, I am happy to merge the support for tee shm when that is ready.
> What we have now is buggy and needs to be dropped. Sorry for not identifying
> it early.
>
> --
> Regards,
> Sudeep

Re: [PATCH] firmware: arm_scmi: optee: Drop the support for the OPTEE shared dynamic buffer

[Etienne Carriere]

Hi Sudeep,

(sorry, previous mail was empty)

On Fri, 26 Nov 2021 at 15:28, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Fri, Nov 26, 2021 at 08:59:45AM +0100, Etienne Carriere wrote:
> > Hello Sudeep,
> >
> > On Thu, 25 Nov 2021 at 19:25, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > >
> > > On Thu, Nov 25, 2021 at 03:07:30PM +0000, Sudeep Holla wrote:
> > > > The shared memory buffer allocated by the optee driver is normal cached
> > > > memory and can't be used with IOMEM APIs used in shmem_*.
> > > >
> > > > We currently support only IO memory for shared memory and supporting
> > > > normal cached memory needs more changes and needs to be thought through
> > > > properly. So for now, let us drop the support for this OPTEE shared buffer.
> > > >
> > > > Cc: Cristian Marussi <cristian.marussi@arm.com>
> > > > Cc: Etienne Carriere <etienne.carriere@linaro.org>
> > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > > > ---
> > >
> > > Hi,
> > >
> > >
> > > >  drivers/firmware/arm_scmi/optee.c | 19 +------------------
> > > >  1 file changed, 1 insertion(+), 18 deletions(-)
> > > >
> > > > diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
> > > > index 901737c9f5f8..175b39bcd470 100644
> > > > --- a/drivers/firmware/arm_scmi/optee.c
> > > > +++ b/drivers/firmware/arm_scmi/optee.c
> > > > @@ -282,23 +282,6 @@ static void scmi_optee_clear_channel(struct scmi_chan_info *cinfo)
> > > >       shmem_clear_channel(channel->shmem);
> > > >  }
> > > >
> > > > -static int setup_dynamic_shmem(struct device *dev, struct scmi_optee_channel *channel)
> > > > -{
> > > > -     const size_t msg_size = SCMI_OPTEE_MAX_MSG_SIZE;
> > > > -
> > > > -     channel->tee_shm = tee_shm_alloc_kernel_buf(scmi_optee_private->tee_ctx, msg_size);
> > > > -     if (IS_ERR(channel->tee_shm)) {
> > > > -             dev_err(channel->cinfo->dev, "shmem allocation failed\n");
> > > > -             return -ENOMEM;
> > > > -     }
> > > > -
> > > > -     channel->shmem = (void *)tee_shm_get_va(channel->tee_shm, 0);
> > > > -     memset(channel->shmem, 0, msg_size);
> > > > -     shmem_clear_channel(channel->shmem);
> > > > -
> > > > -     return 0;
> > > > -}
> > > > -
> > > >  static int setup_static_shmem(struct device *dev, struct scmi_chan_info *cinfo,
> > > >                             struct scmi_optee_channel *channel)
> > > >  {
> > > > @@ -342,7 +325,7 @@ static int setup_shmem(struct device *dev, struct scmi_chan_info *cinfo,
> > > >       if (of_find_property(cinfo->dev->of_node, "shmem", NULL))
> > > >               return setup_static_shmem(dev, cinfo, channel);
> > > >       else
> > > > -             return setup_dynamic_shmem(dev, channel);
> > > > +             return -ENOMEM;
> > > >  }
> > > >
> >
> > I would rather find an alternate way to support tee shared memory.
>
> Sure
>
> > I think OP-TEE could use msg.c format when handling tee memory.
>
> Okay
>
> > Linux and OP-TEE Scmi transport discovery negotiate the channel type and
> > support for msg format could allow OP-TEE to use its shm management.
>
> I am fine with that, just that what we have in for-next/scmi is broken and
> I want to remove the support just because it is buggy and not because I
> disagree with the requirement.
>
> > I will prepare an implementation but if you prefer the current remove
> > support and later introduce back tee shm support, I'm fine.
> >
>
> Sure, we may need to support this in a generic way. I mean in a way, other
> transport can also use them if they need it. I remember someone else had
> asked this in the past.
>
> So yes, I am happy to merge the support for tee shm when that is ready.
> What we have now is buggy and needs to be dropped. Sorry for not identifying
> it early.
>

Fine. Sorry for the burden.
`Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>` for the changes
By the way Yes I tested tee shm setup on read hardware, with tee
cached shared memory. shmem.c uses ioread()/iowrite() that adds some
memory barriers. I don't think it hurts to do so when accessing cached
memory.

br,
etienne


> --
> Regards,
> Sudeep

Re: [PATCH] firmware: arm_scmi: optee: Drop the support for the OPTEE shared dynamic buffer

[Sudeep Holla]

On Thu, 25 Nov 2021 15:07:30 +0000, Sudeep Holla wrote:
> The shared memory buffer allocated by the optee driver is normal cached
> memory and can't be used with IOMEM APIs used in shmem_*.
> 
> We currently support only IO memory for shared memory and supporting
> normal cached memory needs more changes and needs to be thought through
> properly. So for now, let us drop the support for this OPTEE shared buffer.
> 
> [...]

Applied to sudeep.holla/linux (for-next/scmi), thanks!

[1/1] firmware: arm_scmi: optee: Drop the support for the OPTEE shared dynamic buffer
      https://git.kernel.org/sudeep.holla/c/afc9c1e26b

--
Regards,
Sudeep