Re: [PATCH 2/9] mei: late_bind: add late binding component driver

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

[-- Attachment #1: Type: text/plain, Size: 4250 bytes --]

Hi Greg,

On 27-07-2025 20:16, Usyskin, Alexander wrote:
>> Subject: Re: [PATCH 2/9] mei: late_bind: add late binding component driver
>>
>> On Wed, Jul 16, 2025 at 02:26:26PM +0000, Usyskin, Alexander wrote:
>>>>>>> +	if (bytes < sizeof(rsp)) {
>>>>>>> +		dev_err(dev, "bad response from the firmware: size
>> %zd <
>>>>>> %zu\n",
>>>>>>> +			bytes, sizeof(rsp));
>>>>>>> +		ret = -EPROTO;
>>>>>>> +		goto end;
>>>>>>> +	}
>>>>>> Why not check this above when you check against the size of the
>> header?
>>>>>> You only need one size check, not 2.
>>>>> Firmware may return only header with result field set without the data.
>>>> Then the firmware is broken :)
>>>>
>>>>> We are parsing the header first and then starting to parse data.
>>>>> If we check for whole message size at the beginning we'll miss the result
>>>> data.
>>>>
>>>> You mean you will make it harder to debug the firmware, as you will not
>>>> be printing out the header information?  Or something else?  The
>>>> bytes variable HAS to match the full structure size, not just the header
>>>> size, according to this code.  So just test for that and be done with
>>>> it!
>>> The CSME firmware returns only command header if, like, command is not
>> recognised.
>>> This may happen because of firmware bug or for firmware is
>> configured/compiled
>>> that way.
>>> This seems reasonable for the complex protocols where firmware may not be
>>> aware of this particular command at all and have no idea what the data size
>> it
>>> should send in reply.
>>> Printing result from the header will allow us to understand either this is the
>> firmware
>>> problem or driver sent something wrong.
>> Then make it obvious in your checking and in your error messages as to
>> what you are doing here.  Checking the size of the buffer in two
>> different places, with different values is very odd, and deserves a lot
>> of explaination.
>>
> Is this addition
>         /*
>          * Received message size may be smaller than the full message size when
>          * reply contains only MKHI header with result field set to the error code.
>          * Check the header size and content first to output exact error, if needed,
>          * and then process to the whole message.
>          */
>
> and remodelling error messages like "received less then header size from the firmware"
> made it clean for people not involved with our firmware?
> I'm too deep in this to judge the wording.

I'm planning to include the following code in the next revision. Does 
this change align with your recommendation?

    +      /*
    +        * Received message size may be smaller than the full
    message size when
    +        * reply contains only MKHI header with result field set to
    the error code.
    +        * Check the header size and content first to output exact
    error, if needed,
    +        * and then process to the whole message.
    +        */
             if (bytes < sizeof(rsp.header)) {
    -               dev_err(dev, "bad response header from the firmware:
    size %zd < %zu\n",
    +               dev_err(dev, "received less than header size from
    the firmware: %zd < %zu\n",
                             bytes, sizeof(rsp.header));
                     ret = -EPROTO;
                     goto end;
             }
             if (mei_lb_check_response(dev, &rsp.header)) {
    -               dev_err(dev, "bad result response from the firmware:
    0x%x\n",
    +               dev_err(dev, "bad response from the firmware:
    header: 0x%x\n",
                             *(uint32_t *)&rsp.header);
                     ret = -EPROTO;
                     goto end;
             }
             if (bytes < sizeof(rsp)) {
    -               dev_err(dev, "bad response from the firmware: size
    %zd < %zu\n",
    +               dev_err(dev, "received less than message size from
    the firmware: %zd < %zu\n",
                             bytes, sizeof(rsp));
                     ret = -EPROTO;
                     goto end;

Thanks,
Badal

>
> - -
> Thanks,
> Sasha
>
>

[-- Attachment #2: Type: text/html, Size: 9573 bytes --]