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

[Greg KH]

On Fri, Jul 04, 2025 at 12:21:42PM +0000, Gupta, Anshuman wrote:
> 
> 
> > -----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?

Yes.  The firmware defines these values as __le32, right?  And if you
read a chunk of memory and cast it into this structure, those fields
are now also __le32, right?  So to read them in the driver you need to
then call le32_to_cpu() on those values.

Just like data on the USB bus, or any other hardware type.  You must
define what endian the data is in and then convert it to "native" before
accessing it properly.

thanks,

greg k-h