netdev - RE: [PATCH net-next v3 2/4] net: ethernet: renesas: rswitch: Simplify struct phy * handling

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] [day] [month] [year] [list]

Message-ID: <TYBPR01MB5341B6EE4C61215612936370D8CC9@TYBPR01MB5341.jpnprd01.prod.outlook.com>
Date:   Fri, 27 Jan 2023 10:49:45 +0000
From:   Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
CC:     "linux@...linux.org.uk" <linux@...linux.org.uk>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>
Subject: RE: [PATCH net-next v3 2/4] net: ethernet: renesas: rswitch: Simplify
 struct phy * handling

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Friday, January 27, 2023 5:35 PM
> 
> Hi Shimoda-san,
> 
> On Fri, Jan 27, 2023 at 2:49 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@...esas.com> wrote:
> > Simplify struct phy *serdes handling by keeping the valiable in
> > the struct rswitch_device.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
> 
> Thanks for your patch!

Thank you for your review!

> > --- a/drivers/net/ethernet/renesas/rswitch.c
> > +++ b/drivers/net/ethernet/renesas/rswitch.c
> > @@ -1222,49 +1222,40 @@ static void rswitch_phylink_deinit(struct rswitch_device *rdev)
> >         phylink_destroy(rdev->phylink);
> >  }
> >
> > -static int rswitch_serdes_set_params(struct rswitch_device *rdev)
> > +static int rswitch_serdes_phy_get(struct rswitch_device *rdev)
> >  {
> >         struct device_node *port = rswitch_get_port_node(rdev);
> >         struct phy *serdes;
> > -       int err;
> >
> >         serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL);
> >         of_node_put(port);
> >         if (IS_ERR(serdes))
> >                 return PTR_ERR(serdes);
> 
> You may as well just return serdes...
> 
> > +       rdev->serdes = serdes;
> 
> ... and move the above assignment into the caller.
> That would save one if (...) check.
> 
> After that, not much is left in this function, so I'm wondering if it
> can just be inlined at the single callsite?

I think so. Thank you for your suggestion!

> BTW, there seem to be several calls to rswitch_get_port_node(), which
> calls into DT tree traversal, so you may want to call it once, and store
> a pointer to the port device node, too.  Then rswitch_serdes_phy_get()
> becomes a candidate for manual inlining for sure.

I understood it. I'll modify it on v4 patch.

> > +
> > +       return 0;
> > +}
> > +
> > +static int rswitch_serdes_set_params(struct rswitch_device *rdev)
> > +{
> > +       int err;
> >
> > -       err = phy_set_mode_ext(serdes, PHY_MODE_ETHERNET,
> > +       err = phy_set_mode_ext(rdev->serdes, PHY_MODE_ETHERNET,
> >                                rdev->etha->phy_interface);
> >         if (err < 0)
> >                 return err;
> >
> > -       return phy_set_speed(serdes, rdev->etha->speed);
> > +       return phy_set_speed(rdev->serdes, rdev->etha->speed);
> >  }
> >
> >  static int rswitch_serdes_init(struct rswitch_device *rdev)
> >  {
> > -       struct device_node *port = rswitch_get_port_node(rdev);
> > -       struct phy *serdes;
> > -
> > -       serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL);
> > -       of_node_put(port);
> > -       if (IS_ERR(serdes))
> > -               return PTR_ERR(serdes);
> > -
> > -       return phy_init(serdes);
> > +       return phy_init(rdev->serdes);
> >  }
> 
> As this is now a one-line function, just call phy_init() in all
> callers instead?

I think so.

> >
> >  static int rswitch_serdes_deinit(struct rswitch_device *rdev)
> >  {
> > -       struct device_node *port = rswitch_get_port_node(rdev);
> > -       struct phy *serdes;
> > -
> > -       serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL);
> > -       of_node_put(port);
> > -       if (IS_ERR(serdes))
> > -               return PTR_ERR(serdes);
> > -
> > -       return phy_exit(serdes);
> > +       return phy_exit(rdev->serdes);
> >  }
> 
> Just call phy_exit() in all callers instead?

I got it. I'll fix it.

Best regards,
Yoshihiro Shimoda