[patch net] sky2: fix rx filter setup on link up

[patch net] sky2: fix rx filter setup on link up

[Jiri Pirko]

In my case I have following problem. sky2_set_multicast() sets registers
GM_MC_ADDR_H[1-4] correctly to:
0000 0800 0001 0410
However, when adapter gets link and sky2_link_up() is called, the values
are for some reason different:
0000 0800 0016 0410
This in my case prevents iface to be able to receive packets with dst mac
01:80:C2:00:00:02 (LACPDU dst mac), which I set up previously by
SIOCADDMULTI.

So remember computed rx_filter data and write it to GM_MC_ADDR_H[1-4] on
link_up.

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 drivers/net/ethernet/marvell/sky2.c | 30 ++++++++++++++++++++++--------
 drivers/net/ethernet/marvell/sky2.h |  2 ++
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
index 2b0748d..5293ff4 100644
--- a/drivers/net/ethernet/marvell/sky2.c
+++ b/drivers/net/ethernet/marvell/sky2.c
@@ -2186,6 +2186,8 @@ static u16 sky2_phy_speed(const struct sky2_hw *hw, u16 aux)
 	}
 }
 
+static void sky2_write_rx_filter(struct sky2_port *sky2, u8 *filter);
+
 static void sky2_link_up(struct sky2_port *sky2)
 {
 	struct sky2_hw *hw = sky2->hw;
@@ -2211,6 +2213,9 @@ static void sky2_link_up(struct sky2_port *sky2)
 	sky2_write8(hw, SK_REG(port, LNK_LED_REG),
 		    LINKLED_ON | LINKLED_BLINK_OFF | LINKLED_LINKSYNC_OFF);
 
+	/* Refresh RX filter */
+	sky2_write_rx_filter(sky2, sky2->rx_filter);
+
 	netif_info(sky2, link, sky2->netdev,
 		   "Link is up at %d Mbps, %s duplex, flow control %s\n",
 		   sky2->speed,
@@ -3849,6 +3854,21 @@ static inline void sky2_add_filter(u8 filter[8], const u8 *addr)
 	filter[bit >> 3] |= 1 << (bit & 7);
 }
 
+static void sky2_write_rx_filter(struct sky2_port *sky2, u8 *filter)
+{
+	struct sky2_hw *hw = sky2->hw;
+	unsigned port = sky2->port;
+
+	gma_write16(hw, port, GM_MC_ADDR_H1,
+		    (u16) filter[0] | ((u16) filter[1] << 8));
+	gma_write16(hw, port, GM_MC_ADDR_H2,
+		    (u16) filter[2] | ((u16) filter[3] << 8));
+	gma_write16(hw, port, GM_MC_ADDR_H3,
+		    (u16) filter[4] | ((u16) filter[5] << 8));
+	gma_write16(hw, port, GM_MC_ADDR_H4,
+		    (u16) filter[6] | ((u16) filter[7] << 8));
+}
+
 static void sky2_set_multicast(struct net_device *dev)
 {
 	struct sky2_port *sky2 = netdev_priv(dev);
@@ -3882,14 +3902,8 @@ static void sky2_set_multicast(struct net_device *dev)
 			sky2_add_filter(filter, ha->addr);
 	}
 
-	gma_write16(hw, port, GM_MC_ADDR_H1,
-		    (u16) filter[0] | ((u16) filter[1] << 8));
-	gma_write16(hw, port, GM_MC_ADDR_H2,
-		    (u16) filter[2] | ((u16) filter[3] << 8));
-	gma_write16(hw, port, GM_MC_ADDR_H3,
-		    (u16) filter[4] | ((u16) filter[5] << 8));
-	gma_write16(hw, port, GM_MC_ADDR_H4,
-		    (u16) filter[6] | ((u16) filter[7] << 8));
+	sky2_write_rx_filter(sky2, filter);
+	memcpy(sky2->rx_filter, filter, sizeof(sky2->rx_filter));
 
 	gma_write16(hw, port, GM_RX_CTRL, reg);
 }
diff --git a/drivers/net/ethernet/marvell/sky2.h b/drivers/net/ethernet/marvell/sky2.h
index 615ac63..513c266 100644
--- a/drivers/net/ethernet/marvell/sky2.h
+++ b/drivers/net/ethernet/marvell/sky2.h
@@ -2272,6 +2272,8 @@ struct sky2_port {
 #ifdef CONFIG_SKY2_DEBUG
 	struct dentry	     *debugfs;
 #endif
+
+	u8		     rx_filter[8];
 };
 
 struct sky2_hw {
-- 
1.7.11.4

Re: [patch net] sky2: fix rx filter setup on link up

[Stephen Hemminger]

On Mon, 17 Sep 2012 17:10:17 +0200
Jiri Pirko <jiri@resnulli.us> wrote:

> In my case I have following problem. sky2_set_multicast() sets registers
> GM_MC_ADDR_H[1-4] correctly to:
> 0000 0800 0001 0410
> However, when adapter gets link and sky2_link_up() is called, the values
> are for some reason different:
> 0000 0800 0016 0410

Rather than papering over the problem, it would be better to
trace back what is setting those registers and fix that code.

> This in my case prevents iface to be able to receive packets with dst mac
> 01:80:C2:00:00:02 (LACPDU dst mac), which I set up previously by
> SIOCADDMULTI.
> 
> So remember computed rx_filter data and write it to GM_MC_ADDR_H[1-4] on
> link_up.
>

Please do some more root cause analysis. Just save/restoring the
registers is just a temporary workaround.

Re: [patch net] sky2: fix rx filter setup on link up

[Jiri Pirko]

Mon, Sep 17, 2012 at 06:12:14PM CEST, shemminger@vyatta.com wrote:
>On Mon, 17 Sep 2012 17:10:17 +0200
>Jiri Pirko <jiri@resnulli.us> wrote:
>
>> In my case I have following problem. sky2_set_multicast() sets registers
>> GM_MC_ADDR_H[1-4] correctly to:
>> 0000 0800 0001 0410
>> However, when adapter gets link and sky2_link_up() is called, the values
>> are for some reason different:
>> 0000 0800 0016 0410
>
>Rather than papering over the problem, it would be better to
>trace back what is setting those registers and fix that code.

Yes, I did that. No code at sky2.[ch] is writing to this registers other
than sky2_set_multicast() and sky2_gmac_reset() (I hooked on sky2_write*()).
So I strongly believe this is a HW issue (maybe only issue of my revision
"Yukon-2 EC chip revision 2")

>
>> This in my case prevents iface to be able to receive packets with dst mac
>> 01:80:C2:00:00:02 (LACPDU dst mac), which I set up previously by
>> SIOCADDMULTI.
>> 
>> So remember computed rx_filter data and write it to GM_MC_ADDR_H[1-4] on
>> link_up.
>>
>
>Please do some more root cause analysis. Just save/restoring the
>registers is just a temporary workaround.

Re: [patch net] sky2: fix rx filter setup on link up

[Stephen Hemminger]

On Mon, 17 Sep 2012 22:47:24 +0200
Jiri Pirko <jiri@resnulli.us> wrote:

> Mon, Sep 17, 2012 at 06:12:14PM CEST, shemminger@vyatta.com wrote:
> >On Mon, 17 Sep 2012 17:10:17 +0200
> >Jiri Pirko <jiri@resnulli.us> wrote:
> >
> >> In my case I have following problem. sky2_set_multicast() sets registers
> >> GM_MC_ADDR_H[1-4] correctly to:
> >> 0000 0800 0001 0410
> >> However, when adapter gets link and sky2_link_up() is called, the values
> >> are for some reason different:
> >> 0000 0800 0016 0410
> >
> >Rather than papering over the problem, it would be better to
> >trace back what is setting those registers and fix that code.
> 
> Yes, I did that. No code at sky2.[ch] is writing to this registers other
> than sky2_set_multicast() and sky2_gmac_reset() (I hooked on sky2_write*()).
> So I strongly believe this is a HW issue (maybe only issue of my revision
> "Yukon-2 EC chip revision 2")
> 
> >
> >> This in my case prevents iface to be able to receive packets with dst mac
> >> 01:80:C2:00:00:02 (LACPDU dst mac), which I set up previously by
> >> SIOCADDMULTI.
> >> 
> >> So remember computed rx_filter data and write it to GM_MC_ADDR_H[1-4] on
> >> link_up.
> >>
> >
> >Please do some more root cause analysis. Just save/restoring the
> >registers is just a temporary workaround.

Are you sure it isn't IPv6 or something else setting additional mulitcast
addresses. You may need to instrument the set_multicast call.

Re: [patch net] sky2: fix rx filter setup on link up

[Jiri Pirko]

Mon, Sep 17, 2012 at 11:15:07PM CEST, shemminger@vyatta.com wrote:
>On Mon, 17 Sep 2012 22:47:24 +0200
>Jiri Pirko <jiri@resnulli.us> wrote:
>
>> Mon, Sep 17, 2012 at 06:12:14PM CEST, shemminger@vyatta.com wrote:
>> >On Mon, 17 Sep 2012 17:10:17 +0200
>> >Jiri Pirko <jiri@resnulli.us> wrote:
>> >
>> >> In my case I have following problem. sky2_set_multicast() sets registers
>> >> GM_MC_ADDR_H[1-4] correctly to:
>> >> 0000 0800 0001 0410
>> >> However, when adapter gets link and sky2_link_up() is called, the values
>> >> are for some reason different:
>> >> 0000 0800 0016 0410
>> >
>> >Rather than papering over the problem, it would be better to
>> >trace back what is setting those registers and fix that code.
>> 
>> Yes, I did that. No code at sky2.[ch] is writing to this registers other
>> than sky2_set_multicast() and sky2_gmac_reset() (I hooked on sky2_write*()).
>> So I strongly believe this is a HW issue (maybe only issue of my revision
>> "Yukon-2 EC chip revision 2")
>> 
>> >
>> >> This in my case prevents iface to be able to receive packets with dst mac
>> >> 01:80:C2:00:00:02 (LACPDU dst mac), which I set up previously by
>> >> SIOCADDMULTI.
>> >> 
>> >> So remember computed rx_filter data and write it to GM_MC_ADDR_H[1-4] on
>> >> link_up.
>> >>
>> >
>> >Please do some more root cause analysis. Just save/restoring the
>> >registers is just a temporary workaround.
>
>Are you sure it isn't IPv6 or something else setting additional mulitcast
>addresses. You may need to instrument the set_multicast call.

I'm very sure that no code in sky2 is writing to GM_MC_ADDR_H[1-4] the
change I see in sky2_link_up(). When sky2_set_multicast() is called
again for any reason, the issue goes away and lacpdus are coming in.

I also experimentally used sky2_set_multicast() called from
sky2_link_up() and it helped as well.

Re: [patch net] sky2: fix rx filter setup on link up

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 17 Sep 2012 14:15:07 -0700

> Are you sure it isn't IPv6 or something else setting additional mulitcast
> addresses. You may need to instrument the set_multicast call.

I'm confident in Jiri's analysis and patch and it's clear the hardware
is doing this on it's own, so please take his bug fix seriously.

RE: [patch net] sky2: fix rx filter setup on link up

[Mirko Lindner]

>Mon, Sep 17, 2012 at 06:12:14PM CEST, shemminger@vyatta.com wrote:
>>On Mon, 17 Sep 2012 17:10:17 +0200
>>Jiri Pirko <jiri@resnulli.us> wrote:
>>
>>> In my case I have following problem. sky2_set_multicast() sets registers
>>> GM_MC_ADDR_H[1-4] correctly to:
>>> 0000 0800 0001 0410
>>> However, when adapter gets link and sky2_link_up() is called, the values
>>> are for some reason different:
>>> 0000 0800 0016 0410
>>
>>Rather than papering over the problem, it would be better to
>>trace back what is setting those registers and fix that code.

>Yes, I did that. No code at sky2.[ch] is writing to this registers other
>than sky2_set_multicast() and sky2_gmac_reset() (I hooked on sky2_write*()).
>So I strongly believe this is a HW issue (maybe only issue of my revision
>"Yukon-2 EC chip revision 2")

I would like to check the registers as soon as I'm back in my office next week and report my findings.
Could you also please check the hint from Stephen?

Re: [patch net] sky2: fix rx filter setup on link up

[Jiri Pirko]

Tue, Sep 18, 2012 at 02:38:52AM CEST, mlindner@marvell.com wrote:
>>Mon, Sep 17, 2012 at 06:12:14PM CEST, shemminger@vyatta.com wrote:
>>>On Mon, 17 Sep 2012 17:10:17 +0200
>>>Jiri Pirko <jiri@resnulli.us> wrote:
>>>
>>>> In my case I have following problem. sky2_set_multicast() sets registers
>>>> GM_MC_ADDR_H[1-4] correctly to:
>>>> 0000 0800 0001 0410
>>>> However, when adapter gets link and sky2_link_up() is called, the values
>>>> are for some reason different:
>>>> 0000 0800 0016 0410
>>>
>>>Rather than papering over the problem, it would be better to
>>>trace back what is setting those registers and fix that code.
>
>>Yes, I did that. No code at sky2.[ch] is writing to this registers other
>>than sky2_set_multicast() and sky2_gmac_reset() (I hooked on sky2_write*()).
>>So I strongly believe this is a HW issue (maybe only issue of my revision
>>"Yukon-2 EC chip revision 2")
>
>I would like to check the registers as soon as I'm back in my office next week and report my findings.

Okay, I'll wait for you. If you need more info from my side, please do
not hesitate to ask. Thanks!

>Could you also please check the hint from Stephen?

Re: [patch net] sky2: fix rx filter setup on link up

[Jiri Pirko]

Tue, Sep 18, 2012 at 02:38:52AM CEST, mlindner@marvell.com wrote:
>>Mon, Sep 17, 2012 at 06:12:14PM CEST, shemminger@vyatta.com wrote:
>>>On Mon, 17 Sep 2012 17:10:17 +0200
>>>Jiri Pirko <jiri@resnulli.us> wrote:
>>>
>>>> In my case I have following problem. sky2_set_multicast() sets registers
>>>> GM_MC_ADDR_H[1-4] correctly to:
>>>> 0000 0800 0001 0410
>>>> However, when adapter gets link and sky2_link_up() is called, the values
>>>> are for some reason different:
>>>> 0000 0800 0016 0410
>>>
>>>Rather than papering over the problem, it would be better to
>>>trace back what is setting those registers and fix that code.
>
>>Yes, I did that. No code at sky2.[ch] is writing to this registers other
>>than sky2_set_multicast() and sky2_gmac_reset() (I hooked on sky2_write*()).
>>So I strongly believe this is a HW issue (maybe only issue of my revision
>>"Yukon-2 EC chip revision 2")
>
>I would like to check the registers as soon as I'm back in my office next week and report my findings.
>Could you also please check the hint from Stephen?

Mirko, did you have a chance to look at this?

Re: [patch net] sky2: fix rx filter setup on link up

[Mirko Lindner]

On Thu, 2012-10-04 at 00:40 -0700, Jiri Pirko wrote:
> Tue, Sep 18, 2012 at 02:38:52AM CEST, mlindner@marvell.com wrote:
> >>Mon, Sep 17, 2012 at 06:12:14PM CEST, shemminger@vyatta.com wrote:
> >>>On Mon, 17 Sep 2012 17:10:17 +0200
> >>>Jiri Pirko <jiri@resnulli.us> wrote:
> >>>
> >>>> In my case I have following problem. sky2_set_multicast() sets registers
> >>>> GM_MC_ADDR_H[1-4] correctly to:
> >>>> 0000 0800 0001 0410
> >>>> However, when adapter gets link and sky2_link_up() is called, the values
> >>>> are for some reason different:
> >>>> 0000 0800 0016 0410
> >>>
> >>>Rather than papering over the problem, it would be better to
> >>>trace back what is setting those registers and fix that code.
> >
> >>Yes, I did that. No code at sky2.[ch] is writing to this registers other
> >>than sky2_set_multicast() and sky2_gmac_reset() (I hooked on sky2_write*()).
> >>So I strongly believe this is a HW issue (maybe only issue of my revision
> >>"Yukon-2 EC chip revision 2")
> >
> >I would like to check the registers as soon as I'm back in my office next week and report my findings.
> >Could you also please check the hint from Stephen?
> 
> Mirko, did you have a chance to look at this?
> 


Sorry Jiri,

I was the last three weeks at a customer's office and not in our lab.
I'll check the issue this week.

Mirko

Re: [patch net] sky2: fix rx filter setup on link up

[Stephen Hemminger]

Rather than saving and restoring values, why not just redo the
full setup? This would also determine if the change was a result
of something outside the driver.


--- a/drivers/net/ethernet/marvell/sky2.c	2012-09-18 21:12:01.156438131 -0700
+++ b/drivers/net/ethernet/marvell/sky2.c	2012-09-19 13:20:40.373620276 -0700
@@ -2201,6 +2201,8 @@ static void sky2_link_up(struct sky2_por
 
 	sky2_enable_rx_tx(sky2);
 
+	sky2_set_multicast(sky2->netdev);
+
 	gm_phy_write(hw, port, PHY_MARV_INT_MASK, PHY_M_DEF_MSK);
 
 	netif_carrier_on(sky2->netdev);

Re: [patch net] sky2: fix rx filter setup on link up

[Jiri Pirko]

Wed, Sep 19, 2012 at 10:22:48PM CEST, shemminger@vyatta.com wrote:
>Rather than saving and restoring values, why not just redo the
>full setup? This would also determine if the change was a result
>of something outside the driver.

You cannot call sky2_set_multicast() directly here. It is called from
__dev_set_rx_mode(). You would have to take at lease netif_addr_lock()
here. I think that clearer is to remember computed value....

>
>
>--- a/drivers/net/ethernet/marvell/sky2.c	2012-09-18 21:12:01.156438131 -0700
>+++ b/drivers/net/ethernet/marvell/sky2.c	2012-09-19 13:20:40.373620276 -0700
>@@ -2201,6 +2201,8 @@ static void sky2_link_up(struct sky2_por
> 
> 	sky2_enable_rx_tx(sky2);
> 
>+	sky2_set_multicast(sky2->netdev);
>+
> 	gm_phy_write(hw, port, PHY_MARV_INT_MASK, PHY_M_DEF_MSK);
> 
> 	netif_carrier_on(sky2->netdev);

Re: [patch net] sky2: fix rx filter setup on link up

[Stephen Hemminger]

On Wed, 19 Sep 2012 22:38:25 +0200
Jiri Pirko <jiri@resnulli.us> wrote:

> You cannot call sky2_set_multicast() directly here. It is called from
> __dev_set_rx_mode(). You would have to take at lease netif_addr_lock()
> here. I think that clearer is to remember computed value....

Ugh, it would mean changing link_up to being done via workqueue.
But your code wasnt safe against changes to values from set_multicast race
either.

Re: [patch net] sky2: fix rx filter setup on link up

[Stephen Hemminger]

On Mon, 17 Sep 2012 17:10:17 +0200
Jiri Pirko <jiri@resnulli.us> wrote:

> +	sky2_write_rx_filter(sky2, filter);
> +	memcpy(sky2->rx_filter, filter, sizeof(sky2->rx_filter));

This isn't safe since rx_filter might be referred to from interrupt
context (ie link up PHY interrupt). Maybe using atomic64 would be
best.