diff mbox series

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

Message ID 20240226093144.31965-1-piotrwejman90@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [v2] net: stmmac: fix rx queue priority assignment | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 942 this patch: 942
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 9 of 9 maintainers
netdev/build_clang success Errors and warnings before: 957 this patch: 957
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 959 this patch: 959
netdev/checkpatch fail ERROR: trailing whitespace
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-02-27--03-00 (tests: 1456)

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,