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.usys...@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.usys...@intel.com>
> > > Signed-off-by: Badal Nilawar <badal.nila...@intel.com>
> > > Reviewed-by: Anshuman Gupta <anshuman.gu...@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
