Received-SPF: pass (google.com: domain of linux-kernel+bounces-83665-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Tue, 27 Feb 2024 17:00:12 +0000
From: Simon Horman <horms@kernel.org>
To: Piotr Wejman <piotrwejman90@gmail.com>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>,
	Jose Abreu <joabreu@synopsys.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>, netdev@vger.kernel.org,
	linux-stm32@st-md-mailman.stormreply.com,
	linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] net: stmmac: fix rx queue priority assignment
Message-ID: <20240227170012.GC277116@kernel.org>
References: <20240226093144.31965-1-piotrwejman90@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240226093144.31965-1-piotrwejman90@gmail.com>

On Mon, Feb 26, 2024 at 10:31:44AM +0100, Piotr Wejman wrote:
> The driver should ensure that same priority is not mapped to multiple
> rx queues. Currently rx_queue_priority() function is adding
> priorities for a queue without clearing them from others.
> 
> >From DesignWare Cores Ethernet Quality-of-Service
> Databook, section 17.1.29 MAC_RxQ_Ctrl2:
> "[...]The software must ensure that the content of this field is
> mutually exclusive to the PSRQ fields for other queues, that is,
> the same priority is not mapped to multiple Rx queues[...]"
> 
> After this patch, rx_queue_priority() function will:
> - assign desired priorities to a queue
> - remove those priorities from all other queues
> The write sequence of CTRL2 and CTRL3 registers is done in the way to
> ensure this order.
> 
> Signed-off-by: Piotr Wejman <piotrwejman90@gmail.com>
> ---
> Changes in v2:
>   - Add some comments
>   - Apply same changes to dwxgmac2_rx_queue_prio()
>   - Revert "Rename prio argument to prio_mask"
>   - Link to v1: https://lore.kernel.org/netdev/20240219102405.32015-1-piotrwejman90@gmail.com/T/#u
> 
>  .../net/ethernet/stmicro/stmmac/dwmac4_core.c | 42 +++++++++++++++----
>  .../ethernet/stmicro/stmmac/dwxgmac2_core.c   | 40 ++++++++++++++----
>  2 files changed, 66 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
> index 6b6d0de09619..76ec0c1da250 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
> @@ -92,19 +92,43 @@ static void dwmac4_rx_queue_priority(struct mac_device_info *hw,
>  				     u32 prio, u32 queue)
>  {
>  	void __iomem *ioaddr = hw->pcsr;
> -	u32 base_register;
> -	u32 value;
> +	u32 clear_mask = 0;
> +	u32 ctrl2, ctrl3;
> +	int i;
>  
> -	base_register = (queue < 4) ? GMAC_RXQ_CTRL2 : GMAC_RXQ_CTRL3;
> -	if (queue >= 4)
> -		queue -= 4;
> +	ctrl2 = readl(ioaddr + GMAC_RXQ_CTRL2);
> +	ctrl3 = readl(ioaddr + GMAC_RXQ_CTRL3);
> +	
> +	/* The software must ensure that the same priority
> +	 * is not mapped to multiple Rx queues.
> +	 */
> +	for (i = 0; i < 4; i++)
> +		clear_mask |= ((prio << GMAC_RXQCTRL_PSRQX_SHIFT(i)) &
> +						GMAC_RXQCTRL_PSRQX_MASK(i));
>  
> -	value = readl(ioaddr + base_register);
> +	ctrl2 &= ~clear_mask;
> +	ctrl3 &= ~clear_mask;
>  
> -	value &= ~GMAC_RXQCTRL_PSRQX_MASK(queue);
> -	value |= (prio << GMAC_RXQCTRL_PSRQX_SHIFT(queue)) &
> +	/* Assign new priorities to a queue and
> +	 * clear them from others queues.
> +	 * The CTRL2 and CTRL3 registers write sequence is done
> +	 * in the way to ensure this order.
> +	 */
> +	if (queue < 4) {
> +		ctrl2 |= (prio << GMAC_RXQCTRL_PSRQX_SHIFT(queue)) &
>  						GMAC_RXQCTRL_PSRQX_MASK(queue);
> -	writel(value, ioaddr + base_register);
> +
> +		writel(ctrl2, ioaddr + GMAC_RXQ_CTRL2);
> +		writel(ctrl3, ioaddr + GMAC_RXQ_CTRL3);
> +	} else {
> +		queue -= 4;
> +
> +		ctrl3 |= (prio << GMAC_RXQCTRL_PSRQX_SHIFT(queue)) &
> +						GMAC_RXQCTRL_PSRQX_MASK(queue);
> +
> +		writel(ctrl3, ioaddr + GMAC_RXQ_CTRL3);
> +		writel(ctrl2, ioaddr + GMAC_RXQ_CTRL2);
> +	}
>  }

Hi Piotr,

Sorry if I am on the wrong track here, but this seems a little odd to me.

My reading is that each byte of GMAC_RXQ_CTRL2 and GMAC_RXQ_CTRL3
hold the priority value - an integer in the range of 0-255 - for
each of 8 queues.

This corresponds with the way that the queue is set both before and
after this patch.

But the code immediately above treats these bytes as bit fields.

Consider the case where all queues are initialised to 0
(I have no idea if this is valid queue values).

Now suppose we wish to set Queue 0 to Priority 7.
Then my my reading we will end up with.

clear_mask = 0x07070707
ctrl0 = (0x00000000 & ~clear_mask) | 0x00000007 = 0x00000007
ctrl3 =  0x00000000 & ~clear_mask               = 0x00000000

So far so good, but now suppose we now wish to set Queue 1 to Priority 9.
Then we get:

clear_mask = 0x09090909
ctrl0 = (0x00000007 & ~clear_mask) | 0x00000900 = 0x00000906
ctrl3 =  0x00000000 & ~clear_mask               = 0x00000000

Now queue 0 seems to have priority 6.

..