netdev - Re: [PATCH v3 2/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] [day] [month] [year] [list]

Message-ID: <08913b53-0960-d8f4-9209-51a059b62fa2@seco.com>
Date:   Thu, 14 Oct 2021 12:01:25 -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:     Claudiu Beznea <claudiu.beznea@...rochip.com>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Russell King <linux@...linux.org.uk>
Subject: Re: [PATCH v3 2/2] net: macb: Clean up macb_validate



On 10/13/21 4:29 AM, Antoine Tenart wrote:
> Quoting Sean Anderson (2021-10-12 21:46:44)
>> +       /* 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
>> +        * Because GMII and MII both support 10/100, GMII falls through to MII.
>> +        *
>> +        * If we can't support an interface mode, we just clear the supported
>> +        * mask and return. The major complication is that if we get
>> +        * PHY_INTERFACE_MODE_NA, we must return all modes we support.  Because
>> +        * of this, NA starts at the top of the switch and falls all the way to
>> +        * the bottom, and doesn't return early if we don't support a
>> +        * particular mode.
>> +        */
>> +       switch (state->interface) {
>> +       case PHY_INTERFACE_MODE_NA:
>> +       case PHY_INTERFACE_MODE_10GBASER:
>> +               if (bp->caps & MACB_CAPS_HIGH_SPEED &&
>> +                   bp->caps & MACB_CAPS_PCS &&
>> +                   bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
>> +                       phylink_set_10g_modes(mask);
>> +                       phylink_set(mask, 10000baseKR_Full);
>> +               } else if (one) {
>> +                       bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
>> +                       return;
>> +               }
>> +               if (one)
>> +                       break;
>
> This can go in the first if block.

OK.

>> +               fallthrough;
>> +       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 (macb_is_gem(bp)) {
>> +                       if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
>
> Is not having MACB_CAPS_GIGABIT_MODE_AVAILABLE acceptable here, or
> should the two above checks be merged?

As I understand it, macb_is_gem does not imply GIGABIT_MODE. I'm not
sure if GIGABIT_MODE implies macb_is_gem. The logic here is mostly to
match that in prepare()/config(). From what I can gather, all accesses
to GEM registers are protected by macb_is_gem.

--Sean

>> +                               phylink_set(mask, 1000baseT_Full);
>> +                               phylink_set(mask, 1000baseX_Full);
>> +                               if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF))
>> +                                       phylink_set(mask, 1000baseT_Half);
>> +                       }
>> +               } else if (one) {
>> +                       bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
>> +                       return;
>> +               }
>> +               fallthrough;
>> +       case PHY_INTERFACE_MODE_MII:
>> +       case PHY_INTERFACE_MODE_RMII:
>> +               phylink_set(mask, 10baseT_Half);
>> +               phylink_set(mask, 10baseT_Full);
>> +               phylink_set(mask, 100baseT_Half);
>> +               phylink_set(mask, 100baseT_Full);
>> +               break;
>> +       default:
>>                 bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
>>                 return;
>>         }
>
> (For readability, it's not for me to decide in the end).
>
> Thanks,
> Antoine
>