netdev - Re: [PATCH net-next] net: dsa: realtek: rtl8366rb: Configure ports properly

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:   Tue, 26 Jul 2022 16:20:18 +0200
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        Alvin Šipraga <alsi@...g-olufsen.dk>
Subject: Re: [PATCH net-next] net: dsa: realtek: rtl8366rb: Configure ports properly

On Tue, Jul 26, 2022 at 1:02 PM Vladimir Oltean <olteanv@...il.com> wrote:
> On Mon, Jul 25, 2022 at 10:29:57PM +0200, Linus Walleij wrote:

> > Instead of just hammering the CPU port up at 1GBit at
> > .phylink_mac_link_up calls for that specific port, support
> > configuring any port: this works like a charm.
>
> Could you clarify what is intended to be functionally improved with this
> change, exactly?

I can try, but as usual I am probably confused :)

> According to your phylink_get_caps() implementation, I see that all
> ports are internal, so presumably the CPU ports too (and the user ports
> are connected to internal PHYs).

Correct, if by internal you mean there is no external, discrete PHY
component. They all route out to the physical sockets, maybe with
some small analog filter inbetween.

> Is it just to act upon the phylink parameters rather than assuming the
> CPU port is at gigabit? Can you actually set the CPU port at lower rates?

I think you can, actually. The Realtek vendor mess does support it.

Hm I should test to gear it down to 100Mbit and 10Mbit and see
what happens.

> As for the internal PHY ports, do they need their link speed to be
> forced at 10/100, or did those previously work at those lower speeds,
> just left unforced?

They were left in "power-on"-state. Which I *guess* is
autonegotiate. But haven't really tested.

It leaves me a bit uneasy since these registers are never explicit
set up to autonegotiate. Maybe I should do a separate patch
to just set them explicitly in autonegotiation mode?

I have a small 10MBit router, I will try to connect it and see what
happens, if anything happens and can be detected.

> > -static void
> > -rtl8366rb_mac_link_up(struct dsa_switch *ds, int port, unsigned int mode,
> > -                   phy_interface_t interface, struct phy_device *phydev,
> > -                   int speed, int duplex, bool tx_pause, bool rx_pause)
> > +static void rtl8366rb_link_get_caps(struct dsa_switch *ds, int port,
> > +                                 struct phylink_config *config)
> >  {
> > -     struct realtek_priv *priv = ds->priv;
> > -     int ret;
> > +     /* The SYM and ASYM pause is RX and TX pause */
>
> No, SYM and ASYM pause are not RX and TX pause, but rather they are
> advertisement bits. After autoneg completes, the 4 SYM and ASYM pause
> advertisement bits of you and your link partner get resolved independently
> by you and your link partner according to the table described in
> linkmode_resolve_pause(), and the result of that resolution is what RX
> and TX pause are.

I stand corrected. I'll reword or drop this.

Yours,
Linus Walleij