Re: [PATCH net-next v2] net: phy: Add driver for Motorcomm yt8531 gigabit ethernet phy

* Re: [PATCH net-next v2] net: phy: Add driver for Motorcomm yt8531 gigabit ethernet phy
@ 2022-12-14 11:07 Frank
  0 siblings, 0 replies; 7+ messages in thread
From: Frank @ 2022-12-14 11:07 UTC (permalink / raw)
  To: andrew
  Cc: Frank.Sae, davem, edumazet, fei.zhang, hkallweit1, hua.sun, kuba,
	linux-kernel, linux, netdev, pabeni, pgwipeout, yinghong.zhang

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!

 change the mdio_bus.c like follow ok?

 -add "ASSERT_MDIO"
 -add "ASSERT_MDIO(bus);" in __mdiobus_read and __mdiobus_write function 

 static inline bool mdiobus_is_locked(struct mii_bus *bus)
{
	return mutex_is_locked(&bus->mdio_lock);
}

#define ASSERT_MDIO(bus) \
	WARN_ONCE(!mdiobus_is_locked(bus), \
		  "MDIO: assertion failed\n")

 int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum)
{
	int retval;

	ASSERT_MDIO(bus);
	lockdep_assert_held_once(&bus->mdio_lock);

	retval = bus->read(bus, addr, regnum);
...
}

 int __mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val)
{
	int err;

	ASSERT_MDIO(bus);
	lockdep_assert_held_once(&bus->mdio_lock);

	err = bus->write(bus, addr, regnum, val);
...
}

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. 
 ASSERT_MDIO in motorcomm.c will be removed. 

> > +/**
> > + * 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
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread