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

["Nilawar, Badal" <badal.nilawar@intel.com>]

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