Received-SPF: pass (google.com: domain of linux-kernel+bounces-195530-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 30 May 2024 17:35:22 +0300
From: Vladimir Oltean <olteanv@gmail.com>
To: "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc: Xiaolei Wang <xiaolei.wang@windriver.com>, Andrew Lunn <andrew@lunn.ch>,
	alexandre.torgue@foss.st.com, joabreu@synopsys.com,
	davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
	pabeni@redhat.com, mcoquelin.stm32@gmail.com,
	netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [net v2 PATCH] net: stmmac: Update CBS parameters when speed
 changes after linking up
Message-ID: <20240530143522.32c5lhkg42yiggob@skbuf>
References: <20240530061453.561708-1-xiaolei.wang@windriver.com>
 <f8b0843f-7900-4ad0-9e70-c16175e893d9@lunn.ch>
 <20240530132822.xv23at32wj73hzfj@skbuf>
 <ZliBzo7eETml/+bl@shell.armlinux.org.uk>
 <20240530135335.yanffjb3ketmoo7u@skbuf>
 <ZliJ2O+bj18jQ0B8@shell.armlinux.org.uk>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZliJ2O+bj18jQ0B8@shell.armlinux.org.uk>

On Thu, May 30, 2024 at 03:14:48PM +0100, Russell King (Oracle) wrote:
> > I don't see why the CBS parameters would need to be de-programmed from
> > hardware on a link down event. Is that some stmmac specific thing?
> 
> If the driver is having to do computation on the parameters based on
> the link speed, then when the link speed changes, the parameters
> no longer match what the kernel _thinks_ those parameters were
> programmed with.
> 
> What I'm trying to get over to you is that what you propose causes
> an inconsistency between how the hardware is _programmed_ to behave
> for CBS and what the kernel reports the CBS settings are if the
> link speed changes.
> 
> It's all very well saying that userspace should basically reconstruct
> the tc settings when the link changes, but not everyone is aware of
> that. I'm saying it's a problem if one isn't aware of this issue with
> this hardware, and one looks at tc qdisc show output, and assumes
> that reflects what is actually being used when it isn't.
> 
> It's quality of implmentation - as far as I'm concerned, the kernel
> should *not* mislead the user like this.

I was saying that the tc-cbs parameters input into the kernel should
already have the link speed baked into them:
portTransmitRate = idleSlope - sendSlope. In theory one could feed any
data into the kernel, but this is based on the IEEE 802.1Q formulas.

I had missed the fact that there is a calculation dependent on
priv->speed within tc_setup_cbs(), and I'm sorry for that. I thought
that the values were passed unaltered down to stmmac_config_cbs(). So
"make no change to the driver" is no longer my recommendation.

In that case, my recommendation is to do as sja1105_setup_tc_cbs() does:
replace priv->speed with the portTransmitRate recovered from the tc-cbs
parameters, and fully expect that when the link speed changes, user
space comes along and changes those parameters.