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

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

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.
>
>>>> +/**
>>>> + * struct csc_heci_late_bind_req - late binding request
>>>> + * @header: @ref mkhi_msg_hdr
>>>> + * @type: type of the late binding payload
>>>> + * @flags: flags to be passed to the firmware
>>>> + * @reserved: reserved field
>>> Reserved for what?  Set to what?
>> Reserved by firmware for future use, default value set to 0, I will update
>> above doc.
>>
>>>> + * @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;

>
>> Are you fine with this?
> Please be explicit.
>
>>>> +	ret = (int)rsp.status;
>>>> +end:
>>>> +	mei_cldev_disable(cldev);
>>>> +	kfree(req);
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static const struct late_bind_component_ops mei_late_bind_ops = {
>>>> +	.owner = THIS_MODULE,
>>> I thought you were going to drop the .owner stuff?
>>>
>>> Or if not, please implement it properly (i.e. by NOT forcing people to
>>> manually set it here.)
>> Somehow I missed this. I will drop it.
> And from the structure definition please.

Sure.

Thanks,
Badal

>
> thanks,
>
> greg k-h