lkml.org 
[lkml]   [2022]   [Nov]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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!

\
 
 \ /
  Last update: 2022-11-30 10:56    [W:0.070 / U:0.372 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site