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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAKxU2N-f=B2Kc4iLi_f4e1kyGgKEi5K2B8GfnzLi1=qLovJu+Q@mail.gmail.com>
Date: Sat, 7 Sep 2024 12:58:08 -0700
From: Rosen Penev <rosenp@...il.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc: netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com, 
	kuba@...nel.org, pabeni@...hat.com, linux-kernel@...r.kernel.org, 
	jacob.e.keller@...el.com, horms@...nel.org, sd@...asysnail.net, 
	chunkeey@...il.com
Subject: Re: [PATCHv3 net-next 6/9] net: ibm: emac: use netdev's phydev directly

On Sat, Sep 7, 2024 at 12:32 PM Christophe JAILLET
<christophe.jaillet@...adoo.fr> wrote:
>
> Le 05/09/2024 à 22:15, Rosen Penev a écrit :
> > Avoids having to use own struct member.
> >
> > Signed-off-by: Rosen Penev <rosenp@...il.com>
> > ---
> >   drivers/net/ethernet/ibm/emac/core.c | 47 +++++++++++++---------------
> >   drivers/net/ethernet/ibm/emac/core.h |  3 --
> >   2 files changed, 21 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
> > index e2abda947a51..cda368701ae4 100644
> > --- a/drivers/net/ethernet/ibm/emac/core.c
> > +++ b/drivers/net/ethernet/ibm/emac/core.c
> > @@ -2459,7 +2459,7 @@ static int emac_read_uint_prop(struct device_node *np, const char *name,
> >   static void emac_adjust_link(struct net_device *ndev)
> >   {
> >       struct emac_instance *dev = netdev_priv(ndev);
> > -     struct phy_device *phy = dev->phy_dev;
> > +     struct phy_device *phy = ndev->phydev;
> >
> >       dev->phy.autoneg = phy->autoneg;
> >       dev->phy.speed = phy->speed;
> > @@ -2510,22 +2510,20 @@ static int emac_mdio_phy_start_aneg(struct mii_phy *phy,
> >   static int emac_mdio_setup_aneg(struct mii_phy *phy, u32 advertise)
> >   {
> >       struct net_device *ndev = phy->dev;
> > -     struct emac_instance *dev = netdev_priv(ndev);
> >
> >       phy->autoneg = AUTONEG_ENABLE;
> >       phy->advertising = advertise;
> > -     return emac_mdio_phy_start_aneg(phy, dev->phy_dev);
> > +     return emac_mdio_phy_start_aneg(phy, ndev->phydev);
> >   }
> >
> >   static int emac_mdio_setup_forced(struct mii_phy *phy, int speed, int fd)
> >   {
> >       struct net_device *ndev = phy->dev;
> > -     struct emac_instance *dev = netdev_priv(ndev);
> >
> >       phy->autoneg = AUTONEG_DISABLE;
> >       phy->speed = speed;
> >       phy->duplex = fd;
> > -     return emac_mdio_phy_start_aneg(phy, dev->phy_dev);
> > +     return emac_mdio_phy_start_aneg(phy, ndev->phydev);
> >   }
> >
> >   static int emac_mdio_poll_link(struct mii_phy *phy)
> > @@ -2534,20 +2532,19 @@ static int emac_mdio_poll_link(struct mii_phy *phy)
> >       struct emac_instance *dev = netdev_priv(ndev);
> >       int res;
> >
> > -     res = phy_read_status(dev->phy_dev);
> > +     res = phy_read_status(ndev->phydev);
> >       if (res) {
> >               dev_err(&dev->ofdev->dev, "link update failed (%d).", res);
> >               return ethtool_op_get_link(ndev);
> >       }
> >
> > -     return dev->phy_dev->link;
> > +     return ndev->phydev->link;
> >   }
> >
> >   static int emac_mdio_read_link(struct mii_phy *phy)
> >   {
> >       struct net_device *ndev = phy->dev;
> > -     struct emac_instance *dev = netdev_priv(ndev);
> > -     struct phy_device *phy_dev = dev->phy_dev;
> > +     struct phy_device *phy_dev = ndev->phydev;
> >       int res;
> >
> >       res = phy_read_status(phy_dev);
> > @@ -2564,10 +2561,9 @@ static int emac_mdio_read_link(struct mii_phy *phy)
> >   static int emac_mdio_init_phy(struct mii_phy *phy)
> >   {
> >       struct net_device *ndev = phy->dev;
> > -     struct emac_instance *dev = netdev_priv(ndev);
> >
> > -     phy_start(dev->phy_dev);
> > -     return phy_init_hw(dev->phy_dev);
> > +     phy_start(ndev->phydev);
> > +     return phy_init_hw(ndev->phydev);
> >   }
> >
> >   static const struct mii_phy_ops emac_dt_mdio_phy_ops = {
> > @@ -2622,26 +2618,28 @@ static int emac_dt_mdio_probe(struct emac_instance *dev)
> >   static int emac_dt_phy_connect(struct emac_instance *dev,
> >                              struct device_node *phy_handle)
> >   {
> > +     struct phy_device *phy_dev;
> > +
> >       dev->phy.def = devm_kzalloc(&dev->ofdev->dev, sizeof(*dev->phy.def),
> >                                   GFP_KERNEL);
> >       if (!dev->phy.def)
> >               return -ENOMEM;
> >
> > -     dev->phy_dev = of_phy_connect(dev->ndev, phy_handle, &emac_adjust_link,
> > +     phy_dev = of_phy_connect(dev->ndev, phy_handle, &emac_adjust_link,
> >                                     0, dev->phy_mode);
> > -     if (!dev->phy_dev) {
> > +     if (!phy_dev) {
> >               dev_err(&dev->ofdev->dev, "failed to connect to PHY.\n");
> >               return -ENODEV;
> >       }
> >
> > -     dev->phy.def->phy_id = dev->phy_dev->drv->phy_id;
> > -     dev->phy.def->phy_id_mask = dev->phy_dev->drv->phy_id_mask;
> > -     dev->phy.def->name = dev->phy_dev->drv->name;
> > +     dev->phy.def->phy_id = phy_dev->drv->phy_id;
> > +     dev->phy.def->phy_id_mask = phy_dev->drv->phy_id_mask;
> > +     dev->phy.def->name = phy_dev->drv->name;
> >       dev->phy.def->ops = &emac_dt_mdio_phy_ops;
> >       ethtool_convert_link_mode_to_legacy_u32(&dev->phy.features,
> > -                                             dev->phy_dev->supported);
> > -     dev->phy.address = dev->phy_dev->mdio.addr;
> > -     dev->phy.mode = dev->phy_dev->interface;
> > +                                             phy_dev->supported);
> > +     dev->phy.address = phy_dev->mdio.addr;
> > +     dev->phy.mode = phy_dev->interface;
> >       return 0;
> >   }
> >
> > @@ -2695,11 +2693,11 @@ static int emac_init_phy(struct emac_instance *dev)
> >                               return res;
> >
> >                       res = of_phy_register_fixed_link(np);
> > -                     dev->phy_dev = of_phy_find_device(np);
> > -                     if (res || !dev->phy_dev)
> > +                     ndev->phydev = of_phy_find_device(np);
> > +                     if (res || !ndev->phydev)
> >                               return res ? res : -EINVAL;
> >                       emac_adjust_link(dev->ndev);
> > -                     put_device(&dev->phy_dev->mdio.dev);
> > +                     put_device(&ndev->phydev->mdio.dev);
> >               }
> >               return 0;
> >       }
> > @@ -3254,9 +3252,6 @@ static void emac_remove(struct platform_device *ofdev)
> >       if (emac_has_feature(dev, EMAC_FTR_HAS_ZMII))
> >               zmii_detach(dev->zmii_dev, dev->zmii_port);
> >
> > -     if (dev->phy_dev)
> > -             phy_disconnect(dev->phy_dev);
> > -
>
> Hi,
>
> I guess that this call was to balance the of_phy_connect() from
> emac_dt_phy_connect().
>
> Is it ok to just remove this phy_disconnect()?
I would think free or unregister_netdev handles it given that phydev
is a member of netdev.

On a separate note, A lot of drivers seem to do this in open/close as
opposed to probe/remove.
>
> CJ
>
> >       busy_phy_map &= ~(1 << dev->phy.address);
> >       DBG(dev, "busy_phy_map now %#x" NL, busy_phy_map);
> >
> > diff --git a/drivers/net/ethernet/ibm/emac/core.h b/drivers/net/ethernet/ibm/emac/core.h
> > index f4bd4cd8ac4a..b820a4f6e8c7 100644
> > --- a/drivers/net/ethernet/ibm/emac/core.h
> > +++ b/drivers/net/ethernet/ibm/emac/core.h
> > @@ -188,9 +188,6 @@ struct emac_instance {
> >       struct emac_instance            *mdio_instance;
> >       struct mutex                    mdio_lock;
> >
> > -     /* Device-tree based phy configuration */
> > -     struct phy_device               *phy_dev;
> > -
> >       /* ZMII infos if any */
> >       u32                             zmii_ph;
> >       u32                             zmii_port;
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ