linux-kernel - Re: [PATCH V4 2/2] mailbox: tmelite-qmp: Introduce TMEL QMP mailbox driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <597fa937-56a3-4adf-90cc-8ac95dac5cb3@quicinc.com>
Date: Tue, 20 May 2025 15:06:07 +0530
From: Sricharan Ramabadhran <quic_srichara@...cinc.com>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>, <jassisinghbrar@...il.com>,
        <robh@...nel.org>, <krzk+dt@...nel.org>, <conor+dt@...nel.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <andersson@...nel.org>,
        <konradybcio@...nel.org>, <manivannan.sadhasivam@...aro.org>,
        <dmitry.baryshkov@...aro.org>
Subject: Re: [PATCH V4 2/2] mailbox: tmelite-qmp: Introduce TMEL QMP mailbox
 driver



On 4/26/2025 3:19 PM, Konrad Dybcio wrote:
> On 3/27/25 7:17 PM, Sricharan R wrote:
>> From: Sricharan Ramabadhran <quic_srichara@...cinc.com>
>>
>> This mailbox facilitates the communication between the TMEL server
>> subsystem (Trust Management Engine Lite) and the TMEL client
>> (APPSS/BTSS/AUDIOSS), used for secure services like secure image
>> authentication, enable/disable efuses, crypto services etc. Each client in
>> the SoC has its own block of message RAM and IRQ for communication with the
>> TMEL SS. The protocol used to communicate in the message RAM is known as
>> Qualcomm Messaging Protocol (QMP).
>>
>> Remote proc driver subscribes to this mailbox and uses the
>> mbox_send_message to use TMEL to securely authenticate/teardown the images.
>>
>> Signed-off-by: Sricharan Ramabadhran <quic_srichara@...cinc.com>
>> ---
> 
> [...]
> 
>> +
>> +#define QMP_NUM_CHANS		0x1
> 
> Quantities make more sense in decimal, but since this is effectively
> a single-use value, you can put in the '1' literal in num_chans and use
> devm_kzalloc instead of devm_kcalloc in the other use
> 
ok

>> +#define QMP_TOUT_MS		1000
> 
> "TIMEOUT"
>
ok

>> +#define QMP_CTRL_DATA_SIZE	4
>> +#define QMP_MAX_PKT_SIZE	0x18
> 
> This is very handwavy, please structurize all data that comes in and
> out of the mailbox.
>
ok

>> +#define QMP_UCORE_DESC_OFFSET	0x1000
>> +#define QMP_SEND_TIMEOUT	30000
> 
> Please include the unit in the macro name - although 30s is quite a
> timeout for a couple bytes..
> 
ok, will reduce timeout as well

> [...]
> 
>> +#define QMP_HW_MBOX_SIZE		32
>> +#define QMP_MBOX_RSV_SIZE		4
>> +#define QMP_MBOX_IPC_PACKET_SIZE	(QMP_HW_MBOX_SIZE - QMP_CTRL_DATA_SIZE - QMP_MBOX_RSV_SIZE)
>> +#define QMP_MBOX_IPC_MAX_PARAMS		5
>> +
>> +#define QMP_MAX_PARAM_IN_PARAM_ID	14
>> +#define QMP_PARAM_CNT_FOR_OUTBUF	3
>> +#define QMP_SRAM_IPC_MAX_PARAMS		(QMP_MAX_PARAM_IN_PARAM_ID * QMP_PARAM_CNT_FOR_OUTBUF)
>> +#define QMP_SRAM_IPC_MAX_BUF_SIZE	(QMP_SRAM_IPC_MAX_PARAMS * sizeof(u32))
> 
> These should be expressed in terms of structures and sizeof() instead,
> as well
ok

> 
>> +
>> +#define TMEL_ERROR_GENERIC		(0x1u)
>> +#define TMEL_ERROR_NOT_SUPPORTED	(0x2u)
>> +#define TMEL_ERROR_BAD_PARAMETER	(0x3u)
>> +#define TMEL_ERROR_BAD_MESSAGE		(0x4u)
>> +#define TMEL_ERROR_BAD_ADDRESS		(0x5u)
>> +#define TMEL_ERROR_TMELCOM_FAILURE	(0x6u)
>> +#define TMEL_ERROR_TMEL_BUSY		(0x7u)
> 
> Oh I didn't notice this during the first review.. I assume these are
> returned by the mbox. Please create a dictionary such as:
> 
> u32 tmel_error_dict[] = {
> 	[TMEL_ERROR_GENERIC] = EINVAL,
> 	[TMEL_ERROR_NOT_SUPPORTED] = EOPNOTSUPP
> 	...
> };
> 
> that we can then plug into the function down below that currently does
> error ? -EINVAL : 0

ok, agree. will add.

Regards,
Sricharan