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

[Greg KH <gregkh@linuxfoundation.org>]

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/ ?

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