| Message ID | 20240303190339.52496-1-piotrwejman90@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] net: stmmac: fix rx queue priority assignment | expand |
On Sun, 3 Mar 2024 20:03:38 +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. Do you know what user-visible mis-behavior this may result in? > 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 But also you seem to remove clearing all other prios from the queue: - value &= ~GMAC_RXQCTRL_PSRQX_MASK(queue); and - value &= ~XGMAC_PSRQ(queue); is that intentional? Commit message should explain why. > The write sequence of CTRL2 and CTRL3 registers is done in the way to > ensure this order. Ensure which order? Looks like you're actually writing in the opposite order than what I'd expect :S First the register you want to assign to, and then the register you only clear from. When you repost please include a Fixes tag.
On Mon, Mar 11, 2024 at 01:41:44PM -0700, Jakub Kicinski wrote: > On Sun, 3 Mar 2024 20:03:38 +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. > > Do you know what user-visible mis-behavior this may result in? When changing priority to rx queue mapping with tc qdisc taprio command (man tc-taprio), all packets with priority assigned to multiple queues are dropped. > > > 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 > > But also you seem to remove clearing all other prios from the queue: > > - value &= ~GMAC_RXQCTRL_PSRQX_MASK(queue); > > and > > - value &= ~XGMAC_PSRQ(queue); > > is that intentional? Commit message should explain why. Yes, that keeps other priorities assigned to that queue and only clears the same priorities from all other queues. > > > The write sequence of CTRL2 and CTRL3 registers is done in the way to > > ensure this order. > > Ensure which order? Looks like you're actually writing in the opposite > order than what I'd expect :S First the register you want to assign to, > and then the register you only clear from. > I meant the order you wrote: first assign new priorities to a queue, then clear them from others queues. > When you repost please include a Fixes tag. > -- > pw-bot: cr
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c index 6b6d0de09619..a0e6d33ca87e 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); - value = readl(ioaddr + base_register); + /* 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)); + + 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 << GMAC_RXQCTRL_PSRQX_SHIFT(queue)) & + GMAC_RXQCTRL_PSRQX_MASK(queue); - value &= ~GMAC_RXQCTRL_PSRQX_MASK(queue); - value |= (prio << GMAC_RXQCTRL_PSRQX_SHIFT(queue)) & + 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(value, ioaddr + base_register); + + 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..d15752823d93 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,
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 Changes in v3: - Fix trailing whitespace - Link to v2: https://lore.kernel.org/netdev/20240226093144.31965-1-piotrwejman90@gmail.com/ .../net/ethernet/stmicro/stmmac/dwmac4_core.c | 42 +++++++++++++++---- .../ethernet/stmicro/stmmac/dwxgmac2_core.c | 40 ++++++++++++++---- 2 files changed, 66 insertions(+), 16 deletions(-)