netdev - Re: [BUG] mv88e6xxx: tx regression in v5.3

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: <20191217182643.augknhx57pafnelv@sapphire.tkos.co.il>
Date:   Tue, 17 Dec 2019 20:26:43 +0200
From:   Baruch Siach <baruch@...s.co.il>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Marek Behun <marek.behun@....cz>,
        Vivien Didelot <vivien.didelot@...il.com>,
        netdev@...r.kernel.org,
        Denis Odintsov <d.odintsov@...viangames.com>,
        Hubert Feurstein <h.feurstein@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>
Subject: Re: [BUG] mv88e6xxx: tx regression in v5.3

Hi Andrew,

On Sun, Dec 15, 2019 at 05:14:23PM +0100, Andrew Lunn wrote:
> On Sun, Dec 15, 2019 at 05:08:01PM +0200, Baruch Siach wrote:
> > On Sun, Dec 15 2019, Andrew Lunn wrote:
> > >> This fixes cpu port configuration for me:
> > >>
> > >> diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
> > >> index 7fe256c5739d..a6c320978bcf 100644
> > >> --- a/drivers/net/dsa/mv88e6xxx/port.c
> > >> +++ b/drivers/net/dsa/mv88e6xxx/port.c
> > >> @@ -427,10 +427,6 @@ static int mv88e6xxx_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
> > >>  		cmode = 0;
> > >>  	}
> > >>
> > >> -	/* cmode doesn't change, nothing to do for us */
> > >> -	if (cmode == chip->ports[port].cmode)
> > >> -		return 0;
> > >> -
> > >>  	lane = mv88e6xxx_serdes_get_lane(chip, port);
> > >>  	if (lane) {
> > >>  		if (chip->ports[port].serdes_irq) {
> > >>
> > >> Does that make sense?
> > >
> > > This needs testing on a 6390, with a port 9 or 10 using fixed link. We
> > > have had issues in the past where mac_config() has been called once
> > > per second, and each time it reconfigured the MAC, causing the link to
> > > go down/up. So we try to avoid doing work which is not requires and
> > > which could upset the link.
> > 
> > You refer to ed8fe20205ac ("net: dsa: mv88e6xxx: prevent interrupt storm
> > caused by mv88e6390x_port_set_cmode") that introduced this code.
> > 
> > The alternative is to call ->port_get_cmode() to refresh the cmode cache
> > after mv88e6xxx_port_hidden_write().
> 
> Refreshing the cmode after mv88e6xxx_port_hidden_write() sounds like a
> good idea. But please limit it to just switches which need to make
> cmode writable. cmode is rather fragile and the 6390 family is easy to
> break in this area.

This turned out to be much harder than expected. cmode update after 
mv88e6xxx_port_hidden_write() breaks the serdes configuration. The link change 
irq does not trigger on serdes interface up. The strange thing is that if I 
add 10ms delay after cmode read (or anywhere before the mv88e6xxx_port_write() 
call in mv88e6xxx_port_set_cmode()) it works again. I have no idea what is 
going on here. The cmode cache is used in many places, so maybe setting it to 
an invalid value (6) is not a good idea.

I ended up with cmode write force in mv88e6341_port_set_cmode_writable() like 
this:

diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 8a8e38bfb161..70284f100d87 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -233,6 +233,7 @@ struct mv88e6xxx_port {
 	u64 vtu_member_violation;
 	u64 vtu_miss_violation;
 	u8 cmode;
+	bool force_cmode;
 	bool mirror_ingress;
 	bool mirror_egress;
 	unsigned int serdes_irq;
diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index 7fe256c5739d..8e8724eb5669 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -427,9 +427,10 @@ static int mv88e6xxx_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
 		cmode = 0;
 	}
 
-	/* cmode doesn't change, nothing to do for us */
-	if (cmode == chip->ports[port].cmode)
+	/* cmode doesn't change, nothing to do for us unless forced */
+	if (cmode == chip->ports[port].cmode && !chip->ports[port].force_cmode)
 		return 0;
+	chip->ports[port].force_cmode = false;
 
 	lane = mv88e6xxx_serdes_get_lane(chip, port);
 	if (lane) {
@@ -516,6 +517,8 @@ static int mv88e6341_port_set_cmode_writable(struct mv88e6xxx_chip *chip,
 	if (port != 5)
 		return -EOPNOTSUPP;
 
+	chip->ports[port].force_cmode = true;
+
 	addr = chip->info->port_base_addr + port;
 
 	err = mv88e6xxx_port_hidden_read(chip, 0x7, addr, 0, &reg);

Does that look right?

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@...s.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -