Re: [PATCH net v2 2/7] net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7530

[Arınç ÜNAL]

On 13.06.2023 18:08, Vladimir Oltean wrote:
> On Sun, Jun 11, 2023 at 11:15:42AM +0300, Arınç ÜNAL wrote:
> > The CPU_PORT bits represent the CPU port to trap frames to for the MT7530
> > switch. This switch traps frames to the CPU port set on the CPU_PORT bits,
> > regardless of the affinity of the user port which the frames are received
> > from.
> >
> > When multiple CPU ports are being used, the trapped frames won't be
> > received when the DSA conduit interface, which the frames are supposed to
> > be trapped to, is down because it's not affine to any user port. This
> > requires the DSA conduit interface to be manually set up for the trapped
> > frames to be received.
> >
> > To fix this, implement ds->ops->master_state_change() on this subdriver and
> > set the CPU_PORT bits to the CPU port which the DSA conduit interface its
> > affine to is up. Introduce the active_cpu_ports field to store the
> > information of the active CPU ports. Correct the macros, CPU_PORT is bits 4
> > through 6 of the register.
> >
> > Add comments to explain frame trapping for this switch.
> >
> > Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> > Suggested-by: Vladimir Oltean <olteanv@xxxxxxxxx>
> > Signed-off-by: Arınç ÜNAL <arinc.unal@xxxxxxxxxx>
> > ---
>
> My only concern with this patch is that it depends upon functionality
> that was introduced in kernel v5.18 - commit 295ab96f478d ("net: dsa:
> provide switch operations for tracking the master state"). But otherwise
> it is correct, does not require subsequent net-next rework, and
> relatively clean, at least I think it's cleaner than checking which of
> the multiple CPU ports is the active CPU port - the other will have no
> user port dp->cpu_dp pointing to it. But strictly, the master_state_change()
> logic is not needed when you can't change the CPU port assignment.
>
> It might also be that your patch "net: dsa: introduce
> preferred_default_local_cpu_port and use on MT7530" gets backported
> to stable kernels that this patch doesn't get backported to, and then,
> we have a problem, because that will cause even more breakage.
>
> I wonder if there's a way to specify a dependency from this to that
> other patch, to ensure that at least that does not happen?

Actually, having only "net: dsa: introduce 
preferred_default_local_cpu_port and use on MT7530" backported is an 
enough solution for the current stable kernels.

When multiple CPU ports are defined on the devicetree, the CPU_PORT bits 
will be set to port 6. The active CPU port will also be port 6.

This would only become an issue with the changing the DSA conduit 
support. But that's never going to happen as this patch will always be 
on the kernels that support changing the DSA conduit.

Arınç