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>
---
v2:
 - Use generic naming (Jani)
 - Drop xe_late_bind_component struct to move to xe code (Daniele/Sasha)
v3:
 - Updated kconfig description
 - Move CSC late binding specific flags/defines to late_bind_mei_interface.h (Daniele)
v4:
 - Add match for PCI_CLASS_DISPLAY_OTHER to support headless cards (Anshuman)
v5:
 - Add fixes in push_config (Sasha)
 - Use INTEL_ prefix for component, refine doc,
   add status enum to headerlate_bind_mei_interface.h (Anshuman)
---
 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  | 281 ++++++++++++++++++++
 include/drm/intel/i915_component.h          |   1 +
 include/drm/intel/late_bind_mei_interface.h |  64 +++++
 7 files changed, 370 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..ffb89ccdfbb1
--- /dev/null
+++ b/drivers/misc/mei/late_bind/mei_late_bind.c
@@ -0,0 +1,281 @@
+// 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;
+}
+
+/**
+ * mei_late_bind_push_config - Sends a config to the firmware.
+ * @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
+ */
+static int mei_late_bind_push_config(struct device *dev, u32 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 ret;
+
+	if (!dev || !payload || !payload_size)
+		return -EINVAL;
+
+	cldev = to_mei_cl_device(dev);
+
+	ret = mei_cldev_enable(cldev);
+	if (ret < 0) {
+		dev_dbg(dev, "mei_cldev_enable failed. %zd\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);
+
+	ret = mei_cldev_send_timeout(cldev, (void *)req, req_size, LATE_BIND_SEND_TIMEOUT_MSEC);
+	if (ret < 0) {
+		dev_err(dev, "mei_cldev_send failed. %zd\n", ret);
+		goto end;
+	}
+
+	ret = mei_cldev_recv_timeout(cldev, (void *)&rsp, sizeof(rsp), LATE_BIND_RECV_TIMEOUT_MSEC);
+	if (ret < 0) {
+		dev_err(dev, "mei_cldev_recv failed. %zd\n", ret);
+		goto end;
+	}
+	if (ret < sizeof(rsp.header)) {
+		dev_err(dev, "bad response header from the firmware: size %zd < %zu\n",
+			ret, sizeof(rsp.header));
+		goto end;
+	}
+	if (ret < sizeof(rsp)) {
+		dev_err(dev, "bad response from the firmware: size %zd < %zu\n",
+			ret, sizeof(rsp));
+		goto end;
+	}
+
+	ret = mei_late_bind_check_response(dev, &rsp.header);
+	if (ret) {
+		dev_err(dev, "bad result response from the firmware: 0x%x\n",
+			*(uint32_t *)&rsp.header);
+		goto end;
+	}
+
+	ret = (int)rsp.status;
+	dev_dbg(dev, "%s status = %zd\n", __func__, ret);
+
+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.
+ *
+ *    The function checks if requester is Intel PCI_CLASS_DISPLAY_VGA or
+ *    PCI_CLASS_DISPLAY_OTHER device, and checks if the parent of requester
+ *    and the grand parent of mei_if are the same 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.1

On Wed, Jun 25, 2025 at 10:30:07PM +0530, Badal Nilawar wrote:
> +/**
> + * 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;

I don't think you ever set this field, so why is it here?

Or did I miss it somewhere?

thanks,

greg k-h

On 28-06-2025 17:49, Greg KH wrote:
> On Wed, Jun 25, 2025 at 10:30:07PM +0530, Badal Nilawar wrote:
>> +/**
>> + * 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;
> I don't think you ever set this field, so why is it here?
>
> Or did I miss it somewhere?

It is set in drivers/misc/mei/late_bind/mei_late_bind.c

static const struct late_bind_component_ops mei_late_bind_ops = {
         .owner = THIS_MODULE,
         .push_config = mei_late_bind_push_config,
};

Thanks,
Badal

>
> thanks,
>
> greg k-h

On Tue, Jul 01, 2025 at 01:37:36PM +0530, Nilawar, Badal wrote:
> 
> On 28-06-2025 17:49, Greg KH wrote:
> > On Wed, Jun 25, 2025 at 10:30:07PM +0530, Badal Nilawar wrote:
> > > +/**
> > > + * 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;
> > I don't think you ever set this field, so why is it here?
> > 
> > Or did I miss it somewhere?
> 
> It is set in drivers/misc/mei/late_bind/mei_late_bind.c
> 
> static const struct late_bind_component_ops mei_late_bind_ops = {
>         .owner = THIS_MODULE,
>         .push_config = mei_late_bind_push_config,
> };

Ah.  But then who uses it?  And why?  Normally forcing callers to set
.owner is frowned apon, use a #define correctly to have it automatically
set for you in the registration function please.

And are you _sure_ you need it?

thanks,

greg k-h

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: 01 July 2025 13:48
> 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 v4 02/10] mei: late_bind: add late binding component
> driver
> 
> On Tue, Jul 01, 2025 at 01:37:36PM +0530, Nilawar, Badal wrote:
> >
> > On 28-06-2025 17:49, Greg KH wrote:
> > > On Wed, Jun 25, 2025 at 10:30:07PM +0530, Badal Nilawar wrote:
> > > > +/**
> > > > + * 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;
> > > I don't think you ever set this field, so why is it here?
> > >
> > > Or did I miss it somewhere?
> >
> > It is set in drivers/misc/mei/late_bind/mei_late_bind.c
> >
> > static const struct late_bind_component_ops mei_late_bind_ops = {
> >         .owner = THIS_MODULE,
> >         .push_config = mei_late_bind_push_config, };
> 
> Ah.  But then who uses it?  And why?  Normally forcing callers to set .owner is
> frowned apon, use a #define correctly to have it automatically set for you in
> the registration function please.
> 
> And are you _sure_ you need it?

In xe kmd only uses .push_config so .owner can be dropped. Looks like it got propagated from previously implemented mei components but for none of the component .owner is used.  So it's fine to drop it. 
@Usyskin, Alexander please share your thoughts on this. 

Badal
  
> 
> thanks,
> 
> greg k-h

> > Subject: Re: [PATCH v4 02/10] mei: late_bind: add late binding component
> > driver
> >
> > On Tue, Jul 01, 2025 at 01:37:36PM +0530, Nilawar, Badal wrote:
> > >
> > > On 28-06-2025 17:49, Greg KH wrote:
> > > > On Wed, Jun 25, 2025 at 10:30:07PM +0530, Badal Nilawar wrote:
> > > > > +/**
> > > > > + * 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;
> > > > I don't think you ever set this field, so why is it here?
> > > >
> > > > Or did I miss it somewhere?
> > >
> > > It is set in drivers/misc/mei/late_bind/mei_late_bind.c
> > >
> > > static const struct late_bind_component_ops mei_late_bind_ops = {
> > >         .owner = THIS_MODULE,
> > >         .push_config = mei_late_bind_push_config, };
> >
> > Ah.  But then who uses it?  And why?  Normally forcing callers to set .owner
> is
> > frowned apon, use a #define correctly to have it automatically set for you in
> > the registration function please.
> >
> > And are you _sure_ you need it?
> 
> In xe kmd only uses .push_config so .owner can be dropped. Looks like it got
> propagated from previously implemented mei components but for none of
> the component .owner is used.  So it's fine to drop it.
> @Usyskin, Alexander please share your thoughts on this.
> 

As caller do not need this, can be dropped.

- - 
Thanks,
Sasha



> Badal
> 
> >
> > thanks,
> >
> > greg k-h

On Wed, Jun 25, 2025 at 10:30:07PM +0530, Badal Nilawar wrote:
> --- /dev/null
> +++ b/drivers/misc/mei/late_bind/mei_late_bind.c
> @@ -0,0 +1,281 @@
> +// 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

Reserved for what?  All reserved fields need to be set to a default
value, please document that here.

> + * @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;

What is the endian of these fields?  And as this crosses the
kernel/hardware boundry, shouldn't these be __u32?

> +/**
> + * 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;

Same questions as above.

> +} __packed;
> +/**
> + * mei_late_bind_push_config - Sends a config to the firmware.
> + * @dev: device struct corresponding to the mei device
> + * @type: payload type

Shouldn't type be an enum?

> + * @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
> + */
> +static int mei_late_bind_push_config(struct device *dev, u32 type, u32 flags,
> +				     const void *payload, size_t payload_size)

Why do static functions need kerneldoc formatting?

> +{
> +	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 ret;
> +
> +	if (!dev || !payload || !payload_size)
> +		return -EINVAL;

How can any of these ever happen as you control the callers of this
function?


> +
> +	cldev = to_mei_cl_device(dev);
> +
> +	ret = mei_cldev_enable(cldev);
> +	if (ret < 0) {

You mean:
	if (ret)
right?


> +		dev_dbg(dev, "mei_cldev_enable failed. %zd\n", ret);

Why display the error again if this failed?  The caller already did
that.

And the function returns an int, not a ssize_t, didn't the compiler
complain?

thanks,

greg k-h

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Saturday, June 28, 2025 3:18 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 v4 02/10] mei: late_bind: add late binding component
> driver
> 
> On Wed, Jun 25, 2025 at 10:30:07PM +0530, Badal Nilawar wrote:
> > --- /dev/null
> > +++ b/drivers/misc/mei/late_bind/mei_late_bind.c
> > @@ -0,0 +1,281 @@
> > +// 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
> 
> Reserved for what?  All reserved fields need to be set to a default
> value, please document that here.
> 
> > + * @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;
> 
> What is the endian of these fields?  And as this crosses the
> kernel/hardware boundry, shouldn't these be __u32?
> 
> > +/**
> > + * 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;
> 
> Same questions as above.
> 
> > +} __packed;
> > +/**
> > + * mei_late_bind_push_config - Sends a config to the firmware.
> > + * @dev: device struct corresponding to the mei device
> > + * @type: payload type
> 
> Shouldn't type be an enum?
> 
> > + * @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
> > + */
> > +static int mei_late_bind_push_config(struct device *dev, u32 type, u32
> flags,
> > +				     const void *payload, size_t payload_size)
> 
> Why do static functions need kerneldoc formatting?
> 

The push_config function pointer is documented in late_bind_component_ops.
We can drop one here.

> > +{
> > +	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 ret;
> > +
> > +	if (!dev || !payload || !payload_size)
> > +		return -EINVAL;
> 
> How can any of these ever happen as you control the callers of this
> function?
> 

This is the callback provided to another driver via component framework,
there is no control.
Should we trust caller here?

> 
> > +
> > +	cldev = to_mei_cl_device(dev);
> > +
> > +	ret = mei_cldev_enable(cldev);
> > +	if (ret < 0) {
> 
> You mean:
> 	if (ret)
> right?
> 
Yes, mei_cldev_enable should never return >0

> 
> > +		dev_dbg(dev, "mei_cldev_enable failed. %zd\n", ret);
> 
> Why display the error again if this failed?  The caller already did
> that.
> 

It is a separate module, and dynamic debug can be enabled separately.
I see it as debug refinement, but this can be dropped if seemed unneeded.

> And the function returns an int, not a ssize_t, didn't the compiler
> complain?
> 

Never seen that, do you suggest to add "return (int)ret;"
as we know that in this stage only error codes can be in this variable?

> thanks,
> 
> greg k-h


- - 
Thanks,
Sasha

On 28-06-2025 17:48, Greg KH wrote:
> On Wed, Jun 25, 2025 at 10:30:07PM +0530, Badal Nilawar wrote:
>> --- /dev/null
>> +++ b/drivers/misc/mei/late_bind/mei_late_bind.c
>> @@ -0,0 +1,281 @@
>> +// 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
> Reserved for what?  All reserved fields need to be set to a default
> value, please document that here.
Reserved by CSC firmware probably for future use.  default value should 
be 0.
>
>> + * @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;
> What is the endian of these fields?  And as this crosses the
> kernel/hardware boundry, shouldn't these be __u32?

endian of these fields is little endian, all the headers are little 
endian.  I will add comment at top.
On __u32 I doubt we need to do it as csc send copy it to internal buffer.

Sasha can help to answer.

>
>> +/**
>> + * 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;
> Same questions as above.
>
>> +} __packed;
>> +/**
>> + * mei_late_bind_push_config - Sends a config to the firmware.
>> + * @dev: device struct corresponding to the mei device
>> + * @type: payload type
> Shouldn't type be an enum?
Sure will make enum.
>
>> + * @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
>> + */
>> +static int mei_late_bind_push_config(struct device *dev, u32 type, u32 flags,
>> +				     const void *payload, size_t payload_size)
> Why do static functions need kerneldoc formatting?
Sasha can help to answer this.
>
>> +{
>> +	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 ret;
>> +
>> +	if (!dev || !payload || !payload_size)
>> +		return -EINVAL;
> How can any of these ever happen as you control the callers of this
> function?
I will add WARN here.
>
>
>> +
>> +	cldev = to_mei_cl_device(dev);
>> +
>> +	ret = mei_cldev_enable(cldev);
>> +	if (ret < 0) {
> You mean:
> 	if (ret)
> right?
yes
>
>
>> +		dev_dbg(dev, "mei_cldev_enable failed. %zd\n", ret);
> Why display the error again if this failed?  The caller already did
> that.
>
> And the function returns an int, not a ssize_t, didn't the compiler
> complain

It didn't. This is for debug from mei side, this can be removed or will 
fix format specifier.

Thanks,
Badal

>
> thanks,
>
> greg k-h

On Tue, Jul 01, 2025 at 02:02:15PM +0530, Nilawar, Badal wrote:
> On 28-06-2025 17:48, Greg KH wrote:
> > > + * @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;
> > What is the endian of these fields?  And as this crosses the
> > kernel/hardware boundry, shouldn't these be __u32?
> 
> endian of these fields is little endian, all the headers are little endian. 
> I will add comment at top.

No, use the proper types if this is little endian.  Don't rely on a
comment to catch things when it goes wrong.

> On __u32 I doubt we need to do it as csc send copy it to internal buffer.

If this crosses the kernel boundry, it needs to use the proper type.

> > > +{
> > > +	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 ret;
> > > +
> > > +	if (!dev || !payload || !payload_size)
> > > +		return -EINVAL;
> > How can any of these ever happen as you control the callers of this
> > function?
> I will add WARN here.

So you will end up crashing the machine and getting a CVE assigned for
it?

Please no.  If it can't happen, then don't check for it.  If it can
happen, great, handle it properly.  Don't just give up and cause a
system to reboot, that's a horrible way to write kernel code.

thanks,

greg k-h

On 01-07-2025 15:15, Greg KH wrote:
> On Tue, Jul 01, 2025 at 02:02:15PM +0530, Nilawar, Badal wrote:
>> On 28-06-2025 17:48, Greg KH wrote:
>>>> + * @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;
>>> What is the endian of these fields?  And as this crosses the
>>> kernel/hardware boundry, shouldn't these be __u32?
>> endian of these fields is little endian, all the headers are little endian.
>> I will add comment at top.
> No, use the proper types if this is little endian.  Don't rely on a
> comment to catch things when it goes wrong.
>
>> On __u32 I doubt we need to do it as csc send copy it to internal buffer.
> If this crosses the kernel boundry, it needs to use the proper type.

Understood. I will proceed with using __le32 in this context, provided 
that Sasha agrees.

>
>>>> +{
>>>> +	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 ret;
>>>> +
>>>> +	if (!dev || !payload || !payload_size)
>>>> +		return -EINVAL;
>>> How can any of these ever happen as you control the callers of this
>>> function?
>> I will add WARN here.
> So you will end up crashing the machine and getting a CVE assigned for
> it?
>
> Please no.  If it can't happen, then don't check for it.  If it can
> happen, great, handle it properly.  Don't just give up and cause a
> system to reboot, that's a horrible way to write kernel code.

Fine, will drop the idea of WARN here.

Thanks,
Badal

>
> thanks,
>
> greg k-h

On 01-07-2025 18:04, Nilawar, Badal wrote:
>
> On 01-07-2025 15:15, Greg KH wrote:
>> On Tue, Jul 01, 2025 at 02:02:15PM +0530, Nilawar, Badal wrote:
>>> On 28-06-2025 17:48, Greg KH wrote:
>>>>> + * @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;
>>>> What is the endian of these fields?  And as this crosses the
>>>> kernel/hardware boundry, shouldn't these be __u32?
>>> endian of these fields is little endian, all the headers are little 
>>> endian.
>>> I will add comment at top.
>> No, use the proper types if this is little endian.  Don't rely on a
>> comment to catch things when it goes wrong.
>>
>>> On __u32 I doubt we need to do it as csc send copy it to internal 
>>> buffer.
>> If this crosses the kernel boundry, it needs to use the proper type.
>
> Understood. I will proceed with using __le32 in this context, provided 
> that Sasha agrees.

I believe __le{32, 16} is used only when the byte order is fixed and 
matches the host system's native endianness. Since the CSC controller is 
little-endian, is it necessary to specify the endianness here?
If it is mandatory to use the __le{32, 16} endian type, then is there 
need to convert endianness using cpu_to_le and le_to_cpu?

>
>>
>>>>> +{
>>>>> +    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 ret;
>>>>> +
>>>>> +    if (!dev || !payload || !payload_size)
>>>>> +        return -EINVAL;
>>>> How can any of these ever happen as you control the callers of this
>>>> function?
>>> I will add WARN here.
>> So you will end up crashing the machine and getting a CVE assigned for
>> it?
>>
>> Please no.  If it can't happen, then don't check for it.  If it can
>> happen, great, handle it properly.  Don't just give up and cause a
>> system to reboot, that's a horrible way to write kernel code.
>
> Fine, will drop the idea of WARN here.
>
> Thanks,
> Badal
>
>>
>> thanks,
>>
>> greg k-h

On Tue, Jul 01, 2025 at 10:11:54PM +0530, Nilawar, Badal wrote:
> 
> On 01-07-2025 18:04, Nilawar, Badal wrote:
> > 
> > On 01-07-2025 15:15, Greg KH wrote:
> > > On Tue, Jul 01, 2025 at 02:02:15PM +0530, Nilawar, Badal wrote:
> > > > On 28-06-2025 17:48, Greg KH wrote:
> > > > > > + * @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;
> > > > > What is the endian of these fields?  And as this crosses the
> > > > > kernel/hardware boundry, shouldn't these be __u32?
> > > > endian of these fields is little endian, all the headers are
> > > > little endian.
> > > > I will add comment at top.
> > > No, use the proper types if this is little endian.  Don't rely on a
> > > comment to catch things when it goes wrong.
> > > 
> > > > On __u32 I doubt we need to do it as csc send copy it to
> > > > internal buffer.
> > > If this crosses the kernel boundry, it needs to use the proper type.
> > 
> > Understood. I will proceed with using __le32 in this context, provided
> > that Sasha agrees.
> 
> I believe __le{32, 16} is used only when the byte order is fixed and matches
> the host system's native endianness. Since the CSC controller is
> little-endian, is it necessary to specify the endianness here?
> If it is mandatory to use the __le{32, 16} endian type, then is there need
> to convert endianness using cpu_to_le and le_to_cpu?

I honestly don't believe that specifying endianness here is **needed**.
I mean, it might be future safe to use the __le32 and
flags = cpu_to_le32(1 << 0) just in case someone decide to port all the
GPU code to run in big-endian CPU. Very unlikely I'd say, and much more cases
to resolve before we get to this gpu use case here I'm afraid.

Weel, unless this mei here can be used outside of GPU context?!

> 
> > 
> > > 
> > > > > > +{
> > > > > > +    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 ret;
> > > > > > +
> > > > > > +    if (!dev || !payload || !payload_size)
> > > > > > +        return -EINVAL;
> > > > > How can any of these ever happen as you control the callers of this
> > > > > function?
> > > > I will add WARN here.
> > > So you will end up crashing the machine and getting a CVE assigned for
> > > it?
> > > 
> > > Please no.  If it can't happen, then don't check for it.  If it can
> > > happen, great, handle it properly.  Don't just give up and cause a
> > > system to reboot, that's a horrible way to write kernel code.

I agree here that the WARN is not a good way to handle that.
We either don't check (remove it) or handle properly (keep as is).

With the context of where this driver is used I'd say it can't happen.
Since xe is properly setting it right now and I don't believe we have
other usages of this mei driver here.

But if there's a chance of this getting used outside of xe, then
we need to keep the check...

But if you keep the check, then also use __lb32() because we need
some consistency in the reasoning, one way or the other.

> > 
> > Fine, will drop the idea of WARN here.
> > 
> > Thanks,
> > Badal
> > 
> > > 
> > > thanks,
> > > 
> > > greg k-h

> Subject: Re: [PATCH v4 02/10] mei: late_bind: add late binding component
> driver
> 
> On Tue, Jul 01, 2025 at 10:11:54PM +0530, Nilawar, Badal wrote:
> >
> > On 01-07-2025 18:04, Nilawar, Badal wrote:
> > >
> > > On 01-07-2025 15:15, Greg KH wrote:
> > > > On Tue, Jul 01, 2025 at 02:02:15PM +0530, Nilawar, Badal wrote:
> > > > > On 28-06-2025 17:48, Greg KH wrote:
> > > > > > > + * @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;
> > > > > > What is the endian of these fields?  And as this crosses the
> > > > > > kernel/hardware boundry, shouldn't these be __u32?
> > > > > endian of these fields is little endian, all the headers are
> > > > > little endian.
> > > > > I will add comment at top.
> > > > No, use the proper types if this is little endian.  Don't rely on a
> > > > comment to catch things when it goes wrong.
> > > >
> > > > > On __u32 I doubt we need to do it as csc send copy it to
> > > > > internal buffer.
> > > > If this crosses the kernel boundry, it needs to use the proper type.
> > >
> > > Understood. I will proceed with using __le32 in this context, provided
> > > that Sasha agrees.
> >
> > I believe __le{32, 16} is used only when the byte order is fixed and matches
> > the host system's native endianness. Since the CSC controller is
> > little-endian, is it necessary to specify the endianness here?
> > If it is mandatory to use the __le{32, 16} endian type, then is there need
> > to convert endianness using cpu_to_le and le_to_cpu?
> 
> I honestly don't believe that specifying endianness here is **needed**.
> I mean, it might be future safe to use the __le32 and
> flags = cpu_to_le32(1 << 0) just in case someone decide to port all the
> GPU code to run in big-endian CPU. Very unlikely I'd say, and much more cases
> to resolve before we get to this gpu use case here I'm afraid.
> 
> Weel, unless this mei here can be used outside of GPU context?!
> 

There is nothing useful in this outside of GPU context.
This module is tailored for GPU use-case.
If Xe driver is bound to be little-endian, this one should be too.
Other similar modules use u32.

- - 
Thanks,
Sasha


> >
> > >
> > > >
> > > > > > > +{
> > > > > > > +    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 ret;
> > > > > > > +
> > > > > > > +    if (!dev || !payload || !payload_size)
> > > > > > > +        return -EINVAL;
> > > > > > How can any of these ever happen as you control the callers of this
> > > > > > function?
> > > > > I will add WARN here.
> > > > So you will end up crashing the machine and getting a CVE assigned for
> > > > it?
> > > >
> > > > Please no.  If it can't happen, then don't check for it.  If it can
> > > > happen, great, handle it properly.  Don't just give up and cause a
> > > > system to reboot, that's a horrible way to write kernel code.
> 
> I agree here that the WARN is not a good way to handle that.
> We either don't check (remove it) or handle properly (keep as is).
> 
> With the context of where this driver is used I'd say it can't happen.
> Since xe is properly setting it right now and I don't believe we have
> other usages of this mei driver here.
> 
> But if there's a chance of this getting used outside of xe, then
> we need to keep the check...
> 
> But if you keep the check, then also use __lb32() because we need
> some consistency in the reasoning, one way or the other.
> 
> > >
> > > Fine, will drop the idea of WARN here.
> > >
> > > Thanks,
> > > Badal
> > >
> > > >
> > > > thanks,
> > > >
> > > > greg k-h

> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Sent: Tuesday, July 1, 2025 11:05 PM
> To: Nilawar, Badal <badal.nilawar@intel.com>
> Cc: Greg KH <gregkh@linuxfoundation.org>; Usyskin, Alexander
> <alexander.usyskin@intel.com>; intel-xe@lists.freedesktop.org; dri-
> devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; Gupta, Anshuman
> <anshuman.gupta@intel.com>; Ceraolo Spurio, Daniele
> <daniele.ceraolospurio@intel.com>
> Subject: Re: [PATCH v4 02/10] mei: late_bind: add late binding component
> driver
> 
> On Tue, Jul 01, 2025 at 10:11:54PM +0530, Nilawar, Badal wrote:
> >
> > On 01-07-2025 18:04, Nilawar, Badal wrote:
> > >
> > > On 01-07-2025 15:15, Greg KH wrote:
> > > > On Tue, Jul 01, 2025 at 02:02:15PM +0530, Nilawar, Badal wrote:
> > > > > On 28-06-2025 17:48, Greg KH wrote:
> > > > > > > + * @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;
> > > > > > What is the endian of these fields?  And as this crosses the
> > > > > > kernel/hardware boundry, shouldn't these be __u32?
> > > > > endian of these fields is little endian, all the headers are
> > > > > little endian.
> > > > > I will add comment at top.
> > > > No, use the proper types if this is little endian.  Don't rely on
> > > > a comment to catch things when it goes wrong.
> > > >
> > > > > On __u32 I doubt we need to do it as csc send copy it to
> > > > > internal buffer.
> > > > If this crosses the kernel boundry, it needs to use the proper type.
> > >
> > > Understood. I will proceed with using __le32 in this context,
> > > provided that Sasha agrees.
> >
> > I believe __le{32, 16} is used only when the byte order is fixed and
> > matches the host system's native endianness. Since the CSC controller
> > is little-endian, is it necessary to specify the endianness here?
> > If it is mandatory to use the __le{32, 16} endian type, then is there
> > need to convert endianness using cpu_to_le and le_to_cpu?
> 
> I honestly don't believe that specifying endianness here is **needed**.
> I mean, it might be future safe to use the __le32 and flags = cpu_to_le32(1 <<
> 0) just in case someone decide to port all the GPU code to run in big-endian
> CPU. Very unlikely I'd say, and much more cases to resolve before we get to
> this gpu use case here I'm afraid.
> 
> Weel, unless this mei here can be used outside of GPU context?!
MEI is interface driver for CSC firmware that is also part of our GPU.
So, it is completely un-realistic CSC having different endianness as compared to HOST and GPU.
@Usyskin, Alexander what is you opinion ?
Thanks,
Anshuman.


> 
> >
> > >
> > > >
> > > > > > > +{
> > > > > > > +    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 ret;
> > > > > > > +
> > > > > > > +    if (!dev || !payload || !payload_size)
> > > > > > > +        return -EINVAL;
> > > > > > How can any of these ever happen as you control the callers of
> > > > > > this function?
> > > > > I will add WARN here.
> > > > So you will end up crashing the machine and getting a CVE assigned
> > > > for it?
> > > >
> > > > Please no.  If it can't happen, then don't check for it.  If it
> > > > can happen, great, handle it properly.  Don't just give up and
> > > > cause a system to reboot, that's a horrible way to write kernel code.
> 
> I agree here that the WARN is not a good way to handle that.
> We either don't check (remove it) or handle properly (keep as is).
> 
> With the context of where this driver is used I'd say it can't happen.
> Since xe is properly setting it right now and I don't believe we have other
> usages of this mei driver here.
> 
> But if there's a chance of this getting used outside of xe, then we need to keep
> the check...
> 
> But if you keep the check, then also use __lb32() because we need some
> consistency in the reasoning, one way or the other.
> 
> > >
> > > Fine, will drop the idea of WARN here.
> > >
> > > Thanks,
> > > Badal
> > >
> > > >
> > > > thanks,
> > > >
> > > > greg k-h

> -----Original Message-----
> From: Nilawar, Badal <badal.nilawar@intel.com>
> Sent: Wednesday, June 25, 2025 10:30 PM
> To: intel-xe@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-
> kernel@vger.kernel.org
> Cc: Gupta, Anshuman <anshuman.gupta@intel.com>; Vivi, Rodrigo
> <rodrigo.vivi@intel.com>; Usyskin, Alexander <alexander.usyskin@intel.com>;
> gregkh@linuxfoundation.org; Ceraolo Spurio, Daniele
> <daniele.ceraolospurio@intel.com>
> Subject: [PATCH v4 02/10] mei: late_bind: add late binding component driver
> 
> 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>
> ---
> v2:
>  - Use generic naming (Jani)
>  - Drop xe_late_bind_component struct to move to xe code (Daniele/Sasha)
> v3:
>  - Updated kconfig description
>  - Move CSC late binding specific flags/defines to late_bind_mei_interface.h
> (Daniele)
> v4:
>  - Add match for PCI_CLASS_DISPLAY_OTHER to support headless cards
> (Anshuman)
> v5:
>  - Add fixes in push_config (Sasha)
>  - Use INTEL_ prefix for component, refine doc,
>    add status enum to headerlate_bind_mei_interface.h (Anshuman)
> ---
>  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  | 281 ++++++++++++++++++++
>  include/drm/intel/i915_component.h          |   1 +
>  include/drm/intel/late_bind_mei_interface.h |  64 +++++
>  7 files changed, 370 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..ffb89ccdfbb1
> --- /dev/null
> +++ b/drivers/misc/mei/late_bind/mei_late_bind.c
> @@ -0,0 +1,281 @@
> +// 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;
> +}
> +
> +/**
> + * mei_late_bind_push_config - Sends a config to the firmware.
> + * @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
> + */
> +static int mei_late_bind_push_config(struct device *dev, u32 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 ret;
> +
> +	if (!dev || !payload || !payload_size)
> +		return -EINVAL;
> +
> +	cldev = to_mei_cl_device(dev);
> +
> +	ret = mei_cldev_enable(cldev);
> +	if (ret < 0) {
> +		dev_dbg(dev, "mei_cldev_enable failed. %zd\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);
> +
> +	ret = mei_cldev_send_timeout(cldev, (void *)req, req_size,
> LATE_BIND_SEND_TIMEOUT_MSEC);
> +	if (ret < 0) {
> +		dev_err(dev, "mei_cldev_send failed. %zd\n", ret);
> +		goto end;
> +	}
> +
> +	ret = mei_cldev_recv_timeout(cldev, (void *)&rsp, sizeof(rsp),
> LATE_BIND_RECV_TIMEOUT_MSEC);
> +	if (ret < 0) {
> +		dev_err(dev, "mei_cldev_recv failed. %zd\n", ret);
> +		goto end;
> +	}
> +	if (ret < sizeof(rsp.header)) {
> +		dev_err(dev, "bad response header from the firmware: size
> %zd < %zu\n",
> +			ret, sizeof(rsp.header));
> +		goto end;
> +	}
> +	if (ret < sizeof(rsp)) {
> +		dev_err(dev, "bad response from the firmware: size %zd <
> %zu\n",
> +			ret, sizeof(rsp));
> +		goto end;
> +	}
> +
> +	ret = mei_late_bind_check_response(dev, &rsp.header);
> +	if (ret) {
> +		dev_err(dev, "bad result response from the firmware:
> 0x%x\n",
> +			*(uint32_t *)&rsp.header);
> +		goto end;
> +	}
> +
> +	ret = (int)rsp.status;
> +	dev_dbg(dev, "%s status = %zd\n", __func__, ret);
> +
> +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.
> + *
> + *    The function checks if requester is Intel PCI_CLASS_DISPLAY_VGA or
> + *    PCI_CLASS_DISPLAY_OTHER device, and checks if the parent of requester
DOC is still wrong dev is requester here, you are checking base == dev.
With fixing of that.
Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>

Thanks,
Anshuman 
> + *    and the grand parent of mei_if are the same 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.1

On 26-06-2025 09:20, Gupta, Anshuman wrote:
>
>> -----Original Message-----
>> From: Nilawar, Badal <badal.nilawar@intel.com>
>> Sent: Wednesday, June 25, 2025 10:30 PM
>> To: intel-xe@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-
>> kernel@vger.kernel.org
>> Cc: Gupta, Anshuman <anshuman.gupta@intel.com>; Vivi, Rodrigo
>> <rodrigo.vivi@intel.com>; Usyskin, Alexander <alexander.usyskin@intel.com>;
>> gregkh@linuxfoundation.org; Ceraolo Spurio, Daniele
>> <daniele.ceraolospurio@intel.com>
>> Subject: [PATCH v4 02/10] mei: late_bind: add late binding component driver
>>
>> 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>
>> ---
>> v2:
>>   - Use generic naming (Jani)
>>   - Drop xe_late_bind_component struct to move to xe code (Daniele/Sasha)
>> v3:
>>   - Updated kconfig description
>>   - Move CSC late binding specific flags/defines to late_bind_mei_interface.h
>> (Daniele)
>> v4:
>>   - Add match for PCI_CLASS_DISPLAY_OTHER to support headless cards
>> (Anshuman)
>> v5:
>>   - Add fixes in push_config (Sasha)
>>   - Use INTEL_ prefix for component, refine doc,
>>     add status enum to headerlate_bind_mei_interface.h (Anshuman)
>> ---
>>   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  | 281 ++++++++++++++++++++
>>   include/drm/intel/i915_component.h          |   1 +
>>   include/drm/intel/late_bind_mei_interface.h |  64 +++++
>>   7 files changed, 370 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..ffb89ccdfbb1
>> --- /dev/null
>> +++ b/drivers/misc/mei/late_bind/mei_late_bind.c
>> @@ -0,0 +1,281 @@
>> +// 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;
>> +}
>> +
>> +/**
>> + * mei_late_bind_push_config - Sends a config to the firmware.
>> + * @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
>> + */
>> +static int mei_late_bind_push_config(struct device *dev, u32 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 ret;
>> +
>> +	if (!dev || !payload || !payload_size)
>> +		return -EINVAL;
>> +
>> +	cldev = to_mei_cl_device(dev);
>> +
>> +	ret = mei_cldev_enable(cldev);
>> +	if (ret < 0) {
>> +		dev_dbg(dev, "mei_cldev_enable failed. %zd\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);
>> +
>> +	ret = mei_cldev_send_timeout(cldev, (void *)req, req_size,
>> LATE_BIND_SEND_TIMEOUT_MSEC);
>> +	if (ret < 0) {
>> +		dev_err(dev, "mei_cldev_send failed. %zd\n", ret);
>> +		goto end;
>> +	}
>> +
>> +	ret = mei_cldev_recv_timeout(cldev, (void *)&rsp, sizeof(rsp),
>> LATE_BIND_RECV_TIMEOUT_MSEC);
>> +	if (ret < 0) {
>> +		dev_err(dev, "mei_cldev_recv failed. %zd\n", ret);
>> +		goto end;
>> +	}
>> +	if (ret < sizeof(rsp.header)) {
>> +		dev_err(dev, "bad response header from the firmware: size
>> %zd < %zu\n",
>> +			ret, sizeof(rsp.header));
>> +		goto end;
>> +	}
>> +	if (ret < sizeof(rsp)) {
>> +		dev_err(dev, "bad response from the firmware: size %zd <
>> %zu\n",
>> +			ret, sizeof(rsp));
>> +		goto end;
>> +	}
>> +
>> +	ret = mei_late_bind_check_response(dev, &rsp.header);
>> +	if (ret) {
>> +		dev_err(dev, "bad result response from the firmware:
>> 0x%x\n",
>> +			*(uint32_t *)&rsp.header);
>> +		goto end;
>> +	}
>> +
>> +	ret = (int)rsp.status;
>> +	dev_dbg(dev, "%s status = %zd\n", __func__, ret);
>> +
>> +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.
>> + *
>> + *    The function checks if requester is Intel PCI_CLASS_DISPLAY_VGA or
>> + *    PCI_CLASS_DISPLAY_OTHER device, and checks if the parent of requester
> DOC is still wrong dev is requester here, you are checking base == dev.
> With fixing of that.
> Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>

It should be requester is grand parent of late_bind mei device.

Thanks,
Badal

>
> Thanks,
> Anshuman
>> + *    and the grand parent of mei_if are the same 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.1