Re: [PATCH 02/14] firmware: qcom: Add a generic PAS service

[Sumit Garg <sumit.garg@kernel.org>]

On Fri, Mar 06, 2026 at 12:15:01PM +0100, Krzysztof Kozlowski wrote:
> On 06/03/2026 11:50, Sumit Garg wrote:
> > From: Sumit Garg <sumit.garg@oss.qualcomm.com>
> > 
> > Qcom platforms has the legacy of using non-standard SCM calls
> > splintered over the various kernel drivers. These SCM calls aren't
> > compliant with the standard SMC calling conventions which is a
> > prerequisite to enable migration to the FF-A specifications from
> > Arm.
> > 
> > OP-TEE as an alternative trusted OS to QTEE can't support these non-
> > standard SCM calls. And even for newer architectures QTEE won't be able
> > to support SCM calls either with FF-A requirements coming in. And with
> > both OP-TEE and QTEE drivers well integrated in the TEE subsystem, it
> > makes further sense to reuse the TEE bus client drivers infrastructure.
> > 
> > The added benefit of TEE bus infrastructure is that there is support
> > for discoverable/enumerable services. With that client drivers don't
> > have to manually invoke a special SCM call to know the service status.
> > 
> > So enable the generic Peripheral Authentication Service (PAS) provided
> > by the firmware. It acts as the common layer with different TZ
> > backends plugged in whether it's an SCM implementation or a proper
> > TEE bus based PAS service implementation.
> > 
> > Signed-off-by: Sumit Garg <sumit.garg@oss.qualcomm.com>
> > ---
> >  drivers/firmware/qcom/Kconfig          |   8 +
> >  drivers/firmware/qcom/Makefile         |   1 +
> >  drivers/firmware/qcom/qcom_pas.c       | 295 +++++++++++++++++++++++++
> >  drivers/firmware/qcom/qcom_pas.h       |  53 +++++
> >  include/linux/firmware/qcom/qcom_pas.h |  41 ++++
> >  5 files changed, 398 insertions(+)
> >  create mode 100644 drivers/firmware/qcom/qcom_pas.c
> >  create mode 100644 drivers/firmware/qcom/qcom_pas.h
> >  create mode 100644 include/linux/firmware/qcom/qcom_pas.h
> > 
> > diff --git a/drivers/firmware/qcom/Kconfig b/drivers/firmware/qcom/Kconfig
> > index b477d54b495a..8653639d06db 100644
> > --- a/drivers/firmware/qcom/Kconfig
> > +++ b/drivers/firmware/qcom/Kconfig
> > @@ -6,6 +6,14 @@
> >  
> >  menu "Qualcomm firmware drivers"
> >  
> > +config QCOM_PAS
> > +	tristate
> > +	help
> > +	  Enable the generic Peripheral Authentication Service (PAS) provided
> > +	  by the firmware. It acts as the common layer with different TZ
> > +	  backends plugged in whether it's an SCM implementation or a proper
> > +	  TEE bus based PAS service implementation.
> > +
> >  config QCOM_SCM
> >  	select QCOM_TZMEM
> >  	tristate
> > diff --git a/drivers/firmware/qcom/Makefile b/drivers/firmware/qcom/Makefile
> > index 0be40a1abc13..dc5ab45f906a 100644
> > --- a/drivers/firmware/qcom/Makefile
> > +++ b/drivers/firmware/qcom/Makefile
> > @@ -8,3 +8,4 @@ qcom-scm-objs += qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o
> >  obj-$(CONFIG_QCOM_TZMEM)	+= qcom_tzmem.o
> >  obj-$(CONFIG_QCOM_QSEECOM)	+= qcom_qseecom.o
> >  obj-$(CONFIG_QCOM_QSEECOM_UEFISECAPP) += qcom_qseecom_uefisecapp.o
> > +obj-$(CONFIG_QCOM_PAS)		+= qcom_pas.o
> > diff --git a/drivers/firmware/qcom/qcom_pas.c b/drivers/firmware/qcom/qcom_pas.c
> > new file mode 100644
> > index 000000000000..dc04ff1b6be0
> > --- /dev/null
> > +++ b/drivers/firmware/qcom/qcom_pas.c
> > @@ -0,0 +1,295 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/device/devres.h>
> > +#include <linux/firmware/qcom/qcom_pas.h>
> > +#include <linux/of.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +
> > +#include "qcom_pas.h"
> > +#include "qcom_scm.h"
> > +
> > +static struct qcom_pas_ops *ops_ptr;
> 
> I really dislike this singleton design. And it is not even needed! If
> you were storing here some allocated instance of SCM/PAS I could
> understand, but singleton for only ops? Just implement one driver (so
> SCM + whatever you have here) which will decide which ops to use,
> through the probe. Really, this is neither needed nor beneficial.

The motivation here is rather quite opposite to the single monolithic
SCM driver design. The TZ services like PAS, ICE and so on are going to
be implemented as independent discoverable devices on TEE bus which
rather needs independent kernel client drivers.

Also, the single driver probe can't work here since the SCM driver is
bound to the platform bus whereas the TEE PAS driver is bound to the TEE
bus. So there is a reason for the current design.

> 
> It actually leads to more problems with this barrier handling, see
> further comments.

The barrier handling is something that I carried over from existing
implmentation but I can't see a reason why it can't be replaced with a
simple mutex. See diff below for mutex.

> ...
> 
> > +
> > +/**
> > + * qcom_pas_shutdown() - Shut down the remote processor
> > + * @pas_id:	peripheral authentication service id
> > + *
> > + * Returns 0 on success.
> > + */
> > +int qcom_pas_shutdown(u32 pas_id)
> > +{
> > +	if (ops_ptr)
> > +		return ops_ptr->shutdown(ops_ptr->dev, pas_id);
> > +
> > +	return -ENODEV;
> > +}
> > +EXPORT_SYMBOL_GPL(qcom_pas_shutdown);
> > +
> > +/**
> > + * qcom_pas_supported() - Check if the peripheral authentication service is
> > + *			  available for the given peripheral
> > + * @pas_id:	peripheral authentication service id
> > + *
> > + * Returns true if PAS is supported for this peripheral, otherwise false.
> > + */
> > +bool qcom_pas_supported(u32 pas_id)
> > +{
> > +	if (ops_ptr)
> 
> Lack of barriers here is not looking right. Existing/old code is not a
> good example, I fixed only the obvious issue, but new code should be
> correct from the beginning.
> 
> Barriers should normally be always paired, unless you have some clear
> path no concurrent execution can happen here, but such explanation is
> missing, look:

Actually concurrent execution is rather required here since TZ can
support parallel bring-up of co-processors. The synchonization is only
needed when PAS client drivers are performing a deferred probe waiting
for the service to be available. However, you are right explanation is
missing here which I will add in the next version.

> 
> > +		return ops_ptr->supported(ops_ptr->dev, pas_id);
> > +
> > +	return false;
> > +}
> > +EXPORT_SYMBOL_GPL(qcom_pas_supported);
> > +
> > +/**
> > + * qcom_pas_is_available() - Check for PAS service
> > + *
> > + * Returns true on success.
> > + */
> > +bool qcom_pas_is_available(void)
> > +{
> > +	/* The barrier is needed to synchronize with client drivers. */
> 
> 
> Here. This is pretty pointless/redundant comment. Obviously barriers are
> to synchronize with whoever is calling this and only clients are calling.
> 
> You must say something useful, not just barrier is a barrier... It's
> like documenting mutex "to synchronize".

Sure, I can expand the comments.

> 
> > +	return !!smp_load_acquire(&ops_ptr);
> 


diff --git a/drivers/firmware/qcom/qcom_pas.c b/drivers/firmware/qcom/qcom_pas.c
index dc04ff1b6be0..82312ff5a1d0 100644
--- a/drivers/firmware/qcom/qcom_pas.c
+++ b/drivers/firmware/qcom/qcom_pas.c
@@ -14,6 +14,7 @@
 #include "qcom_pas.h"
 #include "qcom_scm.h"

+static DEFINE_MUTEX(ops_mutex);
 static struct qcom_pas_ops *ops_ptr;

 /**
@@ -261,8 +262,8 @@ EXPORT_SYMBOL_GPL(qcom_pas_supported);
  */
 bool qcom_pas_is_available(void)
 {
-       /* The barrier is needed to synchronize with client drivers. */
-       return !!smp_load_acquire(&ops_ptr);
+       guard(mutex)(&ops_mutex);
+       return !!ops_ptr;
 }
 EXPORT_SYMBOL_GPL(qcom_pas_is_available);

@@ -272,11 +273,12 @@ EXPORT_SYMBOL_GPL(qcom_pas_is_available);
  */
 void qcom_pas_ops_register(struct qcom_pas_ops *ops)
 {
-       if (!qcom_pas_is_available())
-               /* The barrier is needed to synchronize with client drivers. */
-               smp_store_release(&ops_ptr, ops);
-       else
+       if (!qcom_pas_is_available()) {
+               guard(mutex)(&ops_mutex);
+               ops_ptr = ops;
+       } else {
                pr_err("qcom_pas: ops already registered\n");
+       }
 }
 EXPORT_SYMBOL_GPL(qcom_pas_ops_register);

@@ -285,8 +287,8 @@ EXPORT_SYMBOL_GPL(qcom_pas_ops_register);
  */
 void qcom_pas_ops_unregister(void)
 {
-       /* The barrier is needed to synchronize with client drivers. */
-       smp_store_release(&ops_ptr, NULL);
+       guard(mutex)(&ops_mutex);
+       ops_ptr = NULL;
 }
 EXPORT_SYMBOL_GPL(qcom_pas_ops_unregister);

-Sumit