netdev - Re: [PATCH net-next] rswitch: Add PM ops

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMuHMdU9Udd7xX7Us+3T06YMLAb4y0xnSOipN-5c-d-MfpvKdg@mail.gmail.com>
Date: Fri, 13 Oct 2023 09:28:30 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
Cc: "s.shtylyov@....ru" <s.shtylyov@....ru>, "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] rswitch: Add PM ops

Hi Shimoda-san,

On Fri, Oct 13, 2023 at 4:10 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@...esas.com> wrote:
> > From: Geert Uytterhoeven, Sent: Thursday, October 12, 2023 9:35 PM
> > On Thu, Oct 12, 2023 at 2:16 PM Yoshihiro Shimoda
> > <yoshihiro.shimoda.uh@...esas.com> wrote:
> > > Add PM ops for Suspend to Idle. When the system suspended,
> > > the Ethernet Serdes's clock will be stopped. So, this driver needs
> > > to re-initialize the Ethernet Serdes by phy_init() in
> > > renesas_eth_sw_resume(). Otherwise, timeout happened in phy_power_on().
> > >
> > > 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
> > > @@ -17,6 +17,7 @@
> > >  #include <linux/of_net.h>
> > >  #include <linux/phy/phy.h>
> > >  #include <linux/platform_device.h>
> > > +#include <linux/pm.h>
> > >  #include <linux/pm_runtime.h>
> > >  #include <linux/rtnetlink.h>
> > >  #include <linux/slab.h>
> > > @@ -1315,6 +1316,7 @@ static int rswitch_phy_device_init(struct rswitch_device *rdev)
> > >         if (!phydev)
> > >                 goto out;
> > >         __set_bit(rdev->etha->phy_interface, phydev->host_interfaces);
> > > +       phydev->mac_managed_pm = true;
> > >
> > >         phydev = of_phy_connect(rdev->ndev, phy, rswitch_adjust_link, 0,
> > >                                 rdev->etha->phy_interface);
> > > @@ -1991,11 +1993,52 @@ static void renesas_eth_sw_remove(struct platform_device *pdev)
> > >         platform_set_drvdata(pdev, NULL);
> > >  }
> > >
> > > +static int __maybe_unused renesas_eth_sw_suspend(struct device *dev)
> > > +{
> > > +       struct rswitch_private *priv = dev_get_drvdata(dev);
> > > +       struct net_device *ndev;
> > > +       int i;
> >
> > unsigned int (also below)
>
> I don't know why unsigned int is needed. Other functions use

There's this old mantra "prefer unsigned over signed in C",
and a valid port array index here is always unsigned.

> rswitch_for_each_enabled_port{_continue_reverse}() with int.
> Especially, rswitch_for_each_enabled_port_continue_reverse()
> has the following code, unsigned int will not work correctly:

Oh, there is also a reverse variant, which indeed needs a signed
iterator, currently...

> ---
> #define rswitch_for_each_enabled_port_continue_reverse(priv, i) \
>         for (i--; i >= 0; i--)                                  \

I think this can be made to work with an unsigned iterator using

    for (; i-- > 0; )

>                 if (priv->rdev[i]->disabled)                    \
>                         continue;                               \
>                 else
> ---
>
> So, I would like to keep this for consistency with other functions'
> implementation. But, what do you think?

Consistency is good...
Surprising readers (why is this signed?) is bad...
It's hard to keep a good balance...

BTW, perhaps it would make sense to use the reverse order in suspend?
Although it probably doesn't matter, as rswitch_deinit() uses the
non-reverse order, too.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds