Message-ID: <20230526165548.d6ewov743orxviz3@skbuf>
Date: Fri, 26 May 2023 19:55:48 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: arinc9.unal@...il.com
Cc: Sean Wang <sean.wang@...iatek.com>,
	Landen Chao <Landen.Chao@...iatek.com>,
	DENG Qingfang <dqfext@...il.com>,
	Daniel Golle <daniel@...rotopia.org>, Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...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>,
	Russell King <linux@...linux.org.uk>,
	Arınç ÜNAL <arinc.unal@...nc9.com>,
	Richard van Schagen <richard@...terhints.com>,
	Richard van Schagen <vschagen@...com>,
	Frank Wunderlich <frank-w@...lic-files.de>,
	Bartel Eerdekens <bartel.eerdekens@...stell8.be>,
	erkin.bozoglu@...ont.com, mithat.guner@...ont.com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH net-next 26/30] net: dsa: mt7530: properly set
 MT7530_CPU_PORT

On Mon, May 22, 2023 at 03:15:28PM +0300, arinc9.unal@...il.com wrote:
> From: Arınç ÜNAL <arinc.unal@...nc9.com>
> 
> The MT7530_CPU_PORT bits represent the CPU port to trap frames to for the
> MT7530 switch. There are two issues with the current way of setting these
> bits. ID_MT7530 which is for the standalone MT7530 switch is not included.

It's best to say in the commit title what the change does, rather than
the equivalent of "here, this way is proper!". Commit titles should be
uniquely identifiable, and "properly set MT7530_CPU_PORT" doesn't say a
lot about how proper it is. It's enough to imagine a future person
finding something else that's perfectible and writing another "net: dsa:
mt7530: properly set MT7530_CPU_PORT" commit. Try to be less definitive
and at the same time more specific.

If there are 2 issues, there should be 2 changes with individual titles
which each describes what was wrong and how that was changed.

> 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.
> 
> Address these issues by implementing ds->ops->master_state_change() on this
> subdriver and setting the MT7530_CPU_PORT bits there. Introduce the
> active_cpu_ports field to store the information of active CPU ports.
> Correct the macros, MT7530_CPU_PORT is bits 4 through 6 of the register.
> 
> Any frames set for trapping to CPU port will be trapped to the numerically
> smallest CPU port which is affine to the DSA conduit interface that is set
> up. To make the understatement obvious, the frames won't necessarily be
> trapped to the CPU port the user port, which these frames are received
> from, is affine to. This operation is only there to make sure the trapped
> frames always reach the CPU.
> 
> Tested-by: Arınç ÜNAL <arinc.unal@...nc9.com>
> Co-developed-by: Vladimir Oltean <olteanv@...il.com>
> Signed-off-by: Vladimir Oltean <olteanv@...il.com>

A single Suggested-by: is fine. As a rule of thumb, I would use Co-developed-by
when I'm working with a patch formally pre-formatted or committed by somebody else,
that I've changed in a significant manner. Since all I did was to comment with
a suggestion of how to handle this, and with a code snippet written in the email
client to a patch of yours, I don't believe that's necessary here.

> Signed-off-by: Arınç ÜNAL <arinc.unal@...nc9.com>
> ---

Powered by blists - more mailing lists