Re: [PATCH V4 2/2] firmware: arm_scmi: add smc/hvc transport

[Sudeep Holla <sudeep.holla@arm.com>]

Hi Peng,

On Wed, Mar 04, 2020 at 02:16:00PM +0000, Peng Fan wrote:
> > Subject: RE: [PATCH V4 2/2] firmware: arm_scmi: add smc/hvc transport
> >
> > Hi Sudeep,
> >
> > > Subject: Re: [PATCH V4 2/2] firmware: arm_scmi: add smc/hvc transport
> > >
> > > On Tue, Mar 03, 2020 at 10:06:59AM +0800, peng.fan@nxp.com wrote:
> > > > From: Peng Fan <peng.fan@nxp.com>
> > > >
> > > > Take arm,smc-id as the 1st arg, leave the other args as zero for now.
> > > > There is no Rx, only Tx because of smc/hvc not support Rx.
> > > >
> > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > >
> > > [...]
> > >
> > > > +static int smc_send_message(struct scmi_chan_info *cinfo,
> > > > +			    struct scmi_xfer *xfer)
> > > > +{
> > > > +	struct scmi_smc *scmi_info = cinfo->transport_info;
> > > > +	struct arm_smccc_res res;
> > > > +
> > > > +	shmem_tx_prepare(scmi_info->shmem, xfer);
> > >
> > > How do we protect another thread/process on another CPU going and
> > > modifying the same shmem with another request ? We may need notion of
> > > channel with associated shmem and it is protected with some lock.
> >
> > This is valid concern. But I think if shmem is shared bwteen protocols, the
> > access to shmem should be protected in
> > drivers/firmware/arm_scmi/driver.c: scmi_do_xfer, because send_message
> > and fetch_response both touches shmem
> >
> > The mailbox transport also has the issue you mentioned, I think.

No, it doesn't. I hope you realised that now based on your statement below.

>
> Ignore my upper comments. How do think the following diff based on current patch?
>
> If ok, I'll squash it with current patch and send out v5.
>
> diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
> index 88f91b68f297..7d770112f339 100644
> --- a/drivers/firmware/arm_scmi/smc.c
> +++ b/drivers/firmware/arm_scmi/smc.c
> @@ -29,6 +29,8 @@ struct scmi_smc {
>         u32 func_id;
>  };
>
> +static DEFINE_MUTEX(smc_mutex);
> +
>  static bool smc_chan_available(struct device *dev, int idx)
>  {
>         return true;
> @@ -99,11 +101,15 @@ 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);
> +
>         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);
> +
>         return res.a0;
>  }
>

Yes, this may fix the issue. However I would like to know if we need to
support multiple channels/shared memory simultaneously. It is fair
requirement and may need some work which should be fine. I just want to
make sure we don't need anything more from DT or if we need to add more
to DT bindings, we need to ensure it won't break single channel. I will
think about that, but I would like to hear from other users of this SMC
for SCMI.

--
Regards,
Sudeep