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

[Greg KH <gregkh@linuxfoundation.org>]

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?

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


> +
> +	cldev = to_mei_cl_device(dev);
> +
> +	ret = mei_cldev_enable(cldev);
> +	if (ret < 0) {

You mean:
	if (ret)
right?


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

thanks,

greg k-h