Re: [PATCH v6 02/10] mei: late_bind: add late binding component driver

From: Nilawar, Badal
Date: Fri Jul 04 2025 - 06:35:29 EST


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@xxxxxxxxx>

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@xxxxxxxxx>
Signed-off-by: Badal Nilawar <badal.nilawar@xxxxxxxxx>
Reviewed-by: Anshuman Gupta <anshuman.gupta@xxxxxxxxx>
---
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