* [PATCH net] net: dsa: mv88e6xxx: prevent interrupt storm caused by mv88e6390x_port_set_cmode
@ 2019-02-27 19:55 Heiner Kallweit
2019-02-27 23:40 ` Vivien Didelot
0 siblings, 1 reply; 3+ messages in thread
From: Heiner Kallweit @ 2019-02-27 19:55 UTC (permalink / raw)
To: Vivien Didelot, Andrew Lunn, Florian Fainelli, David Miller
Cc: netdev@vger.kernel.org
When debugging another issue I faced an interrupt storm in this
driver (88E6390, port 9 in SGMII mode), consisting of alternating
link-up / link-down interrupts. Analysis showed that the driver
wanted to set a cmode that was set already. But so far
mv88e6390x_port_set_cmode() doesn't check this and powers down
SERDES, what causes the link to break, and eventually results in
the described interrupt storm.
Fix this by checking whether the cmode actually changes. We want
that the very first call to mv88e6390x_port_set_cmode() always
configures the registers, therefore initialize port.cmode with
a value that is different from any supported cmode value.
Fixes: 364e9d7776a3 ("net: dsa: mv88e6xxx: Power on/off SERDES on cmode change")
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/dsa/mv88e6xxx/chip.c | 4 ++++
drivers/net/dsa/mv88e6xxx/port.c | 4 ++++
drivers/net/dsa/mv88e6xxx/port.h | 1 +
3 files changed, 9 insertions(+)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 32e7af5ca..d4edb61e8 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -4568,6 +4568,7 @@ static int mv88e6xxx_detect(struct mv88e6xxx_chip *chip)
static struct mv88e6xxx_chip *mv88e6xxx_alloc_chip(struct device *dev)
{
struct mv88e6xxx_chip *chip;
+ int i;
chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
if (!chip)
@@ -4578,6 +4579,9 @@ static struct mv88e6xxx_chip *mv88e6xxx_alloc_chip(struct device *dev)
mutex_init(&chip->reg_lock);
INIT_LIST_HEAD(&chip->mdios);
+ for (i = 0; i < ARRAY_SIZE(chip->ports); i++)
+ chip->ports[i].cmode = MV88E6XXX_PORT_STS_CMODE_INVALID;
+
return chip;
}
diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index ebd26b6a9..70b7a1463 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -398,6 +398,10 @@ int mv88e6390x_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 = mv88e6390x_serdes_get_lane(chip, port);
if (lane < 0)
return lane;
diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
index e583641de..4aadf321e 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h
@@ -52,6 +52,7 @@
#define MV88E6185_PORT_STS_CMODE_1000BASE_X 0x0005
#define MV88E6185_PORT_STS_CMODE_PHY 0x0006
#define MV88E6185_PORT_STS_CMODE_DISABLED 0x0007
+#define MV88E6XXX_PORT_STS_CMODE_INVALID 0xff
/* Offset 0x01: MAC (or PCS or Physical) Control Register */
#define MV88E6XXX_PORT_MAC_CTL 0x01
--
2.21.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH net] net: dsa: mv88e6xxx: prevent interrupt storm caused by mv88e6390x_port_set_cmode
2019-02-27 19:55 [PATCH net] net: dsa: mv88e6xxx: prevent interrupt storm caused by mv88e6390x_port_set_cmode Heiner Kallweit
@ 2019-02-27 23:40 ` Vivien Didelot
2019-02-27 23:48 ` Heiner Kallweit
0 siblings, 1 reply; 3+ messages in thread
From: Vivien Didelot @ 2019-02-27 23:40 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Andrew Lunn, Florian Fainelli, David Miller,
netdev@vger.kernel.org
Hi Heiner,
On Wed, 27 Feb 2019 20:55:22 +0100, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> When debugging another issue I faced an interrupt storm in this
> driver (88E6390, port 9 in SGMII mode), consisting of alternating
> link-up / link-down interrupts. Analysis showed that the driver
> wanted to set a cmode that was set already. But so far
> mv88e6390x_port_set_cmode() doesn't check this and powers down
> SERDES, what causes the link to break, and eventually results in
> the described interrupt storm.
>
> Fix this by checking whether the cmode actually changes. We want
> that the very first call to mv88e6390x_port_set_cmode() always
> configures the registers, therefore initialize port.cmode with
> a value that is different from any supported cmode value.
>
> Fixes: 364e9d7776a3 ("net: dsa: mv88e6xxx: Power on/off SERDES on cmode change")
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/net/dsa/mv88e6xxx/chip.c | 4 ++++
> drivers/net/dsa/mv88e6xxx/port.c | 4 ++++
> drivers/net/dsa/mv88e6xxx/port.h | 1 +
> 3 files changed, 9 insertions(+)
>
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 32e7af5ca..d4edb61e8 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -4568,6 +4568,7 @@ static int mv88e6xxx_detect(struct mv88e6xxx_chip *chip)
> static struct mv88e6xxx_chip *mv88e6xxx_alloc_chip(struct device *dev)
> {
> struct mv88e6xxx_chip *chip;
> + int i;
>
> chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> if (!chip)
> @@ -4578,6 +4579,9 @@ static struct mv88e6xxx_chip *mv88e6xxx_alloc_chip(struct device *dev)
> mutex_init(&chip->reg_lock);
> INIT_LIST_HEAD(&chip->mdios);
>
> + for (i = 0; i < ARRAY_SIZE(chip->ports); i++)
mv88e6xxx_num_ports(chip)
> + chip->ports[i].cmode = MV88E6XXX_PORT_STS_CMODE_INVALID;
> +
> return chip;
> }
>
> diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
> index ebd26b6a9..70b7a1463 100644
> --- a/drivers/net/dsa/mv88e6xxx/port.c
> +++ b/drivers/net/dsa/mv88e6xxx/port.c
> @@ -398,6 +398,10 @@ int mv88e6390x_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 = mv88e6390x_serdes_get_lane(chip, port);
> if (lane < 0)
> return lane;
> diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
> index e583641de..4aadf321e 100644
> --- a/drivers/net/dsa/mv88e6xxx/port.h
> +++ b/drivers/net/dsa/mv88e6xxx/port.h
> @@ -52,6 +52,7 @@
> #define MV88E6185_PORT_STS_CMODE_1000BASE_X 0x0005
> #define MV88E6185_PORT_STS_CMODE_PHY 0x0006
> #define MV88E6185_PORT_STS_CMODE_DISABLED 0x0007
> +#define MV88E6XXX_PORT_STS_CMODE_INVALID 0xff
Is this 0xff a mask value from the Port Status register? If so please
the 0x1234 format like above, to make this mask value obvious.
>
> /* Offset 0x01: MAC (or PCS or Physical) Control Register */
> #define MV88E6XXX_PORT_MAC_CTL 0x01
Thanks,
Vivien
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net] net: dsa: mv88e6xxx: prevent interrupt storm caused by mv88e6390x_port_set_cmode
2019-02-27 23:40 ` Vivien Didelot
@ 2019-02-27 23:48 ` Heiner Kallweit
0 siblings, 0 replies; 3+ messages in thread
From: Heiner Kallweit @ 2019-02-27 23:48 UTC (permalink / raw)
To: Vivien Didelot
Cc: Andrew Lunn, Florian Fainelli, David Miller,
netdev@vger.kernel.org
On 28.02.2019 00:40, Vivien Didelot wrote:
> Hi Heiner,
>
> On Wed, 27 Feb 2019 20:55:22 +0100, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>> When debugging another issue I faced an interrupt storm in this
>> driver (88E6390, port 9 in SGMII mode), consisting of alternating
>> link-up / link-down interrupts. Analysis showed that the driver
>> wanted to set a cmode that was set already. But so far
>> mv88e6390x_port_set_cmode() doesn't check this and powers down
>> SERDES, what causes the link to break, and eventually results in
>> the described interrupt storm.
>>
>> Fix this by checking whether the cmode actually changes. We want
>> that the very first call to mv88e6390x_port_set_cmode() always
>> configures the registers, therefore initialize port.cmode with
>> a value that is different from any supported cmode value.
>>
>> Fixes: 364e9d7776a3 ("net: dsa: mv88e6xxx: Power on/off SERDES on cmode change")
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> drivers/net/dsa/mv88e6xxx/chip.c | 4 ++++
>> drivers/net/dsa/mv88e6xxx/port.c | 4 ++++
>> drivers/net/dsa/mv88e6xxx/port.h | 1 +
>> 3 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
>> index 32e7af5ca..d4edb61e8 100644
>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>> @@ -4568,6 +4568,7 @@ static int mv88e6xxx_detect(struct mv88e6xxx_chip *chip)
>> static struct mv88e6xxx_chip *mv88e6xxx_alloc_chip(struct device *dev)
>> {
>> struct mv88e6xxx_chip *chip;
>> + int i;
>>
>> chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
>> if (!chip)
>> @@ -4578,6 +4579,9 @@ static struct mv88e6xxx_chip *mv88e6xxx_alloc_chip(struct device *dev)
>> mutex_init(&chip->reg_lock);
>> INIT_LIST_HEAD(&chip->mdios);
>>
>> + for (i = 0; i < ARRAY_SIZE(chip->ports); i++)
>
> mv88e6xxx_num_ports(chip)
>
OK
>> + chip->ports[i].cmode = MV88E6XXX_PORT_STS_CMODE_INVALID;
>> +
>> return chip;
>> }
>>
>> diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
>> index ebd26b6a9..70b7a1463 100644
>> --- a/drivers/net/dsa/mv88e6xxx/port.c
>> +++ b/drivers/net/dsa/mv88e6xxx/port.c
>> @@ -398,6 +398,10 @@ int mv88e6390x_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 = mv88e6390x_serdes_get_lane(chip, port);
>> if (lane < 0)
>> return lane;
>> diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
>> index e583641de..4aadf321e 100644
>> --- a/drivers/net/dsa/mv88e6xxx/port.h
>> +++ b/drivers/net/dsa/mv88e6xxx/port.h
>> @@ -52,6 +52,7 @@
>> #define MV88E6185_PORT_STS_CMODE_1000BASE_X 0x0005
>> #define MV88E6185_PORT_STS_CMODE_PHY 0x0006
>> #define MV88E6185_PORT_STS_CMODE_DISABLED 0x0007
>> +#define MV88E6XXX_PORT_STS_CMODE_INVALID 0xff
>
> Is this 0xff a mask value from the Port Status register? If so please
> the 0x1234 format like above, to make this mask value obvious.
>
No, it's not a mask. I couldn't use 0 for "invalid" because 0 is used
otherwise, so I went with 0xff. All these STS_CMODE values are also
stored in mv88e6xxx_port.cmode that has type u8. Therefore I decided
to write it as an 8 bit value.
>>
>> /* Offset 0x01: MAC (or PCS or Physical) Control Register */
>> #define MV88E6XXX_PORT_MAC_CTL 0x01
>
>
> Thanks,
>
> Vivien
>
Heiner
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-02-27 23:48 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-02-27 19:55 [PATCH net] net: dsa: mv88e6xxx: prevent interrupt storm caused by mv88e6390x_port_set_cmode Heiner Kallweit
2019-02-27 23:40 ` Vivien Didelot
2019-02-27 23:48 ` Heiner Kallweit
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).