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

[Nilawar, Badal]

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