Re: [PATCH v6 02/10] mei: late_bind: add late binding component driver
From: Nilawar, Badal
Date: Fri Jul 04 2025 - 09:06:19 EST
On 04-07-2025 17:59, Greg KH wrote:
On Fri, Jul 04, 2025 at 12:21:42PM +0000, Gupta, Anshuman wrote:
-----Original Message-----
From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
Sent: Friday, July 4, 2025 5:31 PM
To: Nilawar, Badal <badal.nilawar@xxxxxxxxx>
Cc: intel-xe@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; linux-
kernel@xxxxxxxxxxxxxxx; Gupta, Anshuman <anshuman.gupta@xxxxxxxxx>;
Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>; Usyskin, Alexander
<alexander.usyskin@xxxxxxxxx>; Ceraolo Spurio, Daniele
<daniele.ceraolospurio@xxxxxxxxx>
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@xxxxxxxxx>
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@xxxxxxxxx>
Signed-off-by: Badal Nilawar <badal.nilawar@xxxxxxxxx>
Reviewed-by: Anshuman Gupta <anshuman.gupta@xxxxxxxxx>
---
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.
Agreed. Therefore, the following assignment is valid and needed as ret
can be BE if CPU is BE.
e.g. ret = (int)le32_to_cpu(rsp.status);
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.
Ok
thanks,
greg k-h