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

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Jul 04, 2025 at 03:59:40PM +0530, Nilawar, Badal wrote:
> 
> 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@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@intel.com>
> > > Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> > > Reviewed-by: Anshuman Gupta <anshuman.gupta@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/

For "modules" that are just a single file, yeah, that's silly, don't do
that.

> > > +/**
> > > + * 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.

How do you know?  Where is that defined?  Where did the conversion
happen?

> Are you fine with this?

Please be explicit.

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

And from the structure definition please.

thanks,

greg k-h