netdev - RE: [PATCH net v2 1/1] igc: read before write to SRRCTL register

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] [day] [month] [year] [list]

Message-ID: <PH0PR11MB5830E7158428C51912981EB0D8999@PH0PR11MB5830.namprd11.prod.outlook.com>
Date:   Fri, 14 Apr 2023 14:48:23 +0000
From:   "Song, Yoong Siang" <yoong.siang.song@...el.com>
To:     Jesper Dangaard Brouer <jbrouer@...hat.com>,
        David Laight <David.Laight@...LAB.COM>,
        "Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
        "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        "Jesper Dangaard Brouer" <hawk@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        "Fijalkowski, Maciej" <maciej.fijalkowski@...el.com>,
        Vedang Patel <vedang.patel@...el.com>,
        "Joseph, Jithu" <jithu.joseph@...el.com>,
        "Andre Guedes" <andre.guedes@...el.com>,
        Stanislav Fomichev <sdf@...gle.com>,
        "Keller, Jacob E" <jacob.e.keller@...el.com>
CC:     "Brouer, Jesper" <brouer@...hat.com>,
        "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "xdp-hints@...-project.net" <xdp-hints@...-project.net>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>,
        "Lobakin, Aleksander" <aleksander.lobakin@...el.com>
Subject: RE: [PATCH net v2 1/1] igc: read before write to SRRCTL register

On Friday, April 14, 2023 10:19 PM, Jesper Dangaard Brouer <jbrouer@...hat.com> wrote:
>On 14/04/2023 14.32, David Laight wrote:
>> From: Song, Yoong Siang
>>> Sent: 14 April 2023 12:16
>> ...
>>>> I have checked Foxville manual for SRRCTL (Split and Replication
>>>> Receive
>>>> Control) register and below GENMASKs looks correct.
>>>>
>>>>> -#define IGC_SRRCTL_BSIZEPKT_SHIFT		10 /* Shift _right_ */
>>>>> -#define IGC_SRRCTL_BSIZEHDRSIZE_SHIFT		2  /* Shift _left_ */
>>>>> +#define IGC_SRRCTL_BSIZEPKT_MASK	GENMASK(6, 0)
>>>>> +#define IGC_SRRCTL_BSIZEPKT_SHIFT	10 /* Shift _right_ */
>>>>
>>>> Shift due to 1 KB resolution of BSIZEPKT (manual field BSIZEPACKET)
>>>
>>> Ya, 1K = BIT(10), so need to shift right 10 bits.
>>
>> I bet the code would be easier to read if it did 'value / 1024u'.
>> The object code will be (much) the same.
>
>I agree. Code becomes more readable for humans and machine code will be the
>same.
>
>>>>> +#define IGC_SRRCTL_BSIZEHDRSIZE_MASK	GENMASK(13, 8)
>>>>> +#define IGC_SRRCTL_BSIZEHDRSIZE_SHIFT	2  /* Shift _left_ */
>>>>
>>>> This shift is suspicious, but as you inherited it I guess it works.
>>>> I did the math, and it happens to work, knowing (from manual) value
>>>> is in 64 bytes resolution.
>>>
>>> It is in 64 = BIT(6) resolution, so need to shift right 6 bits.
>>> But it start on 8th bit, so need to shift left 8 bits.
>>> Thus, total = shift left 2 bits.
>>>
>>> I didnt put the explanation into the header file because it is too
>>> lengthy and user can know from databook.
>
>Well, users usually don't have access to the databook (Programming
>Interface) PDF.  Personally I have it, but I had to go though a lot of red-tape to
>get it (under Red Hat NDA).
>
>
>>>
>>> How do you feel on the necessary of explaining the shifting logic?
>>
>> Not everyone trying to grok the code will have the manual.
>> Even writing (8 - 6) will help.
>> Or (I think) if the value is in bits 13-8 in units of 64 then just:
>> 	((value >> 8) & 0x1f) * 64
>> gcc will do a single shift right and a mask 9at some point).
>> You might want some defines, but if they aren't used much just
>> comments that refer to the names in the manual/datasheet can be
>> enough.
>>
>
>After Alexander Lobakin opened my eyes for GENMASK, FIELD_PREP and
>FIELD_GET, I find that easier to read and work-with these kind of register value
>manipulations, see[1] include/linux/bitfield.h.  It will also detect if the assigned
>value exceeds the mask (like David code handled via mask). (thx Alex)
>
>  [1]
>https://elixir.bootlin.com/linux/v6.3-rc6/source/include/linux/bitfield.h#L14
>
>So, instead of:
>   srrctl |= IGC_RX_HDR_LEN << IGC_SRRCTL_BSIZEHDRSIZE_SHIFT;
>
>I would write
>
>   /* BSIZEHDR value in 64 bytes resolution */
>   srrctl |= FIELD_PREP(IGC_SRRCTL_BSIZEHDRSIZE_MASK, (IGC_RX_HDR_LEN / 64));
>
>--Jesper

Thanks David and Jesper for the comments.
I agree to make the code more human readable.
Will refactor the code and send out v3 for review.

Thanks & Regards
Siang