| 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
| ||
|
Message-ID: <3vj55wm7j6fz32q2pynzqikl4yysno5zdlpt6zz6xylcv237ak@bwejn7zn5o3o>
Date: Mon, 7 Oct 2024 21:13:40 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Sibi Sankar <quic_sibis@...cinc.com>
Cc: sudeep.holla@....com, cristian.marussi@....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, arm-scmi@...r.kernel.org, Amir Vajid <avajid@...cinc.com>
Subject: Re: [PATCH V4 3/5] firmware: arm_scmi: vendors: Add QCOM SCMI
Generic Extensions
On Mon, Oct 07, 2024 at 11:40:21AM GMT, Sibi Sankar wrote:
> The QCOM SCMI Generic Extensions 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.
>
> 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>
> Reviewed-by: Cristian Marussi <cristian.marussi@....com>
> ---
>
> v3:
> * Pick up Rb tag and fixup/re-order elements of the vendor ops [Christian]
> * Follow naming convention and folder structure used by IMX
> * Add missing enum in the vendor protocol and fix documentation [Konrad]
>
> drivers/firmware/arm_scmi/Kconfig | 1 +
> drivers/firmware/arm_scmi/Makefile | 1 +
> .../firmware/arm_scmi/vendors/qcom/Kconfig | 15 ++
> .../firmware/arm_scmi/vendors/qcom/Makefile | 2 +
> .../arm_scmi/vendors/qcom/qcom-generic-ext.c | 184 ++++++++++++++++++
> include/linux/scmi_qcom_protocol.h | 39 ++++
> 6 files changed, 242 insertions(+)
> create mode 100644 drivers/firmware/arm_scmi/vendors/qcom/Kconfig
> create mode 100644 drivers/firmware/arm_scmi/vendors/qcom/Makefile
> create mode 100644 drivers/firmware/arm_scmi/vendors/qcom/qcom-generic-ext.c
> create mode 100644 include/linux/scmi_qcom_protocol.h
>
> diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> index dabd874641d0..73128442d97b 100644
> --- a/drivers/firmware/arm_scmi/Kconfig
> +++ b/drivers/firmware/arm_scmi/Kconfig
> @@ -71,6 +71,7 @@ config ARM_SCMI_DEBUG_COUNTERS
>
> source "drivers/firmware/arm_scmi/transports/Kconfig"
> source "drivers/firmware/arm_scmi/vendors/imx/Kconfig"
> +source "drivers/firmware/arm_scmi/vendors/qcom/Kconfig"
>
> endif #ARM_SCMI_PROTOCOL
>
> diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> index 9ac81adff567..58cf4d656cbb 100644
> --- a/drivers/firmware/arm_scmi/Makefile
> +++ b/drivers/firmware/arm_scmi/Makefile
> @@ -12,6 +12,7 @@ scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
>
> obj-$(CONFIG_ARM_SCMI_PROTOCOL) += transports/
> obj-$(CONFIG_ARM_SCMI_PROTOCOL) += vendors/imx/
> +obj-$(CONFIG_ARM_SCMI_PROTOCOL) += vendors/qcom/
>
> obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
> obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-module.o
> diff --git a/drivers/firmware/arm_scmi/vendors/qcom/Kconfig b/drivers/firmware/arm_scmi/vendors/qcom/Kconfig
> new file mode 100644
> index 000000000000..5dd9e8a6b75f
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/vendors/qcom/Kconfig
> @@ -0,0 +1,15 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +menu "ARM SCMI QCOM Vendor Protocols"
> +
> +config QCOM_SCMI_GENERIC_EXT
> + tristate "Qualcomm Technologies, Inc. Qcom SCMI vendor Protocol"
> + depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
> + help
> + The QCOM SCMI 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 operations used by the clients.
> +endmenu
> diff --git a/drivers/firmware/arm_scmi/vendors/qcom/Makefile b/drivers/firmware/arm_scmi/vendors/qcom/Makefile
> new file mode 100644
> index 000000000000..6b98fabbebb8
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/vendors/qcom/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_QCOM_SCMI_GENERIC_EXT) += qcom-generic-ext.o
> diff --git a/drivers/firmware/arm_scmi/vendors/qcom/qcom-generic-ext.c b/drivers/firmware/arm_scmi/vendors/qcom/qcom-generic-ext.c
> new file mode 100644
> index 000000000000..f7cd949161df
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/vendors/qcom/qcom-generic-ext.c
> @@ -0,0 +1,184 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/scmi_qcom_protocol.h>
> +
> +#include "../../common.h"
> +
> +/**
> + * enum qcom_generic_ext_protocol_cmd - vendor specific commands supported by SCMI Qualcomm
> + * generic vendor protocol.
> + *
> + * This protocol is intended as a generic way of exposing a number of Qualcomm SoC
> + * specific features through a mixture of pre-determined algorithm string and param_id
> + * pairs hosted on the SCMI controller.
> + *
> + * The QCOM SCMI Vendor Protocol has the protocol id as 0x80 and vendor id set to
> + * Qualcomm and the implementation version set to 0x20000. The PROTOCOL_VERSION command
> + * returns version 1.0.
> + *
> + * @QCOM_SCMI_SET_PARAM: message_id: 0x10 is used to set the parameter of a specific algo_str
> + * hosted on QCOM SCMI Vendor Protocol. The tx len depends on the
> + * algo_str used.
> + * @QCOM_SCMI_GET_PARAM: message_id: 0x11 is used to get parameter information of a specific
> + * algo_str hosted on QCOM SCMI Vendor Protocol. The tx and rx len
> + * depends on the algo_str used.
> + * @QCOM_SCMI_START_ACTIVITY: message_id: 0x12 is used to start the activity performed by
> + * the algo_str.
> + * @QCOM_SCMI_STOP_ACTIVITY: message_id: 0x13 is used to stop a pre-existing activity
> + * performed by the algo_str.
Drop message_id's from the definitions. They duplicate enum values.
> + */
> +enum qcom_generic_ext_protocol_cmd {
> + QCOM_SCMI_SET_PARAM = 0x10,
> + QCOM_SCMI_GET_PARAM = 0x11,
> + QCOM_SCMI_START_ACTIVITY = 0x12,
> + QCOM_SCMI_STOP_ACTIVITY = 0x13,
> +};
> +
> +/**
> + * struct qcom_scmi_msg - represents the various parameters to be populated
> + * for using the QCOM SCMI Vendor Protocol
> + *
> + * @ext_id: reserved, must be zero
> + * @algo_low: lower 32 bits of the algo_str
> + * @algo_high: upper 32 bits of the algo_str
> + * @param_id: serves as token message id to the specific algo_str
> + * @buf: serves as the payload to the specified param_id and algo_str pair
> + */
> +struct qcom_scmi_msg {
> + __le32 ext_id;
> + __le32 algo_low;
> + __le32 algo_high;
> + __le32 param_id;
> + __le32 buf[];
> +};
> +
> +static int qcom_scmi_set_param(const struct scmi_protocol_handle *ph, void *buf, size_t buf_len,
> + u64 algo_str, u32 param_id)
> +{
> + struct scmi_xfer *t;
> + struct qcom_scmi_msg *msg;
> + int ret;
> +
> + ret = ph->xops->xfer_get_init(ph, QCOM_SCMI_SET_PARAM, buf_len + sizeof(*msg), 0, &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);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
> +static int qcom_scmi_get_param(const struct scmi_protocol_handle *ph, void *buf, size_t buf_len,
> + u64 algo_str, u32 param_id, size_t rx_size)
> +{
> + struct scmi_xfer *t;
> + struct qcom_scmi_msg *msg;
> + int ret;
> +
> + ret = ph->xops->xfer_get_init(ph, QCOM_SCMI_GET_PARAM, buf_len + 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));
Isn't it buf_len?
> +
> + ret = ph->xops->do_xfer(ph, t);
> + 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,
> + size_t buf_len, u64 algo_str, u32 param_id)
> +{
> + struct scmi_xfer *t;
> + struct qcom_scmi_msg *msg;
> + int ret;
> +
> + ret = ph->xops->xfer_get_init(ph, QCOM_SCMI_START_ACTIVITY, buf_len + sizeof(*msg), 0, &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));
Isn't it buf_len?
> +
> + 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,
> + size_t buf_len, u64 algo_str, u32 param_id)
> +{
> + struct scmi_xfer *t;
> + struct qcom_scmi_msg *msg;
> + int ret;
> +
> + ret = ph->xops->xfer_get_init(ph, QCOM_SCMI_STOP_ACTIVITY, buf_len + sizeof(*msg), 0, &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));
Isn't it buf_len?
> +
> + ret = ph->xops->do_xfer(ph, t);
> + ph->xops->xfer_put(ph, t);
Could you please extract a common helper that handles xfer for you?
Seeing the same code 4 times is 3 times too much in my opinion.
> +
> + return ret;
> +}
> +
> +static struct qcom_generic_ext_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_generic_ext_protocol_init(const struct scmi_protocol_handle *ph)
> +{
> + u32 version;
> +
> + ph->xops->version_get(ph, &version);
> +
> + dev_info(ph->dev, "QCOM Generic Vendor Version %d.%d\n",
> + PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
dev_dbg, please.
> +
> + return 0;
> +}
> +
> +static const struct scmi_protocol qcom_generic_ext = {
> + .id = SCMI_PROTOCOL_QCOM_GENERIC,
> + .owner = THIS_MODULE,
> + .instance_init = &qcom_generic_ext_protocol_init,
> + .ops = &qcom_proto_ops,
> + .vendor_id = "Qualcomm",
> + .impl_ver = 0x20000,
> +};
> +module_scmi_protocol(qcom_generic_ext);
> +
> +MODULE_DESCRIPTION("QCOM SCMI Generic Vendor protocol");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/scmi_qcom_protocol.h b/include/linux/scmi_qcom_protocol.h
> new file mode 100644
> index 000000000000..8f82c42e566d
> --- /dev/null
> +++ b/include/linux/scmi_qcom_protocol.h
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * SCMI Message Protocol driver QCOM extension header
> + *
> + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#ifndef _LINUX_SCMI_QCOM_PROTOCOL_H
> +#define _LINUX_SCMI_QCOM_PROTOCOL_H
> +
> +#include <linux/bitfield.h>
> +#include <linux/device.h>
It doesn't look like you need those two headers.
> +#include <linux/types.h>
> +
> +#define SCMI_PROTOCOL_QCOM_GENERIC 0x80
> +
> +struct scmi_protocol_handle;
> +
> +/**
> + * struct qcom_generic_ext_ops - represents the various operations provided
> + * by QCOM Generic Vendor Protocol
> + *
> + * @set_param: set parameter specified by param_id and algo_str pair.
> + * @get_param: retrieve parameter specified by param_id and algo_str pair.
> + * @start_activity: initiate a specific activity defined by algo_str.
> + * @stop_activity: halt previously initiated activity defined by algo_str.
> + */
> +struct qcom_generic_ext_ops {
> + int (*set_param)(const struct scmi_protocol_handle *ph, void *buf, size_t buf_len,
> + u64 algo_str, u32 param_id);
> + int (*get_param)(const struct scmi_protocol_handle *ph, void *buf, size_t buf_len,
> + u64 algo_str, u32 param_id, size_t rx_size);
> + int (*start_activity)(const struct scmi_protocol_handle *ph, void *buf, size_t buf_len,
> + u64 algo_str, u32 param_id);
> + int (*stop_activity)(const struct scmi_protocol_handle *ph, void *buf, size_t buf_len,
> + u64 algo_str, u32 param_id);
> +};
> +
> +#endif /* _LINUX_SCMI_QCOM_PROTOCOL_H */
> --
> 2.34.1
>
--
With best wishes
Dmitry
Powered by blists - more mailing lists