| 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
| ||
|
Date: Tue, 11 Apr 2023 10:56:26 +0200
From: Oleksij Rempel <o.rempel@...gutronix.de>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>,
Woojung Huh <woojung.huh@...rochip.com>,
Arun Ramadoss <arun.ramadoss@...rochip.com>,
Florian Fainelli <f.fainelli@...il.com>,
netdev@...r.kernel.org, UNGLinuxDriver@...rochip.com,
Eric Dumazet <edumazet@...gle.com>,
Vladimir Oltean <olteanv@...il.com>, kernel@...gutronix.de,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
"David S. Miller" <davem@...emloft.net>
Subject: Re: FWD: Re: [PATCH net-next v1 1/1] net: dsa: microchip: ksz8: Make
flow control, speed, and duplex on CPU port configurable
On Fri, Apr 07, 2023 at 06:44:20PM +0100, Russell King (Oracle) wrote:
> On Fri, Apr 07, 2023 at 04:25:57PM +0200, Andrew Lunn wrote:
> > > +void ksz8_phylink_mac_link_up(struct ksz_device *dev, int port,
> > > + unsigned int mode, phy_interface_t interface,
> > > + struct phy_device *phydev, int speed, int duplex,
> > > + bool tx_pause, bool rx_pause)
> > > +{
> > > + struct dsa_switch *ds = dev->ds;
> > > + struct ksz_port *p;
> > > + u8 ctrl = 0;
> > > +
> > > + p = &dev->ports[port];
> > > +
> > > + if (dsa_upstream_port(ds, port)) {
> > > + u8 mask = SW_HALF_DUPLEX_FLOW_CTRL | SW_HALF_DUPLEX |
> > > + SW_FLOW_CTRL | SW_10_MBIT;
> > > +
> > > + if (duplex) {
> > > + if (tx_pause && rx_pause)
> > > + ctrl |= SW_FLOW_CTRL;
> > > + } else {
> > > + ctrl |= SW_HALF_DUPLEX;
> > > + if (tx_pause && rx_pause)
> > > + ctrl |= SW_HALF_DUPLEX_FLOW_CTRL;
> > > + }
> > > +
> > > + if (speed == SPEED_10)
> > > + ctrl |= SW_10_MBIT;
> > > +
> > > + ksz_rmw8(dev, REG_SW_CTRL_4, mask, ctrl);
> > > +
> > > + p->phydev.speed = speed;
> > > + } else {
> > > + const u16 *regs = dev->info->regs;
> > > +
> > > + if (duplex) {
> > > + if (tx_pause && rx_pause)
> > > + ctrl |= PORT_FORCE_FLOW_CTRL;
> > > + } else {
> > > + if (tx_pause && rx_pause)
> > > + ctrl |= PORT_BACK_PRESSURE;
> > > + }
> > > +
> > > + ksz_rmw8(dev, regs[P_STP_CTRL], PORT_FORCE_FLOW_CTRL |
> > > + PORT_BACK_PRESSURE, ctrl);
>
> So, I guess the idea here is to enable some form of flow control when
> both tx and rx pause are enabled.
>
> Here's a bunch of questions I would like answered before I give a tag:
>
> 1) It looks like the device only supports symmetric pause?
This part of driver supports two family of switches: ksz88xx and
ksz87xx.
According to KSZ8765CLX datasheet:
Per port, we control pause rx and tx with one bit:
Register 18 (0x12): Port 1 Control 2
Bit 4 - Force Flow Control
1 = Enables Rx and Tx flow control on the port, regardless of the AN result.
0 = Flow control is enabled based on the AN result (Default)
Globally, pause tx and/or rx can be disabled:
Register 3 (0x03): Global Control 1
Bit 5 - IEEE 802.3x Transmit Flow Control Disable
0 = Enables transmit flow control based on AN result.
1 = Will not enable transmit flow control regardless of the AN result.
Bit 4 - IEEE 802.3x Receive Flow Control Disable
0 = Enables receive flow control based on AN result.
1 = Will not enable receive flow control regardless of the AN result.
So, it is possible to configure the entire switch in SYNC or ASYNC mode
only. Still not sure what role plays autoneg in this configuration:
Register 55 (0x37): Port 3 Control 7 (only for ports 3 and 4)
Bits 5 - 4 - Advertised_Flow_Control _Capability
00 = No pause
01 = Symmetric PAUSE
10 = Asymmetric PAUSE
11 = Both Symmetric PAUSE and Asymmetric
According to this bits, it is possible to announce both Symmetric
and Asymmetric PAUSE, but will the switch enable asymmetric mode
properly if link partner advertise asymmetric too?
Suddenly I do not have ksz87xx variants to test it.
> 2) If yes, are you *not* providing MAC_ASYM_PAUSE in the MAC
> capabilities? If not, why not?
Current code looks as follow:
1402 void ksz8_get_caps(struct ksz_device *dev, int port,
1403 ┊ ┊ struct phylink_config *config)
1404 {
1405 ┊ config->mac_capabilities = MAC_10 | MAC_100;
1406 ┊
1407 ┊ /* Silicon Errata Sheet (DS80000830A):
1408 ┊ * "Port 1 does not respond to received flow control PAUSE frames"
1409 ┊ * So, disable Pause support on "Port 1" (port == 0) for all ksz88x3
1410 ┊ * switches.
1411 ┊ */
1412 ┊ if (!ksz_is_ksz88x3(dev) || port)
1413 ┊ ┊ config->mac_capabilities |= MAC_SYM_PAUSE;
1414 ┊
1415 ┊ /* Asym pause is not supported on KSZ8863 and KSZ8873 */
1416 ┊ if (!ksz_is_ksz88x3(dev))
1417 ┊ ┊ config->mac_capabilities |= MAC_ASYM_PAUSE;
1418 }
wich will set SYM support for all ksz87xx ports and all except of port == 0
on ksz87xx.
but SYM and ASYM modes depend on global switch configuration, so this
code is probably correct as long as global defaults are not changed. The
Autoneg based MAC_ASYM_PAUSE is still not clear for me.
> 3) If yes, then please do as others do and use tx_pause || rx_pause
> here.
Hm... you are right.
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Powered by blists - more mailing lists