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

On Mon, Mar 09, 2026 at 08:10:02AM +0100, Krzysztof Kozlowski wrote:
> On 09/03/2026 05:55, Sumit Garg wrote:
> > 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 <[email protected]>
> >>>
> >>> 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 <[email protected]>
> >>> ---
> >>>  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.
> 
> You still have singleton here. So if you think you do opposite to
> singleton, then drop this static.

Sure.

> 
> > 
> > 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.
> 
> Hm? Existing comments are completely useless. Your comment said just
> "barrier" basically... That's nothing useful.

Agree, following is something I plan for v2 (using mutex instead of a
barrier):

/*
 * The ops mutex here is only intended to synchronize when client drivers
 * are in parallel checking for PAS service availability. However, once the
 * PAS backend becomes available, it is allowed for multiple threads to enter
 * TZ for parallel bringup of co-processors during boot.
 */
static DEFINE_MUTEX(ops_mutex);

-Sumit