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

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

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