linux-kernel - Re: [RFC 3/7] firmware: arm_scmi: Add QCOM vendor protocol

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: <6980738e-207d-4cf5-9ebd-dde40d5d7c37@linaro.org>
Date: Wed, 17 Jan 2024 21:15:53 +0100
From: Konrad Dybcio <konrad.dybcio@...aro.org>
To: Sibi Sankar <quic_sibis@...cinc.com>, sudeep.holla@....com,
 cristian.marussi@....com, andersson@...nel.org, jassisinghbrar@...il.com,
 robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org
Cc: linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
 devicetree@...r.kernel.org, quic_rgottimu@...cinc.com,
 quic_kshivnan@...cinc.com, conor+dt@...nel.org,
 Amir Vajid <avajid@...cinc.com>
Subject: Re: [RFC 3/7] firmware: arm_scmi: Add QCOM vendor protocol



On 1/17/24 18:34, Sibi Sankar wrote:
> From: Shivnandan Kumar <quic_kshivnan@...cinc.com>
> 
> SCMI QCOM vendor protocol provides interface to communicate with SCMI
> controller and enable vendor specific features like bus scaling capable
> of running on it.
> 
> 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>
> Co-developed-by: Sibi Sankar <quic_sibis@...cinc.com>
> Signed-off-by: Sibi Sankar <quic_sibis@...cinc.com>
> ---

[...]

> +
> +static int qcom_scmi_set_param(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str,
> +			       u32 param_id, size_t size)
> +{
> +	int ret = -EINVAL;
> +	struct scmi_xfer *t;
> +	u32 *msg;

After you apply Dmitry's suggestions on returning -EINVAL
directly, you can also sort definitions in a reverse-Christmas-
tree order throughout the file.

> +	msg = t->tx.buf;
> +	*msg++ = cpu_to_le32(EXTENDED_MSG_ID);
> +	*msg++ = cpu_to_le32(algo_str & GENMASK(31, 0));
> +	*msg++ = cpu_to_le32((algo_str & GENMASK(63, 32)) >> 32);

lower/upper_32_bits()?

[...]

> +	if (t->rx.len > rx_size) {
> +		pr_err("SCMI received buffer size %zu is more than expected size %zu\n",
> +		       t->rx.len, rx_size);
> +		return -EMSGSIZE;

No other driver seems to be checking for this, should this:

a) go to common code
b) be ignored

?

Konrad