[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y4copjAzKpGSeunB@shell.armlinux.org.uk>
Date: Wed, 30 Nov 2022 09:55:50 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Frank <Frank.Sae@...or-comm.com>
Cc: Peter Geis <pgwipeout@...il.com>, Andrew Lunn <andrew@...n.ch>,
Heiner Kallweit <hkallweit1@...il.com>,
"David S . Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, yinghong.zhang@...or-comm.com,
fei.zhang@...or-comm.com, hua.sun@...or-comm.com,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
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!
Powered by blists - more mailing lists