Re: [PATCH net-next v2 08/10] net: dsa: realtek: rtl8365mb: program the PCP of untagged frames

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

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] rtl8365mb_qos_set_port_prio()
  (drivers/net/dsa/realtek/rtl8365mb_qos.c) now does two separate…

--

net: dsa: realtek: rtl8365mb: program the PCP of untagged frames

With this patch, setting the port default priority also programs the PCP
the switch gives to untagged frames. Under PCP trust, that value decides
how untagged frames are classified. It is also what the switch writes into
the tag it adds on egress.

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_qos.c b/drivers/net/dsa/realtek/rtl8365mb_qos.c
> index 378cd6ed7fe64..2a849d0e4898a 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_qos.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_qos.c

[ ... ]

> @@ -192,6 +201,13 @@ int rtl8365mb_qos_set_port_prio(struct realtek_priv *priv, int port, u8 iprio)
>  	if (ret)
>  		return ret;
>  
> +	ret = rtl8365mb_qos_regmap_update_field(priv,
> +						RTL8365MB_QOS_PORT_UNTAG_PCP_REG(port),
> +						rtl8365mb_qos_sel_mask(port),
> +						untag_pcp);
> +	if (ret)
> +		return ret;
> +
>  	return rtl8365mb_qos_regmap_update_field(priv,
>  						 RTL8365MB_QOS_PORT_PRI_REG(port),
>  						 rtl8365mb_qos_sel_mask(port),

[Severity: Medium]
What happens to the untagged PCP field if the second write fails?

In rtl8365mb_qos_set_port_prio(), the UNTAG_PCP write goes to the hardware
before the PORT_PRI write. If the PORT_PRI regmap_update_bits() then fails,
for example with an SMI ACK timeout from realtek_smi_wait_for_ack() or a
bus->write() error in realtek_mdio_write(), the error is returned. The
untagged PCP is not restored.

On that error, the DSA core rolls back the software state:

net/dsa/user.c:dsa_user_dcbnl_set_default_prio() {
    ...
	err = ds->ops->port_set_default_prio(ds, port, new_prio);
	if (err) {
		dcb_ieee_delapp(dev, app);
		return err;
	}
    ...
}

dsa_user_dcbnl_del_default_prio() does the same with dcb_ieee_setapp().

After that, dcbnl reports the old default priority. So does
rtl83xx_port_get_default_prio(), because rtl8365mb_qos_get_port_prio()
reads back only RTL8365MB_QOS_PORT_PRI_REG. The switch, however, keeps
using the new untagged PCP: it tags untagged frames on egress with it and,
with PCP trust enabled, classifies them by it.

Could this leave untagged frames tagged and classified by a priority whose
request failed, with no way for software to see it, until a later set
succeeds?

Elsewhere the series avoids this kind of state. For example,
rtl8365mb_qos_set_apptrust() batches its writes "so a failed write cannot
leave the trusted table half programmed". Would it make sense to restore
the previous untagged PCP when the PORT_PRI write fails, or to order the
writes so that a failure cannot leave the two fields out of step?

The code is still the same at the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261112-rtl8365mb-dcb-ingress-v2-0-f8e6fbdf8c99%40pengutronix.de