linux-kernel - Re: [PATCH v2 4/5] arm: dts: mt7623: mux phy0 on Bananapi BPI-R2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <B3729137-7B3E-4B74-8E9E-E1774F698FAA@public-files.de>
Date:   Fri, 03 Feb 2023 19:54:35 +0100
From:   Frank Wunderlich <frank-w@...lic-files.de>
To:     Arınç ÜNAL <arinc.unal@...nc9.com>,
        arinc9.unal@...il.com, Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Matthias Brugger <matthias.bgg@...il.com>
CC:     devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
        erkin.bozoglu@...ont.com, Sean Wang <sean.wang@...iatek.com>,
        DENG Qingfang <dqfext@...il.com>
Subject: Re: [PATCH v2 4/5] arm: dts: mt7623: mux phy0 on Bananapi BPI-R2

Am 3. Februar 2023 19:20:48 MEZ schrieb "Arınç ÜNAL" <arinc.unal@...nc9.com>:
>On 3.02.2023 18:36, Frank Wunderlich wrote:
>> Am 1. Februar 2023 19:56:55 MEZ schrieb arinc9.unal@...il.com:
>>> From: Arınç ÜNAL <arinc.unal@...nc9.com>
>>> 
>>> Mux the MT7530 switch's phy0 to gmac5 which is wired to the SoC's gmac1.
>>> This achieves 2 Gbps total bandwidth to the CPU using the second RGMII.
>>> 
>>> With this, the interface name to access phy0 changes from wan to eth1.
>>> 
>>> Signed-off-by: Arınç ÜNAL <arinc.unal@...nc9.com>
>>> ---
>>> arch/arm/boot/dts/mt7623n-bananapi-bpi-r2.dts | 15 ++++++++++-----
>>> 1 file changed, 10 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/arch/arm/boot/dts/mt7623n-bananapi-bpi-r2.dts b/arch/arm/boot/dts/mt7623n-bananapi-bpi-r2.dts
>>> index dc9b4f99eb8b..64700253fd35 100644
>>> --- a/arch/arm/boot/dts/mt7623n-bananapi-bpi-r2.dts
>>> +++ b/arch/arm/boot/dts/mt7623n-bananapi-bpi-r2.dts
>>> @@ -182,6 +182,12 @@ fixed-link {
>>> 	};
>>> };
>>> 
>>> +&gmac1 {
>>> +	status = "okay";
>>> +	phy-mode = "rgmii";
>>> +	phy-handle = <&ethphy0>;
>>> +};
>>> +
>>> &eth {
>>> 	status = "okay";
>>> 
>>> @@ -189,6 +195,10 @@ mdio-bus {
>>> 		#address-cells = <1>;
>>> 		#size-cells = <0>;
>>> 
>>> +		ethphy0: ethernet-phy@0 {
>>> +			reg = <0>;
>>> +		};
>>> +
>>> 		switch@1f {
>>> 			compatible = "mediatek,mt7530";
>>> 			reg = <0x1f>;
>>> @@ -200,11 +210,6 @@ ports {
>>> 				#address-cells = <1>;
>>> 				#size-cells = <0>;
>>> 
>>> -				port@0 {
>>> -					reg = <0>;
>>> -					label = "wan";
>>> -				};
>>> -
>>> 				port@1 {
>>> 					reg = <1>;
>>> 					label = "lan0";
>> 
>> Hi
>> 
>> I still see Problem with "renaming" the wan from users PoV. I got another way of using second gmac for wan some time ago using vlan-aware bridge (have not tested with recent kernel versions).
>> 
>> Maybe this works for you too? If yes imho it will be a better way.
>> 
>> https://github.com/frank-w/BPI-Router-Linux/commit/c92b648bac996b34dc75a4fff15d7fb429bfe74b
>
>Frank, the comment section of that page is full of my comments testing it out and chatting with you. I don't understand why you're wording it like it's new to me.

Not new,but you have not told me the issues you have with this way.

>> 
>> Have same for r64/mt7622 in my tree...
>> 
>> It should use eth1 for wan-traffic too but is full userspace configuration without breaking userspace for users not wanting it.
>
>If your argument is that connecting the wan port to the second mac should stay off mainline and to be left to the individual to do so, to not break the existing userspace configuration, this hack will need much more complex changes to the userspace configuration compared to this patch.

Yes,userspace config is more complex,but till now i thought both ways had same result except that user can decide if he wants interface name changed. I try to find some time to test your approach this weekend.

>On top of this, you still need to change the devicetree to enable gmac1. And it will cause issues due to the nature of this method. Frames with the same MAC address will appear on different interfaces which will flood the kernel log, if eth1 were to be put in a bridge with other interfaces.

This information is new to me. Had only made some basic tests with the way in my repo and did not noticed this flooding.

>To summarise:
>					This patch	Your method
>Changes to the devicetree		Yes	(-)	Yes	(-)
>Changes to the userspace configuration	Yes	(-)	Yes	(-)
>Changes required in userspace		Simple	(+)	Complex	(-)
>Does it work properly?			Yes	(+)	No	(-)
>
>Using this patch would be the proper way to connect the wan port to the second mac.
>
>I can also argue that I see no good reason to not want this, therefore this should be the default way. The UTP port for the wan port is seperated from the other 4 ports which already supports that this should've been there when the DT of this device was added in the first place.

There was a hope that dsa will support multiple cpu ports some time and there are several attempts to get it into mainline.

>If someone were to not want this, they could change the devicetree to fit their own purpose.
>
>To conclude, in my opinion, gaining 2 Gbps total bandwidth to the CPU at the expense of a tiny change in userspace configuration is absolutely worth it. You clearly don't think that way and that's fine. It's up to the maintainer, Matthias, to decide. Matthias can take the remaining patches if they please.

I appreciate you efforts to getting better performance here. Idk how many users will benefit from it...i have only 25mbit/s connection and can configure my dt/userspace...only thought about users not able to do so and may confused by this and do not "need" the change.

>Arınç


regards Frank