Re: [PATCH v4] net: macb: Fix several edge cases in validate

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

On 10/25/21 5:19 PM, Russell King (Oracle) wrote:
> On Mon, Oct 25, 2021 at 01:24:05PM -0400, Sean Anderson wrote:
>> There were several cases where validate() would return bogus supported
>> modes with unusual combinations of interfaces and capabilities. For
>> example, if state->interface was 10GBASER and the macb had HIGH_SPEED
>> and PCS but not GIGABIT MODE, then 10/100 modes would be set anyway. In
>> another case, SGMII could be enabled even if the mac was not a GEM
>> (despite this being checked for later on in mac_config()). These
>> inconsistencies make it difficult to refactor this function cleanly.
>>
>> This attempts to address these by reusing the same conditions used to
>> decide whether to return early when setting mode bits. The logic is
>> pretty messy, but this preserves the existing logic where possible.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>>
>> Changes in v4:
>> - Drop cleanup patch
>>
>> Changes in v3:
>> - Order bugfix patch first
>>
>> Changes in v2:
>> - New
>>
>>  drivers/net/ethernet/cadence/macb_main.c | 59 +++++++++++++++++-------
>>  1 file changed, 42 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index 309371abfe23..40bd5a069368 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -510,11 +510,16 @@ static void macb_validate(struct phylink_config *config,
>>  			  unsigned long *supported,
>>  			  struct phylink_link_state *state)
>>  {
>> +	bool have_1g = true, have_10g = true;
>>  	struct net_device *ndev = to_net_dev(config->dev);
>>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
>
> I think DaveM would ask for this to be reverse-christmas-tree, so the
> new bool should be here.

Ah, I wasn't aware that there was another variable-ordering style in use for net.

>>  	struct macb *bp = netdev_priv(ndev);
>>
>> -	/* We only support MII, RMII, GMII, RGMII & SGMII. */
>> +	/* There are three major types of interfaces we support:
>> +	 * - (R)MII supporting 10/100 Mbit/s
>> +	 * - GMII, RGMII, and SGMII supporting 10/100/1000 Mbit/s
>> +	 * - 10GBASER supporting 10 Gbit/s only
>> +	 */
>>  	if (state->interface != PHY_INTERFACE_MODE_NA &&
>>  	    state->interface != PHY_INTERFACE_MODE_MII &&
>>  	    state->interface != PHY_INTERFACE_MODE_RMII &&
>> @@ -526,27 +531,48 @@ static void macb_validate(struct phylink_config *config,
>>  		return;
>>  	}
>>
>> -	if (!macb_is_gem(bp) &&
>> -	    (state->interface == PHY_INTERFACE_MODE_GMII ||
>> -	     phy_interface_mode_is_rgmii(state->interface))) {
>> -		linkmode_zero(supported);
>> -		return;
>> +	/* For 1G and up we must have both have a GEM and GIGABIT_MODE */
>> +	if (!macb_is_gem(bp) ||
>> +	    (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)) {
>> +		if (state->interface == PHY_INTERFACE_MODE_GMII ||
>> +		    phy_interface_mode_is_rgmii(state->interface) ||
>> +		    state->interface == PHY_INTERFACE_MODE_SGMII ||
>> +		    state->interface == PHY_INTERFACE_MODE_10GBASER) {
>> +			linkmode_zero(supported);
>> +			return;
>> +		} else if (state->interface == PHY_INTERFACE_MODE_NA) {
>> +			have_1g = false;
>> +			have_10g = false;
>> +		}
>>  	}
>
> Would it make more sense to do:
>
> 	bool have_1g = false, have_10g = false;
>
> 	if (macb_is_gem(bp) &&
> 	    (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)) {
> 		if (bp->caps & MACB_CAPS_PCS)
> 			have_1g = true;
> 		if (bp->caps & MACB_CAPS_HIGH_SPEED)
> 			have_10g = true;
> 	}
>
> 	switch (state->interface) {
> 	case PHY_INTERFACE_MODE_NA:
> 	case PHY_INTERFACE_MODE_MII:
> 	case PHY_INTERFACE_MODE_RMII:
> 		break;
>
> 	case PHY_INTERFACE_MODE_GMII:
> 	case PHY_INTERFACE_MODE_RGMII:
> 	case PHY_INTERFACE_MODE_RGMII_ID:
> 	case PHY_INTERFACE_MODE_RGMII_RXID:
> 	case PHY_INTERFACE_MODE_RGMII_TXID:
> 	case PHY_INTERFACE_MODE_SGMII:
> 		if (!have_1g) {
> 			linkmode_zero(supported);
> 			return;
> 		}
> 		break;
>
> 	case PHY_INTERFACE_MODE_10GBASER:
> 		if (!have_10g) {
> 			linkmode_zero(supported);
> 			return;
> 		}
> 		break;
>
> 	default:
> 		linkmode_zero(supported);
> 		return;
> 	}
>
> This uses positive logic to derive have_1g and have_10g, and then uses
> the switch statement to validate against those. Would the above result
> in more understandable code?

I experimented with something like the above, but I wasn't able to
express it cleanly. I think what you have would work nicely.

--Sean