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
| ||
|
Message-ID: <TYBPR01MB5341F5AAE321E3E373012E4DD8D2A@TYBPR01MB5341.jpnprd01.prod.outlook.com> Date: Fri, 13 Oct 2023 02:10:42 +0000 From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com> To: Geert Uytterhoeven <geert@...ux-m68k.org> 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 Geert-san, > From: Geert Uytterhoeven, Sent: Thursday, October 12, 2023 9:35 PM > > Hi Shimoda-san, > > 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 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: --- #define rswitch_for_each_enabled_port_continue_reverse(priv, i) \ for (i--; i >= 0; i--) \ 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? > > + > > + rswitch_for_each_enabled_port(priv, i) { > > + ndev = priv->rdev[i]->ndev; > > + if (netif_running(ndev)) { > > + netif_device_detach(ndev); > > + rswitch_stop(ndev); > > + } > > + if (priv->rdev[i]->serdes->init_count) > > + phy_exit(priv->rdev[i]->serdes); > > If !init_count, the PHY was not initialized before suspending? ... Yes, it was possible if renesas_eth_sw_resume() called phy_init() and it failed. > > + } > > + > > + return 0; > > +} > > + > > +static int __maybe_unused renesas_eth_sw_resume(struct device *dev) > > +{ > > + struct rswitch_private *priv = dev_get_drvdata(dev); > > + struct net_device *ndev; > > + int i; > > + > > + rswitch_for_each_enabled_port(priv, i) { > > + phy_init(priv->rdev[i]->serdes); > > ... while it is always initialized after resuming? Is that intentional, > or should the pre-suspend state be preserved? Yes, that is intentional. After this driver was probed, the phy was always initialized. > > + ndev = priv->rdev[i]->ndev; > > + if (netif_running(ndev)) { > > + rswitch_open(ndev); > > + netif_device_attach(ndev); > > + } > > + } > > + > > + return 0; > > +} > > + > > +static SIMPLE_DEV_PM_OPS(renesas_eth_sw_pm_ops, renesas_eth_sw_suspend, > > + renesas_eth_sw_resume); > > Please use DEFINE_SIMPLE_DEV_PM_OPS() instead, so you can drop the > __maybe_unused tags from the callbacks. I got it. I'll fix them. > > + > > static struct platform_driver renesas_eth_sw_driver_platform = { > > .probe = renesas_eth_sw_probe, > > .remove_new = renesas_eth_sw_remove, > > .driver = { > > .name = "renesas_eth_sw", > > + .pm = &renesas_eth_sw_pm_ops, > > pm_sleep_ptr(...) I'll use it on v2. Best regards, Yoshihiro Shimoda > > .of_match_table = renesas_eth_sw_of_table, > > } > > }; > > 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
Powered by blists - more mailing lists