| Message ID | 20200327163654.13389-2-sudeep.holla@arm.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 18988265b87615b93cd4cf15001c8c883236a137 |
| Headers | show |
| Series | firmware: arm_scmi: Extend SMC/HVC to support multiple channels | expand |
Hi Sudeep, > Subject: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific > > In order to support multiple SMC/HVC transport channels with associated > shared memory, Does this mean each channel will have its own shared memory? Or All channels share the same shared memory? it is better to maintain the mutex per channel instead of > existing global one. If all channels shared the same memory, use per channel mutex lock will not be able to prevent other channels accessing shared memory at the same time. Thanks, Peng. > > Move the smc_mutex into the scmi_smc structure and also rename it to > shmem_lock which is more appropriate for it's use. > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/firmware/arm_scmi/smc.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/firmware/arm_scmi/smc.c > b/drivers/firmware/arm_scmi/smc.c index 336168e40f49..6dc8a88cc91b > 100644 > --- a/drivers/firmware/arm_scmi/smc.c > +++ b/drivers/firmware/arm_scmi/smc.c > @@ -27,11 +27,10 @@ > struct scmi_smc { > struct scmi_chan_info *cinfo; > struct scmi_shared_mem __iomem *shmem; > + struct mutex shmem_lock; > u32 func_id; > }; > > -static DEFINE_MUTEX(smc_mutex); > - > static bool smc_chan_available(struct device *dev, int idx) { > return true; > @@ -78,6 +77,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, > struct device *dev, > > scmi_info->func_id = func_id; > scmi_info->cinfo = cinfo; > + mutex_init(&scmi_info->shmem_lock); > cinfo->transport_info = scmi_info; > > return 0; > @@ -102,14 +102,14 @@ static int smc_send_message(struct > scmi_chan_info *cinfo, > struct scmi_smc *scmi_info = cinfo->transport_info; > struct arm_smccc_res res; > > - mutex_lock(&smc_mutex); > + mutex_lock(&scmi_info->shmem_lock); > > shmem_tx_prepare(scmi_info->shmem, xfer); > > arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res); > scmi_rx_callback(scmi_info->cinfo, > shmem_read_header(scmi_info->shmem)); > > - mutex_unlock(&smc_mutex); > + mutex_unlock(&scmi_info->shmem_lock); > > return res.a0; > } > -- > 2.17.1
On Wed, Apr 01, 2020 at 01:12:37AM +0000, Peng Fan wrote: > Hi Sudeep, > > > Subject: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific > > > > In order to support multiple SMC/HVC transport channels with associated > > shared memory, > > Does this mean each channel will have its own shared memory? Or > All channels share the same shared memory? > It depends on platform firmware and DT. If there is only one shmem at the top level scmi node, all share that single channel. If some/all protocols have their own channel, they there must be shmem entry in the corresponding child node. > it is better to maintain the mutex per channel instead of > > existing global one. > > If all channels shared the same memory, use per channel mutex lock > will not be able to prevent other channels accessing shared memory > at the same time. > No we don't create channel per protocol. If they share, we just share the channel pointer. Look at: if (!info->desc->ops->chan_available(dev, idx)) { cinfo = idr_find(idr, SCMI_PROTOCOL_BASE); if (unlikely(!cinfo)) /* Possible only if platform has no Rx */ return -EINVAL; goto idr_alloc; } If a protocol doesn't have a dedicated channel, we just assign the base protocol channel to it. We don't call chan_setup at all on that channel. Your patch assumed so but the core driver never did that. Hope this clarifies you doubt. -- Regards, Sudeep
> Subject: Re: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific > > On Wed, Apr 01, 2020 at 01:12:37AM +0000, Peng Fan wrote: > > Hi Sudeep, > > > > > Subject: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific > > > > > > In order to support multiple SMC/HVC transport channels with > > > associated shared memory, > > > > Does this mean each channel will have its own shared memory? Or All > > channels share the same shared memory? > > > > It depends on platform firmware and DT. If there is only one shmem at the top > level scmi node, all share that single channel. If some/all protocols have their > own channel, they there must be shmem entry in the corresponding child > node. > > > it is better to maintain the mutex per channel instead of > > > existing global one. > > > > If all channels shared the same memory, use per channel mutex lock > > will not be able to prevent other channels accessing shared memory at > > the same time. > > > > No we don't create channel per protocol. If they share, we just share the > channel pointer. Look at: > > if (!info->desc->ops->chan_available(dev, idx)) { > cinfo = idr_find(idr, SCMI_PROTOCOL_BASE); > if (unlikely(!cinfo)) /* Possible only if platform has no Rx */ > return -EINVAL; > goto idr_alloc; > } > > If a protocol doesn't have a dedicated channel, we just assign the base > protocol channel to it. We don't call chan_setup at all on that channel. > Your patch assumed so but the core driver never did that. > > Hope this clarifies you doubt. Yes. Thanks for the explainaiton. Thanks, Peng. > > -- > Regards, > Sudeep
> Subject: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific > > In order to support multiple SMC/HVC transport channels with associated > shared memory, it is better to maintain the mutex per channel instead of > existing global one. > > Move the smc_mutex into the scmi_smc structure and also rename it to > shmem_lock which is more appropriate for it's use. > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> Reviewed-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/firmware/arm_scmi/smc.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/firmware/arm_scmi/smc.c > b/drivers/firmware/arm_scmi/smc.c index 336168e40f49..6dc8a88cc91b > 100644 > --- a/drivers/firmware/arm_scmi/smc.c > +++ b/drivers/firmware/arm_scmi/smc.c > @@ -27,11 +27,10 @@ > struct scmi_smc { > struct scmi_chan_info *cinfo; > struct scmi_shared_mem __iomem *shmem; > + struct mutex shmem_lock; > u32 func_id; > }; > > -static DEFINE_MUTEX(smc_mutex); > - > static bool smc_chan_available(struct device *dev, int idx) { > return true; > @@ -78,6 +77,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, > struct device *dev, > > scmi_info->func_id = func_id; > scmi_info->cinfo = cinfo; > + mutex_init(&scmi_info->shmem_lock); > cinfo->transport_info = scmi_info; > > return 0; > @@ -102,14 +102,14 @@ static int smc_send_message(struct > scmi_chan_info *cinfo, > struct scmi_smc *scmi_info = cinfo->transport_info; > struct arm_smccc_res res; > > - mutex_lock(&smc_mutex); > + mutex_lock(&scmi_info->shmem_lock); > > shmem_tx_prepare(scmi_info->shmem, xfer); > > arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res); > scmi_rx_callback(scmi_info->cinfo, > shmem_read_header(scmi_info->shmem)); > > - mutex_unlock(&smc_mutex); > + mutex_unlock(&scmi_info->shmem_lock); > > return res.a0; > } > -- > 2.17.1
On Wed, Apr 01, 2020 at 09:14:36AM +0000, Peng Fan wrote: > > Subject: Re: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific > > > > On Wed, Apr 01, 2020 at 01:12:37AM +0000, Peng Fan wrote: > > > Hi Sudeep, > > > > > > > Subject: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific > > > > > > > > In order to support multiple SMC/HVC transport channels with > > > > associated shared memory, > > > > > > Does this mean each channel will have its own shared memory? Or All > > > channels share the same shared memory? > > > > > > > It depends on platform firmware and DT. If there is only one shmem at the top > > level scmi node, all share that single channel. If some/all protocols have their > > own channel, they there must be shmem entry in the corresponding child > > node. > > > > > it is better to maintain the mutex per channel instead of > > > > existing global one. > > > > > > If all channels shared the same memory, use per channel mutex lock > > > will not be able to prevent other channels accessing shared memory at > > > the same time. > > > > > > > No we don't create channel per protocol. If they share, we just share the > > channel pointer. Look at: > > > > if (!info->desc->ops->chan_available(dev, idx)) { > > cinfo = idr_find(idr, SCMI_PROTOCOL_BASE); > > if (unlikely(!cinfo)) /* Possible only if platform has no Rx */ > > return -EINVAL; > > goto idr_alloc; > > } > > > > If a protocol doesn't have a dedicated channel, we just assign the base > > protocol channel to it. We don't call chan_setup at all on that channel. > > Your patch assumed so but the core driver never did that. > > > > Hope this clarifies you doubt. > > Yes. Thanks for the explainaiton. > No worries, I should have seen this during initial review, just missed few trivial things. -- Regards, Sudeep
diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c index 336168e40f49..6dc8a88cc91b 100644 --- a/drivers/firmware/arm_scmi/smc.c +++ b/drivers/firmware/arm_scmi/smc.c @@ -27,11 +27,10 @@ struct scmi_smc { struct scmi_chan_info *cinfo; struct scmi_shared_mem __iomem *shmem; + struct mutex shmem_lock; u32 func_id; }; -static DEFINE_MUTEX(smc_mutex); - static bool smc_chan_available(struct device *dev, int idx) { return true; @@ -78,6 +77,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, scmi_info->func_id = func_id; scmi_info->cinfo = cinfo; + mutex_init(&scmi_info->shmem_lock); cinfo->transport_info = scmi_info; return 0; @@ -102,14 +102,14 @@ static int smc_send_message(struct scmi_chan_info *cinfo, struct scmi_smc *scmi_info = cinfo->transport_info; struct arm_smccc_res res; - mutex_lock(&smc_mutex); + mutex_lock(&scmi_info->shmem_lock); shmem_tx_prepare(scmi_info->shmem, xfer); arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res); scmi_rx_callback(scmi_info->cinfo, shmem_read_header(scmi_info->shmem)); - mutex_unlock(&smc_mutex); + mutex_unlock(&scmi_info->shmem_lock); return res.a0; }
In order to support multiple SMC/HVC transport channels with associated shared memory, it is better to maintain the mutex per channel instead of existing global one. Move the smc_mutex into the scmi_smc structure and also rename it to shmem_lock which is more appropriate for it's use. Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- drivers/firmware/arm_scmi/smc.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)