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

[Nicolas Ferre <nicolas.ferre@microchip.com>]

On 25/10/2021 at 23:35, Sean Anderson wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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.

I like it as well. Thanks a lot for these enhancements.

Regards,
   Nicolas


-- 
Nicolas Ferre