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

[Sean Anderson <sean.anderson@seco.com>]

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