netdev - Re: [PATCH v2 1/2] net: macb: Clean up macb

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: <419c2d47-4748-8ba4-613c-cc99558eeb48@seco.com>
Date:   Tue, 12 Oct 2021 12:34:50 -0400
From:   Sean Anderson <sean.anderson@...o.com>
To:     Antoine Tenart <atenart@...nel.org>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org
Cc:     Russell King <linux@...linux.org.uk>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Claudiu Beznea <claudiu.beznea@...rochip.com>
Subject: Re: [PATCH v2 1/2] net: macb: Clean up macb_validate

On 10/12/21 4:33 AM, Antoine Tenart wrote:
> Hello Sean,
>
> Quoting Sean Anderson (2021-10-11 18:55:16)
>> As the number of interfaces grows, the number of if statements grows
>> ever more unweildy. Clean everything up a bit by using a switch
>> statement. No functional change intended.
>
> I'm not 100% convinced this makes macb_validate more readable: there are
> lots of conditions, and jumps, in the switch.

The conditions are necessary to determine if the mac actually supports
the mode being requested. The jumps are all forward, and all but one
could be replaced with

	bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
	return;

The idea being that the NA mode goes from top to bottom, and all the
other modes do as well.

> Maybe you could try a mixed approach; keeping the invalid modes checks
> (bitmap_zero) at the beginning and once we know the mode is valid using
> a switch statement. That might make it easier to read as this should
> remove lots of conditionals. (We'll still have the one/_NA checks
> though).

This is actually the issue I wanted to address. The interface checks are
effectively performed twice or sometimes three times. There are also
gotos in the original design to deal with e.g. 10GBASE not having
10/100/1000 modes. This makes it easy to introduce bugs when adding new
modes, such as what happened with SGMII.

> (Also having patch 1 first will improve things).

Yes. Some of the complexity is simply to deal with SGMII being a special
case.

--Sean