After suspend and resume, the meson GXL internal PHY config needs to
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 <[email protected]>
Signed-off-by: Da Xue <[email protected]>
---
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);
+ 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,
}, {
--
2.40.1
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 <[email protected]>
> Signed-off-by: Da Xue <[email protected]>
> ---
> 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,
> }, {
> > 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
HI Heiner Kallweit:
On Tue, Aug 8, 2023 at 6:42 AM Heiner Kallweit <[email protected]> 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 <[email protected]>
> > Signed-off-by: Da Xue <[email protected]>
> > ---
> > 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,
> > }, {
>