| 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: <a777f419-05a5-fecc-b51c-fa442502ac47@quicinc.com>
Date: Mon, 12 Feb 2024 14:01:16 +0530
From: Sibi Sankar <quic_sibis@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: <sudeep.holla@....com>, <cristian.marussi@....com>, <andersson@...nel.org>,
<konrad.dybcio@...aro.org>, <jassisinghbrar@...il.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 1/18/24 00:39, Dmitry Baryshkov wrote:
> On Wed, 17 Jan 2024 at 19:36, Sibi Sankar <quic_sibis@...cinc.com> 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.
>>
Hey Dmitry,
Thanks for taking time to review the series!
Will get all of these done in the next re-spin.
>> 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.
>> +
>> + This driver defines the commands or message ID's used for this
>> + communication and also exposes the ops used by the clients.
>> +
>> endmenu
>> diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
>> index a7bc4796519c..eaeb788b93c6 100644
>> --- a/drivers/firmware/arm_scmi/Makefile
>> +++ b/drivers/firmware/arm_scmi/Makefile
>> @@ -17,6 +17,7 @@ obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
>> obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-module.o
>>
>> obj-$(CONFIG_ARM_SCMI_POWER_CONTROL) += scmi_power_control.o
>> +obj-$(CONFIG_QCOM_SCMI_VENDOR_PROTOCOL) += qcom_scmi_vendor.o
>>
>> ifeq ($(CONFIG_THUMB2_KERNEL)$(CONFIG_CC_IS_CLANG),yy)
>> # The use of R7 in the SMCCC conflicts with the compiler's use of R7 as a frame
>> 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
>> +#define SCMI_MAX_TX_RX_SIZE 128
>> +#define PROTOCOL_PAYLOAD_SIZE 16
>> +#define SET_PARAM 0x10
>> +#define GET_PARAM 0x11
>> +#define START_ACTIVITY 0x12
>> +#define STOP_ACTIVITY 0x13
>> +
>> +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;
>
> Drop init of ret, return -EINVAL directly here.
>
>> +
>> + 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);
>
> First, this header ops looks like a generic code which can be extracted.
>
> Second, using GENMASK here in the ops doesn't make any sense. The
> values will be limited to u32 anyway.
>
>> + 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;
>
> Drop init of ret, return -EINVAL directly here.
>
>> +
>> + 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;
>
> You can guess the comment here.
>
>> +
>> + 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,
>> + .owner = THIS_MODULE,
>> + .instance_init = &qcom_scmi_vendor_protocol_init,
>> + .ops = &qcom_proto_ops,
>> +};
>> +module_scmi_protocol(qcom_scmi_vendor);
>> +
>> +MODULE_DESCRIPTION("QTI SCMI vendor protocol");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/linux/qcom_scmi_vendor.h b/include/linux/qcom_scmi_vendor.h
>> new file mode 100644
>> index 000000000000..bde57bb18367
>> --- /dev/null
>> +++ b/include/linux/qcom_scmi_vendor.h
>> @@ -0,0 +1,36 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * QTI SCMI vendor protocol's header
>> + *
>> + * Copyright (c) 2024, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#ifndef _QCOM_SCMI_VENDOR_H
>> +#define _QCOM_SCMI_VENDOR_H
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/device.h>
>> +#include <linux/types.h>
>> +
>> +#define QCOM_SCMI_VENDOR_PROTOCOL 0x80
>> +
>> +struct scmi_protocol_handle;
>> +extern struct scmi_device *get_qcom_scmi_device(void);
>> +
>> +/**
>> + * struct qcom_scmi_vendor_ops - represents the various operations provided
>> + * by qcom scmi vendor protocol
>> + */
>> +struct qcom_scmi_vendor_ops {
>> + int (*set_param)(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str,
>> + u32 param_id, size_t size);
>> + int (*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 (*start_activity)(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str,
>> + u32 param_id, size_t size);
>> + int (*stop_activity)(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str,
>> + u32 param_id, size_t size);
>> +};
>> +
>> +#endif /* _QCOM_SCMI_VENDOR_H */
>> +
>> --
>> 2.34.1
>>
>>
>
>
Powered by blists - more mailing lists