From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server-vie001.gnuweeb.org X-Spam-Level: X-Spam-Status: No, score=-1.2 required=5.0 tests=ALL_TRUSTED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,URIBL_ZEN_BLOCKED_OPENDNS autolearn=ham autolearn_force=no version=3.4.6 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gnuweeb.org; s=new2025; t=1754438179; bh=Fp4dneLqTfyYyNFcsLTlIeIqD6d5bnThYhVIbjpAZ6M=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:In-Reply-To:Message-ID:Date:From:Reply-To:Subject:To: Cc:In-Reply-To:References:Resent-Date:Resent-From:Resent-To: Resent-Cc:User-Agent:Content-Type:Content-Transfer-Encoding; b=iSXA1EcLlpi139YZSA1TqLJ/mAPinPiVHWwrOcGJ7FgLsfZSIUIa61dgmfgItkkwN KteFPcdOx0HUAaUCCbRWFIi8deXz5CKVKurtsjcP/SRol9tzTj/QlW018R55Cv8GTt 8Hai+R0mT+A/ziU4FHJjbWGTOM2HKpq3Sa7ZJtQYLAt/+L/wZqrMg3f7qynCB9Awsz 3vZ9TFDcDM+xZB05QD8Hnv9/Zp20B2VcEq+hhTtPl+Kfs3J0cyPy4XyaC1dJuyDrKJ fYL3lNXqtWcP95HaRSGNyN0jtpq4w5PDcWCItLxlW1gkAn0E3ULveXM4sXivy7rx/5 qGSPKvDNSXIhw== Received: from linux.gnuweeb.org (unknown [182.253.126.229]) by server-vie001.gnuweeb.org (Postfix) with ESMTPSA id 710BF3127C24; Tue, 5 Aug 2025 23:56:16 +0000 (UTC) Date: Wed, 6 Aug 2025 06:56:13 +0700 From: Ammar Faizi To: Linus Torvalds Cc: Simon Horman , Oliver Neukum , Andrew Lunn , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Greg Kroah-Hartman , Linux Netdev Mailing List , Linux USB Mailing List , Linux Kernel Mailing List , Armando Budianto , gwml@vger.gnuweeb.org, stable@vger.kernel.org, John Ernberg Subject: Re: [PATCH net v2] net: usbnet: Fix the wrong netif_carrier_on() call placement Message-ID: References: <20250801190310.58443-1-ammarfaizi2@gnuweeb.org> <20250804100050.GQ8494@horms.kernel.org> <20250805202848.GC61519@horms.kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Machine-Hash: hUx9VaHkTWcLO7S8CQCslj6OzqBx2hfLChRz45nPESx5VSB/xuJQVOKOB1zSXE3yc9ntP27bV1M1 List-Id: On Wed, Aug 06, 2025 at 01:40:37AM +0300, Linus Torvalds wrote: > In particular, the whole *rest* of the code in that > > if (!netif_carrier_ok(dev->net)) { > > no longer makes sense after we've turned the link on with that > > if (test_and_clear_bit(EVENT_LINK_CARRIER_ON, &dev->flags)) > netif_carrier_on(dev->net); > > sequence. > > Put another way - once we've turned the carrier on, now that whole > > /* kill URBs for reading packets to save bus bandwidth */ > unlink_urbs(dev, &dev->rxq); > > /* > * tx_timeout will unlink URBs for sending packets and > * tx queue is stopped by netcore after link becomes off > */ > > thing makes no sense. After taking a look further, I agree with you. I git-blamed the unlink_urbs()'s line and it's indeed expected to be called after link becomes off. So yes, it makes no sense to call that when we're turning the link on. commit 4b49f58fff00e6e9b24eaa31d4c6324393d76b0a Author: Ming Lei Date: Thu Apr 11 04:40:40 2013 +0000 usbnet: handle link change The link change is detected via the interrupt pipe, and bulk pipes are responsible for transfering packets, so it is reasonable to stop bulk transfer after link is reported as off. Even though my patch works on my machine. Something may go wrong. > So my gut feel is that the > > if (test_and_clear_bit(EVENT_LINK_CARRIER_ON, &dev->flags)) > netif_carrier_on(dev->net); > > should actually be done outside that if-statement entirely, because it > literally ends up changing the thing that if-statement is testing. Apart from moving it outside that if-statement, unlink_urbs() call should probably also be guarded as we agreed it makes no sense to call it when we're turning the link on. -- Ammar Faizi