RE: [PATCH net-next] rswitch: Add PM ops

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Friday, October 13, 2023 4:29 PM
> 
> Hi Shimoda-san,
> 
> On Fri, Oct 13, 2023 at 4:10 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > >
> > > 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.

I understood it.

> > 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; )

I think that this loop cannot access index 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.

Thank you for your suggestion. I also think that we can use the
non-reverse order. So,
 1) Change "_reverse()" macro to others somehow.
 2) Change the type of i for "for_each" to unsigned int.
 3) Add PM ops.

Perhaps, can/should we merge the 1) and 2) to one patch?

Best regards,
Yoshihiro Shimoda

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
> 
> 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




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux