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()?
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.
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.
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
>
