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

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

On Mon, Mar 09, 2026 at 12:46:43PM +0530, Sumit Garg wrote:
> 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 <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.
> > 
> > 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);

After more testing, it came out that there are corner cases where the
registered ops structure writes aren't visible to other cores. So indeed
a data barrier is needed instead of mutex. I will add relevant code
comments.

-Sumit