Messages in this thread |  | | From | Frank <> | Subject | Re: [PATCH net-next v2] net: phy: Add driver for Motorcomm yt8531 gigabit ethernet phy | Date | Wed, 14 Dec 2022 15:37:43 +0800 |
| |
> -------------------------------------------------------------------------------- > 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.
|  |