diff mbox series

[v2] net: stmmac: fix rx queue priority assignment

Message ID 20240226093144.31965-1-piotrwejman90@gmail.com
State New
Headers
Series [v2] net: stmmac: fix rx queue priority assignment |

Commit Message

Piotr Wejman Feb. 26, 2024, 9:31 a.m. UTC 
  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(-)

Comments

Simon Horman Feb. 27, 2024, 5 p.m. UTC | #1 
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.

..
Simon Horman Feb. 27, 2024, 9:01 p.m. UTC | #2 
On Tue, Feb 27, 2024 at 05:00:12PM +0000, Simon Horman wrote:
> 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.

Thinking about this some more, and checking the code some more, I realise I
am wrong here. I now see that the priority values are bit-fields not
integers. So I think what you have is fine.

Sorry about the noise.
Jakub Kicinski Feb. 28, 2024, 3:24 a.m. UTC | #3 
On Mon, 26 Feb 2024 10:31:44 +0100 Piotr Wejman wrote:
> +	ctrl2 = readl(ioaddr + XGMAC_RXQ_CTRL2);
> +	ctrl3 = readl(ioaddr + XGMAC_RXQ_CTRL3);
> +	

checkpatch points out there is an unnecessary tab on this empty line

> +	/* The software must ensure that the same priority
> +	 * is not mapped to multiple Rx queues.
> +	 */
diff mbox series

Patch

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);
+	}
 }
 
 static void dwmac4_tx_queue_priority(struct mac_device_info *hw,
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
index 1af2f89a0504..7ad73e1d353e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
@@ -105,17 +105,43 @@  static void dwxgmac2_rx_queue_prio(struct mac_device_info *hw, u32 prio,
 				   u32 queue)
 {
 	void __iomem *ioaddr = hw->pcsr;
-	u32 value, reg;
+	u32 clear_mask = 0;
+	u32 ctrl2, ctrl3;
+	int i;
 
-	reg = (queue < 4) ? XGMAC_RXQ_CTRL2 : XGMAC_RXQ_CTRL3;
-	if (queue >= 4)
+	ctrl2 = readl(ioaddr + XGMAC_RXQ_CTRL2);
+	ctrl3 = readl(ioaddr + XGMAC_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 << XGMAC_PSRQ_SHIFT(i)) &
+						XGMAC_PSRQ(i));
+
+	ctrl2 &= ~clear_mask;
+	ctrl3 &= ~clear_mask;
+
+	/* 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 << XGMAC_PSRQ_SHIFT(queue)) &
+						XGMAC_PSRQ(queue);
+
+		writel(ctrl2, ioaddr + XGMAC_RXQ_CTRL2);
+		writel(ctrl3, ioaddr + XGMAC_RXQ_CTRL3);
+	} else {
 		queue -= 4;
 
-	value = readl(ioaddr + reg);
-	value &= ~XGMAC_PSRQ(queue);
-	value |= (prio << XGMAC_PSRQ_SHIFT(queue)) & XGMAC_PSRQ(queue);
+		ctrl3 |= (prio << XGMAC_PSRQ_SHIFT(queue)) &
+						XGMAC_PSRQ(queue);
 
-	writel(value, ioaddr + reg);
+		writel(ctrl3, ioaddr + XGMAC_RXQ_CTRL3);
+		writel(ctrl2, ioaddr + XGMAC_RXQ_CTRL2);
+	}
 }
 
 static void dwxgmac2_tx_queue_prio(struct mac_device_info *hw, u32 prio,