| Message ID | 20230808050016.1911447-1-da@libre.computer (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] net: phy: meson-gxl: implement meson_gxl_phy_resume() | expand |
On 08.08.2023 07:00, Da Xue wrote: > After suspend and resume, the meson GXL internal PHY config needs to To avoid misunderstandings: You mean suspend/resume just of the PHY, or of the system? Description sounds like this patch is a fix and should go to stable. So add a Fixes tag. And a formal remark: Your patch misses the net / net-next annotation. > be initialized again, otherwise the carrier cannot be found: > > eth0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc mq state > DOWN group default qlen 1000 > > After the patch, resume: > > eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP > group default qlen 1000 > > Signed-off-by: Luke Lu <luke.lu@libre.computer> > Signed-off-by: Da Xue <da@libre.computer> > --- > Changes since v2: > - fix missing parameter of genphy_resume() > > Changes since v1: > - call generic genphy_resume() > --- > drivers/net/phy/meson-gxl.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c > index bb9b33b6bce2..bbad26b7c5a1 100644 > --- a/drivers/net/phy/meson-gxl.c > +++ b/drivers/net/phy/meson-gxl.c > @@ -132,6 +132,18 @@ static int meson_gxl_config_init(struct phy_device *phydev) > return 0; > } > > +static int meson_gxl_phy_resume(struct phy_device *phydev) > +{ > + int ret; > + > + genphy_resume(phydev); Return value of this function should be checked. > + ret = meson_gxl_config_init(phydev); > + if (ret) > + return ret; > + > + return 0; > +} > + > /* This function is provided to cope with the possible failures of this phy > * during aneg process. When aneg fails, the PHY reports that aneg is done > * but the value found in MII_LPA is wrong: > @@ -196,7 +208,7 @@ static struct phy_driver meson_gxl_phy[] = { > .config_intr = smsc_phy_config_intr, > .handle_interrupt = smsc_phy_handle_interrupt, > .suspend = genphy_suspend, > - .resume = genphy_resume, > + .resume = meson_gxl_phy_resume, > .read_mmd = genphy_read_mmd_unsupported, > .write_mmd = genphy_write_mmd_unsupported, > }, {
HI Heiner Kallweit: On Tue, Aug 8, 2023 at 6:42 AM Heiner Kallweit <hkallweit1@gmail.com> wrote: > > On 08.08.2023 07:00, Da Xue wrote: > > After suspend and resume, the meson GXL internal PHY config needs to > > To avoid misunderstandings: > You mean suspend/resume just of the PHY, or of the system? > We found this issue during the test of whole system suspend/resume(including ethernet/network), so it's more proper to interpret as "the system" here > Description sounds like this patch is a fix and should go to stable. I agree it's worth the effort to push the patch to stable tree, but found a conflict with commit 69ff53e4a4c9 ("net: phy: meson-gxl: use MMD access dummy stubs for GXL, internal PHY") It will prevent maintainers doing a clean cherry-pick, we can slightly rework the patch and send it to the stable tree separately once this patch is accepted by mainline. > So add a Fixes tag. Sure, as the issue is introduced with first resume(), so will add Fixes: 7334b3e47aee ("net: phy: Add Meson GXL Internal PHY driver") > And a formal remark: Your patch misses the net / net-next annotation. > Not sure if we understand this correctly, do you mean the one line summary of this patch? or the content of the commit message that needs to improve to reflect this is an ethernet/net related fix? I'd appreciate if you can explain a little bit more, so we can better fix this, thanks > > be initialized again, otherwise the carrier cannot be found: > > > > eth0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc mq state > > DOWN group default qlen 1000 > > > > After the patch, resume: > > > > eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP > > group default qlen 1000 > > > > Signed-off-by: Luke Lu <luke.lu@libre.computer> > > Signed-off-by: Da Xue <da@libre.computer> > > --- > > Changes since v2: > > - fix missing parameter of genphy_resume() > > > > Changes since v1: > > - call generic genphy_resume() > > --- > > drivers/net/phy/meson-gxl.c | 14 +++++++++++++- > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c > > index bb9b33b6bce2..bbad26b7c5a1 100644 > > --- a/drivers/net/phy/meson-gxl.c > > +++ b/drivers/net/phy/meson-gxl.c > > @@ -132,6 +132,18 @@ static int meson_gxl_config_init(struct phy_device *phydev) > > return 0; > > } > > > > +static int meson_gxl_phy_resume(struct phy_device *phydev) > > +{ > > + int ret; > > + > > + genphy_resume(phydev); > > Return value of this function should be checked. > will fix in v4 > > + ret = meson_gxl_config_init(phydev); > > + if (ret) > > + return ret; > > + > > + return 0; > > +} > > + > > /* This function is provided to cope with the possible failures of this phy > > * during aneg process. When aneg fails, the PHY reports that aneg is done > > * but the value found in MII_LPA is wrong: > > @@ -196,7 +208,7 @@ static struct phy_driver meson_gxl_phy[] = { > > .config_intr = smsc_phy_config_intr, > > .handle_interrupt = smsc_phy_handle_interrupt, > > .suspend = genphy_suspend, > > - .resume = genphy_resume, > > + .resume = meson_gxl_phy_resume, > > .read_mmd = genphy_read_mmd_unsupported, > > .write_mmd = genphy_write_mmd_unsupported, > > }, { >
> > And a formal remark: Your patch misses the net / net-next annotation. > > > Not sure if we understand this correctly, do you mean the one line > summary of this patch? > or the content of the commit message that needs to improve to reflect this is an > ethernet/net related fix? https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#netdev-faq Andrew
diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c index bb9b33b6bce2..bbad26b7c5a1 100644 --- a/drivers/net/phy/meson-gxl.c +++ b/drivers/net/phy/meson-gxl.c @@ -132,6 +132,18 @@ static int meson_gxl_config_init(struct phy_device *phydev) return 0; } +static int meson_gxl_phy_resume(struct phy_device *phydev) +{ + int ret; + + genphy_resume(phydev); + ret = meson_gxl_config_init(phydev); + if (ret) + return ret; + + return 0; +} + /* This function is provided to cope with the possible failures of this phy * during aneg process. When aneg fails, the PHY reports that aneg is done * but the value found in MII_LPA is wrong: @@ -196,7 +208,7 @@ static struct phy_driver meson_gxl_phy[] = { .config_intr = smsc_phy_config_intr, .handle_interrupt = smsc_phy_handle_interrupt, .suspend = genphy_suspend, - .resume = genphy_resume, + .resume = meson_gxl_phy_resume, .read_mmd = genphy_read_mmd_unsupported, .write_mmd = genphy_write_mmd_unsupported, }, {