Re: [PATCH net-next v2 3/7] net: ethernet: oa_tc6: Add support for MDIO indirect access

[netdev-bot+sashiko@kernel.org]

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 indirect MDIO sequencing in drivers/net/ethernet/oa_tc6.c
  relies on MDIOACC register behaviour that the patch never checks or…
- [Low] oa_tc6_mdiobus_register() decides that indirect MDIOACC access is
  available only because STDCAP.DPRAC (BIT(8),…

--

net: ethernet: oa_tc6: Add support for MDIO indirect access

This adds indirect PHY register access through the MDIOACC registers
for OA TC6 MAC-PHYs that don't advertise direct access in STDCAP.DPRAC.
Clause 22 operations write a single frame to MDIOACC1. Clause 45
operations write an address frame to MDIOACC0 and a read or write frame
to MDIOACC1, then poll MDIOACC1 for TRDONE. The patch also removes the
-ENODEV failure that happened when DPRAC was clear.

> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index 549997ba363ad..be1833c232e37 100644
> --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c

[ ... ]

> @@ -436,24 +437,149 @@ int oa_tc6_write_register_mms(struct oa_tc6 *tc6, u8 mms, u16 address,

[ ... ]

> +static int oa_tc6_indirect_mdiobus_read_c45(struct oa_tc6 *tc6, int addr,
> +					    int devnum, int regnum)
> +{
> +	u32 reg_val;
> +	int ret;
> +
> +	reg_val = FIELD_PREP(OA_TC6_MDIOACC_OP, OA_TC6_MDIOACC_OP_ADDR);
> +	reg_val |= FIELD_PREP(OA_TC6_MDIOACC_ST, OA_TC6_MDIOACC_ST_C45);
> +	reg_val |= FIELD_PREP(OA_TC6_MDIOACC_PRTAD, addr);
> +	reg_val |= FIELD_PREP(OA_TC6_MDIOACC_DEVAD, devnum);
> +	reg_val |= FIELD_PREP(OA_TC6_MDIOACC_DATA, regnum);
> +
> +	ret = oa_tc6_write_register(tc6, OA_TC6_REG_MDIOACC(0), reg_val);
> +	if (ret)
> +		return ret;
> +
> +	reg_val = FIELD_PREP(OA_TC6_MDIOACC_OP, OA_TC6_MDIOACC_OP_RD);
> +	reg_val |= FIELD_PREP(OA_TC6_MDIOACC_ST, OA_TC6_MDIOACC_ST_C45);
> +	reg_val |= FIELD_PREP(OA_TC6_MDIOACC_PRTAD, addr);
> +	reg_val |= FIELD_PREP(OA_TC6_MDIOACC_DEVAD, devnum);
> +
> +	ret = oa_tc6_write_register(tc6, OA_TC6_REG_MDIOACC(1), reg_val);
> +	if (ret)
> +		return ret;
> +
> +	ret = oa_tc6_mdioacc_wait_done(tc6, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	return oa_tc6_mdioacc_read_data(reg_val);
>  }

[ ... ]

> +static int oa_tc6_indirect_mdiobus_read(struct oa_tc6 *tc6, int addr,
> +					int regnum)
> +{
> +	u32 reg_val;
> +	int ret;
> +
> +	reg_val = FIELD_PREP(OA_TC6_MDIOACC_OP, OA_TC6_MDIOACC_OP_RD);
> +	reg_val |= FIELD_PREP(OA_TC6_MDIOACC_ST, OA_TC6_MDIOACC_ST_C22);
> +	reg_val |= FIELD_PREP(OA_TC6_MDIOACC_PRTAD, addr);
> +	reg_val |= FIELD_PREP(OA_TC6_MDIOACC_DEVAD, regnum);
> +
> +	ret = oa_tc6_write_register(tc6, OA_TC6_REG_MDIOACC(1), reg_val);
> +	if (ret)
> +		return ret;

[Severity: Medium]
Both of these sequences rely on MDIOACC behaviour that the code doesn't
check and the commit message doesn't cite from the spec.

oa_tc6_indirect_mdiobus_read_c45() and oa_tc6_indirect_mdiobus_write_c45()
write the address frame to MDIOACC0 and then write the read or write frame
to MDIOACC1 straight away. oa_tc6_mdioacc_wait_done() only polls
OA_TC6_REG_MDIOACC(1), so nothing ever checks TRDONE or TAERR for the
MDIOACC0 frame.

Do all TC6 MAC-PHYs queue MDIOACCn writes and run them in order? If a
device doesn't, could the data frame run before the MMD address is set and
return the contents of the wrong register?

For Clause 22, oa_tc6_indirect_mdiobus_read() and
oa_tc6_indirect_mdiobus_write() put the single frame in MDIOACC1. Does a
write to MDIOACC1 alone start a transaction, whatever MDIOACC0 holds? That
could be its reset value or a stale C45 address frame from an earlier
access.

The existing ADIN1110 driver uses MDIOACC0 for a single C22 transaction:

drivers/net/ethernet/adi/adin1110.c:adin1110_mdio_read() {
    ...
	/* write the clause 22 read command to the chip */
	mutex_lock(&priv->lock);
	ret = adin1110_write_reg(priv, ADIN1110_MDIOACC, val);
    ...
}

ADIN1110_MDIOACC is defined as 0x20 there.

Suppose a MAC-PHY without DPRAC runs MDIOACC0..n starting at MDIOACC0, or
doesn't trigger on a lone MDIOACC1 write. Could the C22 reads in the
mdiobus_register() scan then time out, or put an extra frame on the bus?

This may well work on ADIN1110, which uses this path later in the series.
But adin1110.c only shows that MDIOACC0 works when used on its own. Could
the commit message point to the section of the OA TC6 spec that defines
this ordering and triggering behaviour?

[ ... ]

> @@ -535,8 +675,16 @@ EXPORT_SYMBOL_GPL(oa_tc6_mdiobus_write_c45);
>  
>  static int oa_tc6_mdiobus_register(struct oa_tc6 *tc6)
>  {
> +	u32 regval;
>  	int ret;
>  
> +	ret = oa_tc6_read_register(tc6, OA_TC6_REG_STDCAP, &regval);
> +	if (ret)
> +		return ret;
> +
> +	if (!(regval & OA_TC6_STDCAP_DIRECT_PHY_REG_ACCESS))
> +		tc6->mdio_indirect_mode = true;

[Severity: Low]
Should this also check the STDCAP bit for indirect PHY register access
(IPRAC, bit 9 in the OA TC6 STDCAP layout)? include/linux/oa_tc6.h only
defines OA_TC6_STDCAP_DIRECT_PHY_REG_ACCESS (BIT(8)).

The patch also removes oa_tc6_check_phy_reg_direct_access_capability()
from oa_tc6_phy_init(). So a MAC-PHY that advertises neither direct nor
indirect access, and doesn't set OA_TC6_BROKEN_PHY, is now driven through
MDIOACC0/1 anyway. Before this patch it failed with -ENODEV and "Direct
PHY register access is not supported by the MAC-PHY".

On such a device, if TRDONE never sets, each C22 scan read polls for up to
30 ms and returns -ETIMEDOUT. get_phy_c22_id() turns that into -EIO, and
probe fails with the generic "Could not register MDIO bus".

If the unimplemented register reads back with bit 31 set, the DATA field
would be used as PHY ID or register data that no PHY produced.

None of the in-tree users reach this case: lan865x has DPRAC, adin1140
sets OA_TC6_BROKEN_PHY, and ADIN1110 supports indirect access.

Could indirect mode be selected only when IPRAC is set, and the -ENODEV
error kept for when neither bit is set?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109085952.105331-1-iustin.stolniceanu%40analog.com