Re: [PATCH net-next v4 1/4] net: phy: microchip_t1s: fix collision detection on PLCA status change

[<Parthiban.Veerasooran@microchip.com>]

Hi,

On 03/10/26 7:06 pm, netdev-bot+sashiko@kernel.org wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 2
> 
> 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 collision-detect (CDEN) setting is chosen from a software
>    copy of the PLCA enable state (priv->plca_enabled), not from the PHY's…
> - [Low] [Baseline decision: false.
> - [Low] [Baseline decision: false.
> 
> --
> 
> net: phy: microchip_t1s: fix collision detection on PLCA status change
> 
> This patch makes the collision detection enable bit (CDEN) follow
> autonomous PLCA/CSMA-CD transitions. It adds a PLCA status change
> interrupt and a resync in read_status(), and it takes the PRSCTL1
> fallback bit into account. It also adds a probe routine that allocates
> a per-PHY mutex and a cached plca_enabled value, and it treats
> plca_cfg->enabled as tri-state.
> 
>> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
>> index 73c23d311d72a..5ce0304bf0952 100644
>> --- a/drivers/net/phy/microchip_t1s.c
>> +++ b/drivers/net/phy/microchip_t1s.c
> 
> [ ... ]
> 
>> @@ -430,6 +469,57 @@ static int lan867x_revd0_link_active_selection(struct phy_device *phydev,
> 
> [ ... ]
> 
>> +static int lan86xx_update_cden(struct phy_device *phydev)
>> +{
>> +     struct lan86xx_priv *priv = phydev->priv;
>> +     struct phy_plca_status plca_st;
>> +     int fallback, ret;
>> +     u16 cden;
>> +
>> +     fallback = lan86xx_fallback_to_csmacd(phydev);
>> +     if (fallback < 0)
>> +             return fallback;
>> +
>> +     ret = genphy_c45_plca_get_status(phydev, &plca_st);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     /* PLCA disabled                          -> CDEN enabled
>> +      * PLCA enabled + BEACON                  -> CDEN disabled
>> +      * PLCA enabled + no BEACON + fallback    -> CDEN enabled
>> +      * PLCA enabled + no BEACON + no fallback -> CDEN disabled
>> +      */
>> +     if (!priv->plca_enabled)
>> +             cden = COL_DET_ENABLE;
>> +     else if (plca_st.pst)
>> +             cden = COL_DET_DISABLE;
>> +     else if (fallback)
>> +             cden = COL_DET_ENABLE;
>> +     else
>> +             cden = COL_DET_DISABLE;
> 
> [Severity: Medium]
> Can priv->plca_enabled get out of sync with the PLCA_CTRL0 enable bit in
> the hardware? This decision uses the cached copy. read_status(),
> config_intr() and the PSTC handler all rewrite CDEN from it, so a
> mismatch gets written back on every poll.
> 
> The copy is only updated in lan86xx_plca_set_cfg(), and only after
> genphy_c45_plca_set_cfg() fully succeeds. When enabled == 0, the generic
> helper clears PLCA_EN first and then does more MDIO accesses that can
> fail:
> 
> drivers/net/phy/phy-c45.c:genphy_c45_plca_set_cfg() {
>      ...
>          if (plca_cfg->enabled == 0) {
>                  ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
>                                           MDIO_OATC14_PLCA_CTRL0,
>                                           MDIO_OATC14_PLCA_EN);
>      ...
>                  ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
>                                      MDIO_OATC14_PLCA_CTRL1, val);
> 
>                  if (ret < 0)
>                          return ret;
>      ...
> }
> 
> If one of the later writes fails, PLCA is off in the hardware but
> plca_enabled is still 1.
> 
> A PHY reset while the driver stays bound has the same effect. For MACs
> that connect the PHY in ndo_open and have a reset GPIO or reset
> controller, ifdown goes through:
> 
> drivers/net/phy/phy_device.c:phy_detach() {
>      ...
>          /* Assert the reset signal */
>          phy_device_reset(phydev, 1);
>      ...
> }
> 
> A resume after the PHY lost power has the same effect. PLCA_CTRL0 goes
> back to its default, but priv is devm-allocated at probe, so it keeps
> plca_enabled = 1. lan867x_revc_config_init() does not touch PLCA_CTRL0.
> 
> In both cases this function sees plca_enabled = 1 and pst = 0. If
> PRSCTL1 bit 10 (fallback) is clear, it writes COL_DET_DISABLE while the
> PHY is actually running CSMA/CD. That is the situation the commit
> message says it fixes.
> 
> The opposite can happen at probe. lan86xx_probe() zero-allocates priv
> without reading PLCA_CTRL0. If PLCA is already enabled in the hardware
> (for example, a module reload with no reset line), read_status() forces
> CDEN on in PLCA mode, even though genphy_c45_plca_get_cfg() reports PLCA
> as enabled.
> 
> The same logic is still there at the end of the series. Would it be
> more robust for lan86xx_update_cden() to read the PLCA_CTRL0 enable bit
> directly, instead of using the cached value?

priv->plca_enabled intentionally represents the PLCA configuration 
requested through the driver, while PLCA_Status represents the 
autonomous operational state and is read separately.

genphy_c45_plca_set_cfg() returning an error after partially updating 
the PHY leaves the hardware configuration indeterminate, so updating the 
cached state only after the complete operation succeeds avoids recording 
a configuration that the driver did not successfully apply. Reading 
PLCA_CTRL0 on every CDEN update would add an MDIO transaction but would 
not provide a reliable recovery mechanism for such a partially failed 
configuration.

The same cached configured state is therefore intentionally used by the 
CDEN resynchronization paths.

> 
>> +
>> +     return phy_modify_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_COL_DET_CTRL0,
>> +                           COL_DET_CTRL0_ENABLE_BIT_MASK, cden);
>> +}
> 
> [ ... ]
> 
>> @@ -458,14 +552,18 @@ static int lan86xx_plca_set_cfg(struct phy_device *phydev,
>>        if (ret)
>>                return ret;
>>
>> -     if (plca_cfg->enabled)
>> -             return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
>> -                                   LAN86XX_REG_COL_DET_CTRL0,
>> -                                   COL_DET_CTRL0_ENABLE_BIT_MASK,
>> -                                   COL_DET_DISABLE);
>> +     if (plca_cfg->enabled != -1)
>> +             priv->plca_enabled = plca_cfg->enabled;
> 
> [Severity: Low]
> This isn't a bug, but priv->plca_enabled is written here under
> phydev->lock only. lan86xx_config_intr() reads it through
> lan86xx_update_cden() under cden_lock only.
> 
> In practice, the ethtool path (rtnl or the netdev instance lock) and
> the config_intr() callers that reach lan86xx_update_cden() are
> serialized. Also, the locked lan86xx_update_cden() call below writes
> CDEN last using the new value.
> 
> Would it be cleaner to do this store inside the cden_lock section, so
> that the lock covers both the cached state and the register update?

The PLCA configuration path updates priv->plca_enabled and then 
immediately updates CDEN under cden_lock. The mutex is intended to 
serialize CDEN register accesses with the interrupt/status paths; it 
does not need to protect the cached configuration value independently.

> 
>>
>> -     return phy_modify_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_COL_DET_CTRL0,
>> -                           COL_DET_CTRL0_ENABLE_BIT_MASK, COL_DET_ENABLE);
>> +     if (plca_cfg->enabled != -1) {
>> +             mutex_lock(&priv->cden_lock);
>> +             ret = lan86xx_update_cden(phydev);
>> +             mutex_unlock(&priv->cden_lock);
>> +             if (ret)
>> +                     return ret;
>> +     }
> 
> [Severity: Low]
> The commit message says:
> 
>      Rev.D0 is handled separately (follow-on patch).
> 
> At this commit, though, LAN867X Rev.D0 also uses lan86xx_plca_set_cfg(),
> and this block calls lan86xx_update_cden() for it too. The Rev.D0 entry
> in microchip_t1s_driver[] also gets .probe = lan86xx_probe in this
> patch.
> 
> Rev.D0 has no read_status, config_intr or handle_interrupt at this
> point. Enabling PLCA while pst reads 0 and the PRSCTL1 fallback bit is
> set would therefore leave CDEN enabled after PLCA comes up. Is that
> expected for this intermediate commit?
> 
> The follow-on patch, "net: phy: microchip_t1s: fix collision detection
> for LAN867X Rev.D0", adds an early return for PHY_ID_LAN867X_REVD0
> before the CDEN update, so this only affects bisection.
> 
> Rev.D0 does need .probe here, because the shared setter now dereferences
> phydev->priv. Could the commit message mention this Rev.D0 change?

Rev.D0-specific collision-detection handling is intentionally 
implemented in the follow-up patch. The .probe addition in this patch is 
required because the shared PLCA configuration path now uses per-PHY 
state. The final series provides the complete Rev.D0 handling.

Best regards,
Parthiban V

>> +
>> +     return 0;
>>   }
> 
> [ ... ]
> 
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929125928.611784-1-parthiban.veerasooran%40microchip.com