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:

quoted

quoted

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

Hi Sudeep,

quoted

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:

quoted

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>

[...]

quoted

+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.

quoted

Ignore my upper comments. How do think the following diff based on

current patch?

quoted

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,

quoted

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

quoted

        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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel