Re: [PATCH v4 5/9] misc: amd-sbi: Add support for mailbox error codes

["Gupta, Akshay" <Akshay.Gupta@amd.com>]

On 10/18/2024 3:05 PM, Greg KH wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Fri, Oct 18, 2024 at 02:53:26PM +0530, Gupta, Akshay wrote:
>> On 10/15/2024 3:34 PM, Greg KH wrote:
>>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>>
>>>
>>> On Tue, Oct 15, 2024 at 02:42:08PM +0530, Gupta, Akshay wrote:
>>>> On 10/13/2024 8:49 PM, Greg KH wrote:
>>>>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>>>>
>>>>>
>>>>> On Thu, Sep 12, 2024 at 07:08:06AM +0000, Akshay Gupta wrote:
>>>>>> --- a/include/uapi/misc/amd-apml.h
>>>>>> +++ b/include/uapi/misc/amd-apml.h
>>>>>> @@ -38,6 +38,10 @@ struct apml_message {
>>>>>>                  __u32 mb_in[2];
>>>>>>                  __u8 reg_in[8];
>>>>>>          } data_in;
>>>>>> +     /*
>>>>>> +      * Error code is returned in case of soft mailbox
>>>>>> +      */
>>>>>> +     __u32 fw_ret_code;
>>>>>>     } __attribute__((packed));
>>>>> You can not just randomly change the size of a user/kernel structure
>>>>> like this, what just broke because of this?
>>>>>
>>>>> confused,
>>>> The changes are not because of anything is broken, we support 3 different
>>>> protocol under 1 IOCTL using the same structure. I split the patch to make
>>>> it easy to review.
>>>> Modification in patch 4, is only for the existing code. This patch (patch 5)
>>>> has additional functionality, so we do not want add multiple changes in
>>>> single patch (patch 4).
>>>>
>>>> The changes done in patches are as follows:
>>>>
>>>> Patch 4:
>>>>
>>>> - Adding basic structure as per current protocol in upstream kernel
>>> So what if we only take the first 4 patches?  Now any changes after that
>>> would change the user/kernel api and break things.
>> Yes, it will break. We need all the patches to go.
> That's not how to submit a patch series.  Please work with the other
> kernel developers at your company to do this right before resubmitting.
> You shouldn't rely on the community to point out basic engineering
> problems like this.  Would you want to review a series like this?
>
>>> Please don't write changes and then "fix them up" later on, that's not
>>> how to do stuff as it makes it very difficult to review.  What would you
>>> want to see if _you_ had to review this patch series?
>> We submitted a single patch in v1, later split the patch based on each
>> functionality for ease of review.
>>
>> I will squash and submit along with other review comments addressed.
> No, don't squash, do it in a patch series, one at a time properly such
> that if we were to take any moment in time of the series, all would
> still work correctly.  That's the proper way to do any sort of software
> engineering, this isn't unique to us at all.
>
> thanks,
>
> greg k-h

Hi Greg,

We have compiled and verified individual patch in the patch-set over 
reference BMC platforms.

We have an open-sourced user space library 
https://github.com/amd/esmi_oob_library/ 
<https://github.com/amd/esmi_oob_library/> which depend on the 
out-of-tree kernel modules open-sourced 
https://github.com/amd/apml_modules. <https://github.com/amd/apml_modules.>

This patch-set is an effort to upstream the out-of-tree kernel modules 
open-sourced at https://github.com/amd/apml_modules 
<https://github.com/amd/apml_modules>.

After all the patches are accepted into the Linux, we want to update the 
user-space consumers to move to drivers from Linux kernel and deprecate 
out-of-tree modules.

Thanks,

Akshay