Re: [PATCH V4 2/2] firmware: arm_scmi: add smc/hvc transport
From: Sudeep Holla <hidden>
Date: 2020-03-04 17:03:25
Also in:
linux-arm-kernel, lkml
Hi Peng, On Wed, Mar 04, 2020 at 02:16:00PM +0000, Peng Fan wrote:
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 hunk ↗ jump to hunk
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