Messages in this thread | | | Date | Wed, 30 Nov 2022 09:55:50 +0000 | From | "Russell King (Oracle)" <> | Subject | Re: [PATCH net-next] net: phy: Add driver for Motorcomm yt8531 gigabit ethernet phy |
| |
On Wed, Nov 30, 2022 at 05:49:28PM +0800, Frank wrote: > +/** > + * yt8531_set_wol() - turn wake-on-lan on or off > + * @phydev: a pointer to a &struct phy_device > + * @wol: a pointer to a &struct ethtool_wolinfo > + * > + * NOTE: YTPHY_WOL_CONFIG_REG, YTPHY_WOL_MACADDR2_REG, YTPHY_WOL_MACADDR1_REG > + * and YTPHY_WOL_MACADDR0_REG are common ext reg. > + * > + * returns 0 or negative errno code > + */ > +static int yt8531_set_wol(struct phy_device *phydev, > + struct ethtool_wolinfo *wol) > +{
So this is called from the .set_wol method directly, and won't have the MDIO bus lock taken...
> + struct net_device *p_attached_dev; > + const u16 mac_addr_reg[] = { > + YTPHY_WOL_MACADDR2_REG, > + YTPHY_WOL_MACADDR1_REG, > + YTPHY_WOL_MACADDR0_REG, > + }; > + const u8 *mac_addr; > + u16 mask; > + u16 val; > + int ret; > + u8 i; > + > + if (wol->wolopts & WAKE_MAGIC) { > + p_attached_dev = phydev->attached_dev; > + if (!p_attached_dev) > + return -ENODEV; > + > + mac_addr = (const u8 *)p_attached_dev->dev_addr; > + if (!is_valid_ether_addr(mac_addr)) > + return -EINVAL; > + > + /* Store the device address for the magic packet */ > + for (i = 0; i < 3; i++) { > + ret = ytphy_write_ext(phydev, mac_addr_reg[i], > + ((mac_addr[i * 2] << 8)) | > + (mac_addr[i * 2 + 1]));
This accesses the MDIO bus without taking the lock.
> + if (ret < 0) > + return ret; > + } > + > + /* Enable WOL feature */ > + mask = YTPHY_WCR_PULSE_WIDTH_MASK | YTPHY_WCR_INTR_SEL; > + val = YTPHY_WCR_ENABLE | YTPHY_WCR_INTR_SEL; > + val |= YTPHY_WCR_TYPE_PULSE | YTPHY_WCR_PULSE_WIDTH_672MS; > + ret = ytphy_modify_ext(phydev, YTPHY_WOL_CONFIG_REG, mask, val);
This accesses the MDIO bus without taking the lock.
> + if (ret < 0) > + return ret; > + > + /* Enable WOL interrupt */ > + ret = __phy_modify(phydev, YTPHY_INTERRUPT_ENABLE_REG, 0, > + YTPHY_IER_WOL);
This accesses the MDIO bus without taking the lock.
> + if (ret < 0) > + return ret; > + } else { > + /* Disable WOL feature */ > + mask = YTPHY_WCR_ENABLE | YTPHY_WCR_INTR_SEL; > + ret = ytphy_modify_ext(phydev, YTPHY_WOL_CONFIG_REG, mask, 0);
This accesses the MDIO bus without taking the lock.
> + > + /* Disable WOL interrupt */ > + ret = __phy_modify(phydev, YTPHY_INTERRUPT_ENABLE_REG, > + YTPHY_IER_WOL, 0);
This accesses the MDIO bus without taking the lock.
> + if (ret < 0) > + return ret; > + } > + > + return 0; > +}
Which makes this function entirely unsafe as another thread can change the YTPHY_PAGE_SELECT register between writing that register and accessing the YTPHY_PAGE_DATA register.
-- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
| |