Message-ID: <4acff981-c6a9-ac10-d6e5-888386b418ed@arinc9.com>
Date: Tue, 13 Jun 2023 20:07:59 +0300
From: Arınç ÜNAL <arinc.unal@...nc9.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Daniel Golle <daniel@...rotopia.org>,
 Landen Chao <Landen.Chao@...iatek.com>, DENG Qingfang <dqfext@...il.com>,
 Sean Wang <sean.wang@...iatek.com>, Andrew Lunn <andrew@...n.ch>,
 Florian Fainelli <f.fainelli@...il.com>, Vladimir Oltean
 <olteanv@...il.com>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Matthias Brugger <matthias.bgg@...il.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 Frank Wunderlich <frank-w@...lic-files.de>,
 Bartel Eerdekens <bartel.eerdekens@...stell8.be>, mithat.guner@...ont.com,
 erkin.bozoglu@...ont.com, linux-kernel@...r.kernel.org,
 netdev@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH net v2 1/7] net: dsa: mt7530: fix trapping frames with
 multiple CPU ports on MT7531

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ç

Powered by blists - more mailing lists