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 48870EEEC02 for ; Tue, 17 Sep 2024 14:41:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:Cc:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=eegGBaAygOaLYVZ73ZF+hWGW4jgAbW8mG7UbsMPO9lk=; b=qMRhH1MUDl7f2dZYxEa+DkvF/6 PFXO65wD7lypuarH8hthCaedlqtjagouhbx/OiG1AwJDDwMjFoIB9wh4gD6P7c/xe8yOkPzrcrH45 FP2FikuUAZ68iObAESJI64/p61o0akPOXFKXoei55VjZq3Jw5uxGavoIHqlDfSyOSSbuXTCaDYvfg caKCg1jyw1iJGSPR306K8LtEb1NgvLpOeIhyhtjRIIY+MHDIWEfNPGchruV3jrgPlbZEVewF73KV4 StUgVJM4sHyo9xxx3KOjS6LzC4xW23xq8y7KUyRTa7QYdvzfIa7DHzJ/L/ku/+thoFdAmaNsi5b3d ygiJv7Ow==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1sqZON-00000006KQa-2LCP; Tue, 17 Sep 2024 14:41:23 +0000 Received: from out-183.mta0.migadu.com ([2001:41d0:1004:224b::b7]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1sqZNH-00000006KKb-1Mjj for linux-arm-kernel@lists.infradead.org; Tue, 17 Sep 2024 14:40:17 +0000 Message-ID: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1726584006; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=eegGBaAygOaLYVZ73ZF+hWGW4jgAbW8mG7UbsMPO9lk=; b=fMlqe6UovSHDQgQlE/peSO0nS0gcO78vwnJyWtqMM6kHKAp0yAmnh55GhWNN1ys2ZrYVle tpKJgONyriZ0F+oY1VsLgTG9mRCPdDAEqrq3GepCpSqKbCKz+4mf+/6UOkOT2ToQ9wl3KR QerS/mywNCBU2yn787sVFp6rXL+Otv0= Date: Tue, 17 Sep 2024 10:40:00 -0400 MIME-Version: 1.0 Subject: Re: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count overflow To: "Pandey, Radhey Shyam" , "David S . Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , "netdev@vger.kernel.org" , "Gupta, Suraj" , "Katakam, Harini" Cc: Andy Chiu , "linux-kernel@vger.kernel.org" , Simon Horman , Ariane Keller , Daniel Borkmann , "linux-arm-kernel@lists.infradead.org" , "Simek, Michal" References: <20240909230908.1319982-1-sean.anderson@linux.dev> Content-Language: en-US X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Sean Anderson In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Migadu-Flow: FLOW_OUT X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240917_074015_813859_2517F20D X-CRM114-Status: GOOD ( 35.04 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 9/17/24 07:24, Pandey, Radhey Shyam wrote: >> -----Original Message----- >> From: Pandey, Radhey Shyam >> Sent: Thursday, September 12, 2024 8:05 PM >> To: Sean Anderson ; David S . Miller >> ; Eric Dumazet ; Jakub Kicinski >> ; Paolo Abeni ; netdev@vger.kernel.org; >> Gupta, Suraj ; Katakam, Harini >> >> Cc: Andy Chiu ; linux-kernel@vger.kernel.org; Simon >> Horman ; Ariane Keller ; Daniel >> Borkmann ; linux-arm-kernel@lists.infradead.org; Simek, >> Michal >> Subject: RE: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count >> overflow >> >> > -----Original Message----- >> > From: Sean Anderson >> > Sent: Thursday, September 12, 2024 8:01 PM >> > To: Pandey, Radhey Shyam ; David S . Miller >> > ; Eric Dumazet ; Jakub >> Kicinski >> > ; Paolo Abeni ; >> netdev@vger.kernel.org; >> > Gupta, Suraj ; Katakam, Harini >> > >> > Cc: Andy Chiu ; linux-kernel@vger.kernel.org; Simon >> > Horman ; Ariane Keller ; >> Daniel >> > Borkmann ; linux-arm-kernel@lists.infradead.org; Simek, >> > Michal >> > Subject: Re: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count >> > overflow >> > >> > On 9/11/24 03:01, Pandey, Radhey Shyam wrote: >> > >> -----Original Message----- >> > >> From: Sean Anderson >> > >> Sent: Tuesday, September 10, 2024 4:39 AM >> > >> To: Pandey, Radhey Shyam ; David S . >> > >> Miller ; Eric Dumazet ; >> > >> Jakub Kicinski ; Paolo Abeni ; >> > >> netdev@vger.kernel.org >> > >> Cc: Andy Chiu ; linux-kernel@vger.kernel.org; Simon >> > >> Horman ; Ariane Keller ; >> > >> Daniel Borkmann ; linux-arm- >> > >> kernel@lists.infradead.org; Simek, Michal ; Sean >> > >> Anderson >> > >> Subject: [PATCH net v2] net: xilinx: axienet: Fix IRQ coalescing packet count >> > >> overflow >> > >> >> > >> If coalece_count is greater than 255 it will not fit in the register and >> > >> will overflow. This can be reproduced by running >> > >> >> > >> # ethtool -C ethX rx-frames 256 >> > >> >> > >> which will result in a timeout of 0us instead. Fix this by clamping the >> > >> counts to the maximum value. >> > > After this fix - what is o/p we get on rx-frames read? I think silent clamping is not >> a >> > great >> > > idea and user won't know about it. One alternative is to add check in >> set_coalesc >> > > count for valid range? (Similar to axienet_ethtools_set_ringparam so that user is >> > notified >> > > for incorrect range) >> > >> > The value reported will be unclamped. In [1] I improve the driver to >> > return the actual (clamped) value. >> > >> > Remember that without this commit, we have silent wraparound instead. I >> > think clamping is much friendlier, since you at least get something >> > close to the rx-frames value, instead of zero! >> > >> > This commit is just a fix for the overflow issue. To ensure it is >> > appropriate for backporting I have omitted any other >> > changes/improvements. >> >> But the point is the fix also can be to avoid setting coalesce count >> to invalid (or not supported range) value - like done in existing >> axienet_ethtools_set_ringparam() implementation. > > Sean: I think above comment got missed out, so I am bringing it again > to discuss and close on it. I am investigating whether this will work within the broader context of the changes I want to make. I will reply when I have had a chance to work on it. --Sean >> >> And we don't clamp on every dma_start(). >> >> > >> > --Sean >> > >> > [1] https://lore.kernel.org/netdev/20240909235208.1331065-6- >> > sean.anderson@linux.dev/ >> > >> > >> >> > >> Signed-off-by: Sean Anderson >> > >> Fixes: 8a3b7a252dca ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet >> > >> driver") >> > >> --- >> > >> >> > >> Changes in v2: >> > >> - Use FIELD_MAX to extract the max value from the mask >> > >> - Expand the commit message with an example on how to reproduce this >> > >> issue >> > >> >> > >> drivers/net/ethernet/xilinx/xilinx_axienet.h | 5 ++--- >> > >> drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 8 ++++++-- >> > >> 2 files changed, 8 insertions(+), 5 deletions(-) >> > >> >> > >> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h >> > >> b/drivers/net/ethernet/xilinx/xilinx_axienet.h >> > >> index 1223fcc1a8da..54db69893565 100644 >> > >> --- a/drivers/net/ethernet/xilinx/xilinx_axienet.h >> > >> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h >> > >> @@ -109,11 +109,10 @@ >> > >> #define XAXIDMA_BD_CTRL_TXEOF_MASK 0x04000000 /* Last tx packet >> > >> */ >> > >> #define XAXIDMA_BD_CTRL_ALL_MASK 0x0C000000 /* All control bits >> > >> */ >> > >> >> > >> -#define XAXIDMA_DELAY_MASK 0xFF000000 /* Delay timeout >> > >> counter */ >> > >> -#define XAXIDMA_COALESCE_MASK 0x00FF0000 /* Coalesce >> > >> counter */ >> > >> +#define XAXIDMA_DELAY_MASK ((u32)0xFF000000) /* Delay >> > >> timeout counter */ >> > > >> > > Adding typecast here looks odd. Any reason for it? >> > > If needed we do it in specific case where it is required. >> > > >> > >> +#define XAXIDMA_COALESCE_MASK ((u32)0x00FF0000) /* >> > >> Coalesce counter */ >> > >> >> > >> #define XAXIDMA_DELAY_SHIFT 24 >> > >> -#define XAXIDMA_COALESCE_SHIFT 16 >> > >> >> > >> #define XAXIDMA_IRQ_IOC_MASK 0x00001000 /* Completion >> > >> intr */ >> > >> #define XAXIDMA_IRQ_DELAY_MASK 0x00002000 /* Delay >> > >> interrupt */ >> > >> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c >> > >> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c >> > >> index 9eb300fc3590..89b63695293d 100644 >> > >> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c >> > >> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c >> > >> @@ -252,7 +252,9 @@ static u32 axienet_usec_to_timer(struct axienet_local >> > >> *lp, u32 coalesce_usec) >> > >> static void axienet_dma_start(struct axienet_local *lp) >> > >> { >> > >> /* Start updating the Rx channel control register */ >> > >> - lp->rx_dma_cr = (lp->coalesce_count_rx << >> > >> XAXIDMA_COALESCE_SHIFT) | >> > >> + lp->rx_dma_cr = FIELD_PREP(XAXIDMA_COALESCE_MASK, >> > >> + min(lp->coalesce_count_rx, >> > >> + >> > >> FIELD_MAX(XAXIDMA_COALESCE_MASK))) | >> > >> XAXIDMA_IRQ_IOC_MASK | >> > >> XAXIDMA_IRQ_ERROR_MASK; >> > >> /* Only set interrupt delay timer if not generating an interrupt on >> > >> * the first RX packet. Otherwise leave at 0 to disable delay interrupt. >> > >> @@ -264,7 +266,9 @@ static void axienet_dma_start(struct axienet_local >> > >> *lp) >> > >> axienet_dma_out32(lp, XAXIDMA_RX_CR_OFFSET, lp->rx_dma_cr); >> > >> >> > >> /* Start updating the Tx channel control register */ >> > >> - lp->tx_dma_cr = (lp->coalesce_count_tx << >> > >> XAXIDMA_COALESCE_SHIFT) | >> > >> + lp->tx_dma_cr = FIELD_PREP(XAXIDMA_COALESCE_MASK, >> > >> + min(lp->coalesce_count_tx, >> > >> + >> > >> FIELD_MAX(XAXIDMA_COALESCE_MASK))) | >> > >> XAXIDMA_IRQ_IOC_MASK | >> > >> XAXIDMA_IRQ_ERROR_MASK; >> > >> /* Only set interrupt delay timer if not generating an interrupt on >> > >> * the first TX packet. Otherwise leave at 0 to disable delay interrupt. >> > >> -- >> > >> 2.35.1.1320.gc452695387.dirty >> > >