Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Peng Fan <peng.fan@nxp.com>
To:     Sudeep Holla <sudeep.holla@arm.com>
CC:     "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "viresh.kumar@linaro.org" <viresh.kumar@linaro.org>,
        "f.fainelli@gmail.com" <f.fainelli@gmail.com>,
        dl-linux-imx <linux-imx@nxp.com>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
Subject: RE: [PATCH V4 2/2] firmware: arm_scmi: add smc/hvc transport
Thread-Topic: [PATCH V4 2/2] firmware: arm_scmi: add smc/hvc transport
Thread-Index: AQHV8QFMuMBBDfh8hE244PoeMX235qg4QIWAgAAijkCAAAqsUIAAPdoAgAEztyA=
Date:   Thu, 5 Mar 2020 11:25:35 +0000
Message-ID: <AM0PR04MB4481B90D03D1F68573B05BE088E20@AM0PR04MB4481.eurprd04.prod.outlook.com>
References: <1583201219-15839-1-git-send-email-peng.fan@nxp.com>
 <1583201219-15839-3-git-send-email-peng.fan@nxp.com>
 <20200304103954.GA25004@bogus>
 <AM0PR04MB4481A6DB7339C22A848DAFC988E50@AM0PR04MB4481.eurprd04.prod.outlook.com>
 <AM0PR04MB44814B71E92C02956F4BED4588E50@AM0PR04MB4481.eurprd04.prod.outlook.com>
 <20200304170319.GB44525@bogus>
In-Reply-To: <20200304170319.GB44525@bogus>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: nxp.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 4d1a6af1-95ac-4dde-10f2-08d7c0f7ecae
X-MS-Exchange-CrossTenant-originalarrivaltime: 05 Mar 2020 11:25:35.5230
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Hl9AulB+pKSE1awOq/YP3G2MxDS93Z/GdLmYUeuuXhKvS/le0IKLBYLhisHlq98a+EawYtUgx432XJQDEe6fVg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM0PR04MB6930
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> Subject: Re: [PATCH V4 2/2] firmware: arm_scmi: add smc/hvc transport
>=20
> Hi Peng,
>=20
> 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 =3D 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.
>=20
> No, it doesn't. I hope you realised that now based on your statement belo=
w.
>=20
> >
> > 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 =3D 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;
> >  }
> >
>=20
> Yes, this may fix the issue. However I would like to know if we need to s=
upport
> multiple channels/shared memory simultaneously. It is fair requirement an=
d
> may need some work which should be fine.=20

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, bu=
t I
> would like to hear from other users of this SMC for SCMI.
>=20
> --
> Regards,
> Sudeep