Re: [PATCH net-next v2 4/4] net: dsa: soce: Add basic support for SoC-e switch IP cores

[Vasilij Strassheim]

On Wed, 2026-09-09 at 12:12 +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 0 · Medium: 2 · Low: 1
> 
> - [Medium] The MDIO master transaction sequence never verifies that the
>   shared controller is idle before programming PARAMS/WRITE and kicking…
> - [Medium] soce_setup() arms the switch's "tag all frames" and "DSA
>   custom rules tagging" bits, but the driver provides no .teardown…
> - [Low] The compatible string and its binding description declare a
>   register layout with "the core version register at offset 0x00 and…
> 
> --
> 
> [ ... ]
> 
> > +struct soce_probe_desc {
> > +	u32 core_version_offset;
> > +	u32 licensed_features_offset;
> > +	u32 implemented_features0_offset;
> > +	u32 mdio_master_offset;
> > +};
> > +
> > +static const struct soce_probe_desc soce_probe_desc_swip_00_04_0c_10 = {
> > +	.core_version_offset		= 0x0000,
> > +	.licensed_features_offset	= 0x0004,
> > +	.implemented_features0_offset	= 0x000c,
> > +	.mdio_master_offset		= 0x0200,
> > +};
> 
> [Severity: Low]
> The compatible name soce,swip-00-04-0c-10 and the binding text describe a
> register layout with "the core version register at offset 0x00 and feature
> identification registers at offsets 0x04, 0x0c, and 0x10", but this
> descriptor only models 0x00, 0x04 and 0x0c.
> 
> Is the 0x10 register meant to be read somewhere?  soce_sw_detect_features()
> reads only licensed_features_offset and implemented_features0_offset:
> 
> 	regval = readl(local->base_addr + probe_desc->licensed_features_offset);
> 	...
> 	regval = readl(local->base_addr +
> 		       probe_desc->implemented_features0_offset);
> 
> The "features0" suffix also suggests a features1 register at 0x10 that is
> currently ignored.
> 
> Going the other way, mdio_master_offset = 0x0200 is a register block the
> driver depends on for every MDIO transaction, but the binding does not
> mention it at all.  Should the binding describe that block too, or should
> the offset list in the compatible description match what the driver
> actually models?

I will remove this abstraction since only one switch register layout is
currently supported. The MDIO master offset will be removed as well,
because the MDIO controller will be represented by separate child
devices with their own compatibles.

The offsets encoded in soce,swip-00-04-0c-10 describe the complete
switch identification register layout. The current minimal DSA driver
only reads the registers and feature fields it needs, so it does not
access the implemented-features register at 0x10 yet. I would prefer to
retain this information for future compatible driver extensions that
may need that register.

> 
> [ ... ]
> 
> > +static void soce_sw_enable_tagging(struct soce_dsa_local *local)
> > +{
> > +	u32 regval;
> > +
> > +	regval = readl(local->base_addr + SOCE_TAG_ALL_FRAMES_CTRL_OFFSET);
> > +	regval |= SOCE_TAG_ALL_FRAMES_ENABLE;
> > +	writel(regval, local->base_addr + SOCE_TAG_ALL_FRAMES_CTRL_OFFSET);
> > +
> > +	regval = readl(local->base_addr + SOCE_CUSTOM_RULES_TAGGING_OFFSET);
> > +	regval |= SOCE_CUSTOM_RULES_TAGGING_ENABLE;
> > +	writel(regval, local->base_addr + SOCE_CUSTOM_RULES_TAGGING_OFFSET);
> > +}
> > +
> > +static int soce_setup(struct dsa_switch *ds)
> > +{
> > +	struct soce_priv *priv = ds->priv;
> > +
> > +	soce_sw_enable_tagging(&priv->local);
> > +
> > +	return 0;
> > +}
> 
> [ ... ]
> 
> > +static const struct dsa_switch_ops soce_switch_ops = {
> > +	.get_tag_protocol	= soce_get_tag_protocol,
> > +	.setup			= soce_setup,
> > +	.phylink_get_caps	= soce_phylink_get_caps,
> > +};
> 
> [Severity: Medium]
> Should there be a .teardown counterpart that clears what soce_setup()
> armed?
> 
> soce_sw_enable_tagging() sets SOCE_TAG_ALL_FRAMES_ENABLE and
> SOCE_CUSTOM_RULES_TAGGING_ENABLE, and nothing in the driver ever clears
> them again.  There is no .teardown in soce_switch_ops, and the platform
> callbacks only unregister:
> 
> soce_sw_remove()
> 	dsa_unregister_switch(priv->ds);
> 
> soce_sw_shutdown()
> 	dsa_switch_shutdown(priv->ds);
> 
> After an unbind or rmmod, does the core keep inserting SDSA source-port
> tags into every frame delivered to the CPU port, with the tagger that
> would strip them gone?  As described in the commit message, the same bits
> also keep the user ports in CPU-directed operation:
> 
>     "Enabling this tagging feature of the switch, places all user ports
>     into CPU-directed operation"
> 
> so the switch would also stay locked out of normal port-to-port
> forwarding after the driver is removed.

Yes, I had not considered this use case. The tagging state currently
remains enabled after unbind or shutdown. In v3, I will add a .teardown
callback that clears both SOCE_TAG_ALL_FRAMES_ENABLE and
SOCE_CUSTOM_RULES_TAGGING_ENABLE, returning the switch to an unmanaged
state.

> 
> > +static void soce_sw_remove(struct platform_device *pdev)
> > +{
> > +	struct soce_priv *priv = platform_get_drvdata(pdev);
> > +
> > +	if (!priv)
> > +		return;
> > +
> > +	dsa_unregister_switch(priv->ds);
> > +	platform_set_drvdata(pdev, NULL);
> > +}
> 
> [ ... ]
> 
> > diff --git a/drivers/net/dsa/soce/soce_mdio.c b/drivers/net/dsa/soce/soce_mdio.c
> > new file mode 100644
> > index 0000000000000..692dcbe57d1a6
> > --- /dev/null
> > +++ b/drivers/net/dsa/soce/soce_mdio.c
> 
> [ ... ]
> 
> > +static int soce_mdio_read(struct mii_bus *bus, int addr, int reg)
> > +{
> 
> [ ... ]
> 
> > +	mutex_lock(&local->mdio_lock);
> > +	ret = local->mdio_ops->phy_read(state->ds, state->mdio_output, addr,
> > +					reg);
> > +	mutex_unlock(&local->mdio_lock);
> > +
> > +	return ret;
> > +}
> 
> [ ... ]
> 
> > +static int soce_mdio_23_02_wait_for_idle(struct dsa_switch *ds)
> > +{
> > +	struct soce_priv *priv = ds->priv;
> > +	struct soce_dsa_local *local;
> > +	void __iomem *ctrl;
> > +	u32 val;
> > +
> > +	local = &priv->local;
> > +	ctrl = local->mdio_master_addr + SOCE_MDIO_CTRL_OFFSET;
> > +
> > +	return readl_poll_timeout(ctrl, val,
> > +		!(val & (0x1 << SOCE_MDIO_23_02_CTRL_OPSTATUS_OFFSET)), 10,
> > +		SOCE_MDIO_TIMEOUT * 1000);
> > +}
> 
> [Severity: Medium]
> What happens to the hardware state when this poll times out?
> 
> On -ETIMEDOUT the OPSTATUS bit is still set, meaning the operation is
> still in flight, and the callers just propagate the error without aborting
> or resetting the master:
> 
> 	ret = soce_mdio_23_02_wait_for_idle(ds);
> 	if (ret)
> 		return ret;
> 
> The bus wrappers soce_mdio_read(), soce_mdio_write(), soce_mdio_read_c45()
> and soce_mdio_write_c45() then drop mdio_lock unconditionally, so the
> software lock protecting the shared master is released while the hardware
> is still busy.
> 
> None of the transaction helpers check for idle before programming the
> registers either.  In soce_mdio_23_02_read_c22():
> 
> 	regvalue = (regnum << SOCE_MDIO_23_02_CTRL_REGADDRDEVTYPE_OFFSET) +
> 		   (phy_addr << SOCE_MDIO_23_02_CTRL_PHYADDR_OFFSET);
> 	writel(regvalue, params);
> 
> 	regvalue = ((mdio_output << SOCE_MDIO_23_02_CTRL_BUS_OFFSET) + ...
> 
> Can the next transaction, possibly issued through a different logical bus
> sharing the same master, reprogram PARAMS/WRITE and kick CTRL underneath
> a still-running operation?
> 

Good point. The current implementation releases the software lock after
a timeout, allowing a subsequent transaction to overwrite the parameter
registers while the controller may still be busy.

The hardware documentation exposes no abort or reset mechanism. In v3, I
will therefore check that the controller is idle before programming any
transaction register. If OPSTATUS remains set, subsequent accesses will
time out without modifying the controller state or starting another
transaction.

> > +int soce_mdio_23_02_read_c45(struct dsa_switch *ds, int mdio_output,
> > +			     int phy_addr, int devad, int regnum)
> > +{
> 
> [ ... ]
> 
> > +	regvalue = (devad << SOCE_MDIO_23_02_CTRL_REGADDRDEVTYPE_OFFSET) +
> > +		   (phy_addr << SOCE_MDIO_23_02_CTRL_PHYADDR_OFFSET);
> > +	writel(regvalue, params);
> > +
> > +	writel(regnum, write_reg);
> > +
> > +	/* address cycle */
> > +	regvalue = ((mdio_output << SOCE_MDIO_23_02_CTRL_BUS_OFFSET) +
> > +		    (0x0 << SOCE_MDIO_23_02_CTRL_TRANSTYPE_OFFSET) +
> > +		    (0x1 << SOCE_MDIO_23_02_CTRL_CLAUSE_OFFSET) +
> > +		    (0x1 << SOCE_MDIO_23_02_CTRL_OPSTATUS_OFFSET));
> > +	writel(regvalue, ctrl);
> > +
> > +	ret = soce_mdio_23_02_wait_for_idle(ds);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* read cycle */
> 
> [Severity: Medium]
> For Clause 45 the address cycle and the data cycle are separate hardware
> operations.  If the address cycle times out here and is never aborted, can
> a later data cycle land on a stale address, and can the following
> 
> 	return readl(read_reg) & 0xffff;
> 
> hand the previous transaction's result back to phylib as a valid register
> value?
> 
> Would it make sense to poll for idle before programming PARAMS/WRITE, and
> to clear or reset the master on the timeout path before releasing
> mdio_lock?  The same applies to soce_mdio_23_02_write_c22() and
> soce_mdio_23_02_write_c45(), where a corrupted transaction would write to
> an unintended PHY register.
> 

Yes, this will be addressed in v3. All transaction paths will poll for
idle before modifying PARAMS or WRITE. A failed Clause 45 address cycle
will return immediately, without starting the data cycle or returning
stale read data. Subsequent accesses will leave the controller untouched
while OPSTATUS remains set. The hardware documentation does not describe
an abort or reset mechanism, so recovery is not available.

> [ ... ]
>