Re: [PATCH net] net: phy: air_en8811h: restore LED GPIO output after MCU restart

[js node]

Thanks for the review.

I don't have AN8811HB hardware, so I can't confirm whether the MCU
restart clears AN8811HB_GPIO_OUTPUT in the same way.

Looking at the register layout, I agree it is very likely affected.
However, I'd prefer to keep this patch scoped to EN8811H where the
fix is verified, rather than extend it to AN8811HB based on
speculation.

If someone with AN8811HB hardware can confirm, a follow-up patch
adding the same restore to an8811hb_config_init() would make sense.


Jakub Kicinski <kuba@xxxxxxxxxx> 于2026年8月25日周二 02:50写道：
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> Verified. Report follows.
>
> commit 90353ad40d9c49a8f67a9cf009916549d394f63d
> Author: Ziyou Xu <xuziyougm@xxxxxxxxx>
>
> net: phy: air_en8811h: restore LED GPIO output after MCU restart
>
> The LED GPIO3/4/5 output-enable write is moved out of en8811h_probe() and
> into en8811h_config_init(), just before air_leds_init(), so the pads are
> re-enabled after every MD32 restart.
>
> > diff --git a/drivers/net/phy/air_en8811h.c b/drivers/net/phy/air_en8811h.c
> > index edd49c193e476..fbd87fc712bfb 100644
> > --- a/drivers/net/phy/air_en8811h.c
> > +++ b/drivers/net/phy/air_en8811h.c
>
> [ ... ]
>
> > @@ -1317,6 +1310,16 @@ static int en8811h_config_init(struct phy_device *phydev)
> >       if (ret < 0)
> >               return ret;
> >
> > +     /*
> > +      * Restarting MD32 clears the GPIO3/4/5 output enable bits. Restore
> > +      * them after every restart, before enabling the corresponding LEDs.
> > +      */
> > +     ret = air_phy_buckpbus_reg_modify(phydev, EN8811H_GPIO_OUTPUT,
> > +                                       EN8811H_GPIO_OUTPUT_345,
> > +                                       EN8811H_GPIO_OUTPUT_345);
> > +     if (ret < 0)
> > +             return ret;
> > +
>
> [Severity: Medium]
> This isn't a bug introduced by this patch, but should the sibling AN8811HB
> path in the same file get the same treatment?
>
> AN8811HB_GPIO_OUTPUT_345 is written in exactly one place, an8811hb_probe():
>
>         /* Configure led gpio pins as output */
>         ret = air_phy_buckpbus_reg_modify(phydev, AN8811HB_GPIO_OUTPUT,
>                                           AN8811HB_GPIO_OUTPUT_345,
>                                           AN8811HB_GPIO_OUTPUT_345);
>
> an8811hb_config_init() then restarts the MCU on every call after the first
> and goes straight to the LEDs without touching that register:
>
>         if (priv->mcu_needs_restart) {
>                 ret = en8811h_restart_mcu(phydev);
>                 ...
>         }
>         ret = an8811hb_config_serdes_polarity(phydev);
>         ...
>         ret = air_leds_init(phydev, EN8811H_LED_COUNT, AIR_PHY_LED_DUR,
>                             AIR_LED_MODE_USER_DEFINE);
>
> For AN8811HB, en8811h_restart_mcu() does a superset of the EN8811H
> sequence:
>
>         if (phy_id_compare_model(phydev->phy_id, AN8811HB_PHY_ID)) {
>                 ret = an8811hb_mcu_assert(phydev);
>                 ...
>                 ret = an8811hb_mcu_deassert(phydev);
>                 ...
>         }
>         ret = air_phy_buckpbus_reg_write(phydev, EN8811H_FW_CTRL_1,
>                                          EN8811H_FW_CTRL_1_START);
>
> If the premise stated in the new comment ("Restarting MD32 clears the
> GPIO3/4/5 output enable bits") holds for this device family, do the
> AN8811HB LEDs also go dark on the second and later .config_init() call
> (ifdown/ifup, or a repeated phy_init_hw())?
>
> After this patch the two peer paths in the file disagree: en8811h_config_init()
> restores the pad output-enable bits on every restart, while
> an8811hb_config_init() still assumes they survive it.
>
> Would it make sense to place the restore in the shared en8811h_restart_mcu()
> or air_leds_init() path so both models are covered, or duplicate it in
> an8811hb_config_init()? If AN8811HB is genuinely unaffected, could the commit
> message say why?
>
> Note this concern is based on the register definitions rather than measured
> hardware behaviour: AN8811HB_GPIO_OUTPUT is the same 0xcf8b8 offset within
> the AN8811HB PBUS window with an identical BIT(3) | BIT(4) | BIT(5) mask, and
> commit 6b9c9def95cb ("net: phy: air_en8811h: Introduce resume/suspend and
> clk_restore_context...") notes that PHY reinitialisation reverts chip-config
> registers such as CKO to their initial values.
>
> >       ret = air_leds_init(phydev, EN8811H_LED_COUNT, AIR_PHY_LED_DUR,
> >                           AIR_LED_MODE_USER_DEFINE);
> >       if (ret < 0) {
> --
> pw-bot: cr