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

[Krzysztof Kozlowski <krzk@kernel.org>]

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.

> 
> 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.

Best regards,
Krzysztof