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

[Greg KH <gregkh@linuxfoundation.org>]

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@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.
> 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?

thanks,

greg k-h