Re: [PATCH v3] net: phy: meson-gxl: implement meson_gxl_phy_resume()

[Luke Lu]

HI Heiner Kallweit:

On Tue, Aug 8, 2023 at 6:42 AM Heiner Kallweit <hkallweit1@xxxxxxxxx> 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,
> >       }, {
>