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

[Usyskin, Alexander]

> -----Original Message-----
> From: Greg KH <gregkh at linuxfoundation.org>
> Sent: Saturday, June 28, 2025 3:18 PM
> To: Nilawar, Badal <badal.nilawar at intel.com>
> Cc: intel-xe at lists.freedesktop.org; dri-devel at lists.freedesktop.org; linux-
> kernel at vger.kernel.org; Gupta, Anshuman <anshuman.gupta at intel.com>;
> Vivi, Rodrigo <rodrigo.vivi at intel.com>; Usyskin, Alexander
> <alexander.usyskin at intel.com>; Ceraolo Spurio, Daniele
> <daniele.ceraolospurio at intel.com>
> Subject: Re: [PATCH v4 02/10] mei: late_bind: add late binding component
> driver
> 
> 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.
> 
> > + * @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?
> 
> > +/**
> > + * 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?
> 
> > + * @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?
> 

The push_config function pointer is documented in late_bind_component_ops.
We can drop one here.

> > +{
> > +	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?
> 

This is the callback provided to another driver via component framework,
there is no control.
Should we trust caller here?

> 
> > +
> > +	cldev = to_mei_cl_device(dev);
> > +
> > +	ret = mei_cldev_enable(cldev);
> > +	if (ret < 0) {
> 
> You mean:
> 	if (ret)
> right?
> 
Yes, mei_cldev_enable should never return >0

> 
> > +		dev_dbg(dev, "mei_cldev_enable failed. %zd\n", ret);
> 
> Why display the error again if this failed?  The caller already did
> that.
> 

It is a separate module, and dynamic debug can be enabled separately.
I see it as debug refinement, but this can be dropped if seemed unneeded.

> And the function returns an int, not a ssize_t, didn't the compiler
> complain?
> 

Never seen that, do you suggest to add "return (int)ret;"
as we know that in this stage only error codes can be in this variable?

> thanks,
> 
> greg k-h


- - 
Thanks,
Sasha