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

["Nilawar, Badal" <badal.nilawar@intel.com>]

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@linuxfoundation.org>
>>> Sent: Friday, July 4, 2025 5:31 PM
>>> To: Nilawar, Badal <badal.nilawar@intel.com>
>>> Cc: intel-xe@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-
>>> kernel@vger.kernel.org; Gupta, Anshuman <anshuman.gupta@intel.com>;
>>> Vivi, Rodrigo <rodrigo.vivi@intel.com>; Usyskin, Alexander
>>> <alexander.usyskin@intel.com>; Ceraolo Spurio, Daniele
>>> <daniele.ceraolospurio@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@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?
>> 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