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

[Peng Fan]

> Subject: Re: [PATCH V4 2/2] firmware: arm_scmi: add smc/hvc transport
> 
> 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@xxxxxxx wrote:
> > > > > From: Peng Fan <peng.fan@xxxxxxx>
> > > > >
> > > > > 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@xxxxxxx>
> > > >
> > > > [...]
> > > >
> > > > > +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. 

Do you have any suggestions? Currently I have not worked out an good
solution.

Thanks,
Peng.

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