| Message ID | 20240303190339.52496-1-piotrwejman90@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| 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,