| 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
| ||
|
Message-ID: <10e43c4f-8c96-32e7-6721-b17b7c12bc9a@gmail.com>
Date: Mon, 6 Sep 2021 08:11:55 +0200
From: Rafał Miłecki <zajec5@...il.com>
To: Florian Fainelli <f.fainelli@...il.com>,
Andrew Lunn <andrew@...n.ch>,
Vivien Didelot <vivien.didelot@...il.com>,
Vladimir Oltean <olteanv@...il.com>,
"David S . Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org,
Rafał Miłecki <rafal@...ecki.pl>
Subject: Re: [PATCH] net: dsa: b53: Fix IMP port setup on BCM5301x
On 05.09.2021 23:16, Florian Fainelli wrote:
> On 9/5/2021 10:23 AM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@...ecki.pl>
>>
>> Broadcom's b53 switches have one IMP (Inband Management Port) that needs
>> to be programmed using its own designed register. IMP port may be
>> different than CPU port - especially on devices with multiple CPU ports.
>
> There are two choices: port 5 or port 8,
Yes. Depending on model I assign 5 or 8 in the b53_chip_data. What did
I miss?
>> For that reason it's required to explicitly note IMP port index and
>> check for it when choosing a register to use.
>>
>> This commit fixes BCM5301x support. Those switches use CPU port 5 while
>> their IMP port is 8. Before this patch b53 was trying to program port 5
>> with B53_PORT_OVERRIDE_CTRL instead of B53_GMII_PORT_OVERRIDE_CTRL(5).
>>
>> It may be possible to also replace "cpu_port" usages with
>> dsa_is_cpu_port() but that is out of the scope of thix BCM5301x fix.
>
> Actually this would have been well within the scope of this patch.
I guess it's a matter of taste, I prefer to remove "cpu_port" usage
piece by piece. I think it makes it easier to catch mistakes during
review and regression finding easier.
For example I'm not exactly sure how to get rid of "cpu_port" in the:
if (port != dev->cpu_port) {
b53_force_port_config(dev, dev->cpu_port, 2000,
DUPLEX_FULL, true, true);
b53_force_link(dev, dev->cpu_port, 1);
}
>> Fixes: 967dd82ffc52 ("net: dsa: b53: Add support for Broadcom RoboSwitch")
>> Signed-off-by: Rafał Miłecki <rafal@...ecki.pl>
>
> I really don't like the duplication of the "imp_port" and "cpu_port" members, first because this caused us problems before, and second because for all switch entries except the BCM5301X, cpu_port == imp_port, so this a duplication, and a waste of storage space to encode information.
Well, this isn't exactly a duplication as values differ for BCM5301X.
I guess you prefer to handle BCM5301X with extra conditions in code
while I thought it to be cleaner to store that chip data in a struct.
Let's discuss code changes and see how it could be handled differently.
> In fact, there is no such thing as CPU port technically you chose either IMP0 or IMP1. IMP0 is port 8 and IMP1 is port 5.
>
>> ---
>> drivers/net/dsa/b53/b53_common.c | 28 +++++++++++++++++++++++++---
>> drivers/net/dsa/b53/b53_priv.h | 1 +
>> 2 files changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
>> index 5646eb8afe38..604f54112665 100644
>> --- a/drivers/net/dsa/b53/b53_common.c
>> +++ b/drivers/net/dsa/b53/b53_common.c
>> @@ -1144,7 +1144,7 @@ static void b53_force_link(struct b53_device *dev, int port, int link)
>> u8 reg, val, off;
>> /* Override the port settings */
>> - if (port == dev->cpu_port) {
>> + if (port == dev->imp_port) {
>
> This should be port == 8
What about devices that have IMP port 5? I think that change would break
b53_force_link() for them.
>> off = B53_PORT_OVERRIDE_CTRL;
>> val = PORT_OVERRIDE_EN;
>> } else {
>> @@ -1168,7 +1168,7 @@ static void b53_force_port_config(struct b53_device *dev, int port,
>> u8 reg, val, off;
>> /* Override the port settings */
>> - if (port == dev->cpu_port) {
>> + if (port == dev->imp_port) {
>
> Likewise
Same question.
>> off = B53_PORT_OVERRIDE_CTRL;
>> val = PORT_OVERRIDE_EN;
>> } else {
>> @@ -1236,7 +1236,7 @@ static void b53_adjust_link(struct dsa_switch *ds, int port,
>> b53_force_link(dev, port, phydev->link);
>> if (is531x5(dev) && phy_interface_is_rgmii(phydev)) {
>> - if (port == 8)
>> + if (port == dev->imp_port)
>
> That use of port 8 was correct.
I tried to avoid some magic.
>> off = B53_RGMII_CTRL_IMP;
>> else
>> off = B53_RGMII_CTRL_P(port);
>> @@ -2280,6 +2280,7 @@ struct b53_chip_data {
>> const char *dev_name;
>> u16 vlans;
>> u16 enabled_ports;
>> + u8 imp_port;
>> u8 cpu_port;
>> u8 vta_regs[3];
>> u8 arl_bins;
>> @@ -2304,6 +2305,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
>> .enabled_ports = 0x1f,
>> .arl_bins = 2,
>> .arl_buckets = 1024,
>> + .imp_port = 5,
>
> Could have used B53_CPU_PORT_25 here.
I didn't use B53_CPU_PORT* defines as they don't apply anymore. That _25
suffix made sense when support for first devices with CPU/IMP port 5 was
added. They were actually BCM*25 chipsets.
I think we should probably have something like B53_IMP0 and B53_IMP1.
What do you think about proposed names?
>> .cpu_port = B53_CPU_PORT_25,
>> .duplex_reg = B53_DUPLEX_STAT_FE,
>> },
>> @@ -2314,6 +2316,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
>> .enabled_ports = 0x1f,
>> .arl_bins = 2,
>> .arl_buckets = 1024,
>> + .imp_port = 5,
>> .cpu_port = B53_CPU_PORT_25,
>
> and here.
>
>> .duplex_reg = B53_DUPLEX_STAT_FE,
>> },
>> @@ -2324,6 +2327,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
>> .enabled_ports = 0x1f,
>> .arl_bins = 4,
>> .arl_buckets = 1024,
>> + .imp_port = 8,
>> .cpu_port = B53_CPU_PORT,
>
> and B53_CPU_PORT here and for each entry below.
>
> I will put this patch into my local test rack and see what breaks, and we can address this more cleanly with net-next. Another case where if we had more time to do a proper review we could come up with a small fix, and not create additional technical debt to fix in the next release cycle. Hope's spring is eternal, oh and I just came back from France, so I guess I am full of complaints, too :)
You should have visit Disneyland! ;)
Powered by blists - more mailing lists