netdev - Re: [PATCH net-next v2 1/2] net: dsa: microchip: ksz8: Make flow control, speed, and duplex on CPU port configurable

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: Wed, 17 May 2023 15:35:49 +0200
From: Oleksij Rempel <o.rempel@...gutronix.de>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: "David S. Miller" <davem@...emloft.net>, Andrew Lunn <andrew@...n.ch>,
	Eric Dumazet <edumazet@...gle.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Vladimir Oltean <olteanv@...il.com>,
	Woojung Huh <woojung.huh@...rochip.com>,
	Arun Ramadoss <arun.ramadoss@...rochip.com>, kernel@...gutronix.de,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH net-next v2 1/2] net: dsa: microchip: ksz8: Make flow
 control, speed, and duplex on CPU port configurable

On Wed, May 17, 2023 at 01:45:46PM +0100, Russell King (Oracle) wrote:
> On Wed, May 17, 2023 at 02:10:33PM +0200, Oleksij Rempel wrote:
> > +/**
> > + * ksz8_upstram_link_up - Configures the CPU/upstream port of the switch.
> > + * @dev: The KSZ device instance.
> > + * @port: The port number to configure.
> > + * @speed: The desired link speed.
> > + * @duplex: The desired duplex mode.
> > + * @tx_pause: If true, enables transmit pause.
> > + * @rx_pause: If true, enables receive pause.
> > + *
> > + * Description:
> > + * The function configures flow control and speed settings for the CPU/upstream
> > + * port of the switch based on the desired settings, current duplex mode, and
> > + * speed.
> > + */
> > +static void ksz8_upstram_link_up(struct ksz_device *dev, int port, int speed,
> > +				 int duplex, bool tx_pause, bool rx_pause)
> > +{
> > +	u8 ctrl = 0;
> > +
> > +	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;
> 
> It's come up before whether the pause settings should be used to control
> half-duplex flow control, and I believe the decision was they shouldn't.

Got it, back pressure and pause for flow control are two different
things. I'll remove the half-duplex back pressure control using pause
settings from the patch.

> The other thing I find slightly weird is that this is only being done
> for upstream ports - why would a port that's between switches or the
> switch and the CPU be in half duplex mode?

As for the CPU port half-duplex mode, it's currently configurable via
the device tree. I don't have a specific use case for it, but it's there
if needed. If it's causing confusion though, I'm open to removing it.
What do you think?

> Also, why would such a port want to use some kind of flow control? If
> the CPU starts sending pause frames because its got stuck, doesn't
> that eventually kill the entire network? Also doesn't it limit the
> network bandwidth to the ability of the host CPU *not* to send
> pause frames?

Before this patch, flow control on the CPU port was indeed hard-coded.
This patch lets us disable it if we want to, giving us a bit more
flexibility.

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 |