linux-kernel - Re: [RFC V3 2/4] firmware: arm_scmi: vendors: Add ARM SCMI QCOM vendor protocol v1.0

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <489b3828-521a-712e-3976-9496ee4e4f9b@quicinc.com>
Date: Mon, 7 Oct 2024 12:59:47 +0530
From: Sibi Sankar <quic_sibis@...cinc.com>
To: Cristian Marussi <cristian.marussi@....com>
CC: <sudeep.holla@....com>, <andersson@...nel.org>, <konrad.dybcio@...aro.org>,
        <robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
        <linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>,
        <quic_rgottimu@...cinc.com>, <quic_kshivnan@...cinc.com>,
        <conor+dt@...nel.org>, Amir Vajid
	<avajid@...cinc.com>
Subject: Re: [RFC V3 2/4] firmware: arm_scmi: vendors: Add ARM SCMI QCOM
 vendor protocol v1.0



On 7/9/24 23:22, Cristian Marussi wrote:
> On Wed, Jul 03, 2024 at 12:44:38AM +0530, Sibi Sankar wrote:
>> The ARM SCMI QCOM vendor protocol provides a generic way of exposing a
>> number of Qualcomm SoC specific features (like memory bus scaling) through
>> a mixture of pre-determined algorithm strings and param_id pairs hosted on
>> the SCMI controller.
>>
> 
> Hi Sibi,
> 
>> Co-developed-by: Shivnandan Kumar <quic_kshivnan@...cinc.com>
>> Signed-off-by: Shivnandan Kumar <quic_kshivnan@...cinc.com>
>> Co-developed-by: Ramakrishna Gottimukkula <quic_rgottimu@...cinc.com>
>> Signed-off-by: Ramakrishna Gottimukkula <quic_rgottimu@...cinc.com>
>> Co-developed-by: Amir Vajid <avajid@...cinc.com>
>> Signed-off-by: Amir Vajid <avajid@...cinc.com>
>> Signed-off-by: Sibi Sankar <quic_sibis@...cinc.com>
>> ---
>>   drivers/firmware/arm_scmi/vendors/Kconfig     |  12 ++
>>   drivers/firmware/arm_scmi/vendors/Makefile    |   2 +-
>>   .../arm_scmi/vendors/qcom_scmi_vendor.c       | 184 ++++++++++++++++++
>>   include/linux/qcom_scmi_vendor.h              |  39 ++++
>>   4 files changed, 236 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/firmware/arm_scmi/vendors/qcom_scmi_vendor.c
>>   create mode 100644 include/linux/qcom_scmi_vendor.h
>>
>> diff --git a/drivers/firmware/arm_scmi/vendors/Kconfig b/drivers/firmware/arm_scmi/vendors/Kconfig
>> index 7c1ca7a12603..6bff4550fa25 100644
>> --- a/drivers/firmware/arm_scmi/vendors/Kconfig
>> +++ b/drivers/firmware/arm_scmi/vendors/Kconfig
>> @@ -1,4 +1,16 @@
>>   # SPDX-License-Identifier: GPL-2.0-only
>>   menu "ARM SCMI Vendor Protocols"
>>   
>> +config ARM_SCMI_PROTOCOL_VENDOR_QCOM
>> +	tristate "Qualcomm Technologies, Inc. Qcom SCMI vendor Protocol"
>> +	depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
>> +	help
>> +	  The SCMI QCOM vendor protocol provides a generic way of exposing a
>> +	  number of Qualcomm SoC specific features (like memory bus scaling)
>> +	  through a mixture of pre-determined algorithm strings and param_id
>> +	  pairs hosted on the SCMI controller.
>> +
>> +	  This driver defines/documents the message ID's used for this
>> +	  communication and also exposes the ops used by the clients.
> 
> operations
> 
>> +
>>   endmenu
>> diff --git a/drivers/firmware/arm_scmi/vendors/Makefile b/drivers/firmware/arm_scmi/vendors/Makefile
>> index c6c214158dd8..c1d6a355f579 100644
>> --- a/drivers/firmware/arm_scmi/vendors/Makefile
[...]
>> +++ b/drivers/firmware/arm_scmi/vendors/Makefile
>> +	if (ret)
>> +		return ret;
>> +
>> +	msg = t->tx.buf;
>> +	msg->algo_low = cpu_to_le32(lower_32_bits(algo_str));
>> +	msg->algo_high = cpu_to_le32(upper_32_bits(algo_str));
>> +	msg->param_id = cpu_to_le32(param_id);
>> +
>> +	memcpy(msg->buf, buf, t->tx.len - sizeof(*msg));
>> +
>> +	ret = ph->xops->do_xfer(ph, t);
>> +	ph->xops->xfer_put(ph, t);
>> +
>> +	return ret;
>> +}
>> +
>> +static int qcom_scmi_get_param(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str,
>> +			       u32 param_id, size_t tx_size, size_t rx_size)
>> +{
> 
> Similarly...and looking at my past ramblings...this rx_size is the expected RX
> size AND also the size of the provided *buf too, right ?
> 
>> +	struct scmi_xfer *t;
>> +	struct qcom_scmi_msg *msg;
>> +	int ret;
>> +
>> +	ret = ph->xops->xfer_get_init(ph, QCOM_SCMI_GET_PARAM, tx_size + sizeof(*msg), rx_size, &t);
>> +	if (ret)
>> +		return ret;
>> +
>> +	msg = t->tx.buf;
>> +	msg->algo_low = cpu_to_le32(lower_32_bits(algo_str));
>> +	msg->algo_high = cpu_to_le32(upper_32_bits(algo_str));
>> +	msg->param_id = cpu_to_le32(param_id);
>> +	memcpy(msg->buf, buf, t->tx.len - sizeof(*msg));
>> +
>> +	ret = ph->xops->do_xfer(ph, t);
>> +	memcpy(buf, t->rx.buf, t->rx.len);
> 
> ...so that this memcpy is safe since rx.len is equal to rx_size by construction
> (if I read correctly my past review/mublings...)
> 
> ...in that case maybe, for better clarity you could re-name the rx_size
> param as buf_len and have it following *buf in the param list...
> 
> 
> ...sorry for not having spotted this naming/order niptick earlier ...

the only caveat being rx_size can be lower than the tx_size
and we dont't want to copy more that what we expect. Addressed
all your other comments from the series in V4. Sry it took a
while due to its dependency with scmi perf changes. Thanks
again.

-Sibi

[...]
> 
> Thanks,
> Cristian