| 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
| ||
|
Date: Wed, 21 Dec 2022 15:53:14 +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,
xiaogang.fan@...or-comm.com
Subject: Re: [PATCH net-next v2] net: phy: Add driver for Motorcomm yt8531 gigabit ethernet phy
Hi Andrew,
I send you this mail on 2022-12-14 11:07 UTC, but it failed:
"Connect to the recipient Email system failed. (9)connect to the "vps0.lunn.ch(x.x.x.x) of lunn.ch" server failed"
so I send it again.
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
>
Powered by blists - more mailing lists