From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id F2F1AC5478C for ; Tue, 27 Feb 2024 17:00:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=zrveGul4yOsnaWwfIKfCxl+GxKYXh1Ffc7sQ2p0EPAE=; b=k2JhkjEpjgKMoV dHypCa6YcqIQJzb/CWxiM4TsDOnwJwQvRoUdege07Ys4CW+PxrpZUov959vbXNa6Bs+R7TRf9ncj+ aBU0xtfgMCqOpzZz9nlEyfDYcAq3w62iC9tcYVoldIavw781BVrASEeHUldEncOxzLOWJ6MwBfz62 u0nQR7f8gNotxF0PwlrX5ZffvVAnqRtrymdvrwpbhwHtFP9QFvtNvAW9ND87drahL6vkNVeNO2sWo Vu8KTP87huFmdwiBQOCVGu4U6MS2HT906br1TjG4zCIAqBne5YlLfOa/7wS+a5eRk/xAQWbVfq13S TfvkH/7IsywtZpxJhfcQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rf0om-000000066f3-3Iq8; Tue, 27 Feb 2024 17:00:36 +0000 Received: from sin.source.kernel.org ([145.40.73.55]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rf0oc-000000066bB-3fL3 for linux-arm-kernel@lists.infradead.org; Tue, 27 Feb 2024 17:00:34 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id BB548CE01C1; Tue, 27 Feb 2024 17:00:17 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 99DA7C433F1; Tue, 27 Feb 2024 17:00:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1709053217; bh=Rtg/wJB+BwOfbLpXE7ewLdRK21mvIkgnQ8NFpKpylzg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=mEZVkleoAOhydHqEWHg/rQfg/pNdE3CSGBgWQPF8/NJIv8K3UrpYePZbTNCCm2uNf A07aYmhkG2OxxOI4hy+BI4cpj0H/6nQjkSCQ9JiFfrvfzldGbQ3VKf/eHzx9cPSWhq J/w/GXftvQZYCfEutbIu7/HIeJhwiKZU8f4EaWp7YdeRh0AH+SKWnPvUO30c7lvUzz vZrO1FKHgPBXGn/vr2dpjcBaqVAqx+0x3ytQV1y5OyR3GXUVfBMfTFs/Rmf1xu8ULY 24WLz6N4GqSW3UaH+/AJCULFkhFmaLKgJK2dhTNUfaDEJt/3qnHhwWsD5C/Byhl+Gq yw1U2w14cQlLg== Date: Tue, 27 Feb 2024 17:00:12 +0000 From: Simon Horman To: Piotr Wejman Cc: Alexandre Torgue , Jose Abreu , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Maxime Coquelin , 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> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240226093144.31965-1-piotrwejman90@gmail.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240227_090033_248751_2E35EBA6 X-CRM114-Status: GOOD ( 28.20 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 > --- > 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. ... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel