* [PATCH] e1000 in linux-3.18.0: a potential bug
@ 2014-12-20 7:50 Jia-Ju Bai
2014-12-20 10:34 ` [linux-nics] " Jeff Kirsher
2014-12-20 13:27 ` Sergei Shtylyov
0 siblings, 2 replies; 6+ messages in thread
From: Jia-Ju Bai @ 2014-12-20 7:50 UTC (permalink / raw)
To: todd.fujinaka, netdev; +Cc: linux.nics, e1000-devel
I have actually tested e1000 driver on the real hardware(Intel 82540EM PCI
Gigabit Ethernet Controller), and find a potential bug:
The target file is drivers/net/ethernet/intel/e1000/e1000_main.c, which is
used to build e1000.ko.
(1) In the normal process, netif_napi_add is called in e1000_probe, but
netif_napi_del is not called in e1000_remove. However, many other ethernet
card drivers call them in pairs, even in the error handling paths, such as
r8169 and igb.
Meanwhile, I also write the patch to fix the bug. I have run the patch on
the hardware, it can work normally and fix the above bug.
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c
b/drivers/net/ethernet/intel/e1000/e1000_main.c
index 24f3986..f6def7b 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -1004,7 +1004,7 @@ static int e1000_probe(struct pci_dev *pdev, const
struct pci_device_id *ent)
/* make ready for any if (hw->...) below */
err = e1000_init_hw_struct(adapter, hw);
if (err)
- goto err_sw_init;
+ goto err_dma;
/* there is a workaround being applied below that limits
* 64-bit DMA addresses to 64-bit hardware. There are some
@@ -1239,8 +1239,9 @@ err_eeprom:
iounmap(hw->flash_address);
kfree(adapter->tx_ring);
kfree(adapter->rx_ring);
-err_dma:
err_sw_init:
+ netif_napi_del(&adapter->napi);
+err_dma:
err_mdio_ioremap:
iounmap(hw->ce4100_gbe_mdio_base_virt);
iounmap(hw->hw_addr);
@@ -1271,6 +1272,7 @@ static void e1000_remove(struct pci_dev *pdev)
e1000_down_and_stop(adapter);
e1000_release_manageability(adapter);
+ netif_napi_del(&adapter->napi);
unregister_netdev(netdev);
e1000_phy_hw_reset(hw);
Thanks!
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [linux-nics] [PATCH] e1000 in linux-3.18.0: a potential bug
2014-12-20 7:50 [PATCH] e1000 in linux-3.18.0: a potential bug Jia-Ju Bai
@ 2014-12-20 10:34 ` Jeff Kirsher
[not found] ` <79ea17da.119c.14a67bcadd1.Coremail.baijiaju1990@163.com>
2014-12-20 13:27 ` Sergei Shtylyov
1 sibling, 1 reply; 6+ messages in thread
From: Jeff Kirsher @ 2014-12-20 10:34 UTC (permalink / raw)
To: Jia-Ju Bai; +Cc: todd.fujinaka, netdev, e1000-devel, linux.nics
[-- Attachment #1: Type: text/plain, Size: 936 bytes --]
On Sat, 2014-12-20 at 15:50 +0800, Jia-Ju Bai wrote:
> I have actually tested e1000 driver on the real hardware(Intel 82540EM
> PCI
> Gigabit Ethernet Controller), and find a potential bug:
> The target file is drivers/net/ethernet/intel/e1000/e1000_main.c,
> which is
> used to build e1000.ko.
>
> (1) In the normal process, netif_napi_add is called in e1000_probe,
> but
> netif_napi_del is not called in e1000_remove. However, many other
> ethernet
> card drivers call them in pairs, even in the error handling paths,
> such as
> r8169 and igb.
>
> Meanwhile, I also write the patch to fix the bug. I have run the patch
> on
> the hardware, it can work normally and fix the above bug.
Was this a bug you actually saw? Or a theoretical bug based on code
review?
I do not mind adding this to my queue so that we can review and test the
patch, although this will cause a fair amount of regression testing.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re:Re: [linux-nics] [PATCH] e1000 in linux-3.18.0: a potential bug
[not found] ` <79ea17da.119c.14a67bcadd1.Coremail.baijiaju1990@163.com>
@ 2014-12-20 12:49 ` Jeff Kirsher
0 siblings, 0 replies; 6+ messages in thread
From: Jeff Kirsher @ 2014-12-20 12:49 UTC (permalink / raw)
To: 白家驹; +Cc: netdev, e1000-devel
[-- Attachment #1: Type: text/plain, Size: 1389 bytes --]
On Sat, 2014-12-20 at 20:47 +0800, 白家驹 wrote:
> Thanks for the reply!
> I run the driver normally, and monitor all function calls in runtime,
> and then find this violation.
Adding netdev and e1000-devel back onto the CC since Jia-Ju Bai removed
them in his reply...
>
> At 2014-12-20 18:34:20,"Jeff Kirsher" <jeffrey.t.kirsher@intel.com> wrote:
> >On Sat, 2014-12-20 at 15:50 +0800, Jia-Ju Bai wrote:
> >> I have actually tested e1000 driver on the real hardware(Intel 82540EM
> >> PCI
> >> Gigabit Ethernet Controller), and find a potential bug:
> >> The target file is drivers/net/ethernet/intel/e1000/e1000_main.c,
> >> which is
> >> used to build e1000.ko.
> >>
> >> (1) In the normal process, netif_napi_add is called in e1000_probe,
> >> but
> >> netif_napi_del is not called in e1000_remove. However, many other
> >> ethernet
> >> card drivers call them in pairs, even in the error handling paths,
> >> such as
> >> r8169 and igb.
> >>
> >> Meanwhile, I also write the patch to fix the bug. I have run the patch
> >> on
> >> the hardware, it can work normally and fix the above bug.
> >
> >Was this a bug you actually saw? Or a theoretical bug based on code
> >review?
> >
> >I do not mind adding this to my queue so that we can review and test the
> >patch, although this will cause a fair amount of regression testing.
>
>
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] e1000 in linux-3.18.0: a potential bug
2014-12-20 7:50 [PATCH] e1000 in linux-3.18.0: a potential bug Jia-Ju Bai
2014-12-20 10:34 ` [linux-nics] " Jeff Kirsher
@ 2014-12-20 13:27 ` Sergei Shtylyov
1 sibling, 0 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2014-12-20 13:27 UTC (permalink / raw)
To: Jia-Ju Bai, todd.fujinaka, netdev; +Cc: linux.nics, e1000-devel
Hello.
On 12/20/2014 10:50 AM, Jia-Ju Bai wrote:
> I have actually tested e1000 driver on the real hardware(Intel 82540EM PCI
> Gigabit Ethernet Controller), and find a potential bug:
> The target file is drivers/net/ethernet/intel/e1000/e1000_main.c, which is
> used to build e1000.ko.
> (1) In the normal process, netif_napi_add is called in e1000_probe, but
> netif_napi_del is not called in e1000_remove. However, many other ethernet
> card drivers call them in pairs, even in the error handling paths, such as
> r8169 and igb.
> Meanwhile, I also write the patch to fix the bug. I have run the patch on
> the hardware, it can work normally and fix the above bug.
You didn't sign off on your patch, so it can't be applied.
WBR, Sergei
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] e1000 in linux-3.18.0: a potential bug
@ 2014-12-20 13:57 Jia-Ju Bai
2014-12-20 14:15 ` Sergei Shtylyov
0 siblings, 1 reply; 6+ messages in thread
From: Jia-Ju Bai @ 2014-12-20 13:57 UTC (permalink / raw)
To: sergei.shtylyov, netdev
Maybe the email can not be displayed fully...
I send my patch again:
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c
b/drivers/net/ethernet/intel/e1000/e1000_main.c
index 24f3986..f6def7b 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -1004,7 +1004,7 @@ static int e1000_probe(struct pci_dev *pdev, const
struct pci_device_id *ent)
/* make ready for any if (hw->...) below */
err = e1000_init_hw_struct(adapter, hw);
if (err)
- goto err_sw_init;
+ goto err_dma;
/* there is a workaround being applied below that limits
* 64-bit DMA addresses to 64-bit hardware. There are some
@@ -1239,8 +1239,9 @@ err_eeprom:
iounmap(hw->flash_address);
kfree(adapter->tx_ring);
kfree(adapter->rx_ring);
-err_dma:
err_sw_init:
+ netif_napi_del(&adapter->napi);
+err_dma:
err_mdio_ioremap:
iounmap(hw->ce4100_gbe_mdio_base_virt);
iounmap(hw->hw_addr);
@@ -1271,6 +1272,7 @@ static void e1000_remove(struct pci_dev *pdev)
e1000_down_and_stop(adapter);
e1000_release_manageability(adapter);
+ netif_napi_del(&adapter->napi);
unregister_netdev(netdev);
e1000_phy_hw_reset(hw);
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] e1000 in linux-3.18.0: a potential bug
2014-12-20 13:57 Jia-Ju Bai
@ 2014-12-20 14:15 ` Sergei Shtylyov
0 siblings, 0 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2014-12-20 14:15 UTC (permalink / raw)
To: Jia-Ju Bai, netdev
On 12/20/2014 4:57 PM, Jia-Ju Bai wrote:
> Maybe the email can not be displayed fully...
It was displayed alright.
> I send my patch again:
There was no need, especially without the patch description.
WBR, Sergei
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-12-20 14:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-12-20 7:50 [PATCH] e1000 in linux-3.18.0: a potential bug Jia-Ju Bai
2014-12-20 10:34 ` [linux-nics] " Jeff Kirsher
[not found] ` <79ea17da.119c.14a67bcadd1.Coremail.baijiaju1990@163.com>
2014-12-20 12:49 ` Jeff Kirsher
2014-12-20 13:27 ` Sergei Shtylyov
-- strict thread matches above, loose matches on Subject: below --
2014-12-20 13:57 Jia-Ju Bai
2014-12-20 14:15 ` Sergei Shtylyov
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).