From: Alexander Usyskin <alexander.usyskin@intel.com>
Add late binding component driver.
It allows pushing the late binding configuration from, for example,
the Xe graphics driver to the Intel discrete graphics card's CSE device.
Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
drivers/misc/mei/Kconfig | 1 +
drivers/misc/mei/Makefile | 1 +
drivers/misc/mei/late_bind/Kconfig | 13 +
drivers/misc/mei/late_bind/Makefile | 9 +
drivers/misc/mei/late_bind/mei_late_bind.c | 272 ++++++++++++++++++++
include/drm/intel/i915_component.h | 1 +
include/drm/intel/late_bind_mei_interface.h | 64 +++++
7 files changed, 361 insertions(+)
create mode 100644 drivers/misc/mei/late_bind/Kconfig
create mode 100644 drivers/misc/mei/late_bind/Makefile
create mode 100644 drivers/misc/mei/late_bind/mei_late_bind.c
create mode 100644 include/drm/intel/late_bind_mei_interface.h
diff --git a/drivers/misc/mei/Kconfig b/drivers/misc/mei/Kconfig
index 7575fee96cc6..771becc68095 100644
--- a/drivers/misc/mei/Kconfig
+++ b/drivers/misc/mei/Kconfig
@@ -84,5 +84,6 @@ config INTEL_MEI_VSC
source "drivers/misc/mei/hdcp/Kconfig"
source "drivers/misc/mei/pxp/Kconfig"
source "drivers/misc/mei/gsc_proxy/Kconfig"
+source "drivers/misc/mei/late_bind/Kconfig"
endif
diff --git a/drivers/misc/mei/Makefile b/drivers/misc/mei/Makefile
index 6f9fdbf1a495..84bfde888d81 100644
--- a/drivers/misc/mei/Makefile
+++ b/drivers/misc/mei/Makefile
@@ -31,6 +31,7 @@ CFLAGS_mei-trace.o = -I$(src)
obj-$(CONFIG_INTEL_MEI_HDCP) += hdcp/
obj-$(CONFIG_INTEL_MEI_PXP) += pxp/
obj-$(CONFIG_INTEL_MEI_GSC_PROXY) += gsc_proxy/
+obj-$(CONFIG_INTEL_MEI_LATE_BIND) += late_bind/
obj-$(CONFIG_INTEL_MEI_VSC_HW) += mei-vsc-hw.o
mei-vsc-hw-y := vsc-tp.o
diff --git a/drivers/misc/mei/late_bind/Kconfig b/drivers/misc/mei/late_bind/Kconfig
new file mode 100644
index 000000000000..65c7180c5678
--- /dev/null
+++ b/drivers/misc/mei/late_bind/Kconfig
@@ -0,0 +1,13 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2025, Intel Corporation. All rights reserved.
+#
+config INTEL_MEI_LATE_BIND
+ tristate "Intel late binding support on ME Interface"
+ select INTEL_MEI_ME
+ depends on DRM_XE
+ help
+ MEI Support for Late Binding for Intel graphics card.
+
+ Enables the ME FW interfaces for Late Binding feature,
+ allowing loading of firmware for the devices like Fan
+ Controller during by Intel Xe driver.
diff --git a/drivers/misc/mei/late_bind/Makefile b/drivers/misc/mei/late_bind/Makefile
new file mode 100644
index 000000000000..a0aeda5853f0
--- /dev/null
+++ b/drivers/misc/mei/late_bind/Makefile
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Copyright (c) 2025, Intel Corporation. All rights reserved.
+#
+# Makefile - Late Binding client driver for Intel MEI Bus Driver.
+
+subdir-ccflags-y += -I$(srctree)/drivers/misc/mei/
+
+obj-$(CONFIG_INTEL_MEI_LATE_BIND) += mei_late_bind.o
diff --git a/drivers/misc/mei/late_bind/mei_late_bind.c b/drivers/misc/mei/late_bind/mei_late_bind.c
new file mode 100644
index 000000000000..0a8d6b2e0666
--- /dev/null
+++ b/drivers/misc/mei/late_bind/mei_late_bind.c
@@ -0,0 +1,272 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2025 Intel Corporation
+ */
+#include <drm/intel/i915_component.h>
+#include <drm/intel/late_bind_mei_interface.h>
+#include <linux/component.h>
+#include <linux/pci.h>
+#include <linux/mei_cl_bus.h>
+#include <linux/module.h>
+#include <linux/overflow.h>
+#include <linux/slab.h>
+#include <linux/uuid.h>
+
+#include "mkhi.h"
+
+#define GFX_SRV_MKHI_LATE_BINDING_CMD 0x12
+#define GFX_SRV_MKHI_LATE_BINDING_RSP (GFX_SRV_MKHI_LATE_BINDING_CMD | 0x80)
+
+#define LATE_BIND_SEND_TIMEOUT_MSEC 3000
+#define LATE_BIND_RECV_TIMEOUT_MSEC 3000
+
+/**
+ * struct csc_heci_late_bind_req - late binding request
+ * @header: @ref mkhi_msg_hdr
+ * @type: type of the late binding payload
+ * @flags: flags to be passed to the firmware
+ * @reserved: reserved field
+ * @payload_size: size of the payload data in bytes
+ * @payload: data to be sent to the firmware
+ */
+struct csc_heci_late_bind_req {
+ struct mkhi_msg_hdr header;
+ u32 type;
+ u32 flags;
+ u32 reserved[2];
+ u32 payload_size;
+ u8 payload[] __counted_by(payload_size);
+} __packed;
+
+/**
+ * struct csc_heci_late_bind_rsp - late binding response
+ * @header: @ref mkhi_msg_hdr
+ * @type: type of the late binding payload
+ * @reserved: reserved field
+ * @status: status of the late binding command execution by firmware
+ */
+struct csc_heci_late_bind_rsp {
+ struct mkhi_msg_hdr header;
+ u32 type;
+ u32 reserved[2];
+ u32 status;
+} __packed;
+
+static int mei_late_bind_check_response(const struct device *dev, const struct mkhi_msg_hdr *hdr)
+{
+ if (hdr->group_id != MKHI_GROUP_ID_GFX) {
+ dev_err(dev, "Mismatch group id: 0x%x instead of 0x%x\n",
+ hdr->group_id, MKHI_GROUP_ID_GFX);
+ return -EINVAL;
+ }
+
+ if (hdr->command != GFX_SRV_MKHI_LATE_BINDING_RSP) {
+ dev_err(dev, "Mismatch command: 0x%x instead of 0x%x\n",
+ hdr->command, GFX_SRV_MKHI_LATE_BINDING_RSP);
+ return -EINVAL;
+ }
+
+ if (hdr->result) {
+ dev_err(dev, "Error in result: 0x%x\n", hdr->result);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int mei_late_bind_push_config(struct device *dev, enum late_bind_type type, u32 flags,
+ const void *payload, size_t payload_size)
+{
+ struct mei_cl_device *cldev;
+ struct csc_heci_late_bind_req *req = NULL;
+ struct csc_heci_late_bind_rsp rsp;
+ size_t req_size;
+ ssize_t bytes;
+ int ret;
+
+ cldev = to_mei_cl_device(dev);
+
+ ret = mei_cldev_enable(cldev);
+ if (ret) {
+ dev_dbg(dev, "mei_cldev_enable failed. %d\n", ret);
+ return ret;
+ }
+
+ req_size = struct_size(req, payload, payload_size);
+ if (req_size > mei_cldev_mtu(cldev)) {
+ dev_err(dev, "Payload is too big %zu\n", payload_size);
+ ret = -EMSGSIZE;
+ goto end;
+ }
+
+ req = kmalloc(req_size, GFP_KERNEL);
+ if (!req) {
+ ret = -ENOMEM;
+ goto end;
+ }
+
+ req->header.group_id = MKHI_GROUP_ID_GFX;
+ req->header.command = GFX_SRV_MKHI_LATE_BINDING_CMD;
+ req->type = type;
+ req->flags = flags;
+ req->reserved[0] = 0;
+ req->reserved[1] = 0;
+ req->payload_size = payload_size;
+ memcpy(req->payload, payload, payload_size);
+
+ bytes = mei_cldev_send_timeout(cldev,
+ (void *)req, req_size, LATE_BIND_SEND_TIMEOUT_MSEC);
+ if (bytes < 0) {
+ dev_err(dev, "mei_cldev_send failed. %zd\n", bytes);
+ ret = bytes;
+ goto end;
+ }
+
+ bytes = mei_cldev_recv_timeout(cldev,
+ (void *)&rsp, sizeof(rsp), LATE_BIND_RECV_TIMEOUT_MSEC);
+ if (bytes < 0) {
+ dev_err(dev, "mei_cldev_recv failed. %zd\n", bytes);
+ ret = bytes;
+ goto end;
+ }
+ if (bytes < sizeof(rsp.header)) {
+ dev_err(dev, "bad response header from the firmware: size %zd < %zu\n",
+ bytes, sizeof(rsp.header));
+ ret = -EPROTO;
+ goto end;
+ }
+ if (mei_late_bind_check_response(dev, &rsp.header)) {
+ dev_err(dev, "bad result response from the firmware: 0x%x\n",
+ *(uint32_t *)&rsp.header);
+ ret = -EPROTO;
+ goto end;
+ }
+ if (bytes < sizeof(rsp)) {
+ dev_err(dev, "bad response from the firmware: size %zd < %zu\n",
+ bytes, sizeof(rsp));
+ ret = -EPROTO;
+ goto end;
+ }
+
+ dev_dbg(dev, "%s status = %u\n", __func__, rsp.status);
+ ret = (int)rsp.status;
+end:
+ mei_cldev_disable(cldev);
+ kfree(req);
+ return ret;
+}
+
+static const struct late_bind_component_ops mei_late_bind_ops = {
+ .owner = THIS_MODULE,
+ .push_config = mei_late_bind_push_config,
+};
+
+static int mei_component_master_bind(struct device *dev)
+{
+ return component_bind_all(dev, (void *)&mei_late_bind_ops);
+}
+
+static void mei_component_master_unbind(struct device *dev)
+{
+ component_unbind_all(dev, (void *)&mei_late_bind_ops);
+}
+
+static const struct component_master_ops mei_component_master_ops = {
+ .bind = mei_component_master_bind,
+ .unbind = mei_component_master_unbind,
+};
+
+/**
+ * mei_late_bind_component_match - compare function for matching mei late bind.
+ *
+ * This function checks if requester is Intel PCI_CLASS_DISPLAY_VGA or
+ * PCI_CLASS_DISPLAY_OTHER device, and checks if the requester is the
+ * grand parent of mei_if i.e. late_bind mei device
+ *
+ * @dev: master device
+ * @subcomponent: subcomponent to match (INTEL_COMPONENT_LATE_BIND)
+ * @data: compare data (late_bind mei device on mei bus)
+ *
+ * Return:
+ * * 1 - if components match
+ * * 0 - otherwise
+ */
+static int mei_late_bind_component_match(struct device *dev, int subcomponent,
+ void *data)
+{
+ struct device *base = data;
+ struct pci_dev *pdev;
+
+ if (!dev)
+ return 0;
+
+ if (!dev_is_pci(dev))
+ return 0;
+
+ pdev = to_pci_dev(dev);
+
+ if (pdev->vendor != PCI_VENDOR_ID_INTEL)
+ return 0;
+
+ if (pdev->class != (PCI_CLASS_DISPLAY_VGA << 8) &&
+ pdev->class != (PCI_CLASS_DISPLAY_OTHER << 8))
+ return 0;
+
+ if (subcomponent != INTEL_COMPONENT_LATE_BIND)
+ return 0;
+
+ base = base->parent;
+ if (!base) /* mei device */
+ return 0;
+
+ base = base->parent; /* pci device */
+
+ return !!base && dev == base;
+}
+
+static int mei_late_bind_probe(struct mei_cl_device *cldev,
+ const struct mei_cl_device_id *id)
+{
+ struct component_match *master_match = NULL;
+ int ret;
+
+ component_match_add_typed(&cldev->dev, &master_match,
+ mei_late_bind_component_match, &cldev->dev);
+ if (IS_ERR_OR_NULL(master_match))
+ return -ENOMEM;
+
+ ret = component_master_add_with_match(&cldev->dev,
+ &mei_component_master_ops,
+ master_match);
+ if (ret < 0)
+ dev_err(&cldev->dev, "Master comp add failed %d\n", ret);
+
+ return ret;
+}
+
+static void mei_late_bind_remove(struct mei_cl_device *cldev)
+{
+ component_master_del(&cldev->dev, &mei_component_master_ops);
+}
+
+#define MEI_GUID_MKHI UUID_LE(0xe2c2afa2, 0x3817, 0x4d19, \
+ 0x9d, 0x95, 0x6, 0xb1, 0x6b, 0x58, 0x8a, 0x5d)
+
+static struct mei_cl_device_id mei_late_bind_tbl[] = {
+ { .uuid = MEI_GUID_MKHI, .version = MEI_CL_VERSION_ANY },
+ { }
+};
+MODULE_DEVICE_TABLE(mei, mei_late_bind_tbl);
+
+static struct mei_cl_driver mei_late_bind_driver = {
+ .id_table = mei_late_bind_tbl,
+ .name = KBUILD_MODNAME,
+ .probe = mei_late_bind_probe,
+ .remove = mei_late_bind_remove,
+};
+
+module_mei_cl_driver(mei_late_bind_driver);
+
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("MEI Late Binding");
diff --git a/include/drm/intel/i915_component.h b/include/drm/intel/i915_component.h
index 4ea3b17aa143..456849a97d75 100644
--- a/include/drm/intel/i915_component.h
+++ b/include/drm/intel/i915_component.h
@@ -31,6 +31,7 @@ enum i915_component_type {
I915_COMPONENT_HDCP,
I915_COMPONENT_PXP,
I915_COMPONENT_GSC_PROXY,
+ INTEL_COMPONENT_LATE_BIND,
};
/* MAX_PORT is the number of port
diff --git a/include/drm/intel/late_bind_mei_interface.h b/include/drm/intel/late_bind_mei_interface.h
new file mode 100644
index 000000000000..ec58ef1ab4e8
--- /dev/null
+++ b/include/drm/intel/late_bind_mei_interface.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright (c) 2025 Intel Corporation
+ */
+
+#ifndef _LATE_BIND_MEI_INTERFACE_H_
+#define _LATE_BIND_MEI_INTERFACE_H_
+
+#include <linux/types.h>
+
+struct device;
+struct module;
+
+/**
+ * Late Binding flags
+ * Persistent across warm reset
+ */
+#define CSC_LATE_BINDING_FLAGS_IS_PERSISTENT BIT(0)
+
+/**
+ * xe_late_bind_fw_type - enum to determine late binding fw type
+ */
+enum late_bind_type {
+ CSC_LATE_BINDING_TYPE_FAN_CONTROL = 1,
+};
+
+/**
+ * Late Binding payload status
+ */
+enum csc_late_binding_status {
+ CSC_LATE_BINDING_STATUS_SUCCESS = 0,
+ CSC_LATE_BINDING_STATUS_4ID_MISMATCH = 1,
+ CSC_LATE_BINDING_STATUS_ARB_FAILURE = 2,
+ CSC_LATE_BINDING_STATUS_GENERAL_ERROR = 3,
+ CSC_LATE_BINDING_STATUS_INVALID_PARAMS = 4,
+ CSC_LATE_BINDING_STATUS_INVALID_SIGNATURE = 5,
+ CSC_LATE_BINDING_STATUS_INVALID_PAYLOAD = 6,
+ CSC_LATE_BINDING_STATUS_TIMEOUT = 7,
+};
+
+/**
+ * struct late_bind_component_ops - ops for Late Binding services.
+ * @owner: Module providing the ops
+ * @push_config: Sends a config to FW.
+ */
+struct late_bind_component_ops {
+ struct module *owner;
+
+ /**
+ * @push_config: Sends a config to FW.
+ * @dev: device struct corresponding to the mei device
+ * @type: payload type
+ * @flags: payload flags
+ * @payload: payload buffer
+ * @payload_size: payload buffer size
+ *
+ * Return: 0 success, negative errno value on transport failure,
+ * positive status returned by FW
+ */
+ int (*push_config)(struct device *dev, u32 type, u32 flags,
+ const void *payload, size_t payload_size);
+};
+
+#endif /* _LATE_BIND_MEI_INTERFACE_H_ */
--
2.34.1On Fri, Jul 04, 2025 at 01:00:58AM +0530, Badal Nilawar wrote:
> From: Alexander Usyskin <alexander.usyskin@intel.com>
>
> Add late binding component driver.
> It allows pushing the late binding configuration from, for example,
> the Xe graphics driver to the Intel discrete graphics card's CSE device.
>
> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
> drivers/misc/mei/Kconfig | 1 +
> drivers/misc/mei/Makefile | 1 +
> drivers/misc/mei/late_bind/Kconfig | 13 +
> drivers/misc/mei/late_bind/Makefile | 9 +
> drivers/misc/mei/late_bind/mei_late_bind.c | 272 ++++++++++++++++++++
Why do you have a whole subdir for a single .c file? What's wrong with
just keepign it in drivers/misc/mei/ ?
> +/**
> + * struct csc_heci_late_bind_req - late binding request
> + * @header: @ref mkhi_msg_hdr
> + * @type: type of the late binding payload
> + * @flags: flags to be passed to the firmware
> + * @reserved: reserved field
Reserved for what? Set to what?
> + * @payload_size: size of the payload data in bytes
> + * @payload: data to be sent to the firmware
> + */
> +struct csc_heci_late_bind_req {
> + struct mkhi_msg_hdr header;
> + u32 type;
> + u32 flags;
> + u32 reserved[2];
> + u32 payload_size;
As these cross the kernel boundry, they should be the correct type
(__u32), but really, please define the endiness of them (__le32) and use
the proper macros for that.
> + u8 payload[] __counted_by(payload_size);
> +} __packed;
> +
> +/**
> + * struct csc_heci_late_bind_rsp - late binding response
> + * @header: @ref mkhi_msg_hdr
> + * @type: type of the late binding payload
> + * @reserved: reserved field
Same here.
> + * @status: status of the late binding command execution by firmware
> + */
> +struct csc_heci_late_bind_rsp {
> + struct mkhi_msg_hdr header;
> + u32 type;
> + u32 reserved[2];
> + u32 status;
Same on the types.
> +} __packed;
> +
> +static int mei_late_bind_check_response(const struct device *dev, const struct mkhi_msg_hdr *hdr)
> +{
> + if (hdr->group_id != MKHI_GROUP_ID_GFX) {
> + dev_err(dev, "Mismatch group id: 0x%x instead of 0x%x\n",
> + hdr->group_id, MKHI_GROUP_ID_GFX);
> + return -EINVAL;
> + }
> +
> + if (hdr->command != GFX_SRV_MKHI_LATE_BINDING_RSP) {
> + dev_err(dev, "Mismatch command: 0x%x instead of 0x%x\n",
> + hdr->command, GFX_SRV_MKHI_LATE_BINDING_RSP);
> + return -EINVAL;
> + }
> +
> + if (hdr->result) {
> + dev_err(dev, "Error in result: 0x%x\n", hdr->result);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int mei_late_bind_push_config(struct device *dev, enum late_bind_type type, u32 flags,
> + const void *payload, size_t payload_size)
> +{
> + struct mei_cl_device *cldev;
> + struct csc_heci_late_bind_req *req = NULL;
> + struct csc_heci_late_bind_rsp rsp;
> + size_t req_size;
> + ssize_t bytes;
> + int ret;
> +
> + cldev = to_mei_cl_device(dev);
> +
> + ret = mei_cldev_enable(cldev);
> + if (ret) {
> + dev_dbg(dev, "mei_cldev_enable failed. %d\n", ret);
> + return ret;
> + }
> +
> + req_size = struct_size(req, payload, payload_size);
> + if (req_size > mei_cldev_mtu(cldev)) {
> + dev_err(dev, "Payload is too big %zu\n", payload_size);
> + ret = -EMSGSIZE;
> + goto end;
> + }
> +
> + req = kmalloc(req_size, GFP_KERNEL);
> + if (!req) {
> + ret = -ENOMEM;
> + goto end;
> + }
> +
> + req->header.group_id = MKHI_GROUP_ID_GFX;
> + req->header.command = GFX_SRV_MKHI_LATE_BINDING_CMD;
> + req->type = type;
> + req->flags = flags;
> + req->reserved[0] = 0;
> + req->reserved[1] = 0;
> + req->payload_size = payload_size;
> + memcpy(req->payload, payload, payload_size);
> +
> + bytes = mei_cldev_send_timeout(cldev,
> + (void *)req, req_size, LATE_BIND_SEND_TIMEOUT_MSEC);
> + if (bytes < 0) {
> + dev_err(dev, "mei_cldev_send failed. %zd\n", bytes);
> + ret = bytes;
> + goto end;
> + }
> +
> + bytes = mei_cldev_recv_timeout(cldev,
> + (void *)&rsp, sizeof(rsp), LATE_BIND_RECV_TIMEOUT_MSEC);
> + if (bytes < 0) {
> + dev_err(dev, "mei_cldev_recv failed. %zd\n", bytes);
> + ret = bytes;
> + goto end;
> + }
> + if (bytes < sizeof(rsp.header)) {
> + dev_err(dev, "bad response header from the firmware: size %zd < %zu\n",
> + bytes, sizeof(rsp.header));
> + ret = -EPROTO;
> + goto end;
> + }
> + if (mei_late_bind_check_response(dev, &rsp.header)) {
> + dev_err(dev, "bad result response from the firmware: 0x%x\n",
> + *(uint32_t *)&rsp.header);
> + ret = -EPROTO;
> + goto end;
> + }
> + if (bytes < sizeof(rsp)) {
> + dev_err(dev, "bad response from the firmware: size %zd < %zu\n",
> + bytes, sizeof(rsp));
> + ret = -EPROTO;
> + goto end;
> + }
> +
> + dev_dbg(dev, "%s status = %u\n", __func__, rsp.status);
dev_dbg() already contains __func__, you never need to add it again as
you now have duplicate strings. Please remove it.
> + ret = (int)rsp.status;
> +end:
> + mei_cldev_disable(cldev);
> + kfree(req);
> + return ret;
> +}
> +
> +static const struct late_bind_component_ops mei_late_bind_ops = {
> + .owner = THIS_MODULE,
I thought you were going to drop the .owner stuff?
Or if not, please implement it properly (i.e. by NOT forcing people to
manually set it here.)
thanks,
greg k-h
On 04-07-2025 10:44, Greg KH wrote:
> On Fri, Jul 04, 2025 at 01:00:58AM +0530, Badal Nilawar wrote:
>> From: Alexander Usyskin <alexander.usyskin@intel.com>
>>
>> Add late binding component driver.
>> It allows pushing the late binding configuration from, for example,
>> the Xe graphics driver to the Intel discrete graphics card's CSE device.
>>
>> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>> Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
>> ---
>> drivers/misc/mei/Kconfig | 1 +
>> drivers/misc/mei/Makefile | 1 +
>> drivers/misc/mei/late_bind/Kconfig | 13 +
>> drivers/misc/mei/late_bind/Makefile | 9 +
>> drivers/misc/mei/late_bind/mei_late_bind.c | 272 ++++++++++++++++++++
> Why do you have a whole subdir for a single .c file? What's wrong with
> just keepign it in drivers/misc/mei/ ?
There is separate subdir for each component used by i915/xe, so one was
created for late_bind as well. Should we still drop late_bind subdir?
cd drivers/misc/mei/
gsc_proxy/ hdcp/ late_bind/ pxp/
>
>> +/**
>> + * struct csc_heci_late_bind_req - late binding request
>> + * @header: @ref mkhi_msg_hdr
>> + * @type: type of the late binding payload
>> + * @flags: flags to be passed to the firmware
>> + * @reserved: reserved field
> Reserved for what? Set to what?
Reserved by firmware for future use, default value set to 0, I will
update above doc.
>
>> + * @payload_size: size of the payload data in bytes
>> + * @payload: data to be sent to the firmware
>> + */
>> +struct csc_heci_late_bind_req {
>> + struct mkhi_msg_hdr header;
>> + u32 type;
>> + u32 flags;
>> + u32 reserved[2];
>> + u32 payload_size;
> As these cross the kernel boundry, they should be the correct type
> (__u32), but really, please define the endiness of them (__le32) and use
> the proper macros for that.
If we go with __le32 then while populating elements of structure
csc_heci_late_bind_req I will be using cpu_to_le32().
When mapping the response buffer from the firmware with struct
csc_heci_late_bind_rsp, there's no need to use le32_to_cpu() since the
response will already be in little-endian format.
Are you fine with this?
>
>> + u8 payload[] __counted_by(payload_size);
>> +} __packed;
>> +
>> +/**
>> + * struct csc_heci_late_bind_rsp - late binding response
>> + * @header: @ref mkhi_msg_hdr
>> + * @type: type of the late binding payload
>> + * @reserved: reserved field
> Same here.
Will fix this.
>
>> + * @status: status of the late binding command execution by firmware
>> + */
>> +struct csc_heci_late_bind_rsp {
>> + struct mkhi_msg_hdr header;
>> + u32 type;
>> + u32 reserved[2];
>> + u32 status;
> Same on the types.
>
>> +} __packed;
>> +
>> +static int mei_late_bind_check_response(const struct device *dev, const struct mkhi_msg_hdr *hdr)
>> +{
>> + if (hdr->group_id != MKHI_GROUP_ID_GFX) {
>> + dev_err(dev, "Mismatch group id: 0x%x instead of 0x%x\n",
>> + hdr->group_id, MKHI_GROUP_ID_GFX);
>> + return -EINVAL;
>> + }
>> +
>> + if (hdr->command != GFX_SRV_MKHI_LATE_BINDING_RSP) {
>> + dev_err(dev, "Mismatch command: 0x%x instead of 0x%x\n",
>> + hdr->command, GFX_SRV_MKHI_LATE_BINDING_RSP);
>> + return -EINVAL;
>> + }
>> +
>> + if (hdr->result) {
>> + dev_err(dev, "Error in result: 0x%x\n", hdr->result);
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int mei_late_bind_push_config(struct device *dev, enum late_bind_type type, u32 flags,
>> + const void *payload, size_t payload_size)
>> +{
>> + struct mei_cl_device *cldev;
>> + struct csc_heci_late_bind_req *req = NULL;
>> + struct csc_heci_late_bind_rsp rsp;
>> + size_t req_size;
>> + ssize_t bytes;
>> + int ret;
>> +
>> + cldev = to_mei_cl_device(dev);
>> +
>> + ret = mei_cldev_enable(cldev);
>> + if (ret) {
>> + dev_dbg(dev, "mei_cldev_enable failed. %d\n", ret);
>> + return ret;
>> + }
>> +
>> + req_size = struct_size(req, payload, payload_size);
>> + if (req_size > mei_cldev_mtu(cldev)) {
>> + dev_err(dev, "Payload is too big %zu\n", payload_size);
>> + ret = -EMSGSIZE;
>> + goto end;
>> + }
>> +
>> + req = kmalloc(req_size, GFP_KERNEL);
>> + if (!req) {
>> + ret = -ENOMEM;
>> + goto end;
>> + }
>> +
>> + req->header.group_id = MKHI_GROUP_ID_GFX;
>> + req->header.command = GFX_SRV_MKHI_LATE_BINDING_CMD;
>> + req->type = type;
>> + req->flags = flags;
>> + req->reserved[0] = 0;
>> + req->reserved[1] = 0;
>> + req->payload_size = payload_size;
>> + memcpy(req->payload, payload, payload_size);
>> +
>> + bytes = mei_cldev_send_timeout(cldev,
>> + (void *)req, req_size, LATE_BIND_SEND_TIMEOUT_MSEC);
>> + if (bytes < 0) {
>> + dev_err(dev, "mei_cldev_send failed. %zd\n", bytes);
>> + ret = bytes;
>> + goto end;
>> + }
>> +
>> + bytes = mei_cldev_recv_timeout(cldev,
>> + (void *)&rsp, sizeof(rsp), LATE_BIND_RECV_TIMEOUT_MSEC);
>> + if (bytes < 0) {
>> + dev_err(dev, "mei_cldev_recv failed. %zd\n", bytes);
>> + ret = bytes;
>> + goto end;
>> + }
>> + if (bytes < sizeof(rsp.header)) {
>> + dev_err(dev, "bad response header from the firmware: size %zd < %zu\n",
>> + bytes, sizeof(rsp.header));
>> + ret = -EPROTO;
>> + goto end;
>> + }
>> + if (mei_late_bind_check_response(dev, &rsp.header)) {
>> + dev_err(dev, "bad result response from the firmware: 0x%x\n",
>> + *(uint32_t *)&rsp.header);
>> + ret = -EPROTO;
>> + goto end;
>> + }
>> + if (bytes < sizeof(rsp)) {
>> + dev_err(dev, "bad response from the firmware: size %zd < %zu\n",
>> + bytes, sizeof(rsp));
>> + ret = -EPROTO;
>> + goto end;
>> + }
>> +
>> + dev_dbg(dev, "%s status = %u\n", __func__, rsp.status);
> dev_dbg() already contains __func__, you never need to add it again as
> you now have duplicate strings. Please remove it.
Sure.
>
>
>> + ret = (int)rsp.status;
>> +end:
>> + mei_cldev_disable(cldev);
>> + kfree(req);
>> + return ret;
>> +}
>> +
>> +static const struct late_bind_component_ops mei_late_bind_ops = {
>> + .owner = THIS_MODULE,
> I thought you were going to drop the .owner stuff?
>
> Or if not, please implement it properly (i.e. by NOT forcing people to
> manually set it here.)
Somehow I missed this. I will drop it.
Thanks,
Badal
>
> thanks,
>
> greg k-h
On Fri, Jul 04, 2025 at 03:59:40PM +0530, Nilawar, Badal wrote:
>
> On 04-07-2025 10:44, Greg KH wrote:
> > On Fri, Jul 04, 2025 at 01:00:58AM +0530, Badal Nilawar wrote:
> > > From: Alexander Usyskin <alexander.usyskin@intel.com>
> > >
> > > Add late binding component driver.
> > > It allows pushing the late binding configuration from, for example,
> > > the Xe graphics driver to the Intel discrete graphics card's CSE device.
> > >
> > > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> > > Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> > > Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > > ---
> > > drivers/misc/mei/Kconfig | 1 +
> > > drivers/misc/mei/Makefile | 1 +
> > > drivers/misc/mei/late_bind/Kconfig | 13 +
> > > drivers/misc/mei/late_bind/Makefile | 9 +
> > > drivers/misc/mei/late_bind/mei_late_bind.c | 272 ++++++++++++++++++++
> > Why do you have a whole subdir for a single .c file? What's wrong with
> > just keepign it in drivers/misc/mei/ ?
>
> There is separate subdir for each component used by i915/xe, so one was
> created for late_bind as well. Should we still drop late_bind subdir?
>
> cd drivers/misc/mei/
> gsc_proxy/ hdcp/ late_bind/ pxp/
For "modules" that are just a single file, yeah, that's silly, don't do
that.
> > > +/**
> > > + * struct csc_heci_late_bind_req - late binding request
> > > + * @header: @ref mkhi_msg_hdr
> > > + * @type: type of the late binding payload
> > > + * @flags: flags to be passed to the firmware
> > > + * @reserved: reserved field
> > Reserved for what? Set to what?
>
> Reserved by firmware for future use, default value set to 0, I will update
> above doc.
>
> >
> > > + * @payload_size: size of the payload data in bytes
> > > + * @payload: data to be sent to the firmware
> > > + */
> > > +struct csc_heci_late_bind_req {
> > > + struct mkhi_msg_hdr header;
> > > + u32 type;
> > > + u32 flags;
> > > + u32 reserved[2];
> > > + u32 payload_size;
> > As these cross the kernel boundry, they should be the correct type
> > (__u32), but really, please define the endiness of them (__le32) and use
> > the proper macros for that.
> If we go with __le32 then while populating elements of structure
> csc_heci_late_bind_req I will be using cpu_to_le32().
>
> When mapping the response buffer from the firmware with struct
> csc_heci_late_bind_rsp, there's no need to use le32_to_cpu() since the
> response will already be in little-endian format.
How do you know? Where is that defined? Where did the conversion
happen?
> Are you fine with this?
Please be explicit.
> > > + ret = (int)rsp.status;
> > > +end:
> > > + mei_cldev_disable(cldev);
> > > + kfree(req);
> > > + return ret;
> > > +}
> > > +
> > > +static const struct late_bind_component_ops mei_late_bind_ops = {
> > > + .owner = THIS_MODULE,
> > I thought you were going to drop the .owner stuff?
> >
> > Or if not, please implement it properly (i.e. by NOT forcing people to
> > manually set it here.)
>
> Somehow I missed this. I will drop it.
And from the structure definition please.
thanks,
greg k-h
On 04-07-2025 16:04, Greg KH wrote:
> On Fri, Jul 04, 2025 at 03:59:40PM +0530, Nilawar, Badal wrote:
>> On 04-07-2025 10:44, Greg KH wrote:
>>> On Fri, Jul 04, 2025 at 01:00:58AM +0530, Badal Nilawar wrote:
>>>> From: Alexander Usyskin <alexander.usyskin@intel.com>
>>>>
>>>> Add late binding component driver.
>>>> It allows pushing the late binding configuration from, for example,
>>>> the Xe graphics driver to the Intel discrete graphics card's CSE device.
>>>>
>>>> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
>>>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>>>> Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
>>>> ---
>>>> drivers/misc/mei/Kconfig | 1 +
>>>> drivers/misc/mei/Makefile | 1 +
>>>> drivers/misc/mei/late_bind/Kconfig | 13 +
>>>> drivers/misc/mei/late_bind/Makefile | 9 +
>>>> drivers/misc/mei/late_bind/mei_late_bind.c | 272 ++++++++++++++++++++
>>> Why do you have a whole subdir for a single .c file? What's wrong with
>>> just keepign it in drivers/misc/mei/ ?
>> There is separate subdir for each component used by i915/xe, so one was
>> created for late_bind as well. Should we still drop late_bind subdir?
>>
>> cd drivers/misc/mei/
>> gsc_proxy/ hdcp/ late_bind/ pxp/
> For "modules" that are just a single file, yeah, that's silly, don't do
> that.
Another reason to maintain the sub_dir is to accommodate additional
files for future platforms. If you still insist, I'll remove the sub_dir.
>
>>>> +/**
>>>> + * struct csc_heci_late_bind_req - late binding request
>>>> + * @header: @ref mkhi_msg_hdr
>>>> + * @type: type of the late binding payload
>>>> + * @flags: flags to be passed to the firmware
>>>> + * @reserved: reserved field
>>> Reserved for what? Set to what?
>> Reserved by firmware for future use, default value set to 0, I will update
>> above doc.
>>
>>>> + * @payload_size: size of the payload data in bytes
>>>> + * @payload: data to be sent to the firmware
>>>> + */
>>>> +struct csc_heci_late_bind_req {
>>>> + struct mkhi_msg_hdr header;
>>>> + u32 type;
>>>> + u32 flags;
>>>> + u32 reserved[2];
>>>> + u32 payload_size;
>>> As these cross the kernel boundry, they should be the correct type
>>> (__u32), but really, please define the endiness of them (__le32) and use
>>> the proper macros for that.
>> If we go with __le32 then while populating elements of structure
>> csc_heci_late_bind_req I will be using cpu_to_le32().
>>
>> When mapping the response buffer from the firmware with struct
>> csc_heci_late_bind_rsp, there's no need to use le32_to_cpu() since the
>> response will already be in little-endian format.
> How do you know? Where is that defined? Where did the conversion
> happen?
Sorry, I got confused. Conversion is needed when assigning the response
structure elements.
e.g ret = (int)(le32_to_cpu)rsp.status;
>
>> Are you fine with this?
> Please be explicit.
>
>>>> + ret = (int)rsp.status;
>>>> +end:
>>>> + mei_cldev_disable(cldev);
>>>> + kfree(req);
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static const struct late_bind_component_ops mei_late_bind_ops = {
>>>> + .owner = THIS_MODULE,
>>> I thought you were going to drop the .owner stuff?
>>>
>>> Or if not, please implement it properly (i.e. by NOT forcing people to
>>> manually set it here.)
>> Somehow I missed this. I will drop it.
> And from the structure definition please.
Sure.
Thanks,
Badal
>
> thanks,
>
> greg k-h
On Fri, Jul 04, 2025 at 05:18:46PM +0530, Nilawar, Badal wrote:
>
> On 04-07-2025 16:04, Greg KH wrote:
> > On Fri, Jul 04, 2025 at 03:59:40PM +0530, Nilawar, Badal wrote:
> > > On 04-07-2025 10:44, Greg KH wrote:
> > > > On Fri, Jul 04, 2025 at 01:00:58AM +0530, Badal Nilawar wrote:
> > > > > From: Alexander Usyskin <alexander.usyskin@intel.com>
> > > > >
> > > > > Add late binding component driver.
> > > > > It allows pushing the late binding configuration from, for example,
> > > > > the Xe graphics driver to the Intel discrete graphics card's CSE device.
> > > > >
> > > > > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> > > > > Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> > > > > Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > > > > ---
> > > > > drivers/misc/mei/Kconfig | 1 +
> > > > > drivers/misc/mei/Makefile | 1 +
> > > > > drivers/misc/mei/late_bind/Kconfig | 13 +
> > > > > drivers/misc/mei/late_bind/Makefile | 9 +
> > > > > drivers/misc/mei/late_bind/mei_late_bind.c | 272 ++++++++++++++++++++
> > > > Why do you have a whole subdir for a single .c file? What's wrong with
> > > > just keepign it in drivers/misc/mei/ ?
> > > There is separate subdir for each component used by i915/xe, so one was
> > > created for late_bind as well. Should we still drop late_bind subdir?
> > >
> > > cd drivers/misc/mei/
> > > gsc_proxy/ hdcp/ late_bind/ pxp/
> > For "modules" that are just a single file, yeah, that's silly, don't do
> > that.
> Another reason to maintain the sub_dir is to accommodate additional files
> for future platforms. If you still insist, I'll remove the sub_dir.
Move files around when it happens, for now, it's silly and not needed.
> > > > > + * @payload_size: size of the payload data in bytes
> > > > > + * @payload: data to be sent to the firmware
> > > > > + */
> > > > > +struct csc_heci_late_bind_req {
> > > > > + struct mkhi_msg_hdr header;
> > > > > + u32 type;
> > > > > + u32 flags;
> > > > > + u32 reserved[2];
> > > > > + u32 payload_size;
> > > > As these cross the kernel boundry, they should be the correct type
> > > > (__u32), but really, please define the endiness of them (__le32) and use
> > > > the proper macros for that.
> > > If we go with __le32 then while populating elements of structure
> > > csc_heci_late_bind_req I will be using cpu_to_le32().
> > >
> > > When mapping the response buffer from the firmware with struct
> > > csc_heci_late_bind_rsp, there's no need to use le32_to_cpu() since the
> > > response will already be in little-endian format.
> > How do you know? Where is that defined? Where did the conversion
> > happen?
>
> Sorry, I got confused. Conversion is needed when assigning the response
> structure elements.
>
> e.g ret = (int)(le32_to_cpu)rsp.status;
But these are read directly from the hardware? If not, why are they
marked as packed?
thanks,
greg k-h
> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Friday, July 4, 2025 5:31 PM
> To: Nilawar, Badal <badal.nilawar@intel.com>
> Cc: intel-xe@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-
> kernel@vger.kernel.org; Gupta, Anshuman <anshuman.gupta@intel.com>;
> Vivi, Rodrigo <rodrigo.vivi@intel.com>; Usyskin, Alexander
> <alexander.usyskin@intel.com>; Ceraolo Spurio, Daniele
> <daniele.ceraolospurio@intel.com>
> Subject: Re: [PATCH v6 02/10] mei: late_bind: add late binding component
> driver
>
> On Fri, Jul 04, 2025 at 05:18:46PM +0530, Nilawar, Badal wrote:
> >
> > On 04-07-2025 16:04, Greg KH wrote:
> > > On Fri, Jul 04, 2025 at 03:59:40PM +0530, Nilawar, Badal wrote:
> > > > On 04-07-2025 10:44, Greg KH wrote:
> > > > > On Fri, Jul 04, 2025 at 01:00:58AM +0530, Badal Nilawar wrote:
> > > > > > From: Alexander Usyskin <alexander.usyskin@intel.com>
> > > > > >
> > > > > > Add late binding component driver.
> > > > > > It allows pushing the late binding configuration from, for
> > > > > > example, the Xe graphics driver to the Intel discrete graphics card's
> CSE device.
> > > > > >
> > > > > > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> > > > > > Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> > > > > > Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > > > > > ---
> > > > > > drivers/misc/mei/Kconfig | 1 +
> > > > > > drivers/misc/mei/Makefile | 1 +
> > > > > > drivers/misc/mei/late_bind/Kconfig | 13 +
> > > > > > drivers/misc/mei/late_bind/Makefile | 9 +
> > > > > > drivers/misc/mei/late_bind/mei_late_bind.c | 272
> > > > > > ++++++++++++++++++++
> > > > > Why do you have a whole subdir for a single .c file? What's
> > > > > wrong with just keepign it in drivers/misc/mei/ ?
> > > > There is separate subdir for each component used by i915/xe, so
> > > > one was created for late_bind as well. Should we still drop late_bind
> subdir?
> > > >
> > > > cd drivers/misc/mei/
> > > > gsc_proxy/ hdcp/ late_bind/ pxp/
> > > For "modules" that are just a single file, yeah, that's silly, don't
> > > do that.
> > Another reason to maintain the sub_dir is to accommodate additional
> > files for future platforms. If you still insist, I'll remove the sub_dir.
>
> Move files around when it happens, for now, it's silly and not needed.
>
> > > > > > + * @payload_size: size of the payload data in bytes
> > > > > > + * @payload: data to be sent to the firmware */ struct
> > > > > > +csc_heci_late_bind_req {
> > > > > > + struct mkhi_msg_hdr header;
> > > > > > + u32 type;
> > > > > > + u32 flags;
> > > > > > + u32 reserved[2];
> > > > > > + u32 payload_size;
> > > > > As these cross the kernel boundry, they should be the correct
> > > > > type (__u32), but really, please define the endiness of them
> > > > > (__le32) and use the proper macros for that.
> > > > If we go with __le32 then while populating elements of structure
> > > > csc_heci_late_bind_req I will be using cpu_to_le32().
> > > >
> > > > When mapping the response buffer from the firmware with struct
> > > > csc_heci_late_bind_rsp, there's no need to use le32_to_cpu() since
> > > > the response will already be in little-endian format.
> > > How do you know? Where is that defined? Where did the conversion
> > > happen?
> >
> > Sorry, I got confused. Conversion is needed when assigning the
> > response structure elements.
> >
> > e.g ret = (int)(le32_to_cpu)rsp.status;
>
> But these are read directly from the hardware? If not, why are they marked as
> packed?
Yes, these are read from firmware, that is the reason they marked as __packed.
IMHO, don't we need change the explicit endianness of response status to address your comment.
Are we missing something here?
Thanks,
Anshuman
>
> thanks,
>
> greg k-h
On Fri, Jul 04, 2025 at 12:21:42PM +0000, Gupta, Anshuman wrote:
>
>
> > -----Original Message-----
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Sent: Friday, July 4, 2025 5:31 PM
> > To: Nilawar, Badal <badal.nilawar@intel.com>
> > Cc: intel-xe@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-
> > kernel@vger.kernel.org; Gupta, Anshuman <anshuman.gupta@intel.com>;
> > Vivi, Rodrigo <rodrigo.vivi@intel.com>; Usyskin, Alexander
> > <alexander.usyskin@intel.com>; Ceraolo Spurio, Daniele
> > <daniele.ceraolospurio@intel.com>
> > Subject: Re: [PATCH v6 02/10] mei: late_bind: add late binding component
> > driver
> >
> > On Fri, Jul 04, 2025 at 05:18:46PM +0530, Nilawar, Badal wrote:
> > >
> > > On 04-07-2025 16:04, Greg KH wrote:
> > > > On Fri, Jul 04, 2025 at 03:59:40PM +0530, Nilawar, Badal wrote:
> > > > > On 04-07-2025 10:44, Greg KH wrote:
> > > > > > On Fri, Jul 04, 2025 at 01:00:58AM +0530, Badal Nilawar wrote:
> > > > > > > From: Alexander Usyskin <alexander.usyskin@intel.com>
> > > > > > >
> > > > > > > Add late binding component driver.
> > > > > > > It allows pushing the late binding configuration from, for
> > > > > > > example, the Xe graphics driver to the Intel discrete graphics card's
> > CSE device.
> > > > > > >
> > > > > > > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> > > > > > > Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> > > > > > > Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > > > > > > ---
> > > > > > > drivers/misc/mei/Kconfig | 1 +
> > > > > > > drivers/misc/mei/Makefile | 1 +
> > > > > > > drivers/misc/mei/late_bind/Kconfig | 13 +
> > > > > > > drivers/misc/mei/late_bind/Makefile | 9 +
> > > > > > > drivers/misc/mei/late_bind/mei_late_bind.c | 272
> > > > > > > ++++++++++++++++++++
> > > > > > Why do you have a whole subdir for a single .c file? What's
> > > > > > wrong with just keepign it in drivers/misc/mei/ ?
> > > > > There is separate subdir for each component used by i915/xe, so
> > > > > one was created for late_bind as well. Should we still drop late_bind
> > subdir?
> > > > >
> > > > > cd drivers/misc/mei/
> > > > > gsc_proxy/ hdcp/ late_bind/ pxp/
> > > > For "modules" that are just a single file, yeah, that's silly, don't
> > > > do that.
> > > Another reason to maintain the sub_dir is to accommodate additional
> > > files for future platforms. If you still insist, I'll remove the sub_dir.
> >
> > Move files around when it happens, for now, it's silly and not needed.
> >
> > > > > > > + * @payload_size: size of the payload data in bytes
> > > > > > > + * @payload: data to be sent to the firmware */ struct
> > > > > > > +csc_heci_late_bind_req {
> > > > > > > + struct mkhi_msg_hdr header;
> > > > > > > + u32 type;
> > > > > > > + u32 flags;
> > > > > > > + u32 reserved[2];
> > > > > > > + u32 payload_size;
> > > > > > As these cross the kernel boundry, they should be the correct
> > > > > > type (__u32), but really, please define the endiness of them
> > > > > > (__le32) and use the proper macros for that.
> > > > > If we go with __le32 then while populating elements of structure
> > > > > csc_heci_late_bind_req I will be using cpu_to_le32().
> > > > >
> > > > > When mapping the response buffer from the firmware with struct
> > > > > csc_heci_late_bind_rsp, there's no need to use le32_to_cpu() since
> > > > > the response will already be in little-endian format.
> > > > How do you know? Where is that defined? Where did the conversion
> > > > happen?
> > >
> > > Sorry, I got confused. Conversion is needed when assigning the
> > > response structure elements.
> > >
> > > e.g ret = (int)(le32_to_cpu)rsp.status;
> >
> > But these are read directly from the hardware? If not, why are they marked as
> > packed?
> Yes, these are read from firmware, that is the reason they marked as __packed.
> IMHO, don't we need change the explicit endianness of response status to address your comment.
> Are we missing something here?
Yes. The firmware defines these values as __le32, right? And if you
read a chunk of memory and cast it into this structure, those fields
are now also __le32, right? So to read them in the driver you need to
then call le32_to_cpu() on those values.
Just like data on the USB bus, or any other hardware type. You must
define what endian the data is in and then convert it to "native" before
accessing it properly.
thanks,
greg k-h
On 04-07-2025 17:59, Greg KH wrote:
> On Fri, Jul 04, 2025 at 12:21:42PM +0000, Gupta, Anshuman wrote:
>>
>>> -----Original Message-----
>>> From: Greg KH <gregkh@linuxfoundation.org>
>>> Sent: Friday, July 4, 2025 5:31 PM
>>> To: Nilawar, Badal <badal.nilawar@intel.com>
>>> Cc: intel-xe@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-
>>> kernel@vger.kernel.org; Gupta, Anshuman <anshuman.gupta@intel.com>;
>>> Vivi, Rodrigo <rodrigo.vivi@intel.com>; Usyskin, Alexander
>>> <alexander.usyskin@intel.com>; Ceraolo Spurio, Daniele
>>> <daniele.ceraolospurio@intel.com>
>>> Subject: Re: [PATCH v6 02/10] mei: late_bind: add late binding component
>>> driver
>>>
>>> On Fri, Jul 04, 2025 at 05:18:46PM +0530, Nilawar, Badal wrote:
>>>> On 04-07-2025 16:04, Greg KH wrote:
>>>>> On Fri, Jul 04, 2025 at 03:59:40PM +0530, Nilawar, Badal wrote:
>>>>>> On 04-07-2025 10:44, Greg KH wrote:
>>>>>>> On Fri, Jul 04, 2025 at 01:00:58AM +0530, Badal Nilawar wrote:
>>>>>>>> From: Alexander Usyskin <alexander.usyskin@intel.com>
>>>>>>>>
>>>>>>>> Add late binding component driver.
>>>>>>>> It allows pushing the late binding configuration from, for
>>>>>>>> example, the Xe graphics driver to the Intel discrete graphics card's
>>> CSE device.
>>>>>>>> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
>>>>>>>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>>>>>>>> Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
>>>>>>>> ---
>>>>>>>> drivers/misc/mei/Kconfig | 1 +
>>>>>>>> drivers/misc/mei/Makefile | 1 +
>>>>>>>> drivers/misc/mei/late_bind/Kconfig | 13 +
>>>>>>>> drivers/misc/mei/late_bind/Makefile | 9 +
>>>>>>>> drivers/misc/mei/late_bind/mei_late_bind.c | 272
>>>>>>>> ++++++++++++++++++++
>>>>>>> Why do you have a whole subdir for a single .c file? What's
>>>>>>> wrong with just keepign it in drivers/misc/mei/ ?
>>>>>> There is separate subdir for each component used by i915/xe, so
>>>>>> one was created for late_bind as well. Should we still drop late_bind
>>> subdir?
>>>>>> cd drivers/misc/mei/
>>>>>> gsc_proxy/ hdcp/ late_bind/ pxp/
>>>>> For "modules" that are just a single file, yeah, that's silly, don't
>>>>> do that.
>>>> Another reason to maintain the sub_dir is to accommodate additional
>>>> files for future platforms. If you still insist, I'll remove the sub_dir.
>>> Move files around when it happens, for now, it's silly and not needed.
>>>
>>>>>>>> + * @payload_size: size of the payload data in bytes
>>>>>>>> + * @payload: data to be sent to the firmware */ struct
>>>>>>>> +csc_heci_late_bind_req {
>>>>>>>> + struct mkhi_msg_hdr header;
>>>>>>>> + u32 type;
>>>>>>>> + u32 flags;
>>>>>>>> + u32 reserved[2];
>>>>>>>> + u32 payload_size;
>>>>>>> As these cross the kernel boundry, they should be the correct
>>>>>>> type (__u32), but really, please define the endiness of them
>>>>>>> (__le32) and use the proper macros for that.
>>>>>> If we go with __le32 then while populating elements of structure
>>>>>> csc_heci_late_bind_req I will be using cpu_to_le32().
>>>>>>
>>>>>> When mapping the response buffer from the firmware with struct
>>>>>> csc_heci_late_bind_rsp, there's no need to use le32_to_cpu() since
>>>>>> the response will already be in little-endian format.
>>>>> How do you know? Where is that defined? Where did the conversion
>>>>> happen?
>>>> Sorry, I got confused. Conversion is needed when assigning the
>>>> response structure elements.
>>>>
>>>> e.g ret = (int)(le32_to_cpu)rsp.status;
>>> But these are read directly from the hardware? If not, why are they marked as
>>> packed?
>> Yes, these are read from firmware, that is the reason they marked as __packed.
>> IMHO, don't we need change the explicit endianness of response status to address your comment.
>> Are we missing something here?
> Yes. The firmware defines these values as __le32, right? And if you
> read a chunk of memory and cast it into this structure, those fields
> are now also __le32, right? So to read them in the driver you need to
> then call le32_to_cpu() on those values.
Agreed. Therefore, the following assignment is valid and needed as ret
can be BE if CPU is BE.
e.g. ret = (int)le32_to_cpu(rsp.status);
>
> Just like data on the USB bus, or any other hardware type. You must
> define what endian the data is in and then convert it to "native" before
> accessing it properly.
Ok
>
> thanks,
>
> greg k-h
© 2016 - 2025 Red Hat, Inc.