Re: [PATCH v2] net: phy: aquantia: Add support for 5GbaseR

[Hongmei Xu]

On Fri, Sep 25, 2026 at 1:45 AM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 1
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] The commit message does not mention a behaviour change in
>   aqr_gen2_get_rate_matching()…
> - [Low] The submission is incomplete.
>
> --
>
> net: phy: aquantia: Add support for 5GbaseR
>
> This patch decodes PHYXS interface status code 12 as 5GBASER in
> aqr_gen1_read_status(). It also makes aqr_gen1_config_init() accept
> 5GBASER, and makes aqr_gen2_get_rate_matching() report pause rate
> matching for 5GBASER.
>
> > Add support for 5GbaseR within Aquantia phy driver.
> >
> > Signed-off-by: Hongmei Xu <hongmei.xu@xxxxxxxxxxxxxxxx>
> >
> > ---
> > Changes in v2:
> > - EDITME: describe what is new in this series revision.
> > - EDITME: use bulletpoints and terse descriptions.
> > - Link to v1: https://patch.msgid.link/20260923-aquantia-support-5gbaser-v1-1-537d39bb8c35@xxxxxxxxxxxxxxxx
>
> [Severity: Low]
> This isn't a bug, but the v2 changelog is still the unedited b4 template.
> That makes it hard to tell what changed since v1.
>
This is a careless, it is fixed in new version now.

> The commit body is also a single line. Could it describe the three
> separate changes in the patch?
>
Yes, will privide more accurate commit message for it.

>   - aqr_gen1_read_status() now decodes interface status code 12.
>   - aqr_gen1_config_init() now accepts 5GBASER for all gen1 and later
>     PHYs, including AQR105.
>   - aqr_gen2_get_rate_matching() now returns RATE_MATCH_PAUSE for
>     5GBASER on existing gen2 and later PHYs.
>
> Separately, the new define lines up its value with spaces:
>
>   #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_USX_5G   12
>
> The neighbouring MDIO_PHYXS_VEND_IF_STATUS_TYPE_* defines use tabs.
>
OK, will use tabs.

> > diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
> > index e95d391a6dc23..0bcec88fcf727 100644
> > --- a/drivers/net/phy/aquantia/aquantia_main.c
> > +++ b/drivers/net/phy/aquantia/aquantia_main.c
>
> [ ... ]
>
> > @@ -1040,7 +1045,8 @@ static int aqr_gen2_get_rate_matching(struct phy_device *phydev,
> >                                     phy_interface_t iface)
> >  {
> >       if (iface == PHY_INTERFACE_MODE_10GBASER ||
> > -         iface == PHY_INTERFACE_MODE_2500BASEX)
> > +         iface == PHY_INTERFACE_MODE_2500BASEX ||
> > +         iface == PHY_INTERFACE_MODE_5GBASER)
> >               return RATE_MATCH_PAUSE;
>
> [Severity: Medium]
> Every PHY that uses aqr_gen2_get_rate_matching() now gets
> RATE_MATCH_PAUSE for 5GBASER instead of RATE_MATCH_NONE. That covers
> AQR107, AQCS109, AQR111/B0, AQR112, AQR412/C, AQR113/C, AQR114C,
> AQR115/C and AQR813. Does this hold for every firmware provisioning?
>
> aqr_gen2_read_global_syscfg() maps XFI5G to 5GBASER whatever the
> provisioned rate adaptation mode is:
>
> drivers/net/phy/aquantia/aquantia_main.c:aqr_gen2_read_global_syscfg() {
>     ...
>                 case VEND1_GLOBAL_CFG_SERDES_MODE_XFI5G:
>                         interface = PHY_INTERFACE_MODE_5GBASER;
>                         break;
>     ...
> }
>
> So 5GBASER can end up in possible_interfaces when firmware provisions
> XFI5G with RATE_ADAPT_NONE or RATE_ADAPT_USX.
>
> At runtime, aqr_gen2_read_status() still takes the provisioned value:
>
> drivers/net/phy/aquantia/aquantia_main.c:aqr_gen2_read_status() {
>     ...
>                 if (syscfg->rate_adapt == AQR_RATE_ADAPT_PAUSE)
>                         phydev->rate_matching = RATE_MATCH_PAUSE;
>                 else
>                         phydev->rate_matching = RATE_MATCH_NONE;
>     ...
> }
>
> phylink validation, however, uses the fixed answer:
>
> phylink_validate_one()
>     tmp_state.rate_matching = phy_get_rate_matching(phy, interface);
>     ...
>     phylink_get_capabilities()
>
> When 5GBASER returns PAUSE, phylink_get_capabilities() adds every speed
> below 5G to the supported and advertised link modes over 5GBASE-R. The
> fallback path in phylink_validate_phy() also stops widening validation to
> all interfaces for c45 PHYs.
>
> The get_rate_matching kernel-doc in include/linux/phy.h says:
>
>   It is assumed that if a rate matching mode is supported on an
>   interface, then that interface's rate can be adapted to all slower
>   link speeds supported by the phy.
>
> Could phylink then advertise 10M to 2.5G over a 5GBASE-R host link that
> the PHY won't pause-adapt? A MAC that only supports 5GBASE-R is one
> example. Validation and runtime would then disagree about the same
> interface.
>
> The existing 10GBASER and 2500BASEX checks are just as unconditional, but
> the 5GBASER case is new in this patch. Would it make sense to check the
> provisioned rate_adapt in priv->global_cfg[] for the requested interface?
> The commit message also doesn't mention this change in behaviour.
>
> >       return RATE_MATCH_NONE;
> >  }
>
> --

this is  a pre-existing concern, not caused by my change, can consider
to address it
in separate patch, this patch only focus on 5GBaseR mode enablement.

pw-bot: cr


Regards,
Hongmei Xu