lkml.org 
[lkml]   [2022]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH net-next v2] net: phy: Add driver for Motorcomm yt8531 gigabit ethernet phy
Date

> --------------------------------------------------------------------------------
> 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.

\
 
 \ /
  Last update: 2022-12-14 08:38    [W:0.323 / U:0.116 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site