| Message ID | 20211125150730.188487-1-sudeep.holla@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | firmware: arm_scmi: optee: Drop the support for the OPTEE shared dynamic buffer | expand |
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
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
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
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
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
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
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)
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(-)