linux-kernel - Re: [RFC PATCH 2/3] net: sparx5: Add Sparx5 switchdev driver

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]

Date:   Mon, 30 Nov 2020 15:10:36 +0100
From:   Steen Hegelund <steen.hegelund@...rochip.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
CC:     Andrew Lunn <andrew@...n.ch>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Masahiro Yamada <masahiroy@...nel.org>,
        Lars Povlsen <lars.povlsen@...rochip.com>,
        Bjarni Jonasson <bjarni.jonasson@...rochip.com>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Microsemi List <microsemi@...ts.bootlin.com>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 2/3] net: sparx5: Add Sparx5 switchdev driver

On 28.11.2020 22:28, Russell King - ARM Linux admin wrote:
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>On Sat, Nov 28, 2020 at 08:06:16PM +0100, Andrew Lunn wrote:
>> > +static void sparx5_phylink_mac_config(struct phylink_config *config,
>> > +                                 unsigned int mode,
>> > +                                 const struct phylink_link_state *state)
>> > +{
>> > +   struct sparx5_port *port = netdev_priv(to_net_dev(config->dev));
>> > +   struct sparx5_port_config conf;
>> > +   int err = 0;
>> > +
>> > +   conf = port->conf;
>> > +   conf.autoneg = state->an_enabled;
>> > +   conf.pause = state->pause;
>> > +   conf.duplex = state->duplex;
>> > +   conf.power_down = false;
>> > +   conf.portmode = state->interface;
>> > +
>> > +   if (state->speed == SPEED_UNKNOWN) {
>> > +           /* When a SFP is plugged in we use capabilities to
>> > +            * default to the highest supported speed
>> > +            */
>>
>> This looks suspicious.
>
>Yes, it looks highly suspicious. The fact that
>sparx5_phylink_mac_link_up() is empty, and sparx5_phylink_mac_config()
>does all the work suggests that this was developed before the phylink
>re-organisation, and this code hasn't been updated for it.

Hi Russell,

The work started on 5.4 and I think I may have not really
understood the finer details in the changes in 5.9.
>
>Any new code for the kernel really ought to be updated for the new
>phylink methodology before it is accepted.
>

Could you point me to a good example of the new methodology?

>Looking at sparx5_port_config(), it also seems to use
>PHY_INTERFACE_MODE_1000BASEX for both 1000BASE-X and 2500BASE-X. All
>very well for the driver to do that internally, but it's confusing
>when it comes to reviewing this stuff, especially when people outside
>of the driver (such as myself) reviewing it need to understand what's
>going on with the configuration.

Hmmm. I better check if this is as intended.

>
>--
>RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
>FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Thanks for your comments.

BR
Steen

---------------------------------------
Steen Hegelund
steen.hegelund@...rochip.com