Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7d07e930-56a7-3517-c560-c10291dbe92e@arinc9.com>
Date:   Sun, 4 Jun 2023 11:33:30 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH net-next 26/30] net: dsa: mt7530: properly set
 MT7530_CPU_PORT
Content-Language: en-US
To:     Vladimir Oltean <olteanv@gmail.com>
Cc:     Sean Wang <sean.wang@mediatek.com>,
        Landen Chao <Landen.Chao@mediatek.com>,
        DENG Qingfang <dqfext@gmail.com>,
        Daniel Golle <daniel@makrotopia.org>,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>,
        Russell King <linux@armlinux.org.uk>,
        Richard van Schagen <richard@routerhints.com>,
        Richard van Schagen <vschagen@cs.com>,
        Frank Wunderlich <frank-w@public-files.de>,
        Bartel Eerdekens <bartel.eerdekens@constell8.be>,
        erkin.bozoglu@xeront.com, mithat.guner@xeront.com,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org
References: <20230522121532.86610-1-arinc.unal@arinc9.com>
 <20230522121532.86610-1-arinc.unal@arinc9.com>
 <20230522121532.86610-27-arinc.unal@arinc9.com>
 <20230522121532.86610-27-arinc.unal@arinc9.com>
 <20230526165548.d6ewov743orxviz3@skbuf>
From:   =?UTF-8?B?QXLEsW7DpyDDnE5BTA==?= <arinc.unal@arinc9.com>
In-Reply-To: <20230526165548.d6ewov743orxviz3@skbuf>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 26.05.2023 19:55, Vladimir Oltean wrote:
> On Mon, May 22, 2023 at 03:15:28PM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.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.

Got it, this is a bug fix for future devicetrees so I will send a 
2-patch patch series to net. First one sets the MT7530_CPU_PORT bit to 
the active CPU port, the other adds the ID_MT7530 check.

> 
>> 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@arinc9.com>
>> Co-developed-by: Vladimir Oltean <olteanv@gmail.com>
>> Signed-off-by: Vladimir Oltean <olteanv@gmail.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.

Will do, thanks.

Arınç