[1/4] firmware: arm_scmi: Make mutex channel specific
diff mbox series

Message ID 20200327163654.13389-2-sudeep.holla@arm.com
State New
Headers show
Series
  • firmware: arm_scmi: Extend SMC/HVC to support multiple channels
Related show

Commit Message

Sudeep Holla March 27, 2020, 4:36 p.m. UTC
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(-)

Comments

Peng Fan April 1, 2020, 1:12 a.m. UTC | #1
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
Sudeep Holla April 1, 2020, 9:12 a.m. UTC | #2
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
Peng Fan April 1, 2020, 9:14 a.m. UTC | #3
> 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
Peng Fan April 1, 2020, 9:14 a.m. UTC | #4
> 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
Sudeep Holla April 1, 2020, 9:28 a.m. UTC | #5
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

Patch
diff mbox series

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