[PATCH v6 02/10] mei: late_bind: add late binding component driver
Nilawar, Badal
badal.nilawar at intel.com
Fri Jul 4 10:29:40 UTC 2025
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 at 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 at intel.com>
>> Signed-off-by: Badal Nilawar <badal.nilawar at intel.com>
>> Reviewed-by: Anshuman Gupta <anshuman.gupta at 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
More information about the Intel-xe
mailing list