lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-Id: <20221214073743.4314-1-Frank.Sae@motor-comm.com> Date: Wed, 14 Dec 2022 15:37:43 +0800 From: Frank <Frank.Sae@...or-comm.com> To: andrew@...n.ch Cc: Frank.Sae@...or-comm.com, davem@...emloft.net, edumazet@...gle.com, fei.zhang@...or-comm.com, hkallweit1@...il.com, hua.sun@...or-comm.com, kuba@...nel.org, linux-kernel@...r.kernel.org, linux@...linux.org.uk, netdev@...r.kernel.org, pabeni@...hat.com, pgwipeout@...il.com Subject: Re: [PATCH net-next v2] net: phy: Add driver for Motorcomm yt8531 gigabit ethernet phy > -------------------------------------------------------------------------------- > on Dec. 2, 2022, 1:27 p.m. UTC , Andrew Lunn wrote: > > > /** > > * ytphy_read_ext() - read a PHY's extended register > > * @phydev: a pointer to a &struct phy_device > > @@ -258,6 +271,8 @@ static int ytphy_read_ext(struct phy_device *phydev, u16 regnum) > > { > > int ret; > > > > + ASSERT_MDIO(phydev); > > + > > ret = __phy_write(phydev, YTPHY_PAGE_SELECT, regnum); > > if (ret < 0) > > return ret; > > @@ -297,6 +312,8 @@ static int ytphy_write_ext(struct phy_device *phydev, u16 regnum, u16 val) > > { > > int ret; > > > > + ASSERT_MDIO(phydev); > > + > > ret = __phy_write(phydev, YTPHY_PAGE_SELECT, regnum); > > if (ret < 0) > > return ret; > > @@ -342,6 +359,8 @@ static int ytphy_modify_ext(struct phy_device *phydev, u16 regnum, u16 mask, > > { > > int ret; > > > > + ASSERT_MDIO(phydev); > > + > > ret = __phy_write(phydev, YTPHY_PAGE_SELECT, regnum); > > if (ret < 0) > > return ret; > > @@ -479,6 +498,76 @@ static int ytphy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol) > > return phy_restore_page(phydev, old_page, ret); > > } > > Please make the above one patch, which adds the macro and its > users. There are a couple more below as well. > > Did it find any problems in the current code? Any fixes mixed > in here? > > Then add yt8531 is another patch. > Thanks! It not find any problems in the current code. I will move that in another patch. > > +/** > > + * 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 > > + * > > + * returns 0 or negative errno code > > + */ > > +static int yt8531_set_wol(struct phy_device *phydev, > > + struct ethtool_wolinfo *wol) > > +{ > > + 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; > > Why the cast? I'm sorry. What does "Why the cast?" mean? > > > + if (!is_valid_ether_addr(mac_addr)) > > + return -EINVAL; > > Andrew > > -------------------------------------------------------------------------------- > on Dec. 3, 2022, 8:47 p.m. UTC , Andrew Lunn wrote: > > On Fri, Dec 02, 2022 at 01:34:16PM +0000, Russell King (Oracle) wrote: > > On Fri, Dec 02, 2022 at 02:27:43PM +0100, Andrew Lunn wrote: > > > > +static bool mdio_is_locked(struct phy_device *phydev) > > > > +{ > > > > + return mutex_is_locked(&phydev->mdio.bus->mdio_lock); > > > > +} > > > > + > > > > +#define ASSERT_MDIO(phydev) \ > > > > + WARN_ONCE(!mdio_is_locked(phydev), \ > > > > + "MDIO: assertion failed at %s (%d)\n", __FILE__, __LINE__) > > > > + > > > > > > Hi Frank > > > > > > You are not the only one who gets locking wrong. This could be used in > > > other drivers. Please add it to include/linux/phy.h, > > > > That placement doesn't make much sense. > > > > As I already said, we have lockdep checks in drivers/net/phy/mdio_bus.c, > > and if we want to increase their effectiveness, then that's the place > > that it should be done. > > I was following the ASSERT_RTNL model, but that is used in quite deep > and complex call stacks, and it is useful to scatter the macro in lots > of places. PHY drivers are however very shallow, so yes, putting them > in mdio_bus.c makes a lot of sense. > > > I don't see any point in using __FILE__ and __LINE__ in the above > > macro either. Firstly, WARN_ONCE() already includes the file and line, > > and secondly, the backtrace is more useful than the file and line where > > the assertion occurs especially if it's placed in mdio_bus.c > > And PHY driver functions are simpler, there is a lot less inlining > going on, so the function name is probably all you need to know to > find where you messed up the locking. So i agree, they can be removed. > > Andrew Hi Andrew and Russell, Thanks! I will change that in next patch.
Powered by blists - more mailing lists