From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4D5E83911AA; Sat, 13 Jun 2026 22:06:01 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781388362; cv=none; b=Qchh27on+T5LiP2msQMenmrZPyot7/c9A6pk29VaK9AP85N980186gYJMv6rytKV22I0J1ragcIvq2ZCKYsOqEejYfB2SkyBm8vYjRgaawL8aDCFSMt3CE+5TAZsXJVBm6EJFRw9Yoh/s8pnasttf0HsMz9LF3vKvIapvimYI0Y= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781388362; c=relaxed/simple; bh=3nQBGzuQdCssSRx+UZk8Wlt7YmuQmPlcWTseaOCVuKk=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=R0JSLPW4y1fRxdNCrzHKKKklls/gRKVKsNY0B+y62dErPL13S53XyhB1YM7IpCbEFm9RRANF6BKcZLj/HwPc0i6FPbiLlaPzVuPmKDflR0dZHYoudaVEQ3juYHdiASdV+Ns/KYGu8jnApj91503Qt/3Gzhu11riSuVu87nFCCZs= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=oUVMOl1D; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="oUVMOl1D" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 56D3C1F00A3A; Sat, 13 Jun 2026 22:06:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1781388360; bh=eh3E9tbLJK6r2YQL7zhyUJJeEaI2GkjBaTkRJyhcxZg=; h=From:To:Cc:Subject:Date:In-Reply-To:References; b=oUVMOl1D+40ylftuLx3UQYMO33RGElh+6FuqNgxzfRKe1Ee17PBLsZGKa7rKHHyA9 IxjW5/sXOJTPSh1CgESY+JkXyyPMy1b+U+TP6Ygp39Hx3brRyYaEjjGU3oNxasJxDz v/ZALUQnJy7kkaEHO/0cVpz6XSuNRrZJ32P3MUsiwCstfw91Z01coL+TBKkmQaanAW 7UCVCPdlgcj0R7avjhA/Wm8yg2RhuRAnFGmDx/hlx8TCl4yh3qTB2BNSm3df8gxUNu NxCnyqjLqo0njWC6qtq2H44X7wReIoNBHx7cwyl/HQJteWRkoJvOHyTbMs/hQKPovM VG2A7Uwsnswag== From: Jakub Kicinski To: javen_xu@realsil.com.cn Cc: Jakub Kicinski , hkallweit1@gmail.com, nic_swsd@realtek.com, andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com, pabeni@redhat.com, maxime.chevallier@bootlin.com, horms@kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH net-next v2 3/6] r8169: add support for phylink Date: Sat, 13 Jun 2026 15:05:59 -0700 Message-ID: <20260613220559.219642-1-kuba@kernel.org> X-Mailer: git-send-email 2.54.0 In-Reply-To: <20260611094345.880-4-javen_xu@realsil.com.cn> References: <20260611094345.880-4-javen_xu@realsil.com.cn> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. --- r8169: add support for phylink Transfer old framework to phylink. Phylink can support fiber mode card which can not get link status or link speed from standard phy registers. > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c > index 560f987437b63..615bd4107359a 100644 > --- a/drivers/net/ethernet/realtek/r8169_main.c > +++ b/drivers/net/ethernet/realtek/r8169_main.c [ ... ] > @@ -2661,13 +2672,10 @@ static void rtl_jumbo_config(struct rtl8169_private *tp) > pcie_set_readrq(tp->pci_dev, readrq); > > /* Chip doesn't support pause in jumbo mode */ > - if (jumbo) { > - linkmode_clear_bit(ETHTOOL_LINK_MODE_Pause_BIT, > - tp->phydev->advertising); > - linkmode_clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, > - tp->phydev->advertising); > - phy_start_aneg(tp->phydev); > - } > + if (jumbo) > + tp->phylink_config.mac_capabilities &= ~(MAC_SYM_PAUSE | MAC_ASYM_PAUSE); > + else > + tp->phylink_config.mac_capabilities |= (MAC_SYM_PAUSE | MAC_ASYM_PAUSE); > } [Severity: High] Does phylink re-read mac_capabilities on start? It looks like dynamic bitwise modifications to tp->phylink_config.mac_capabilities made here are ignored by phylink. Since phylink does not update the pl->supported link mode mask during phylink_start(), won't the PHY continue to incorrectly advertise pause capabilities to the link partner when jumbo frames are enabled? [ ... ] > @@ -4142,11 +4150,17 @@ static int rtl8169_change_mtu(struct net_device *dev, int new_mtu) > { > struct rtl8169_private *tp = netdev_priv(dev); > > + if (netif_running(dev)) > + phylink_stop(tp->phylink); > + > WRITE_ONCE(dev->mtu, new_mtu); > netdev_update_features(dev); > rtl_jumbo_config(tp); > rtl_set_eee_txidle_timer(tp); > > + if (netif_running(dev)) > + phylink_start(tp->phylink); > + > return 0; > } [ ... ] > +static int rtl_init_phylink(struct rtl8169_private *tp) > +{ > + struct phylink *pl; > + phy_interface_t phy_mode; > + > + tp->phylink_config.dev = &tp->dev->dev; > + tp->phylink_config.type = PHYLINK_NETDEV; > + tp->phylink_config.mac_managed_pm = true; > + tp->phylink_config.lpi_capabilities = rtl8169_get_lpi_caps(tp); > + tp->phylink_config.mac_capabilities |= MAC_ASYM_PAUSE | MAC_SYM_PAUSE; > + > + if (tp->sfp_mode) { > + phy_mode = PHY_INTERFACE_MODE_INTERNAL; > + tp->phylink_config.mac_capabilities |= MAC_10000FD; > + } else { > + tp->phylink_config.mac_capabilities |= MAC_10 | MAC_100; > + phy_mode = PHY_INTERFACE_MODE_INTERNAL; [Severity: High] Does hardcoding phy_mode to PHY_INTERFACE_MODE_INTERNAL here break older adapters that use discrete PHYs over GMII or MII? Prior to this patch, the driver dynamically checked tp->supports_gmii to set either PHY_INTERFACE_MODE_GMII or PHY_INTERFACE_MODE_MII. Forcing INTERNAL might tell the PHY driver that the PHY is integrated into the MAC, which could cause discrete PHY drivers to skip necessary initializations, fail validation checks, or misconfigure interface delays. > + > + if (tp->mac_version == RTL_GIGA_MAC_VER_80) > + tp->phylink_config.mac_capabilities |= MAC_1000FD | MAC_2500FD | > + MAC_5000FD | MAC_10000FD;