linux-kernel - Re: [PATCH net-next v2 1/9] net: ethernet: implement OPEN Alliance control transaction interface

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]

Date:   Wed, 25 Oct 2023 11:16:16 +0000
From:   <Parthiban.Veerasooran@...rochip.com>
To:     <andrew@...n.ch>
CC:     <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
        <pabeni@...hat.com>, <robh+dt@...nel.org>,
        <krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
        <corbet@....net>, <Steen.Hegelund@...rochip.com>,
        <rdunlap@...radead.org>, <horms@...nel.org>,
        <casper.casan@...il.com>, <netdev@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-doc@...r.kernel.org>, <Horatiu.Vultur@...rochip.com>,
        <Woojung.Huh@...rochip.com>, <Nicolas.Ferre@...rochip.com>,
        <UNGLinuxDriver@...rochip.com>, <Thorsten.Kummermehr@...rochip.com>
Subject: Re: [PATCH net-next v2 1/9] net: ethernet: implement OPEN Alliance
 control transaction interface

Hi Andrew,

Thanks for reviewing my patches.

On 24/10/23 2:58 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> +static void oa_tc6_prepare_ctrl_buf(struct oa_tc6 *tc6, u32 addr, u32 val[],
>> +                                 u8 len, bool wnr, u8 *buf, bool prote)
> 
> One of the comments i made last time was that wnr is not obvious. I
> assume it means write-not-read. So why not just write? Since it a
> boolean, i assume war is never needed, so !wnr cal always be
> considered rnw.
> 
> And prote could well be protect, the two extra characters make it a
> lot more obvious. Or better still.
Just to be aligned with the naming in the OPEN Alliance specification 
document, used the same name as it is. wnr and prote are taken from the 
specification document. Please refer the screenshots attached here from 
the specification document.

Still if you feel like using "write" instead of "wnr" and "protect" 
instead of "prote", I will change them in the next revision.
> 
>> +{
>> +     u32 hdr;
>> +
>> +     /* Prepare the control header with the required details */
>> +     hdr = FIELD_PREP(CTRL_HDR_DNC, 0) |
>> +           FIELD_PREP(CTRL_HDR_WNR, wnr) |
>> +           FIELD_PREP(CTRL_HDR_AID, 0) |
>> +           FIELD_PREP(CTRL_HDR_MMS, addr >> 16) |
>> +           FIELD_PREP(CTRL_HDR_ADDR, addr) |
>> +           FIELD_PREP(CTRL_HDR_LEN, len - 1);
>> +     hdr |= FIELD_PREP(CTRL_HDR_P, oa_tc6_get_parity(hdr));
>> +     *(__be32 *)buf = cpu_to_be32(hdr);
>> +
>> +     if (wnr) {
>> +             for (u8 i = 0; i < len; i++) {
>> +                     u16 pos;
>> +
>> +                     if (!prote) {
> 
> nitpick, but please use positive logic. Do the protected case first.
Sure, will do it in the next revision.

Best Regards,
Parthiban V
> 
>           Andrew
> 

Download attachment "wnr.png" of type "image/png" (108929 bytes)

Download attachment "prote.png" of type "image/png" (163542 bytes)