netdev - RE: [PATCH net-next v6 6/6] dsa: lan9303: Migrate to PHYLINK

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, 16 Jan 2023 17:22:05 +0000
From:   <Jerry.Ray@...rochip.com>
To:     <linux@...linux.org.uk>
CC:     <andrew@...n.ch>, <f.fainelli@...il.com>, <olteanv@...il.com>,
        <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
        <pabeni@...hat.com>, <jbe@...gutronix.de>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH net-next v6 6/6] dsa: lan9303: Migrate to PHYLINK

> > +static void lan9303_phylink_get_caps(struct dsa_switch *ds, int port,
> > +                                  struct phylink_config *config)
> > +{
> > +     struct lan9303 *chip = ds->priv;
> > +
> > +     dev_dbg(chip->dev, "%s(%d) entered.", __func__, port);
> > +
> > +     config->mac_capabilities = MAC_10 | MAC_100 | MAC_ASYM_PAUSE |
> > +                                MAC_SYM_PAUSE;
> 
> You indicate that pause modes are supported, but...
> 
> > +static void lan9303_phylink_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)
> > +{
> > +     u32 ctl;
> > +
> > +     /* On this device, we are only interested in doing something here if
> > +      * this is the xMII port. All other ports are 10/100 phys using MDIO
> > +      * to control there link settings.
> > +      */
> > +     if (port != 0)
> > +             return;
> > +
> > +     ctl = lan9303_phy_read(ds, port, MII_BMCR);
> > +
> > +     ctl &= ~BMCR_ANENABLE;
> > +
> > +     if (speed == SPEED_100)
> > +             ctl |= BMCR_SPEED100;
> > +     else if (speed == SPEED_10)
> > +             ctl &= ~BMCR_SPEED100;
> > +     else
> > +             dev_err(ds->dev, "unsupported speed: %d\n", speed);
> > +
> > +     if (duplex == DUPLEX_FULL)
> > +             ctl |= BMCR_FULLDPLX;
> > +     else
> > +             ctl &= ~BMCR_FULLDPLX;
> > +
> > +     lan9303_phy_write(ds, port, MII_BMCR, ctl);
> 
> There is no code here to program the resolved pause modes. Is it handled
> internally within the switch? (Please add a comment to this effect
> either in get_caps or here.)
> 
> Thanks.
> 

As I look into this, the part does have control flags for advertising
Symmetric and Asymmetric pause toward the link partner. The default is set
by a configuration strap on power-up. I am having trouble mapping the rx and
tx pause parameters into symmetric and asymmetric controls. Where can I find
the proper definitions and mappings?

  ctl &= ~( ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_AYM);
  if(tx_pause)
    ctl |= ADVERTISE_PAUSE_CAP;
  if(rx_pause)
    ctl |= ADVERTISE_PAUSE_AYM;

If I can pause my transmissions (receive pause requests), then advertise
symmetric whether I ever sent pause requests or not.
If I can send pause requests (using flow control on my receive side), then make
sure asymmetric support is also advertised as rx_pause might have been clear.
Not that if both rx and tx pause is supported, we can support either symmetric
or asymmetric operating modes.

If I receive a pause request, it affects my transmit data flow. So one could
argue the rx_pause flag controls my ability to receive pause requests. I tend
to overthink and almost always get these 50/50 propositions wrong.

Regards,
Jerry