| Message ID | 1347894617-13614-1-git-send-email-jiri@resnulli.us |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>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?-- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 {
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(-)