netdev - Re: [PATCH 5/7] net: stmmac: Program RX queue size and flow control

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:   Thu, 9 Mar 2017 21:44:09 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Stephen Warren <swarren@...dotorg.org>
Cc:     Mikko Perttunen <cyndis@...si.fi>,
        "David S . Miller" <davem@...emloft.net>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Joao Pinto <Joao.Pinto@...opsys.com>,
        Alexandre Courbot <gnurou@...il.com>,
        Jon Hunter <jonathanh@...dia.com>, netdev@...r.kernel.org,
        linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/7] net: stmmac: Program RX queue size and flow control

On Thu, Mar 09, 2017 at 01:18:11PM -0700, Stephen Warren wrote:
> On 03/09/2017 12:42 PM, Thierry Reding wrote:
> > On Mon, Feb 27, 2017 at 12:09:02PM +0200, Mikko Perttunen wrote:
> > > On 23.02.2017 19:24, Thierry Reding wrote:
> > > > From: Thierry Reding <treding@...dia.com>
> > > > 
> > > > Program the receive queue size based on the RX FIFO size and enable
> > > > hardware flow control for large FIFOs.
> 
> > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
> 
> > > > @@ -252,6 +253,44 @@ static void dwmac4_dma_chan_op_mode(void __iomem *ioaddr, int txmode,
> > > >  			mtl_rx_op |= MTL_OP_MODE_RTC_128;
> > > >  	}
> > > > 
> > > > +	mtl_rx_op &= ~MTL_OP_MODE_RQS_MASK;
> > > > +	mtl_rx_op |= rqs << MTL_OP_MODE_RQS_SHIFT;
> > > > +
> > > > +	/* enable flow control only if each channel gets 4 KiB or more FIFO */
> > > > +	if (rxfifosz >= 4096) {
> > > > +		unsigned int rfd, rfa;
> > > > +
> > > > +		mtl_rx_op |= MTL_OP_MODE_EHFC;
> > > > +
> > > > +		switch (rxfifosz) {
> > > > +		case 4096:
> > > > +			rfd = 0x03;
> > > > +			rfa = 0x01;
> > > > +			break;
> > > > +
> > > > +		case 8192:
> > > > +			rfd = 0x06;
> > > > +			rfa = 0x0a;
> > > > +			break;
> > > > +
> > > > +		case 16384:
> > > > +			rfd = 0x06;
> > > > +			rfa = 0x12;
> > > > +			break;
> > > > +
> > > > +		default:
> > > > +			rfd = 0x06;
> > > > +			rfa = 0x1e;
> > > > +			break;
> > > > +		}
> > > 
> > > Are these values correct? In the 4096 case, rfd > rfa, in all other cases
> > > the other way around. In any case it would be useful to have a comment
> > > clarifying the thresholds in bytes.
> > 
> > I'll investigate. To be honest I simply took this from Stephen's U-Boot
> > driver since that's already tested. I trust Stephen, so I didn't bother
> > double-checking.
> 
> I don't recall for sure, but I think these values came directly from either
> the upstream kernel (the non-stmmac driver) or NV downstream kernel EQoS
> driver, and I re-used them without investigating. I'm not even sure if the
> outer if() expression is true; these numbers might not even end up being
> used?

Yes they are, and they were even the key to making the STMMAC driver
work on Tegra186. Without programming these fields the driver would fail
to receive any packets.

I noticed that you had comments in the U-Boot driver that I had left out
(most likely because I forgot to add them after cleaning up after the
hacking session). Here's the original extract from U-Boot:

		/*
		 * Set Threshold for Activating Flow Contol space for min 2
		 * frames ie, (1500 * 1) = 1500 bytes.
		 *
		 * Set Threshold for Deactivating Flow Contol for space of
		 * min 1 frame (frame size 1500bytes) in receive fifo
		 */
		if (rqs == ((4096 / 256) - 1)) {
			/*
			 * This violates the above formula because of FIFO size
			 * limit therefore overflow may occur inspite of this.
			 */
			rfd = 0x3;	/* Full-3K */
			rfa = 0x1;	/* Full-1.5K */
		} else if (rqs == ((8192 / 256) - 1)) {
			rfd = 0x6;	/* Full-4K */
			rfa = 0xa;	/* Full-6K */
		} else if (rqs == ((16384 / 256) - 1)) {
			rfd = 0x6;	/* Full-4K */
			rfa = 0x12;	/* Full-10K */
		} else {
			rfd = 0x6;	/* Full-4K */
			rfa = 0x1E;	/* Full-16K */
		}

Two things are strange about this:

	1) the first comment says "2 frames", but the threshold value is
	   clearly just one frame

	2) the first set of rfd/rfa values has a wrong comment, by my
	   understanding: Full-3K should really be Full-2.5K

The encoding of these values is essentially:

	threshold = full - (1K + value * 0.5K)

Here's my updated version from the kernel driver:

		/*
		 * Set Threshold for Activating Flow Control to min 2 frames,
		 * i.e. 1500 * 2 = 3000 bytes.
		 *
		 * Set Threshold for Deactivating Flow Control to min 1 frame,
		 * i.e. 1500 bytes.
		 */
		switch (rxfifosz) {
		case 4096:
			/*
			 * This violates the above formula because of FIFO size
			 * limit therefore overflow may occur in spite of this.
			 */
			rfd = 0x03; /* Full-2.5K */
			rfa = 0x01; /* Full-1.5K */
			break;

		case 8192:
			rfd = 0x06; /* Full-4K */
			rfa = 0x0a; /* Full-6K */
			break;

		case 16384:
			rfd = 0x06; /* Full-4K */
			rfa = 0x12; /* Full-10K */
			break;

		default:
			rfd = 0x06; /* Full-4K */
			rfa = 0x1e; /* Full-16K */
			break;
		}

As best as I can tell these values are within the constraints given in
the TRM and they also make sense to me for the purposes they're used
for.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)