linux-kernel - Re: [PATCH v6 10/21] gunyah: rsc_mgr: Add resource manager RPC core

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: <032eeabb-28a5-9568-6aee-5631acb72a7b@quicinc.com>
Date:   Thu, 3 Nov 2022 15:07:44 -0700
From:   Elliot Berman <quic_eberman@...cinc.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:     Jassi Brar <jassisinghbrar@...il.com>,
        Bjorn Andersson <quic_bjorande@...cinc.com>,
        Murali Nalajala <quic_mnalajal@...cinc.com>,
        Trilok Soni <quic_tsoni@...cinc.com>,
        Srivatsa Vaddagiri <quic_svaddagi@...cinc.com>,
        Carl van Schaik <quic_cvanscha@...cinc.com>,
        Prakruthi Deepak Heragu <quic_pheragu@...cinc.com>,
        Andy Gross <agross@...nel.org>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        Mark Rutland <mark.rutland@....com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Sudeep Holla <sudeep.holla@....com>,
        Marc Zyngier <maz@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Jonathan Corbet <corbet@....net>,
        "Will Deacon" <will@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        "Arnd Bergmann" <arnd@...db.de>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Amol Maheshwari <amahesh@....qualcomm.com>,
        Kalle Valo <kvalo@...nel.org>, <devicetree@...r.kernel.org>,
        <linux-doc@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 10/21] gunyah: rsc_mgr: Add resource manager RPC core



On 11/2/2022 5:22 PM, Greg Kroah-Hartman wrote:
> On Wed, Nov 02, 2022 at 11:04:45AM -0700, Elliot Berman wrote:
>>>>>> +/* Resource Manager Header */
>>>>>> +struct gh_rm_rpc_hdr {
>>>>>> +	u8 version : 4, hdr_words : 4;
>>>>>> +	u8 type : 2, fragments : 6;
>>>>>
>>>>> Ick, that's hard to read.  One variable per line please?
>>>>
>>>> Ack.
>>>>
>>>>> And why the bit packed stuff?  Are you sure this is the way to do this?
>>>>> Why not use a bitmask instead?
>>>>>
>>>>
>>>> I felt bit packed implementation is cleaner and easier to map to
>>>> understanding what the fields are used for.
>>>
>>> Ah, so this isn't what is on the "wire", then don't use a bitfield like
>>> this, use a real variable and that will be faster and simpler to
>>> understand.
>>>
>>
>> This is what's on the "wire". Whether I use bitfield or bit packed would be
>> functionally the same and is just a cosmetic change IMO.
> 
> Ah, that wasn't obvious at all.
> 
> Usually using bitfields like this for "wire" protocols is not a good
> idea (endian issues and all of that.)  Please use a bitmask instead, as
> that way you know exactly what is happening, and the compiler can
> usually generate much better code overall.
> 
> And as this is on the wire, please specify the endian values, _AND_ use
> the proper kernel types for stuff that goes between user/kernel or
> kernel/hardware, as you are not doing that here.

Ack