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

[Gupta, Anshuman]

> -----Original Message-----
> From: Greg KH <gregkh at linuxfoundation.org>
> Sent: Friday, July 4, 2025 5:31 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 v6 02/10] mei: late_bind: add late binding component
> driver
> 
> On Fri, Jul 04, 2025 at 05:18:46PM +0530, Nilawar, Badal wrote:
> >
> > On 04-07-2025 16:04, Greg KH wrote:
> > > 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 at 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 at intel.com>
> > > > > > Signed-off-by: Badal Nilawar <badal.nilawar at intel.com>
> > > > > > Reviewed-by: Anshuman Gupta <anshuman.gupta at 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.
> > Another reason to maintain the sub_dir is to accommodate additional
> > files for future platforms. If you still insist, I'll remove the sub_dir.
> 
> Move files around when it happens, for now, it's silly and not needed.
> 
> > > > > > + * @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?
> >
> > Sorry, I got confused. Conversion is needed when assigning the
> > response structure elements.
> >
> > e.g ret = (int)(le32_to_cpu)rsp.status;
> 
> But these are read directly from the hardware?  If not, why are they marked as
> packed?
Yes, these are read from firmware, that is the reason they marked as __packed.
IMHO, don't we need change the explicit endianness of response status to address your comment.
Are we missing something here?

Thanks,
Anshuman
> 
> thanks,
> 
> greg k-h