* [PATCH net 0/2] r8169: fix rx vlan
@ 2014-09-12 3:35 Hayes Wang
2014-09-12 3:35 ` [PATCH net 1/2] r8169: fix the default setting of " Hayes Wang
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Hayes Wang @ 2014-09-12 3:35 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, Hayes Wang
There are two issues for hw rx vlan. The patches are
used to fix them.
Hayes Wang (2):
r8169: fix the default setting of rx vlan
r8169: fix setting rx vlan
drivers/net/ethernet/realtek/r8169.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
--
1.9.3
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH net 1/2] r8169: fix the default setting of rx vlan 2014-09-12 3:35 [PATCH net 0/2] r8169: fix rx vlan Hayes Wang @ 2014-09-12 3:35 ` Hayes Wang 2014-09-12 19:40 ` Francois Romieu 2014-09-12 3:35 ` [PATCH net 2/2] r8169: fix setting rx vlan Hayes Wang 2014-09-13 20:53 ` [PATCH net 0/2] r8169: fix " David Miller 2 siblings, 1 reply; 10+ messages in thread From: Hayes Wang @ 2014-09-12 3:35 UTC (permalink / raw) To: netdev; +Cc: nic_swsd, linux-kernel, Hayes Wang If the parameter "features" of __rtl8169_set_features() is equal to dev->features, the variable "changed" is alwayes 0, and nothing would be changed. Signed-off-by: Hayes Wang <hayeswang@realtek.com> --- drivers/net/ethernet/realtek/r8169.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c index 91652e7..f3ce284 100644 --- a/drivers/net/ethernet/realtek/r8169.c +++ b/drivers/net/ethernet/realtek/r8169.c @@ -6707,7 +6707,12 @@ static int rtl_open(struct net_device *dev) rtl8169_init_phy(dev, tp); - __rtl8169_set_features(dev, dev->features); + if (dev->features & NETIF_F_HW_VLAN_CTAG_RX) + tp->cp_cmd |= RxVlan; + else + tp->cp_cmd &= ~RxVlan; + + RTL_W16(CPlusCmd, tp->cp_cmd); rtl_pll_power_up(tp); -- 1.9.3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net 1/2] r8169: fix the default setting of rx vlan 2014-09-12 3:35 ` [PATCH net 1/2] r8169: fix the default setting of " Hayes Wang @ 2014-09-12 19:40 ` Francois Romieu 2014-09-15 3:22 ` Hayes Wang 0 siblings, 1 reply; 10+ messages in thread From: Francois Romieu @ 2014-09-12 19:40 UTC (permalink / raw) To: Hayes Wang; +Cc: netdev, nic_swsd, linux-kernel Hayes Wang <hayeswang@realtek.com> : > If the parameter "features" of __rtl8169_set_features() is equal to > dev->features, the variable "changed" is alwayes 0, and nothing would > be changed. [...] > diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c > index 91652e7..f3ce284 100644 > --- a/drivers/net/ethernet/realtek/r8169.c > +++ b/drivers/net/ethernet/realtek/r8169.c > @@ -6707,7 +6707,12 @@ static int rtl_open(struct net_device *dev) > > rtl8169_init_phy(dev, tp); > > - __rtl8169_set_features(dev, dev->features); > + if (dev->features & NETIF_F_HW_VLAN_CTAG_RX) > + tp->cp_cmd |= RxVlan; > + else > + tp->cp_cmd &= ~RxVlan; > + > + RTL_W16(CPlusCmd, tp->cp_cmd); Damn good catch. The same fix should be relevant for NETIF_F_RXCSUM. You may thus as well remove the "changed" test in __rtl8169_set_features and keep everything there. The commit message could notify the driver don't behave as expected since 6bbe021d405fff46b64a08dca51b06897b897a67 ("r8169: Support RX-ALL flag.") to ease stable people's work. -- Ueimor ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH net 1/2] r8169: fix the default setting of rx vlan 2014-09-12 19:40 ` Francois Romieu @ 2014-09-15 3:22 ` Hayes Wang 2014-09-16 0:03 ` Francois Romieu 0 siblings, 1 reply; 10+ messages in thread From: Hayes Wang @ 2014-09-15 3:22 UTC (permalink / raw) To: Francois Romieu; +Cc: netdev, nic_swsd, linux-kernel Francois Romieu [mailto:romieu@fr.zoreil.com] > Sent: Saturday, September 13, 2014 3:40 AM [...] > The same fix should be relevant for NETIF_F_RXCSUM. You may thus as > well remove the "changed" test in __rtl8169_set_features and keep > everything there. In the probe function (rtl_init_one), RxChkSum has been assgined to tp->cp_cmd, so only the vlan setting is incorrect. Therefore, I fix it only. Do I still have to remove the "changed" test? Best Regards, Hayes ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net 1/2] r8169: fix the default setting of rx vlan 2014-09-15 3:22 ` Hayes Wang @ 2014-09-16 0:03 ` Francois Romieu 2014-09-16 3:23 ` Hayes Wang 2014-09-16 3:40 ` [PATCH net] r8169: adjust __rtl8169_set_features Hayes Wang 0 siblings, 2 replies; 10+ messages in thread From: Francois Romieu @ 2014-09-16 0:03 UTC (permalink / raw) To: Hayes Wang; +Cc: netdev, nic_swsd, linux-kernel Hayes Wang <hayeswang@realtek.com> : > Francois Romieu [mailto:romieu@fr.zoreil.com] > > Sent: Saturday, September 13, 2014 3:40 AM > [...] > > The same fix should be relevant for NETIF_F_RXCSUM. You may thus as > > well remove the "changed" test in __rtl8169_set_features and keep > > everything there. > > In the probe function (rtl_init_one), RxChkSum has been assgined to > tp->cp_cmd, so only the vlan setting is incorrect. Therefore, I fix > it only. Do I still have to remove the "changed" test? You are right, this part does work. However it's getting messy and the code stomps CPlusCmd in rtl_open right before it's given a chance to be read in hw_start. Something like the untested patch below should avoid it and keep related things together. Thoughts ? diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c index 7a7860a..ef2cee5 100644 --- a/drivers/net/ethernet/realtek/r8169.c +++ b/drivers/net/ethernet/realtek/r8169.c @@ -1783,33 +1783,31 @@ static void __rtl8169_set_features(struct net_device *dev, netdev_features_t features) { struct rtl8169_private *tp = netdev_priv(dev); - netdev_features_t changed = features ^ dev->features; void __iomem *ioaddr = tp->mmio_addr; + u32 rx_config; - if (!(changed & (NETIF_F_RXALL | NETIF_F_RXCSUM | - NETIF_F_HW_VLAN_CTAG_RX))) - return; + rx_config = RTL_R32(RxConfig); + if (features & NETIF_F_RXALL) + rx_config |= (AcceptErr | AcceptRunt); + else + rx_config &= ~(AcceptErr | AcceptRunt); - if (changed & (NETIF_F_RXCSUM | NETIF_F_HW_VLAN_CTAG_RX)) { - if (features & NETIF_F_RXCSUM) - tp->cp_cmd |= RxChkSum; - else - tp->cp_cmd &= ~RxChkSum; + RTL_W32(RxConfig, rx_config); - if (features & NETIF_F_HW_VLAN_CTAG_RX) - tp->cp_cmd |= RxVlan; - else - tp->cp_cmd &= ~RxVlan; + if (features & NETIF_F_RXCSUM) + tp->cp_cmd |= RxChkSum; + else + tp->cp_cmd &= ~RxChkSum; - RTL_W16(CPlusCmd, tp->cp_cmd); - RTL_R16(CPlusCmd); - } - if (changed & NETIF_F_RXALL) { - int tmp = (RTL_R32(RxConfig) & ~(AcceptErr | AcceptRunt)); - if (features & NETIF_F_RXALL) - tmp |= (AcceptErr | AcceptRunt); - RTL_W32(RxConfig, tmp); - } + if (features & NETIF_F_HW_VLAN_CTAG_RX) + tp->cp_cmd |= RxVlan; + else + tp->cp_cmd &= ~RxVlan; + + tp->cp_cmd |= RTL_R16(CPlusCmd) & ~(RxVlan | RxChkSum); + + RTL_W16(CPlusCmd, tp->cp_cmd); + RTL_R16(CPlusCmd); } static int rtl8169_set_features(struct net_device *dev, @@ -1817,8 +1815,11 @@ static int rtl8169_set_features(struct net_device *dev, { struct rtl8169_private *tp = netdev_priv(dev); + features &= NETIF_F_RXALL | NETIF_F_RXCSUM | NETIF_F_HW_VLAN_CTAG_RX; + rtl_lock_work(tp); - __rtl8169_set_features(dev, features); + if (features ^ dev->features); + __rtl8169_set_features(dev, features); rtl_unlock_work(tp); return 0; @@ -6707,12 +6708,7 @@ static int rtl_open(struct net_device *dev) rtl8169_init_phy(dev, tp); - if (dev->features & NETIF_F_HW_VLAN_CTAG_RX) - tp->cp_cmd |= RxVlan; - else - tp->cp_cmd &= ~RxVlan; - - RTL_W16(CPlusCmd, tp->cp_cmd); + __rtl8169_set_features(dev, dev->features); rtl_pll_power_up(tp); @@ -7123,8 +7119,7 @@ static void rtl_hw_initialize(struct rtl8169_private *tp) } } -static int -rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) +static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) { const struct rtl_cfg_info *cfg = rtl_cfg_infos + ent->driver_data; const unsigned int region = cfg->region; @@ -7199,7 +7194,7 @@ rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) goto err_out_mwi_2; } - tp->cp_cmd = RxChkSum; + tp->cp_cmd = 0; if ((sizeof(dma_addr_t) > 4) && !pci_set_dma_mask(pdev, DMA_BIT_MASK(64)) && use_dac) { @@ -7240,13 +7235,6 @@ rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) pci_set_master(pdev); - /* - * Pretend we are using VLANs; This bypasses a nasty bug where - * Interrupts stop flowing on high load on 8110SCd controllers. - */ - if (tp->mac_version == RTL_GIGA_MAC_VER_05) - tp->cp_cmd |= RxVlan; - rtl_init_mdio_ops(tp); rtl_init_pll_power_ops(tp); rtl_init_jumbo_ops(tp); @@ -7307,8 +7295,14 @@ rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO | NETIF_F_HIGHDMA; + tp->cp_cmd |= RxChkSum | RxVlan; + + /* + * Pretend we are using VLANs; This bypasses a nasty bug where + * Interrupts stop flowing on high load on 8110SCd controllers. + */ if (tp->mac_version == RTL_GIGA_MAC_VER_05) - /* 8110SCd requires hardware Rx VLAN - disallow toggling */ + /* Disallow toggling */ dev->hw_features &= ~NETIF_F_HW_VLAN_CTAG_RX; if (tp->txd_version == RTL_TD_0) ^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [PATCH net 1/2] r8169: fix the default setting of rx vlan 2014-09-16 0:03 ` Francois Romieu @ 2014-09-16 3:23 ` Hayes Wang 2014-09-16 3:40 ` [PATCH net] r8169: adjust __rtl8169_set_features Hayes Wang 1 sibling, 0 replies; 10+ messages in thread From: Hayes Wang @ 2014-09-16 3:23 UTC (permalink / raw) To: Francois Romieu; +Cc: netdev, nic_swsd, linux-kernel From: Francois Romieu [mailto:romieu@fr.zoreil.com] > Sent: Tuesday, September 16, 2014 8:03 AM [...] > However it's getting messy and the code stomps CPlusCmd in > rtl_open right > before it's given a chance to be read in hw_start. Something like the > untested patch below should avoid it and keep related things together. You are right. Your patch is more reasonable. I would sent the patch. Thanks. Best Regards, Hayes ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net] r8169: adjust __rtl8169_set_features 2014-09-16 0:03 ` Francois Romieu 2014-09-16 3:23 ` Hayes Wang @ 2014-09-16 3:40 ` Hayes Wang 2014-09-16 19:56 ` David Miller 1 sibling, 1 reply; 10+ messages in thread From: Hayes Wang @ 2014-09-16 3:40 UTC (permalink / raw) To: netdev; +Cc: nic_swsd, linux-kernel, Hayes Wang, Francois Romieu Remove the "changed" test in __rtl8169_set_features(). Instead, do simple test in rtl8169_set_features(). Set the RxChkSum and RxVlan through __rtl8169_set_features() in rtl_open(). Signed-off-by: Francois Romieu <romieu@fr.zoreil.com> Signed-off-by: Hayes Wang <hayeswang@realtek.com> --- drivers/net/ethernet/realtek/r8169.c | 74 +++++++++++++++++------------------- 1 file changed, 34 insertions(+), 40 deletions(-) diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c index 7a7860a..ef2cee5 100644 --- a/drivers/net/ethernet/realtek/r8169.c +++ b/drivers/net/ethernet/realtek/r8169.c @@ -1783,33 +1783,31 @@ static void __rtl8169_set_features(struct net_device *dev, netdev_features_t features) { struct rtl8169_private *tp = netdev_priv(dev); - netdev_features_t changed = features ^ dev->features; void __iomem *ioaddr = tp->mmio_addr; + u32 rx_config; - if (!(changed & (NETIF_F_RXALL | NETIF_F_RXCSUM | - NETIF_F_HW_VLAN_CTAG_RX))) - return; + rx_config = RTL_R32(RxConfig); + if (features & NETIF_F_RXALL) + rx_config |= (AcceptErr | AcceptRunt); + else + rx_config &= ~(AcceptErr | AcceptRunt); - if (changed & (NETIF_F_RXCSUM | NETIF_F_HW_VLAN_CTAG_RX)) { - if (features & NETIF_F_RXCSUM) - tp->cp_cmd |= RxChkSum; - else - tp->cp_cmd &= ~RxChkSum; + RTL_W32(RxConfig, rx_config); - if (features & NETIF_F_HW_VLAN_CTAG_RX) - tp->cp_cmd |= RxVlan; - else - tp->cp_cmd &= ~RxVlan; + if (features & NETIF_F_RXCSUM) + tp->cp_cmd |= RxChkSum; + else + tp->cp_cmd &= ~RxChkSum; - RTL_W16(CPlusCmd, tp->cp_cmd); - RTL_R16(CPlusCmd); - } - if (changed & NETIF_F_RXALL) { - int tmp = (RTL_R32(RxConfig) & ~(AcceptErr | AcceptRunt)); - if (features & NETIF_F_RXALL) - tmp |= (AcceptErr | AcceptRunt); - RTL_W32(RxConfig, tmp); - } + if (features & NETIF_F_HW_VLAN_CTAG_RX) + tp->cp_cmd |= RxVlan; + else + tp->cp_cmd &= ~RxVlan; + + tp->cp_cmd |= RTL_R16(CPlusCmd) & ~(RxVlan | RxChkSum); + + RTL_W16(CPlusCmd, tp->cp_cmd); + RTL_R16(CPlusCmd); } static int rtl8169_set_features(struct net_device *dev, @@ -1817,8 +1815,11 @@ static int rtl8169_set_features(struct net_device *dev, { struct rtl8169_private *tp = netdev_priv(dev); + features &= NETIF_F_RXALL | NETIF_F_RXCSUM | NETIF_F_HW_VLAN_CTAG_RX; + rtl_lock_work(tp); - __rtl8169_set_features(dev, features); + if (features ^ dev->features); + __rtl8169_set_features(dev, features); rtl_unlock_work(tp); return 0; @@ -6707,12 +6708,7 @@ static int rtl_open(struct net_device *dev) rtl8169_init_phy(dev, tp); - if (dev->features & NETIF_F_HW_VLAN_CTAG_RX) - tp->cp_cmd |= RxVlan; - else - tp->cp_cmd &= ~RxVlan; - - RTL_W16(CPlusCmd, tp->cp_cmd); + __rtl8169_set_features(dev, dev->features); rtl_pll_power_up(tp); @@ -7123,8 +7119,7 @@ static void rtl_hw_initialize(struct rtl8169_private *tp) } } -static int -rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) +static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) { const struct rtl_cfg_info *cfg = rtl_cfg_infos + ent->driver_data; const unsigned int region = cfg->region; @@ -7199,7 +7194,7 @@ rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) goto err_out_mwi_2; } - tp->cp_cmd = RxChkSum; + tp->cp_cmd = 0; if ((sizeof(dma_addr_t) > 4) && !pci_set_dma_mask(pdev, DMA_BIT_MASK(64)) && use_dac) { @@ -7240,13 +7235,6 @@ rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) pci_set_master(pdev); - /* - * Pretend we are using VLANs; This bypasses a nasty bug where - * Interrupts stop flowing on high load on 8110SCd controllers. - */ - if (tp->mac_version == RTL_GIGA_MAC_VER_05) - tp->cp_cmd |= RxVlan; - rtl_init_mdio_ops(tp); rtl_init_pll_power_ops(tp); rtl_init_jumbo_ops(tp); @@ -7307,8 +7295,14 @@ rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO | NETIF_F_HIGHDMA; + tp->cp_cmd |= RxChkSum | RxVlan; + + /* + * Pretend we are using VLANs; This bypasses a nasty bug where + * Interrupts stop flowing on high load on 8110SCd controllers. + */ if (tp->mac_version == RTL_GIGA_MAC_VER_05) - /* 8110SCd requires hardware Rx VLAN - disallow toggling */ + /* Disallow toggling */ dev->hw_features &= ~NETIF_F_HW_VLAN_CTAG_RX; if (tp->txd_version == RTL_TD_0) -- 1.9.3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net] r8169: adjust __rtl8169_set_features 2014-09-16 3:40 ` [PATCH net] r8169: adjust __rtl8169_set_features Hayes Wang @ 2014-09-16 19:56 ` David Miller 0 siblings, 0 replies; 10+ messages in thread From: David Miller @ 2014-09-16 19:56 UTC (permalink / raw) To: hayeswang; +Cc: netdev, nic_swsd, linux-kernel, romieu From: Hayes Wang <hayeswang@realtek.com> Date: Tue, 16 Sep 2014 11:40:47 +0800 > Remove the "changed" test in __rtl8169_set_features(). Instead, do > simple test in rtl8169_set_features(). > > Set the RxChkSum and RxVlan through __rtl8169_set_features() in > rtl_open(). > > Signed-off-by: Francois Romieu <romieu@fr.zoreil.com> > Signed-off-by: Hayes Wang <hayeswang@realtek.com> Applied, thanks. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net 2/2] r8169: fix setting rx vlan 2014-09-12 3:35 [PATCH net 0/2] r8169: fix rx vlan Hayes Wang 2014-09-12 3:35 ` [PATCH net 1/2] r8169: fix the default setting of " Hayes Wang @ 2014-09-12 3:35 ` Hayes Wang 2014-09-13 20:53 ` [PATCH net 0/2] r8169: fix " David Miller 2 siblings, 0 replies; 10+ messages in thread From: Hayes Wang @ 2014-09-12 3:35 UTC (permalink / raw) To: netdev; +Cc: nic_swsd, linux-kernel, Hayes Wang The setting should depend on the new features not the current one. Signed-off-by: Hayes Wang <hayeswang@realtek.com> --- drivers/net/ethernet/realtek/r8169.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c index f3ce284..7a7860a 100644 --- a/drivers/net/ethernet/realtek/r8169.c +++ b/drivers/net/ethernet/realtek/r8169.c @@ -1796,7 +1796,7 @@ static void __rtl8169_set_features(struct net_device *dev, else tp->cp_cmd &= ~RxChkSum; - if (dev->features & NETIF_F_HW_VLAN_CTAG_RX) + if (features & NETIF_F_HW_VLAN_CTAG_RX) tp->cp_cmd |= RxVlan; else tp->cp_cmd &= ~RxVlan; -- 1.9.3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net 0/2] r8169: fix rx vlan 2014-09-12 3:35 [PATCH net 0/2] r8169: fix rx vlan Hayes Wang 2014-09-12 3:35 ` [PATCH net 1/2] r8169: fix the default setting of " Hayes Wang 2014-09-12 3:35 ` [PATCH net 2/2] r8169: fix setting rx vlan Hayes Wang @ 2014-09-13 20:53 ` David Miller 2 siblings, 0 replies; 10+ messages in thread From: David Miller @ 2014-09-13 20:53 UTC (permalink / raw) To: hayeswang; +Cc: netdev, nic_swsd, linux-kernel From: Hayes Wang <hayeswang@realtek.com> Date: Fri, 12 Sep 2014 11:35:10 +0800 > There are two issues for hw rx vlan. The patches are > used to fix them. Series applied, thanks. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-09-16 19:58 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-09-12 3:35 [PATCH net 0/2] r8169: fix rx vlan Hayes Wang 2014-09-12 3:35 ` [PATCH net 1/2] r8169: fix the default setting of " Hayes Wang 2014-09-12 19:40 ` Francois Romieu 2014-09-15 3:22 ` Hayes Wang 2014-09-16 0:03 ` Francois Romieu 2014-09-16 3:23 ` Hayes Wang 2014-09-16 3:40 ` [PATCH net] r8169: adjust __rtl8169_set_features Hayes Wang 2014-09-16 19:56 ` David Miller 2014-09-12 3:35 ` [PATCH net 2/2] r8169: fix setting rx vlan Hayes Wang 2014-09-13 20:53 ` [PATCH net 0/2] r8169: fix " David Miller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.