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

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

[Vladimir Oltean]

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@gmail.com>
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---

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?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Arınç ÜNAL]

On 12.06.2023 13:09, Russell King (Oracle) wrote:
> On Mon, Jun 12, 2023 at 09:40:45AM +0300, Arınç ÜNAL wrote:
>> On 11.06.2023 19:04, Russell King (Oracle) wrote:
>>> On Sun, Jun 11, 2023 at 11:15:41AM +0300, Arınç ÜNAL wrote:
>>>> Every bit of the CPU port bitmap for MT7531 and the switch on the MT7988
>>>> SoC represents a CPU port to trap frames to. These switches trap frames to
>>>> the CPU port the user port, which the frames are received from, is affine
>>>> to.
>>>
>>> I think you need to reword that, because at least I went "err what" -
>>> especially the second sentence!
>>
>> Sure, how does this sound:
>>
>> These switches trap frames to the CPU port that is affine to the user port
>> from which the frames are received.
> 
> "... to the inbound user port." I think that's a better way to describe
> "user port from which the frames are received."

Sounds good to me.

> 
> However, I'm still struggling to understand what the overall message for
> this entire commit log actually is.
> 
> The actual affinity of the user ports seems to be not relevant, but this
> commit is more about telling the switch which of its ports are CPU
> ports.

Yes, I also add a comment to explain how frame trapping works. The user 
port - CPU port affinity is only relevant there.

> 
> So, if the problem is that we only end up with a single port set as a
> CPU port when there are multiple, isn't it going to be better to say
> something like:
> 
> "For MT7531 and the switch on MT7988, we are not correctly indicating
> which ports are CPU ports when we have more than one CPU port. In order
> to solve this, we need to set multiple bits in the XYZ register so the
> switch will trap frames to the appropriate CPU port for frames received
> on the inbound user port.

Yes, I'll replace this with the second paragraph.

> 
>>>> Currently, only the bit that corresponds to the first found CPU port is set
>>>> on the bitmap.
>>>
>>> Ok.
>>>
>>>> When multiple CPU ports are being used, frames from the user
>>>> ports affine to the other CPU port which are set to be trapped will be
>>>> dropped as the affine CPU port is not set on the bitmap.
>>>
>>> Hmm. I think this is trying to say:
>>>
>>> "When multiple CPU ports are being used, trapped frames from user ports
>>> not affine to the first CPU port will be dropped we do not set these
>>> ports as being affine to the second CPU port."
>>
>> Yes but it's not the affinity we set here. It's to enable the CPU port for
>> trapping.
> 
> In light of that, is the problem that we only enable one CPU port to
> receive trapped frames from their affine user ports?

Yes.

> 
>>>> Only the MT7531
>>>> switch is affected as there's only one port to be used as a CPU port on the
>>>> switch on the MT7988 SoC.
>>>
>>> Erm, hang on. The previous bit indicated there was a problem when there
>>> are multiple CPU ports, but here you're saying that only one switch is
>>> affected - and that switch has only one CPU port. This at the very least
>>> raises eyebrows, because it's just contradicted the first part
>>> explaining when there's a problem.
>>
>> I meant to say, since I already explained at the start of the patch log that
>> this patch changes the bits of the CPU port bitmap for MT7531 and the switch
>> on the MT7988 SoC, only MT7531 is affected as there's only a single CPU port
>> on the switch on the MT7988 SoC. So the switch on the MT7988 SoC cannot be
>> affected.
> 
> 
> 
>>
>>>
>>>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>>>> index 9bc54e1348cb..8ab4718abb06 100644
>>>> --- a/drivers/net/dsa/mt7530.c
>>>> +++ b/drivers/net/dsa/mt7530.c
>>>> @@ -1010,6 +1010,14 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
>>>>    	if (priv->id == ID_MT7621)
>>>>    		mt7530_rmw(priv, MT7530_MFC, CPU_MASK, CPU_EN | CPU_PORT(port));
>>>> +	/* Add the CPU port to the CPU port bitmap for MT7531 and the switch on
>>>> +	 * the MT7988 SoC. Any frames set for trapping to CPU port will be
>>>> +	 * trapped to the CPU port the user port, which the frames are received
>>>> +	 * from, is affine to.
>>>
>>> Please reword the second sentence.
>>
>> Any frames set for trapping to CPU port will be trapped to the CPU port that
>> is affine to the user port from which the frames are received.
> 
> Too many "port"s. Would:
> 
> "Add this port to the CPU port bitmap for the MT7531 and switch on the
> MT7988. Trapped frames will be sent to the CPU port that is affine to
> the inbound user port."
> 
> explain it better?

Sounds good.

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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@gmail.com>
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
> 
> 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ç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Vladimir Oltean]

On Tue, Jun 13, 2023 at 08:14:35PM +0300, Arınç ÜNAL wrote:
> 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.

Aha, ok. I thought that device trees with CPU port 5 exclusively defined
also exist in the wild. If not, and this patch fixes a theoretical only
issue, then it is net-next material.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Vladimir Oltean]

On Tue, Jun 13, 2023 at 08:18:58PM +0300, Vladimir Oltean wrote:
> On Tue, Jun 13, 2023 at 08:14:35PM +0300, Arınç ÜNAL wrote:
> > 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.
> 
> Aha, ok. I thought that device trees with CPU port 5 exclusively defined
> also exist in the wild. If not, and this patch fixes a theoretical only
> issue, then it is net-next material.

On second thought, compatibility with future device trees is the reason
for this patch set, so that should equally be a reason for keeping this
patch in a "net" series.

If I understand you correctly, port 5 should have worked since commit
c8b8a3c601f2 ("net: dsa: mt7530: permit port 5 to work without port 6 on
MT7621 SoC"), and it did, except for trapping, right?

So how about settling on that as a more modest Fixes: tag, and
explaining clearly in the commit message what's affected?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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 20:24, Vladimir Oltean wrote:
> On Tue, Jun 13, 2023 at 08:18:58PM +0300, Vladimir Oltean wrote:
>> On Tue, Jun 13, 2023 at 08:14:35PM +0300, Arınç ÜNAL wrote:
>>> 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.
>>
>> Aha, ok. I thought that device trees with CPU port 5 exclusively defined
>> also exist in the wild. If not, and this patch fixes a theoretical only
>> issue, then it is net-next material.
> 
> On second thought, compatibility with future device trees is the reason
> for this patch set, so that should equally be a reason for keeping this
> patch in a "net" series.
> 
> If I understand you correctly, port 5 should have worked since commit
> c8b8a3c601f2 ("net: dsa: mt7530: permit port 5 to work without port 6 on
> MT7621 SoC"), and it did, except for trapping, right?

That fixes port 5 on certain variants of the MT7530 switch, as it was 
already working on the other variants, which, in conclusion, fixes port 
5 on all MT7530 variants.

And no, trapping works. Having only CPU port 5 defined on the devicetree 
will cause the CPU_PORT bits to be set to port 5. There's only a problem 
when multiple CPU ports are defined.

> 
> So how about settling on that as a more modest Fixes: tag, and
> explaining clearly in the commit message what's affected?

I don't see anything to change in the patch log except addressing 
Russell's comments.

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Vladimir Oltean]

On Tue, Jun 13, 2023 at 08:30:28PM +0300, Arınç ÜNAL wrote:
> That fixes port 5 on certain variants of the MT7530 switch, as it was
> already working on the other variants, which, in conclusion, fixes port 5 on
> all MT7530 variants.

Ok. I didn't pay enough attention to the commit message.

> And no, trapping works. Having only CPU port 5 defined on the devicetree
> will cause the CPU_PORT bits to be set to port 5. There's only a problem
> when multiple CPU ports are defined.

Got it. Then this is really not a problem, and the commit message frames
it incorrectly.

> > So how about settling on that as a more modest Fixes: tag, and
> > explaining clearly in the commit message what's affected?
> 
> I don't see anything to change in the patch log except addressing Russell's
> comments.

Ok. I see Russell has commented on v4, though I don't see that he particularly
pointed out that this fixes a problem which isn't yet a problem. I got lost in
all the versions. v2 and v3 are out of my inbox now :)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Frank Wunderlich]

Hi

> Gesendet: Dienstag, 13. Juni 2023 um 19:18 Uhr
> Von: "Vladimir Oltean" <olteanv@gmail.com>

> On Tue, Jun 13, 2023 at 08:14:35PM +0300, Arınç ÜNAL wrote:
> > 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.
> 
> Aha, ok. I thought that device trees with CPU port 5 exclusively defined
> also exist in the wild. If not, and this patch fixes a theoretical only
> issue, then it is net-next material.

BananaPi R2Pro (Rockchip rk3568 arm64) has currently port5 only. And there only port5
is connected to the SoC (so port6 is not added there).

Most boards using port6 at the moment.

regards Frank

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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 20:39, Vladimir Oltean wrote:
> On Tue, Jun 13, 2023 at 08:30:28PM +0300, Arınç ÜNAL wrote:
>> That fixes port 5 on certain variants of the MT7530 switch, as it was
>> already working on the other variants, which, in conclusion, fixes port 5 on
>> all MT7530 variants.
> 
> Ok. I didn't pay enough attention to the commit message.
> 
>> And no, trapping works. Having only CPU port 5 defined on the devicetree
>> will cause the CPU_PORT bits to be set to port 5. There's only a problem
>> when multiple CPU ports are defined.
> 
> Got it. Then this is really not a problem, and the commit message frames
> it incorrectly.

Actually this patch fixes the issue it describes. At the state of this 
patch, when multiple CPU ports are defined, port 5 is the active CPU 
port, CPU_PORT bits are set to port 6.

Once "the patch that prefers port 6, I could easily find the exact name 
but your mail snipping makes it hard" is applied, this issue becomes 
redundant.

> 
>>> So how about settling on that as a more modest Fixes: tag, and
>>> explaining clearly in the commit message what's affected?
>>
>> I don't see anything to change in the patch log except addressing Russell's
>> comments.
> 
> Ok. I see Russell has commented on v4, though I don't see that he particularly
> pointed out that this fixes a problem which isn't yet a problem. I got lost in
> all the versions. v2 and v3 are out of my inbox now :)

All good, I had to quickly roll v3 as v2 had wrong author information 
and I couldn't risk getting v2 applied.

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jakub Kicinski]

On Tue, 13 Jun 2023 20:58:33 +0300 Arınç ÜNAL wrote:
> > Ok. I see Russell has commented on v4, though I don't see that he particularly
> > pointed out that this fixes a problem which isn't yet a problem. I got lost in
> > all the versions. v2 and v3 are out of my inbox now :)  
> 
> All good, I had to quickly roll v3 as v2 had wrong author information 
> and I couldn't risk getting v2 applied.

FWIW you can reply with pw-bot: changes-requested to your own patches
and the bot should discard them from patchwork.

https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#updating-patch-status

It's a new capability that nobody has used, yet, so YMMV :)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Russell King (Oracle)]

On Tue, Jun 13, 2023 at 08:39:08PM +0300, Vladimir Oltean wrote:
> Ok. I see Russell has commented on v4, though I don't see that he particularly
> pointed out that this fixes a problem which isn't yet a problem. I got lost in
> all the versions. v2 and v3 are out of my inbox now :)

I didn't really get to that level of detail (which is why I haven't
given any r-b's) - I was more focused on trying to understand what
each commit was doing, and trying to get easier to parse commit
messages.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Russell King (Oracle)]

On Tue, Jun 13, 2023 at 08:58:33PM +0300, Arınç ÜNAL wrote:
> On 13.06.2023 20:39, Vladimir Oltean wrote:
> > On Tue, Jun 13, 2023 at 08:30:28PM +0300, Arınç ÜNAL wrote:
> > > That fixes port 5 on certain variants of the MT7530 switch, as it was
> > > already working on the other variants, which, in conclusion, fixes port 5 on
> > > all MT7530 variants.
> > 
> > Ok. I didn't pay enough attention to the commit message.
> > 
> > > And no, trapping works. Having only CPU port 5 defined on the devicetree
> > > will cause the CPU_PORT bits to be set to port 5. There's only a problem
> > > when multiple CPU ports are defined.
> > 
> > Got it. Then this is really not a problem, and the commit message frames
> > it incorrectly.
> 
> Actually this patch fixes the issue it describes. At the state of this
> patch, when multiple CPU ports are defined, port 5 is the active CPU port,
> CPU_PORT bits are set to port 6.

Maybe it's just me being dumb, but I keep finding things difficult to
understand, such as the above paragraph.

It sounds like you're saying that _before_ this patch, port 5 is the
active CPU port, but the CPU_PORT *FIELD* NOT BITS are set such that
port 6 is the active CPU port. Therefore, things are broken, and this
patch fixes it.

Or are you saying that after this patch is applied, port 5 is the
active CPU port, but the CPU_PORT *FIELD* is set to port 6. If that's
true, then I've no idea what the hell is going on here because it
seems to be senseless.

I think at this point I just give up trying to understand what the
hell these patches are trying to do - in my opinion, the commit
messages are worded attrociously and incomprehensively.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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 21:29, Russell King (Oracle) wrote:
> On Tue, Jun 13, 2023 at 08:58:33PM +0300, Arınç ÜNAL wrote:
>> On 13.06.2023 20:39, Vladimir Oltean wrote:
>>> On Tue, Jun 13, 2023 at 08:30:28PM +0300, Arınç ÜNAL wrote:
>>>> That fixes port 5 on certain variants of the MT7530 switch, as it was
>>>> already working on the other variants, which, in conclusion, fixes port 5 on
>>>> all MT7530 variants.
>>>
>>> Ok. I didn't pay enough attention to the commit message.
>>>
>>>> And no, trapping works. Having only CPU port 5 defined on the devicetree
>>>> will cause the CPU_PORT bits to be set to port 5. There's only a problem
>>>> when multiple CPU ports are defined.
>>>
>>> Got it. Then this is really not a problem, and the commit message frames
>>> it incorrectly.
>>
>> Actually this patch fixes the issue it describes. At the state of this
>> patch, when multiple CPU ports are defined, port 5 is the active CPU port,
>> CPU_PORT bits are set to port 6.
> 
> Maybe it's just me being dumb, but I keep finding things difficult to
> understand, such as the above paragraph.
> 
> It sounds like you're saying that _before_ this patch, port 5 is the
> active CPU port, but the CPU_PORT *FIELD* NOT BITS are set such that
> port 6 is the active CPU port. Therefore, things are broken, and this
> patch fixes it.

Yes, CPU_PORT field, and yes this patch fixes the issue by setting the 
field to 5 when multiple CPU ports are used. Because before this patch, 
the active CPU port is 5. The CPU_PORT field must match this.

> 
> Or are you saying that after this patch is applied, port 5 is the
> active CPU port, but the CPU_PORT *FIELD* is set to port 6. If that's
> true, then I've no idea what the hell is going on here because it
> seems to be senseless.

No, when the "prefer port 6" patch is applied, the active CPU port will 
be port 6.

The CPU_PORT field will always be set to 6, regardless of "net: dsa: 
mt7530: fix trapping frames with multiple CPU ports on MT7530". 
Therefore, the "prefer port 6" patch makes "net: dsa: mt7530: fix 
trapping frames with multiple CPU ports on MT7530" redundant.

"net: dsa: mt7530: fix trapping frames with multiple CPU ports on 
MT7530" becomes important after the changing the DSA conduit support is 
introduced.

> 
> I think at this point I just give up trying to understand what the
> hell these patches are trying to do - in my opinion, the commit
> messages are worded attrociously and incomprehensively.

If that's how you feel, you can tune in to v5 where I will address the 
patch logs.

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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 21:12, Jakub Kicinski wrote:
> On Tue, 13 Jun 2023 20:58:33 +0300 Arınç ÜNAL wrote:
>>> Ok. I see Russell has commented on v4, though I don't see that he particularly
>>> pointed out that this fixes a problem which isn't yet a problem. I got lost in
>>> all the versions. v2 and v3 are out of my inbox now :)
>>
>> All good, I had to quickly roll v3 as v2 had wrong author information
>> and I couldn't risk getting v2 applied.
> 
> FWIW you can reply with pw-bot: changes-requested to your own patches
> and the bot should discard them from patchwork.
> 
> https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#updating-patch-status
> 
> It's a new capability that nobody has used, yet, so YMMV :)

Interesting, I've got some questions regarding this.

> For example to mark a series as Changes Requested one needs to send the following line anywhere in the email thread:
> 
> pw-bot: changes-requested

I suppose a reply to the cover letter or under the cover letter thread 
applies?

> The use of the bot is restricted to authors of the patches (the From: header on patch submission and command must match!)

So, for example, if this patch series was new, Vladimir would reply to 
the patch they're the author of with 'pw-bot: changes-requested', and 
the patchworks would mark the whole patch series as changes requested?

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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 22:03, Arınç ÜNAL wrote:
> On 13.06.2023 21:12, Jakub Kicinski wrote:
>> On Tue, 13 Jun 2023 20:58:33 +0300 Arınç ÜNAL wrote:
>>>> Ok. I see Russell has commented on v4, though I don't see that he 
>>>> particularly
>>>> pointed out that this fixes a problem which isn't yet a problem. I 
>>>> got lost in
>>>> all the versions. v2 and v3 are out of my inbox now :)
>>>
>>> All good, I had to quickly roll v3 as v2 had wrong author information
>>> and I couldn't risk getting v2 applied.
>>
>> FWIW you can reply with pw-bot: changes-requested to your own patches
>> and the bot should discard them from patchwork.
>>
>> https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#updating-patch-status
>>
>> It's a new capability that nobody has used, yet, so YMMV :)
> 
> Interesting, I've got some questions regarding this.
> 
>> For example to mark a series as Changes Requested one needs to send 
>> the following line anywhere in the email thread:
>>
>> pw-bot: changes-requested
> 
> I suppose a reply to the cover letter or under the cover letter thread 
> applies?
> 
>> The use of the bot is restricted to authors of the patches (the From: 
>> header on patch submission and command must match!)
> 
> So, for example, if this patch series was new, Vladimir would reply to 
> the patch they're the author of with 'pw-bot: changes-requested', and 
> the patchworks would mark the whole patch series as changes requested?

Also, replying with 'pw-bot: changes-requested' on an already accepted 
patch series as the author won't change the state from accepted to 
changes requested, correct?

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Vladimir Oltean]

On Tue, Jun 13, 2023 at 08:58:33PM +0300, Arınç ÜNAL wrote:
> On 13.06.2023 20:39, Vladimir Oltean wrote:
> > Got it. Then this is really not a problem, and the commit message frames
> > it incorrectly.
> 
> Actually this patch fixes the issue it describes. At the state of this
> patch, when multiple CPU ports are defined, port 5 is the active CPU port,
> CPU_PORT bits are set to port 6.
> 
> Once "the patch that prefers port 6, I could easily find the exact name but
> your mail snipping makes it hard" is applied, this issue becomes redundant.

Ok. Well, you don't get bonus points for fixing a problem in 2 different
ways, once is enough :)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jakub Kicinski]

On Tue, 13 Jun 2023 22:09:02 +0300 Arınç ÜNAL wrote:
> >> It's a new capability that nobody has used, yet, so YMMV :)  
> > 
> > Interesting, I've got some questions regarding this.
> >   
> >> For example to mark a series as Changes Requested one needs to send 
> >> the following line anywhere in the email thread:
> >>
> >> pw-bot: changes-requested  
> > 
> > I suppose a reply to the cover letter or under the cover letter thread 
> > applies?

Anywhere in the thread, the bot should walk references.

> >> The use of the bot is restricted to authors of the patches (the From: 
> >> header on patch submission and command must match!)  
> > 
> > So, for example, if this patch series was new, Vladimir would reply to 
> > the patch they're the author of with 'pw-bot: changes-requested', and 
> > the patchworks would mark the whole patch series as changes requested?  

Yes.

> Also, replying with 'pw-bot: changes-requested' on an already accepted 
> patch series as the author won't change the state from accepted to 
> changes requested, correct?

Yes, authors can only changes status from active to inactive. Accepted
is an inactive state.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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 23:18, Vladimir Oltean wrote:
> On Tue, Jun 13, 2023 at 08:58:33PM +0300, Arınç ÜNAL wrote:
>> On 13.06.2023 20:39, Vladimir Oltean wrote:
>>> Got it. Then this is really not a problem, and the commit message frames
>>> it incorrectly.
>>
>> Actually this patch fixes the issue it describes. At the state of this
>> patch, when multiple CPU ports are defined, port 5 is the active CPU port,
>> CPU_PORT bits are set to port 6.
>>
>> Once "the patch that prefers port 6, I could easily find the exact name but
>> your mail snipping makes it hard" is applied, this issue becomes redundant.
> 
> Ok. Well, you don't get bonus points for fixing a problem in 2 different
> ways, once is enough :)

This is not the case here though.

This patch fixes an issue that can be stumbled upon in two ways. This is 
for when multiple CPU ports are defined on the devicetree.

As I explained to Russell, the first is the CPU_PORT field not matching 
the active CPU port.

The second is when port 5 becomes the only active CPU port. This can 
only happen with the changing the DSA conduit support.

The "prefer port 6" patch only prevents the first way from happening. 
The latter still can happen. But this feature doesn't exist yet. Hence 
why I think we should apply this series as-is (after some patch log 
changes) and backport it without this patch on kernels older than 5.18.

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Vladimir Oltean]

On Tue, Jun 13, 2023 at 07:29:18PM +0100, Russell King (Oracle) wrote:
> Maybe it's just me being dumb, but I keep finding things difficult to
> understand, such as the above paragraph.
> 
> It sounds like you're saying that _before_ this patch, port 5 is the
> active CPU port, but the CPU_PORT *FIELD* NOT BITS are set such that
> port 6 is the active CPU port. Therefore, things are broken, and this
> patch fixes it.
> 
> Or are you saying that after this patch is applied, port 5 is the
> active CPU port, but the CPU_PORT *FIELD* is set to port 6. If that's
> true, then I've no idea what the hell is going on here because it
> seems to be senseless.

There are 2 distinct patches at play. I'll be showing some tables below.
Their legend is that patch (1) affects only the second column and patch
(2) affects only the third column.

Patch (1): net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7530
----------------------------------------------------------------------------------

What you need to know looking at the current code in net-next is that
mt753x_cpu_port_enable() always overwrites the CPU_MASK field of MT7530_MFC,
because that contains a single port. So when operating on a device tree
with multiple CPU ports defined, only the last CPU port will be recorded
in that register, and this will affect the destination port for
trapped-to-CPU frames.

However, DSA, when operating on a DT with multiple CPU ports, makes the
dp->cpu_dp pointer of all user ports equal to the first CPU port. That
affects which DSA master is automatically brought up when user ports are
brought up. Minor issue, TBH, because it is sufficient for the user to
manually bring up the DSA master corresponding to the second CPU port,
and then trapped frames would be processed just fine. Prior to ~2021/v5.12,
that facility wasn't even a thing - the user always had to bring that
interface up manually.

That means that without patch (1) we have:

CPU ports in the    Trapping CPU port        Default CPU port
device tree         (MT7530_MFC:CPU_MASK)    (all dp->cpu_dp point to it)
-------------------------------------------------------------------------
5                   5                        5
6                   6                        6
5 and 6             6                        5

The semi-problem is that the DSA master of the trapping port (6) is not
automatically brought up by the dsa_slave_open() logic, because no slave
has port 6 (the trapping port) as a master.

All that this patch is doing is that it moves around the trapping CPU
port towards one of the DSA masters that is up, so that the user doesn't
really need to care. The table becomes:

CPU ports in the    Trapping CPU port        Default CPU port
device tree         (MT7530_MFC:CPU_MASK)    (all dp->cpu_dp point to it)
--------------------------------------------------------------------------
5                   5                        5
6                   6                        6
5 and 6             5                        5


Patch (2) net: dsa: introduce preferred_default_local_cpu_port and use on MT7530
--------------------------------------------------------------------------------

This patch influences the choice that DSA makes when it comes to the
dp->cpu_dp assignments of user ports, when fed a device tree with
multiple CPU ports.

The preference of the mt7530 driver is: if port 6 is defined in the DT
as a CPU port, choose that. Otherwise don't care (which implicitly means:
let DSA pick the first CPU port that is defined there, be it 5 or 6).

The effect of this patch taken in isolation is (showing only "after",
because "before" is the same as the "before" of patch 1):

CPU ports in the    Trapping CPU port        Default CPU port
device tree         (MT7530_MFC:CPU_MASK)    (all dp->cpu_dp point to it)
-------------------------------------------------------------------------
5                   5                        5
6                   6                        6
5 and 6             6                        6

As you can see, patch (2) resolves the same semi-problem even in
isolation because it makes the trapping port coincide with the default
CPU port in a different way.

> 
> I think at this point I just give up trying to understand what the
> hell these patches are trying to do - in my opinion, the commit
> messages are worded attrociously and incomprehensively.

+1, could have been better..

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Vladimir Oltean]

On Tue, Jun 13, 2023 at 11:35:08PM +0300, Arınç ÜNAL wrote:
> On 13.06.2023 23:18, Vladimir Oltean wrote:
> > On Tue, Jun 13, 2023 at 08:58:33PM +0300, Arınç ÜNAL wrote:
> > > On 13.06.2023 20:39, Vladimir Oltean wrote:
> > > > Got it. Then this is really not a problem, and the commit message frames
> > > > it incorrectly.
> > > 
> > > Actually this patch fixes the issue it describes. At the state of this
> > > patch, when multiple CPU ports are defined, port 5 is the active CPU port,
> > > CPU_PORT bits are set to port 6.
> > > 
> > > Once "the patch that prefers port 6, I could easily find the exact name but
> > > your mail snipping makes it hard" is applied, this issue becomes redundant.
> > 
> > Ok. Well, you don't get bonus points for fixing a problem in 2 different
> > ways, once is enough :)
> 
> This is not the case here though.
> 
> This patch fixes an issue that can be stumbled upon in two ways. This is for
> when multiple CPU ports are defined on the devicetree.
> 
> As I explained to Russell, the first is the CPU_PORT field not matching the
> active CPU port.
> 
> The second is when port 5 becomes the only active CPU port. This can only
> happen with the changing the DSA conduit support.
> 
> The "prefer port 6" patch only prevents the first way from happening. The
> latter still can happen. But this feature doesn't exist yet. Hence why I
> think we should apply this series as-is (after some patch log changes) and
> backport it without this patch on kernels older than 5.18.
> 
> Arınç

I was following you until the last phrase. Why should we apply this series
as-is [ to net.git ], if this patch fixes a problem (the *only* problem in
lack of .port_change_master() support, aka for stable kernels) that is
already masked by a different patch targeted to net.git?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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 23:59, Vladimir Oltean wrote:
> On Tue, Jun 13, 2023 at 11:35:08PM +0300, Arınç ÜNAL wrote:
>> On 13.06.2023 23:18, Vladimir Oltean wrote:
>>> On Tue, Jun 13, 2023 at 08:58:33PM +0300, Arınç ÜNAL wrote:
>>>> On 13.06.2023 20:39, Vladimir Oltean wrote:
>>>>> Got it. Then this is really not a problem, and the commit message frames
>>>>> it incorrectly.
>>>>
>>>> Actually this patch fixes the issue it describes. At the state of this
>>>> patch, when multiple CPU ports are defined, port 5 is the active CPU port,
>>>> CPU_PORT bits are set to port 6.
>>>>
>>>> Once "the patch that prefers port 6, I could easily find the exact name but
>>>> your mail snipping makes it hard" is applied, this issue becomes redundant.
>>>
>>> Ok. Well, you don't get bonus points for fixing a problem in 2 different
>>> ways, once is enough :)
>>
>> This is not the case here though.
>>
>> This patch fixes an issue that can be stumbled upon in two ways. This is for
>> when multiple CPU ports are defined on the devicetree.
>>
>> As I explained to Russell, the first is the CPU_PORT field not matching the
>> active CPU port.
>>
>> The second is when port 5 becomes the only active CPU port. This can only
>> happen with the changing the DSA conduit support.
>>
>> The "prefer port 6" patch only prevents the first way from happening. The
>> latter still can happen. But this feature doesn't exist yet. Hence why I
>> think we should apply this series as-is (after some patch log changes) and
>> backport it without this patch on kernels older than 5.18.
>>
>> Arınç
> 
> I was following you until the last phrase. Why should we apply this series
> as-is [ to net.git ], if this patch fixes a problem (the *only* problem in
> lack of .port_change_master() support, aka for stable kernels) that is
> already masked by a different patch targeted to net.git?

Because I don't see the latter patch as a fix. It treats the symptom, 
not the cause.

Anyway, I'm fine with taking this patch from this series and put it on 
my series for net-next instead.

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Vladimir Oltean]

On Wed, Jun 14, 2023 at 12:04:10AM +0300, Arınç ÜNAL wrote:
> Because I don't see the latter patch as a fix. It treats the symptom, not
> the cause.
> 
> Anyway, I'm fine with taking this patch from this series and put it on my
> series for net-next instead.

Right, but what seems to have been the case during the net.git
(and linux-stable.git) triage so far is that user impact matters.
A configuration that works by coincidence and not by intention, but
otherwise works reliably, still works, at the end of the day.

If you read the weekly net.git pull requests sent to Linus Torvalds,
you'll see that maintainers try to make a summary of what had to be
changed and why. There isn't really a strong reason why this patch *has*
to be in those pull requests. That's kind of the mindset of what makes
"stable" "stable".

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Arınç ÜNAL]

On 14.06.2023 00:14, Vladimir Oltean wrote:
> On Wed, Jun 14, 2023 at 12:04:10AM +0300, Arınç ÜNAL wrote:
>> Because I don't see the latter patch as a fix. It treats the symptom, not
>> the cause.
>>
>> Anyway, I'm fine with taking this patch from this series and put it on my
>> series for net-next instead.
> 
> Right, but what seems to have been the case during the net.git
> (and linux-stable.git) triage so far is that user impact matters.
> A configuration that works by coincidence and not by intention, but
> otherwise works reliably, still works, at the end of the day.
> 
> If you read the weekly net.git pull requests sent to Linus Torvalds,
> you'll see that maintainers try to make a summary of what had to be
> changed and why. There isn't really a strong reason why this patch *has*
> to be in those pull requests. That's kind of the mindset of what makes
> "stable" "stable".

Makes sense. I have prepared v5 that addresses everything so far, should 
I send it today now that Russell has reviewed v4?

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Vladimir Oltean]

On Wed, Jun 14, 2023 at 10:03:04AM +0300, Arınç ÜNAL wrote:
> Makes sense. I have prepared v5 that addresses everything so far, should I
> send it today now that Russell has reviewed v4?
> 
> Arınç

Let's wait for Russell to ack that all discussions on v2-v4 are closed
and that there aren't any follow-up questions there.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel