Re: [PATCH net-next v2 5/8] net: dsa: mt7530: replace mt7530_read with regmap_read

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: mt7530: replace mt7530_read with regmap_read

This commit removes the mt7530_read() wrapper and converts every caller
to use regmap_read() directly. The WARN_ON_ONCE / dev_err and the
return-zero-on-failure behaviour the wrapper provided are dropped.

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index fe7e4ab5ae9c..4168adca949f 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -152,28 +152,15 @@ core_clear(struct mt7530_priv *priv, u32 reg, u32 val)
>  
>  
>  static u32
> -mt7530_read(struct mt7530_priv *priv, u32 reg)
> +mt7530_mii_poll(struct mt7530_dummy_poll *p)
>  {
> -	int ret;
>  	u32 val;
>  
> -	ret = regmap_read(priv->regmap, reg, &val);
> -	if (ret) {
> -		WARN_ON_ONCE(1);
> -		dev_err(priv->dev,
> -			"failed to read mt7530 register\n");
> -		return 0;
> -	}
> +	regmap_read(p->priv->regmap, p->reg, &val);
>  
>  	return val;
>  }

[Medium]
The old wrapper guaranteed val == 0 on failure. The new mt7530_mii_poll()
declares val without an initializer and ignores the regmap_read() return
code, so on a failed underlying transfer val is whatever was on the stack.

Looking at drivers/net/dsa/mt7530-mdio.c::mt7530_regmap_read():

    ret = bus->write(bus, priv->mdiodev->addr, 0x1f, page);
    if (ret < 0)
        return ret;

    lo = bus->read(bus, priv->mdiodev->addr, r);
    hi = bus->read(bus, priv->mdiodev->addr, 0x10);

    *val = (hi << 16) | (lo & 0xffff);

When the page-select write fails, *val is not written. mt7530_mii_poll()
is the polling callback used by readx_poll_timeout() in mt7530_fdb_cmd(),
mt7530_vlan_cmd(), mt7530_setup() and mt7531_setup() with predicates
such as val != 0 (post-reset stability), !(val & ATC_BUSY) and
!(val & VTCR_BUSY).

Previously a broken MDIO transport produced val == 0 and the post-reset
poll deterministically timed out. Can the new code spuriously satisfy
the val != 0 predicate from an uninitialized stack slot, then feed that
same garbage into (val & MT7530_XTAL_MASK) to choose 25/40 MHz settings
in mt7530_pll_setup()?

> @@ -365,9 +354,9 @@ mt7531_pll_setup(struct mt7530_priv *priv)
>  	u32 hwstrap;
>  	u32 val;
>  
> -	val = mt7530_read(priv, MT7531_CREV);
> -	top_sig = mt7530_read(priv, MT7531_TOP_SIG_SR);
> -	hwstrap = mt7530_read(priv, MT753X_TRAP);
> +	regmap_read(priv->regmap, MT7531_CREV, &val);
> +	regmap_read(priv->regmap, MT7531_TOP_SIG_SR, &top_sig);
> +	regmap_read(priv->regmap, MT753X_TRAP, &hwstrap);

[Medium]
Same concern applies to all the read-modify-write sequences in
mt7531_pll_setup(), e.g.:

    regmap_read(priv->regmap, MT7531_PLLGP_EN, &val);
    val &= ~EN_COREPLL;
    regmap_write(priv->regmap, MT7531_PLLGP_EN, val);

If regmap_read() fails, val is uninitialized and is then OR'd / AND'd
with constants and written straight back into PLLGP / COREPLL control
registers. Was there a reason to drop the wrapper's return-zero (and
WARN_ON_ONCE / dev_err) behaviour without either initializing these
locals to 0 or checking the return code?

> @@ -1010,7 +999,7 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
>  
>  	mutex_lock(&priv->reg_mutex);
>  
> -	val = mt7530_read(priv, MT753X_MTRAP);
> +	regmap_read(priv->regmap, MT753X_MTRAP, &val);

[Low]
mt7530_setup_port5() declares val as int, but regmap_read()'s third
parameter is unsigned int *. The previous code hid this through the
u32 return of mt7530_read() and an implicit assignment conversion;
the direct call now passes &val with mismatched signedness. Should this
be u32 val; like the other call sites in this file (sparse and
-Wpointer-sign would flag this)?

> @@ -2436,7 +2435,7 @@ mt7530_setup(struct dsa_switch *ds)
>  		return ret;
>  	}
>  
> -	id = mt7530_read(priv, MT7530_CREV);
> +	regmap_read(priv->regmap, MT7530_CREV, &id);
>  	id >>= CHIP_NAME_SHIFT;
>  	if (id != MT7530_ID) {
>  		dev_err(priv->dev, "chip %x can't be supported\n", id);

[Medium]
The chip-ID gating in mt7530_setup() and mt7531_setup() was previously
deterministic-fail on MDIO read errors (id == 0 after the wrapper).
With id now uninitialized on failure, can this check spuriously match
or mismatch on garbage and either mis-bind a wrong-id chip or refuse a
real one?

> @@ -2690,7 +2689,7 @@ mt7531_setup(struct dsa_switch *ds)
>  	/* MT7531AE has got two SGMII units. One for port 5, one for port 6.
>  	 * MT7531BE has got only one SGMII unit which is for port 6.
>  	 */
> -	val = mt7530_read(priv, MT7531_TOP_SIG_SR);
> +	regmap_read(priv->regmap, MT7531_TOP_SIG_SR, &val);
>  	priv->p5_sgmii = !!(val & PAD_DUAL_SGMII_EN);

[Medium]
And here, p5_sgmii is set from val & PAD_DUAL_SGMII_EN after an
unchecked read; if the read fails, would this latch a wrong p5_sgmii
state from stack contents and then drive different mt7531_pll_setup()
vs. GPIO mode programming below?

The commit message says:

    The WARN_ON_ONCE error logging is dropped as regmap provides its
    own error handling.

Is that accurate for this driver? regmap_read() does not zero *val on
error and does not log; with all new call sites discarding the return
value, the converted paths appear to have no error handling at all,
which seems like a behavioural regression compared to the prior
wrapper. Would initializing the affected locals to 0 (matching the
old return-zero-on-failure semantics), or checking the regmap_read()
return code at the sites that derive subsequent hardware programming
(chip ID, XTAL select, PLL RMW, post-reset poll), be appropriate here?
-- 
pw-bot: cr