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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <420ab0bf-32cf-cd98-c711-0dacc8bcc161@amd.com>
Date: Fri, 18 Oct 2024 14:53:26 +0530
From: "Gupta, Akshay" <Akshay.Gupta@....com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux@...ck-us.net, arnd@...db.de, naveenkrishna.chatradhi@....com
Subject: Re: [PATCH v4 5/9] misc: amd-sbi: Add support for mailbox error codes


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.

>
> 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.

>
> thanks,
>
> greg k-h