Re: [PATCH v12 2/9] firmware: qcom: scm: provide a read-modify-write function

[Mukesh Ojha]

On 3/3/2024 12:39 AM, Bjorn Andersson wrote:
> On Tue, Feb 27, 2024 at 09:23:01PM +0530, Mukesh Ojha wrote:
> > It is possible that different bits of a secure register is used
> > for different purpose and currently, there is no such available
> > function from SCM driver to do that; one similar usage was pointed
> > by Srinivas K. inside pinctrl-msm where interrupt configuration
> > register lying in secure region and written via read-modify-write
> > operation.
> >
> > Export qcom_scm_io_rmw() to do read-modify-write operation on secure
> > register and reuse it wherever applicable, also document scm_lock
> > to convey its usage.
> >
> > Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> > Signed-off-by: Mukesh Ojha <quic_mojha@xxxxxxxxxxx>
> > Tested-by: Kathiravan Thirumoorthy <quic_kathirav@xxxxxxxxxxx> # IPQ9574 and IPQ5332
> > ---
> >   drivers/firmware/qcom/qcom_scm.c       | 33 ++++++++++++++++++++++++++
> >   include/linux/firmware/qcom/qcom_scm.h |  1 +
> >   2 files changed, 34 insertions(+)
> >
> > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> > index 2d0ba529cf56..8f766fce5f7c 100644
> > --- a/drivers/firmware/qcom/qcom_scm.c
> > +++ b/drivers/firmware/qcom/qcom_scm.c
> > @@ -193,6 +193,11 @@ static void qcom_scm_bw_disable(void)
> >   }
> >   
> >   enum qcom_scm_convention qcom_scm_convention = SMC_CONVENTION_UNKNOWN;
> > +/*
> > + * scm_lock to serialize call to query SMC convention and
> > + * to atomically operate(read-modify-write) on different
> > + * bits of secure register.
> > + */
> >   static DEFINE_SPINLOCK(scm_lock);
> >   
> >   static enum qcom_scm_convention __get_convention(void)
> > @@ -481,6 +486,34 @@ static int qcom_scm_disable_sdi(void)
> >   	return ret ? : res.result[0];
> >   }
> >   
> > +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val)
> > +{
> > +	unsigned long flags;
> > +	unsigned int old;
> > +	unsigned int new;
> > +	int ret;
> > +
> > +	if (!__scm)
> > +		return -EPROBE_DEFER;
> > +
> > +	/*
> > +	 * Lock to atomically do rmw operation on different bits
> > +	 * of secure register
> > +	 */
>
> A spinlock does not make something globally atomic, all you have made
> sure is that:
> 1) qcom_scm_io_rmw() can not happen concurrently with __get_convention()
>
> The reuse of the lock makes me wonder what the use case you're having a
> need to protect #1... When is rmw happening concurrently with convention
> detection?
>
> 2) Two qcom_scm_io_rmw() can not happen concurrently
>
> What happens if a separate process invokes qcom_scm_io_write() of the
> same address concurrently with qcom_scm_io_rmw()?

Yes, that is not protected..

> Quite likely neither of these will happen in practice, and I'm guessing
> that there will not be any caching issues etc among different calls to
> qcom_scm_io_*(), and hence this spinlock serves just to complicate the
> implementation.
>
> Please add a kernel-doc comment to this function (and perhaps
> qcom_scm_io_write()) and describe that we don't guarantee this operation
> to happen atomically - or if you have a valid reason, document that and
> it's exact limitations.

Sure, that make sense !! it is possible for a call to be preempted right
before it reaches to Trust zone(TZ) and it is not being handled 
inherently from SCM driver.

To further add, qcom_scm_io_{read|write}() atomic calls to TZ which
makes sure the does not get interrupted while it is in trust zone by
disabling interrupts and it is other way with non-atomic calls.

> PS. I would have been perfectly happy with us not adding a rmw function.
> You're adding 34 lines of code to save 2*3 lines of duplicated, easy to
> understand, code.

I agree with you..

Global scm spin lock would have only made sense if there could be some
resources to share from secure to non-secure and here, addresses are 
specific to the client driver and lock does need to be taken by the 
client and their address can not get overwritten by other drivers.
So, we already discussed on regmap which does not fit here at scale.

Currently, we have only one place where we have secure rmw() operation
in Qualcomm msm pinctrl driver and that seems to protected 
spin_lock_irqsave(), similarly others can do the same way if there
is a chance of race on the same address.

-Mukesh

> Regards,
> Bjorn
>
> > +	spin_lock_irqsave(&scm_lock, flags);
> > +	ret = qcom_scm_io_readl(addr, &old);
> > +	if (ret)
> > +		goto unlock;
> > +
> > +	new = (old & ~mask) | (val & mask);
> > +
> > +	ret = qcom_scm_io_writel(addr, new);
> > +unlock:
> > +	spin_unlock_irqrestore(&scm_lock, flags);
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(qcom_scm_io_rmw);
> > +
> >   static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
> >   {
> >   	struct qcom_scm_desc desc = {
> > diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
> > index ccaf28846054..3a8bb2e603b3 100644
> > --- a/include/linux/firmware/qcom/qcom_scm.h
> > +++ b/include/linux/firmware/qcom/qcom_scm.h
> > @@ -82,6 +82,7 @@ bool qcom_scm_pas_supported(u32 peripheral);
> >   
> >   int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);
> >   int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
> > +int qcom_scm_io_rmw(phys_addr_t addr, unsigned int mask, unsigned int val);
> >   
> >   bool qcom_scm_restore_sec_cfg_available(void);
> >   int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
> > --
> > 2.43.0.254.ga26002b62827