| 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
| ||
|
Date: Thu, 18 Jan 2024 17:22:21 +0000
From: Sudeep Holla <sudeep.holla@....com>
To: Sibi Sankar <quic_sibis@...cinc.com>
Cc: <cristian.marussi@....com>, <andersson@...nel.org>,
<konrad.dybcio@...aro.org>, <jassisinghbrar@...il.com>,
Sudeep Holla <sudeep.holla@....com>, <robh+dt@...nel.org>,
<krzysztof.kozlowski+dt@...aro.org>, <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 Wed, Jan 17, 2024 at 11:04:54PM +0530, 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>
> ---
> drivers/firmware/arm_scmi/Kconfig | 11 ++
> drivers/firmware/arm_scmi/Makefile | 1 +
> drivers/firmware/arm_scmi/qcom_scmi_vendor.c | 160 +++++++++++++++++++
> include/linux/qcom_scmi_vendor.h | 36 +++++
> 4 files changed, 208 insertions(+)
> create mode 100644 drivers/firmware/arm_scmi/qcom_scmi_vendor.c
> create mode 100644 include/linux/qcom_scmi_vendor.h
>
> diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> index aa5842be19b2..86b5d6c18ec4 100644
> --- a/drivers/firmware/arm_scmi/Kconfig
> +++ b/drivers/firmware/arm_scmi/Kconfig
> @@ -180,4 +180,15 @@ config ARM_SCMI_POWER_CONTROL
> called scmi_power_control. Note this may needed early in boot to catch
> early shutdown/reboot SCMI requests.
>
> +config QCOM_SCMI_VENDOR_PROTOCOL
> + tristate "Qualcomm Technologies, Inc. Qcom SCMI vendor Protocol"
> + depends on ARM || ARM64 || COMPILE_TEST
> + depends on ARM_SCMI_PROTOCOL
> + help
> + The SCMI QCOM vendor protocol provides interface to communicate with SCMI
> + controller and enable vendor specific features like bus scaling.
> +
I assume it will include all the Qualcomm specific vendor protocol
handling here. Not sure how it it implemented across different platforms
and but I already assume different platforms will use same protocol id
for different things and this implementation will abstract all those
details.
> diff --git a/drivers/firmware/arm_scmi/qcom_scmi_vendor.c b/drivers/firmware/arm_scmi/qcom_scmi_vendor.c
> new file mode 100644
> index 000000000000..878b99f0d1ef
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/qcom_scmi_vendor.c
> @@ -0,0 +1,160 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/qcom_scmi_vendor.h>
> +
> +#include "common.h"
> +
> +#define EXTENDED_MSG_ID 0
This gives me no clue what this means ?
> +#define SCMI_MAX_TX_RX_SIZE 128
> +#define PROTOCOL_PAYLOAD_SIZE 16
> +#define SET_PARAM 0x10
I assume these are the actual message IDs ? Any idea why 0x0-0xF is skipped ?
I assume atleast the required 0x0-0x2 are implemented.
> +#define GET_PARAM 0x11
> +#define START_ACTIVITY 0x12
> +#define STOP_ACTIVITY 0x13
In general, good to add description of these in the implementation here
or under Documentation or a pointer to the url where I can get the info.
If documenting within the kernel, please use SCMI spec format as it may
be easy to follow the same pattern even in the vendor protocols.
> +
> +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;
> +
> + if (!ph || !ph->xops)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, SET_PARAM, size + PROTOCOL_PAYLOAD_SIZE,
> + SCMI_MAX_TX_RX_SIZE, &t);
> + if (ret)
> + return ret;
> +
> + 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);
> + *msg++ = cpu_to_le32(param_id);
> + memcpy(msg, buf, size);
> + 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)
> +{
> + int ret = -EINVAL;
> + struct scmi_xfer *t;
> + u32 *msg;
> +
> + if (!ph || !ph->xops || !buf)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, GET_PARAM, tx_size + PROTOCOL_PAYLOAD_SIZE,
> + SCMI_MAX_TX_RX_SIZE, &t);
> + if (ret)
> + return ret;
> +
> + 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);
> + *msg++ = cpu_to_le32(param_id);
> + memcpy(msg, buf, tx_size);
> + ret = ph->xops->do_xfer(ph, t);
> + 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;
> + }
> + memcpy(buf, t->rx.buf, t->rx.len);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
> +static int qcom_scmi_start_activity(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;
> +
> + if (!ph || !ph->xops)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, START_ACTIVITY, size + PROTOCOL_PAYLOAD_SIZE,
> + SCMI_MAX_TX_RX_SIZE, &t);
> + if (ret)
> + return ret;
> +
> + 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);
> + *msg++ = cpu_to_le32(param_id);
> + memcpy(msg, buf, size);
> + ret = ph->xops->do_xfer(ph, t);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
> +static int qcom_scmi_stop_activity(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;
> +
> + if (!ph || !ph->xops)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, STOP_ACTIVITY, size + PROTOCOL_PAYLOAD_SIZE,
> + SCMI_MAX_TX_RX_SIZE, &t);
> + if (ret)
> + return ret;
> +
> + 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);
> + *msg++ = cpu_to_le32(param_id);
> + memcpy(msg, buf, size);
> + ret = ph->xops->do_xfer(ph, t);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
> +static struct qcom_scmi_vendor_ops qcom_proto_ops = {
> + .set_param = qcom_scmi_set_param,
> + .get_param = qcom_scmi_get_param,
> + .start_activity = qcom_scmi_start_activity,
> + .stop_activity = qcom_scmi_stop_activity,
> +};
> +
> +static int qcom_scmi_vendor_protocol_init(const struct scmi_protocol_handle *ph)
> +{
> + u32 version;
> +
> + ph->xops->version_get(ph, &version);
> +
> + dev_info(ph->dev, "qcom scmi version %d.%d\n",
> + PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
> +
> + return 0;
> +}
> +
> +static const struct scmi_protocol qcom_scmi_vendor = {
> + .id = QCOM_SCMI_VENDOR_PROTOCOL,
As Cristian might have pointed out, this will conflict and we need better
matching to ensure each vendor and protocols with each implementation has
unique matching mechanism so that only one match occurs per protocol on
any platform.
--
Regards,
Sudeep
Powered by blists - more mailing lists