Message-ID: <8DD9C595B061AB97+b0f78150-3930-43ac-ac56-b57f24afb5d9@radxa.com>
Date: Sat, 20 Dec 2025 01:16:17 +0800
From: Junhao Xie <bigfoot@...xa.com>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>,
 Bjorn Andersson <andersson@...nel.org>,
 Konrad Dybcio <konradybcio@...nel.org>
Cc: Xilin Wu <sophon@...xa.com>, Miquel Raynal <miquel.raynal@...tlin.com>,
 Richard Weinberger <richard@....at>, Vignesh Raghavendra <vigneshr@...com>,
 linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
 linux-mtd@...ts.infradead.org, Junhao Xie <bigfoot@...xa.com>
Subject: Re: [PATCH 1/2] firmware: qcom: scm: Add SCM storage interface
 support

On 2025/12/19 19:45, Konrad Dybcio wrote:
> On 12/18/25 7:02 PM, Junhao Xie wrote:
>> Add infrastructure to support accessing TrustZone-protected storage
>> devices through SCM (Secure Channel Manager) calls. Some Qualcomm
>> platforms protect their firmware storage (typically SPI NOR flash)
>> via TrustZone, making it inaccessible from the non-secure world.
>>
>> Currently allowlisted for Radxa Dragon Q6A (QCS6490) where it has been
>> validated. Additional platforms can be added as they are tested.
>>
>> Signed-off-by: Junhao Xie <bigfoot@...xa.com>
>> Tested-by: Xilin Wu <sophon@...xa.com>
>> ---
>>  drivers/firmware/qcom/qcom_scm.c       | 183 +++++++++++++++++++++++++
>>  drivers/firmware/qcom/qcom_scm.h       |   3 +
>>  include/linux/firmware/qcom/qcom_scm.h |  47 +++++++
>>  3 files changed, 233 insertions(+)
>>
>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>> index 731074ca1ebbe..b117e1b58e363 100644
>> --- a/drivers/firmware/qcom/qcom_scm.c
>> +++ b/drivers/firmware/qcom/qcom_scm.c
>> @@ -66,6 +66,21 @@ struct qcom_scm_mem_map_info {
>>  	__le64 mem_size;
>>  };
>>  
>> +struct qcom_scm_storage_cmd {
>> +	__le64 storage_type;
>> +	__le64 slot_num;
>> +	__le64 lun;
>> +	__le64 guid_ptr;
>> +	__le64 storage_cmd;
>> +};
>> +
>> +struct qcom_scm_storage_cmd_details {
>> +	__le64 lba;
>> +	__le64 length;
>> +	__le64 data_ptr;
>> +	__le64 data_size;
>> +};
> Let's make them __packed if only to denote that they're shared structures
> (no change to the compiler output because it's n*u64)

I will add the missing __packed to qcom_scm_storage_cmd and
qcom_scm_storage_cmd_details.

> [...]
>
>> +#if IS_ENABLED(CONFIG_MTD_QCOM_SCM_STORAGE)
> I would vouch for this to be always compiled-in

I mimicked CONFIG_QCOM_QSEECOM here, but it seems unneeded. I will remove this macro check.

>> +int qcom_scm_storage_send_cmd(enum qcom_scm_storage_type storage_type,
>> +			      enum qcom_scm_storage_cmd_id cmd_id,
>> +			      u64 lba, void *payload, size_t size)
> Please align the parameter whitespace (checkpatch, maybe w/ --strict
> should point that out)
>

I have already aligned the parameter, but mix of tabs and spaces
causes leading plus in diff to make them appear misaligned?
(checkpatch --strict doesn't report any errors or warnings.)

>> +{
>> +	struct qcom_scm_res scm_res = {};
>> +	struct qcom_scm_desc desc = {};
>> +	struct qcom_scm_storage_cmd *cmd;
>> +	struct qcom_scm_storage_cmd_details *details;
>> +	size_t buf_size;
>> +	void *payload_buf;
>> +	int ret;
> Reverse-Christmas-tree would be neat (it's in a week!)

Thanks, I will fix it.

>> +
>> +	buf_size = sizeof(*cmd) + sizeof(*details);
>> +	if (payload)
>> +		buf_size += size;
>> +	void *data __free(qcom_tzmem) = qcom_tzmem_alloc(__scm->mempool,
>> +							 buf_size,
>> +							 GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +	memset(data, 0, buf_size);
>> +	if (payload) {
>> +		payload_buf = data + sizeof(*cmd) + sizeof(*details);
>> +		memcpy(payload_buf, payload, size);
>> +	}
>> +
>> +	cmd = data;
>> +	cmd->storage_type = storage_type;
>> +	cmd->storage_cmd = cmd_id;
>> +
>> +	details = data + sizeof(*cmd);
>> +	details->lba = lba;
> I'm debating whether adding something like:
>
> struct qcom_scm_storage_payload {
> 	struct qcom_scm_storage_cmd *cmd;
> 	struct qcom_scm_storage_cmd_details *details;
> 	void *data[];
> };
>
> would improve readability, but perhaps for just 3 items it's simply not
> worth the boilerplate

Thanks for the suggestion, I will rework this for better readability in v2.

> [...]
>
>> +static int qcom_scm_storage_init(struct qcom_scm *scm)
>> +{
>> +	struct qcom_scm_storage_info info;
>> +	struct platform_device *storage_dev;
>> +	int ret;
>> +
>> +	ret = qcom_scm_storage_send_cmd(QCOM_SCM_STORAGE_SPINOR,
>> +					QCOM_SCM_STORAGE_GET_INFO,
>> +					0, &info, sizeof(info));
>> +	if (ret < 0) {
>> +		dev_info(scm->dev, "scm storage not available: %d\n", ret);
>> +		return 0;
>> +	}
> You can first call __qcom_scm_is_call_available for even more robustness
>
>> +
>> +	if (!qcom_scm_storage_machine_is_allowed()) {
>> +		dev_info(scm->dev, "scm storage untested, skipping\n");
>> +		return 0;
>> +	}
> FWIW UEFI uses these APIs, so if the implementation is correct, I see no
> reason to worry

I will switch to __qcom_scm_is_call_available instead of qcom_scm_storage_machine_is_allowed,
Thank you for your suggestion!

>> +
>> +	dev_info(scm->dev, "scm storage size %llu bytes\n",
>> +		 info.total_blocks * info.block_size);
> dev_dbg?

I will change to use dev_dbg.

>
>> +
>> +	storage_dev = platform_device_alloc("qcom_scm_storage", -1);
>> +	if (!storage_dev)
>> +		return -ENOMEM;
>> +
>> +	storage_dev->dev.parent = scm->dev;
>> +
>> +	ret = platform_device_add(storage_dev);
>> +	if (ret) {
>> +		platform_device_put(storage_dev);
>> +		return ret;
>> +	}
>> +
>> +	return devm_add_action_or_reset(scm->dev, qcom_scm_storage_free,
>> +					storage_dev);
> fauxbus?

For consistency with the existing code in this file,
I will stick to platform_device_add.

>> +}
>> +
>> +#else /* CONFIG_MTD_QCOM_SCM_STORAGE */
>> +
>> +static int qcom_scm_storage_init(struct qcom_scm *scm)
>> +{
>> +	return 0;
>> +}
>> +
>> +#endif /* CONFIG_MTD_QCOM_SCM_STORAGE */
>> +
>>  /**
>>   * qcom_scm_is_available() - Checks if SCM is available
>>   */
>> @@ -2449,6 +2626,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
>>  	/* Initialize the QTEE object interface. */
>>  	qcom_scm_qtee_init(scm);
>>  
>> +	/*
>> +	 * Initialize the SCM storage interface.
>> +	 */
> /* This fits in a single-line comment */

Thanks, I will fix it.

>> +	ret = qcom_scm_storage_init(scm);
>> +	WARN(ret < 0, "failed to initialize scm storage: %d\n", ret);
>> +
>>  	return 0;
>>  }
>>  
>> diff --git a/drivers/firmware/qcom/qcom_scm.h b/drivers/firmware/qcom/qcom_scm.h
>> index a56c8212cc0c4..3b68b33c5ccc3 100644
>> --- a/drivers/firmware/qcom/qcom_scm.h
>> +++ b/drivers/firmware/qcom/qcom_scm.h
>> @@ -149,6 +149,9 @@ int qcom_scm_shm_bridge_enable(struct device *scm_dev);
>>  #define QCOM_SCM_SMMU_CONFIG_ERRATA1		0x03
>>  #define QCOM_SCM_SMMU_CONFIG_ERRATA1_CLIENT_ALL	0x02
>>  
>> +#define QCOM_SCM_SVC_STORAGE			0x1a
>> +#define QCOM_SCM_STORAGE_CMD			0x01
>> +
>>  #define QCOM_SCM_SVC_WAITQ			0x24
>>  #define QCOM_SCM_WAITQ_RESUME			0x02
>>  #define QCOM_SCM_WAITQ_GET_WQ_CTX		0x03
>> diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
>> index a55ca771286bf..41f799d8de54f 100644
>> --- a/include/linux/firmware/qcom/qcom_scm.h
>> +++ b/include/linux/firmware/qcom/qcom_scm.h
>> @@ -53,6 +53,36 @@ enum qcom_scm_ice_cipher {
>>  	QCOM_SCM_ICE_CIPHER_AES_256_CBC = 4,
>>  };
>>  
>> +enum qcom_scm_storage_cmd_id {
>> +	QCOM_SCM_STORAGE_INIT      = 0,
>> +	QCOM_SCM_STORAGE_READ      = 1,
>> +	QCOM_SCM_STORAGE_WRITE     = 2,
>> +	QCOM_SCM_STORAGE_ERASE     = 3,
>> +	QCOM_SCM_STORAGE_GET_INFO  = 4,
>> +	QCOM_SCM_STORAGE_DEINIT    = 5,
> 6 -> _MAC_MISMATCH -> EBADMSG? (invalid data hash)
> 7 -> _ALREADY_RUNNING -> -EALREADY
> 8 -> _PARTITION_NOT_FOUND -> -ENOENT?
> 9 -> _READONLY -> -EROFS

Thank you for your addition, I will add these codes to qcom_scm_storage_result.

>> +};
>> +
>> +enum qcom_scm_storage_type {
>> +	QCOM_SCM_STORAGE_NULL    = 0,
>> +	QCOM_SCM_STORAGE_SPINOR  = 1,
>> +};
>> +
>> +#define QCOM_SCM_STORAGE_FW_VER_LEN	32
>> +#define QCOM_SCM_STORAGE_MEM_TYPE_LEN	5
>> +#define QCOM_SCM_STORAGE_PROD_NAME_LEN	32
>> +
>> +struct qcom_scm_storage_info {
>> +	u64 total_blocks;
>> +	u32 block_size;
>> +	u32 page_size;
>> +	u32 num_physical;
>> +	u64 manufacturer_id;
>> +	u64 serial_num;
>> +	char fw_version[QCOM_SCM_STORAGE_FW_VER_LEN];
>> +	char memory_type[QCOM_SCM_STORAGE_MEM_TYPE_LEN];
>> +	char product_name[QCOM_SCM_STORAGE_PROD_NAME_LEN];
> I would strongly assume all variables here are little-endian as well

I will change to using __le64 and __le32.

> Konrad
>

Thank you for the review, I will incorporate these changes in v2.

Best regards,
Junhao Xie

Powered by blists - more mailing lists