Re: [PATCH] phy: fix possible double lock calling link changed handler
From: Heiner Kallweit <hkallweit1@gmail.com>
Date: 2021-11-23 20:33:07
Subsystem:
microchip lan743x ethernet driver, networking drivers, the rest · Maintainers:
Bryan Whitehead, Andrew Lunn, "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Linus Torvalds
On 23.11.2021 17:06, Alessandro B Maurici wrote:
On Tue, 23 Nov 2021 15:11:22 +0100 Andrew Lunn [off-list ref] wrote:quoted
Yes, that is the change i would make. When adding the extra locks i missed that a driver was doing something like this. I will check all other callers to see if they are using it in odd contexts. AndrewAndrew, this kinda of implementation is really hard to get in a fast review, fortunately I happen to be testing one lan743x board with a 5.10.79 kernel that had the new locks in place, and noticed that really fast, but I wrongly assumed that call was okayish since the driver was on stable. If you need to do some testing I will still have the hardware with me for some time. Alessandro
Great that you have test hw, could you please test the following patch? The duplex argument of lan743x_phy_update_flowcontrol() seems to be some leftover, it isn't used and can be removed.
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index 4fc97823b..7d7647481 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c@@ -914,8 +914,7 @@ static int lan743x_phy_reset(struct lan743x_adapter *adapter) } static void lan743x_phy_update_flowcontrol(struct lan743x_adapter *adapter, - u8 duplex, u16 local_adv, - u16 remote_adv) + u16 local_adv, u16 remote_adv) { struct lan743x_phy *phy = &adapter->phy; u8 cap;
@@ -943,7 +942,6 @@ static void lan743x_phy_link_status_change(struct net_device *netdev) phy_print_status(phydev); if (phydev->state == PHY_RUNNING) { - struct ethtool_link_ksettings ksettings; int remote_advertisement = 0; int local_advertisement = 0;
@@ -980,18 +978,14 @@ static void lan743x_phy_link_status_change(struct net_device *netdev) } lan743x_csr_write(adapter, MAC_CR, data); - memset(&ksettings, 0, sizeof(ksettings)); - phy_ethtool_get_link_ksettings(netdev, &ksettings); local_advertisement = linkmode_adv_to_mii_adv_t(phydev->advertising); remote_advertisement = linkmode_adv_to_mii_adv_t(phydev->lp_advertising); - lan743x_phy_update_flowcontrol(adapter, - ksettings.base.duplex, - local_advertisement, + lan743x_phy_update_flowcontrol(adapter, local_advertisement, remote_advertisement); - lan743x_ptp_update_latency(adapter, ksettings.base.speed); + lan743x_ptp_update_latency(adapter, phydev->speed); } }
--
2.34.0