Re: bnx2 breaks Dell R815 BMC IPMI since 4.8

[Baoquan He]

Sorry, Brice. This has been reported by people, and it has been fixed by
later post. The commits within linus's tree are:

commit 6df77862f63f389df3b1ad879738e04440d7385d
Author: Baoquan He <b...@redhat.com>
Date:   Sun Nov 13 13:01:33 2016 +0800

bnx2: Wait for in-flight DMA to complete at probe stage

commit 5d0d4b91bf627f14f95167b738d524156c9d440b
Author: Baoquan He <b...@redhat.com>
Date:   Sun Nov 13 13:01:32 2016 +0800

Revert "bnx2: Reset device during driver initialization"

This reverts commit 3e1be7ad2d38c6bd6aeef96df9bd0a7822f4e51c.

And I believe both of them also are picked up into 4.8-stable kernel.
Please have a way to get them.

Sorry again!

Thanks
Baoquan


On 11/29/16 at 07:57am, Brice Goglin wrote:
> Hello
> 
> My Dell PowerEdge R815 doesn't have IPMI anymore when I boot a 4.8
> kernel, the BMC doesn't even ping anymore. Its Ethernet devices are 4 of
> those:
> 
> 01:00.0 Ethernet controller: Broadcom Corporation NetXtreme II BCM5709 
> Gigabit Ethernet (rev 20)
>   DeviceName: Embedded NIC 1  
>   Subsystem: Dell NetXtreme II BCM5709 Gigabit Ethernet
>   Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- 
> Stepping- SERR- FastB2B- DisINTx+
>   Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort-  SERR-Latency: 0, Cache Line Size: 64 bytes
>   Interrupt: pin A routed to IRQ 42
>   Region 0: Memory at e600 (64-bit, non-prefetchable) [size=32M]
>   Capabilities: 
>   Kernel driver in use: bnx2
>   Kernel modules: bnx2
> 
> The only change in bnx2 between 4.7 and 4.8 appears to be this one:
> 
> commit 3e1be7ad2d38c6bd6aeef96df9bd0a7822f4e51c
> Author: Baoquan He <b...@redhat.com>
> Date:   Fri Sep 9 22:43:12 2016 +0800
> 
> bnx2: Reset device during driver initialization
> 
> Could you patch actually break the BMC? What do I need to further debug
> this issue?
> 
> Thanks
> Brice
> 

Re: [PATCH v2 0/2] bnx2: Wait for in-flight DMA to complete at probe stage

[Baoquan He]

On 11/14/16 at 09:25am, Paul Menzel wrote:
> Dear Baoquan,
> 
> On 11/13/16 06:01, Baoquan He wrote:
> > This is v2 post.
> > 
> > In commit 3e1be7a ("bnx2: Reset device during driver initialization"),
> > firmware requesting code was moved from open stage to probe stage.
> > The reason is in kdump kernel hardware iommu need device be reset in
> > driver probe stage, otherwise those in-flight DMA from 1st kernel
> > will continue going and look up into the newly created io-page tables.
> > However bnx2 chip resetting involves firmware requesting issue, that
> > need be done in open stage.
> > 
> > Michale Chan suggested we can just wait for the old in-flight DMA to
> > complete at probe stage, then though without device resetting, we
> > don't need to worry the old in-flight DMA could continue looking up
> > the newly created io-page tables.
> > 
> > v1->v2:
> > Michael suggested to wait for the in-flight DMA to complete at probe
> > stage. So give up the old method of trying to reset chip at probe
> > stage, take the new way accordingly.
> 
> thank you for posting the updated series. Could you please resend a v3 with
> sta...@vger.kernel.org added [1]?

I can add it like:
Cc: <sta...@vger.kernel.org> # 4.8.7

Only v4.8.7, right?

Thanks
Baoquan

[PATCH v2 1/2] Revert "bnx2: Reset device during driver initialization"

[Baoquan He]

This reverts commit 3e1be7ad2d38c6bd6aeef96df9bd0a7822f4e51c.

When people build bnx2 driver into kernel, it will fail to detect
and load firmware because firmware is contained in initramfs and
initramfs has not been uncompressed yet during do_initcalls. So
revert commit 3e1be7a and work out a new way in the later patch.

Signed-off-by: Baoquan He <b...@redhat.com>
Acked-by: Paul Menzel <pmen...@molgen.mpg.de>
---
 drivers/net/ethernet/broadcom/bnx2.c | 12 +---
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2.c 
b/drivers/net/ethernet/broadcom/bnx2.c
index b3791b3..c557972 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -6361,6 +6361,10 @@ bnx2_open(struct net_device *dev)
struct bnx2 *bp = netdev_priv(dev);
int rc;
 
+   rc = bnx2_request_firmware(bp);
+   if (rc < 0)
+   goto out;
+
netif_carrier_off(dev);
 
bnx2_disable_int(bp);
@@ -6429,6 +6433,7 @@ bnx2_open(struct net_device *dev)
bnx2_free_irq(bp);
bnx2_free_mem(bp);
bnx2_del_napi(bp);
+   bnx2_release_firmware(bp);
goto out;
 }
 
@@ -8575,12 +8580,6 @@ bnx2_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
 
pci_set_drvdata(pdev, dev);
 
-   rc = bnx2_request_firmware(bp);
-   if (rc < 0)
-   goto error;
-
-
-   bnx2_reset_chip(bp, BNX2_DRV_MSG_CODE_RESET);
memcpy(dev->dev_addr, bp->mac_addr, ETH_ALEN);
 
dev->hw_features = NETIF_F_IP_CSUM | NETIF_F_SG |
@@ -8613,7 +8612,6 @@ bnx2_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
return 0;
 
 error:
-   bnx2_release_firmware(bp);
pci_iounmap(pdev, bp->regview);
pci_release_regions(pdev);
pci_disable_device(pdev);
-- 
2.5.5

[PATCH v2 0/2] bnx2: Wait for in-flight DMA to complete at probe stage

[Baoquan He]

This is v2 post.

In commit 3e1be7a ("bnx2: Reset device during driver initialization"),
firmware requesting code was moved from open stage to probe stage.
The reason is in kdump kernel hardware iommu need device be reset in
driver probe stage, otherwise those in-flight DMA from 1st kernel
will continue going and look up into the newly created io-page tables.
However bnx2 chip resetting involves firmware requesting issue, that
need be done in open stage. 

Michale Chan suggested we can just wait for the old in-flight DMA to
complete at probe stage, then though without device resetting, we
don't need to worry the old in-flight DMA could continue looking up 
the newly created io-page tables.

v1->v2:
Michael suggested to wait for the in-flight DMA to complete at probe
stage. So give up the old method of trying to reset chip at probe
stage, take the new way accordingly.


Baoquan He (2):
  Revert "bnx2: Reset device during driver initialization"
  bnx2: Wait for in-flight DMA to complete at probe stage

 drivers/net/ethernet/broadcom/bnx2.c | 48 +++-
 1 file changed, 36 insertions(+), 12 deletions(-)

-- 
2.5.5

[PATCH v2 2/2] bnx2: Wait for in-flight DMA to complete at probe stage

[Baoquan He]

In-flight DMA from 1st kernel could continue going in kdump kernel.
New io-page table has been created before bnx2 does reset at open stage.
We have to wait for the in-flight DMA to complete to avoid it look up
into the newly created io-page table at probe stage.

Suggested-by: Michael Chan <michael.c...@broadcom.com>
Signed-off-by: Baoquan He <b...@redhat.com>
---
 drivers/net/ethernet/broadcom/bnx2.c | 38 ++--
 1 file changed, 32 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2.c 
b/drivers/net/ethernet/broadcom/bnx2.c
index c557972..1f7034d 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -49,6 +49,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #if IS_ENABLED(CONFIG_CNIC)
 #define BCM_CNIC 1
@@ -4764,15 +4765,16 @@ bnx2_setup_msix_tbl(struct bnx2 *bp)
BNX2_WR(bp, BNX2_PCI_GRC_WINDOW3_ADDR, BNX2_MSIX_PBA_ADDR);
 }
 
-static int
-bnx2_reset_chip(struct bnx2 *bp, u32 reset_code)
+static void
+bnx2_wait_dma_complete(struct bnx2 *bp)
 {
u32 val;
-   int i, rc = 0;
-   u8 old_port;
+   int i;
 
-   /* Wait for the current PCI transaction to complete before
-* issuing a reset. */
+   /*
+* Wait for the current PCI transaction to complete before
+* issuing a reset.
+*/
if ((BNX2_CHIP(bp) == BNX2_CHIP_5706) ||
(BNX2_CHIP(bp) == BNX2_CHIP_5708)) {
BNX2_WR(bp, BNX2_MISC_ENABLE_CLR_BITS,
@@ -4796,6 +4798,21 @@ bnx2_reset_chip(struct bnx2 *bp, u32 reset_code)
}
}
 
+   return;
+}
+
+
+static int
+bnx2_reset_chip(struct bnx2 *bp, u32 reset_code)
+{
+   u32 val;
+   int i, rc = 0;
+   u8 old_port;
+
+   /* Wait for the current PCI transaction to complete before
+* issuing a reset. */
+   bnx2_wait_dma_complete(bp);
+
/* Wait for the firmware to tell us it is ok to issue a reset. */
bnx2_fw_sync(bp, BNX2_DRV_MSG_DATA_WAIT0 | reset_code, 1, 1);
 
@@ -8580,6 +8597,15 @@ bnx2_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
 
pci_set_drvdata(pdev, dev);
 
+   /*
+* In-flight DMA from 1st kernel could continue going in kdump kernel.
+* New io-page table has been created before bnx2 does reset at open 
stage.
+* We have to wait for the in-flight DMA to complete to avoid it look up
+* into the newly created io-page table.
+*/
+   if (is_kdump_kernel())
+   bnx2_wait_dma_complete(bp);
+
memcpy(dev->dev_addr, bp->mac_addr, ETH_ALEN);
 
dev->hw_features = NETIF_F_IP_CSUM | NETIF_F_SG |
-- 
2.5.5

Re: [PATCH v2 2/2] bnx2: Wait for in-flight DMA to complete at probe stage

[Baoquan He]

On 11/12/16 at 11:40pm, David Miller wrote:
> From: Baoquan He <b...@redhat.com>
> Date: Sun, 13 Nov 2016 12:15:24 +0800
> 
> > In-flight DMA from 1st kernel could continue going in kdump kernel.
> > New io-page table has been created before bnx2 does reset at open stage.
> > We have to wait for the in-flight DMA to complete to avoid it look up
> > into the newly created io-page table at probe stage.
> > 
> > Suggested-by: Michael Chan <michael.c...@broadcom.com>
> > Signed-off-by: Baoquan He <b...@redhat.com>
> > ---
> > v1->v2:
> > Michael suggested to wait for the in-flight DMA to complete at probe
> > stage. So give up the old method of trying to reset chip at probe
> > stage, take the new way accordingly.
> 
> Patch updates don't work this way.
> 
> When you update a patch that is part of a patch series, you must
> resubmit the entire series anew.

Thanks for telling, David!

Learned it. I am not very sure if this is what Michael is suggesting.
Will resubmit the entire patch series.

Thanks
Baoquan

[PATCH v2 2/2] bnx2: Wait for in-flight DMA to complete at probe stage

[Baoquan He]

In-flight DMA from 1st kernel could continue going in kdump kernel.
New io-page table has been created before bnx2 does reset at open stage.
We have to wait for the in-flight DMA to complete to avoid it look up
into the newly created io-page table at probe stage.

Suggested-by: Michael Chan <michael.c...@broadcom.com>
Signed-off-by: Baoquan He <b...@redhat.com>
---
v1->v2:
Michael suggested to wait for the in-flight DMA to complete at probe
stage. So give up the old method of trying to reset chip at probe
stage, take the new way accordingly.

 drivers/net/ethernet/broadcom/bnx2.c | 38 ++--
 1 file changed, 32 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2.c 
b/drivers/net/ethernet/broadcom/bnx2.c
index c557972..1f7034d 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -49,6 +49,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #if IS_ENABLED(CONFIG_CNIC)
 #define BCM_CNIC 1
@@ -4764,15 +4765,16 @@ bnx2_setup_msix_tbl(struct bnx2 *bp)
BNX2_WR(bp, BNX2_PCI_GRC_WINDOW3_ADDR, BNX2_MSIX_PBA_ADDR);
 }
 
-static int
-bnx2_reset_chip(struct bnx2 *bp, u32 reset_code)
+static void
+bnx2_wait_dma_complete(struct bnx2 *bp)
 {
u32 val;
-   int i, rc = 0;
-   u8 old_port;
+   int i;
 
-   /* Wait for the current PCI transaction to complete before
-* issuing a reset. */
+   /*
+* Wait for the current PCI transaction to complete before
+* issuing a reset.
+*/
if ((BNX2_CHIP(bp) == BNX2_CHIP_5706) ||
(BNX2_CHIP(bp) == BNX2_CHIP_5708)) {
BNX2_WR(bp, BNX2_MISC_ENABLE_CLR_BITS,
@@ -4796,6 +4798,21 @@ bnx2_reset_chip(struct bnx2 *bp, u32 reset_code)
}
}
 
+   return;
+}
+
+
+static int
+bnx2_reset_chip(struct bnx2 *bp, u32 reset_code)
+{
+   u32 val;
+   int i, rc = 0;
+   u8 old_port;
+
+   /* Wait for the current PCI transaction to complete before
+* issuing a reset. */
+   bnx2_wait_dma_complete(bp);
+
/* Wait for the firmware to tell us it is ok to issue a reset. */
bnx2_fw_sync(bp, BNX2_DRV_MSG_DATA_WAIT0 | reset_code, 1, 1);
 
@@ -8580,6 +8597,15 @@ bnx2_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
 
pci_set_drvdata(pdev, dev);
 
+   /*
+* In-flight DMA from 1st kernel could continue going in kdump kernel.
+* New io-page table has been created before bnx2 does reset at open 
stage.
+* We have to wait for the in-flight DMA to complete to avoid it look up
+* into the newly created io-page table.
+*/
+   if (is_kdump_kernel())
+   bnx2_wait_dma_complete(bp);
+
memcpy(dev->dev_addr, bp->mac_addr, ETH_ALEN);
 
dev->hw_features = NETIF_F_IP_CSUM | NETIF_F_SG |
-- 
2.5.5

Re: [PATCH 0/2] bnx2: Hard reset bnx2 chip at probe stage

[Baoquan He]

Hi Michael,


On 11/11/16 at 09:37am, Michael Chan wrote:
> On Fri, Nov 11, 2016 at 6:02 AM, Baoquan He <b...@redhat.com> wrote:
> > On 11/11/16 at 09:46pm, Baoquan He wrote:
> >> Hi bnx2 experts,
> >>
> >> In commit 3e1be7a ("bnx2: Reset device during driver initialization"),
> >> firmware requesting code was moved from open stage to probe stage.
> >> The reason is in kdump kernel hardware iommu need device be reset in
> >> driver probe stage, otherwise those in-flight DMA from 1st kernel
> >> will continue going and look up into the newly created io-page tables.
> >> So we need reset device to stop in-flight DMA as early as possibe.
> >>
> >> But with commit 3e1be7a merged, people reported their bnx2 driver init
> >> failed because of failed firmware loading. After discussion, it's found
> >> that they built bnx2 driver into kernel, and that makes probe function
> >> bnx2_init_one be called in do_initcalls(). But at this time the initramfs
> >> has not been uncompressed yet and mounted, kernel can't detect firmware.
> >>
> >> So there's only one way to cover both. Try to hard reset the bnx2 device
> >> at probe stage, without involving firmware issues. I tried to add function
> >> bnx2_hard_reset_chip() to do this and it's only called in kdump kernel.
> >> The thing is I am not quite familiar with bnx2 chip spec, just abstract
> >> code from bnx2_reset_chip, the testing result is good.
> >
> > Here I changed to send BNX2_MISC_COMMAND_HD_RESET in BNX2_CHIP_5709
> > case.
> >
> 
> From my old 5709 Documentation:
> 
> Bit 6 HD_RESET:  Writing this bit as 1 will cause the chip to do a
> hard reset like bit 5 except the sticky bits in the PCI function are
> not reset.
> 
> Bit 5 POR_RESET: Writing this bit as 1 will cause the chip to do an
> internal reset exactly like a power-up reset.  There is no protection
> for this request and it may cause any current PCI cycle to lock up.
> This reset is intended for use under manufacturing conditions only.
> 
> So it sounds like doing HD_RESET can potentially cause a PCI bus lock up.
> 
> Why not just disable DMA gracefully as done at the beginning of
> bnx2_reset_chip()?

Thanks for your suggestion.

If what I undertand is correct, you meant waiting for the current PCI
transaction to complete, right? I tried and it also works. I like this
idea, will repost v2. Please help check if it meets your thoughts.

Thanks
Baoquan

Re: [PATCH 0/2] bnx2: Hard reset bnx2 chip at probe stage

[Baoquan He]

On 11/11/16 at 09:46pm, Baoquan He wrote:
> Hi bnx2 experts,
> 
> In commit 3e1be7a ("bnx2: Reset device during driver initialization"),
> firmware requesting code was moved from open stage to probe stage.
> The reason is in kdump kernel hardware iommu need device be reset in
> driver probe stage, otherwise those in-flight DMA from 1st kernel
> will continue going and look up into the newly created io-page tables.
> So we need reset device to stop in-flight DMA as early as possibe.
> 
> But with commit 3e1be7a merged, people reported their bnx2 driver init
> failed because of failed firmware loading. After discussion, it's found
> that they built bnx2 driver into kernel, and that makes probe function
> bnx2_init_one be called in do_initcalls(). But at this time the initramfs
> has not been uncompressed yet and mounted, kernel can't detect firmware.
> 
> So there's only one way to cover both. Try to hard reset the bnx2 device
> at probe stage, without involving firmware issues. I tried to add function
> bnx2_hard_reset_chip() to do this and it's only called in kdump kernel.
> The thing is I am not quite familiar with bnx2 chip spec, just abstract
> code from bnx2_reset_chip, the testing result is good.

Here I changed to send BNX2_MISC_COMMAND_HD_RESET in BNX2_CHIP_5709
case.

> 
> Any suggestions are welcomed and much appreciated!
> 
> Baoquan He (2):
>   Revert "bnx2: Reset device during driver initialization"
>   bnx2: Hard reset bnx2 chip at probe stage
> 
>  drivers/net/ethernet/broadcom/bnx2.c | 70 
> +---
>  1 file changed, 65 insertions(+), 5 deletions(-)
> 
> -- 
> 2.5.5
> 

[PATCH 0/2] bnx2: Hard reset bnx2 chip at probe stage

[Baoquan He]

Hi bnx2 experts,

In commit 3e1be7a ("bnx2: Reset device during driver initialization"),
firmware requesting code was moved from open stage to probe stage.
The reason is in kdump kernel hardware iommu need device be reset in
driver probe stage, otherwise those in-flight DMA from 1st kernel
will continue going and look up into the newly created io-page tables.
So we need reset device to stop in-flight DMA as early as possibe.

But with commit 3e1be7a merged, people reported their bnx2 driver init
failed because of failed firmware loading. After discussion, it's found
that they built bnx2 driver into kernel, and that makes probe function
bnx2_init_one be called in do_initcalls(). But at this time the initramfs
has not been uncompressed yet and mounted, kernel can't detect firmware.

So there's only one way to cover both. Try to hard reset the bnx2 device
at probe stage, without involving firmware issues. I tried to add function
bnx2_hard_reset_chip() to do this and it's only called in kdump kernel.
The thing is I am not quite familiar with bnx2 chip spec, just abstract
code from bnx2_reset_chip, the testing result is good.

Any suggestions are welcomed and much appreciated!

Baoquan He (2):
  Revert "bnx2: Reset device during driver initialization"
  bnx2: Hard reset bnx2 chip at probe stage

 drivers/net/ethernet/broadcom/bnx2.c | 70 +---
 1 file changed, 65 insertions(+), 5 deletions(-)

-- 
2.5.5

[PATCH 2/2] bnx2: Hard reset bnx2 chip at probe stage

[Baoquan He]

In commit 3e1be7a ("bnx2: Reset device during driver initialization"),
firmware requesting code was moved to driver probe stage, into function
bnx2_init_one. But if bnx2 driver is build into kernel, it will fail
to request firmware because firmware is contained in initramfs, but
initramfs has not been uncommpressed and mounted yet when do_initcalls
called.

So in order to reset the device at probe stage, have to hard reset bnx2
chip wihtout involving firmware handling. So in this patch add function
bnx2_hard_reset_chip which only trys to hard reset bnx2 chip and only
will be called in kdump kernel.

Signed-off-by: Baoquan He <b...@redhat.com>
---
 drivers/net/ethernet/broadcom/bnx2.c | 62 
 1 file changed, 62 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnx2.c 
b/drivers/net/ethernet/broadcom/bnx2.c
index c557972..84e3f12 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -49,6 +49,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #if IS_ENABLED(CONFIG_CNIC)
 #define BCM_CNIC 1
@@ -4765,6 +4766,58 @@ bnx2_setup_msix_tbl(struct bnx2 *bp)
 }
 
 static int
+bnx2_hard_reset_chip(struct bnx2 *bp)
+{
+   u32 val;
+   int i, rc = 0;
+
+   if (BNX2_CHIP(bp) == BNX2_CHIP_5709) {
+   BNX2_WR(bp, BNX2_MISC_COMMAND, BNX2_MISC_COMMAND_HD_RESET);
+   BNX2_RD(bp, BNX2_MISC_COMMAND);
+   udelay(5);
+
+   val = BNX2_PCICFG_MISC_CONFIG_REG_WINDOW_ENA |
+ BNX2_PCICFG_MISC_CONFIG_TARGET_MB_WORD_SWAP;
+
+   BNX2_WR(bp, BNX2_PCICFG_MISC_CONFIG, val);
+
+   } else {
+   val = BNX2_PCICFG_MISC_CONFIG_CORE_RST_REQ |
+ BNX2_PCICFG_MISC_CONFIG_REG_WINDOW_ENA |
+ BNX2_PCICFG_MISC_CONFIG_TARGET_MB_WORD_SWAP;
+
+   /* Chip reset. */
+   BNX2_WR(bp, BNX2_PCICFG_MISC_CONFIG, val);
+
+   /* Reading back any register after chip reset will hang the
+* bus on 5706 A0 and A1.  The msleep below provides plenty
+* of margin for write posting.
+*/
+   if ((BNX2_CHIP_ID(bp) == BNX2_CHIP_ID_5706_A0) ||
+   (BNX2_CHIP_ID(bp) == BNX2_CHIP_ID_5706_A1))
+   msleep(20);
+
+   /* Reset takes approximate 30 usec */
+   for (i = 0; i < 10; i++) {
+   val = BNX2_RD(bp, BNX2_PCICFG_MISC_CONFIG);
+   if ((val & (BNX2_PCICFG_MISC_CONFIG_CORE_RST_REQ |
+   BNX2_PCICFG_MISC_CONFIG_CORE_RST_BSY)) == 0)
+   break;
+   udelay(10);
+   }
+
+   if (val & (BNX2_PCICFG_MISC_CONFIG_CORE_RST_REQ |
+  BNX2_PCICFG_MISC_CONFIG_CORE_RST_BSY)) {
+   pr_err("Chip reset did not complete\n");
+   return -EBUSY;
+   }
+   }
+
+   return rc;
+}
+
+
+static int
 bnx2_reset_chip(struct bnx2 *bp, u32 reset_code)
 {
u32 val;
@@ -8580,6 +8633,15 @@ bnx2_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
 
pci_set_drvdata(pdev, dev);
 
+
+   /*
+* Kdump kernel need reset device at probe stage if hardware iommu
+* is deployed. Otherwise in-flight DMA will continue going until
+* reset is done in open stage.
+*/
+   if (is_kdump_kernel())
+   bnx2_hard_reset_chip(bp);
+
memcpy(dev->dev_addr, bp->mac_addr, ETH_ALEN);
 
dev->hw_features = NETIF_F_IP_CSUM | NETIF_F_SG |
-- 
2.5.5

[PATCH 1/2] Revert "bnx2: Reset device during driver initialization"

[Baoquan He]

This reverts commit 3e1be7ad2d38c6bd6aeef96df9bd0a7822f4e51c.

When people build bnx2 driver into kernel, it will fail to detect
and load firmware because firmware is contained in initramfs and
initramfs has not been uncompressed yet during do_initcalls. So
revert commit 3e1be7a and work out a new way in the later patch.

Signed-off-by: Baoquan He <b...@redhat.com>
---
 drivers/net/ethernet/broadcom/bnx2.c | 12 +---
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2.c 
b/drivers/net/ethernet/broadcom/bnx2.c
index b3791b3..c557972 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -6361,6 +6361,10 @@ bnx2_open(struct net_device *dev)
struct bnx2 *bp = netdev_priv(dev);
int rc;
 
+   rc = bnx2_request_firmware(bp);
+   if (rc < 0)
+   goto out;
+
netif_carrier_off(dev);
 
bnx2_disable_int(bp);
@@ -6429,6 +6433,7 @@ bnx2_open(struct net_device *dev)
bnx2_free_irq(bp);
bnx2_free_mem(bp);
bnx2_del_napi(bp);
+   bnx2_release_firmware(bp);
goto out;
 }
 
@@ -8575,12 +8580,6 @@ bnx2_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
 
pci_set_drvdata(pdev, dev);
 
-   rc = bnx2_request_firmware(bp);
-   if (rc < 0)
-   goto error;
-
-
-   bnx2_reset_chip(bp, BNX2_DRV_MSG_CODE_RESET);
memcpy(dev->dev_addr, bp->mac_addr, ETH_ALEN);
 
dev->hw_features = NETIF_F_IP_CSUM | NETIF_F_SG |
@@ -8613,7 +8612,6 @@ bnx2_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
return 0;
 
 error:
-   bnx2_release_firmware(bp);
pci_iounmap(pdev, bp->regview);
pci_release_regions(pdev);
pci_disable_device(pdev);
-- 
2.5.5

Re: [bnx2] [Regression 4.8] Driver loading fails without firmware

[Baoquan He]

On 10/31/16 at 11:43am, Paul Menzel wrote:
> Dear Baoquan,
> 
> 
> On 10/31/16 11:09, Baoquan He wrote:
> 
> > On 10/26/16 at 12:31pm, Paul Menzel wrote:
> > > Baoquan, could you please fix this regression. My suggestion is, that you
> > > add the old code back, but check if the firmware has been loaded. If it
> > > hasn’t, load it again.
> > > 
> > > That way, people can update their Linux kernel, and it continues working
> > > without changing the initramfs, or anything else.
> > 
> > I checked code and this looks good to me. I can post a patch with this
> > change to upstream, see what maintainers and other reviewers say.
> > 
> > The thing is I don't understand quite well about your requirement. With
> > my understanding, you just didn't add bnx2 firmware into initramfs, but
> > later opening the interface can still request that firmware with "ifup
> > eth-xxx" command. Is that correct? If yes, requesting firmware twice in
> > probing path and opening path looks good.
> > 
> > However I am wondering what's your exact steps to do this.
> > 
> > What I tried to do is I execute command "dracut --add-drivers bnx2 -f
> > /boot/initramfs-4.9.0-rc3+.img 4.9.0-rc3+" to build a new initramfs,
> > meanwhile make sure bnx2.ko is included, then uncompressed initramfs and
> > deleted bnx2 folder under lib/firmware/ of uncompressed initramfs. Then
> > pack them to be /boot/initramfs-4.9.0-rc3+.img and restart. I did saw
> > below failure message. But later how did you really make the bnx2
> > network interface up? Could you say it more specifically?
> > 
> > [7.364186] bnx2: QLogic bnx2 Gigabit Ethernet Driver v2.2.6 (January 
> > 29, 2014)
> > [7.371706] ACPI: PCI Interrupt Link [LN44] enabled at IRQ 44
> > [7.378128] bnx2 :01:00.0: Direct firmware load for 
> > bnx2/bnx2-mips-09-6.2.1b.fw failed with error -2
> > [7.387619] bnx2: Can't load firmware file "bnx2/bnx2-mips-09-6.2.1b.fw"
> > [7.387888] bnx2: probe of :01:00.0 failed with error -2
> > [7.388990] ACPI: PCI Interrupt Link [LN45] enabled at IRQ 45
> > [7.389370] bnx2 :01:00.1: Direct firmware load for 
> > bnx2/bnx2-mips-09-6.2.1b.fw failed with error -2
> > [7.389371] bnx2: Can't load firmware file "bnx2/bnx2-mips-09-6.2.1b.fw"
> > [7.389475] bnx2: probe of :01:00.1 failed with error -2
> 
> Hopefully I understood your questions correctly, so that my answers make
> sense.
> 
> First, sorry for not saying that earlier, on our system the driver is built
> into the Linux kernel.

It's clear. So you built bnx2 driver into kernel and that makes bnx2
probe earlier be executed in do_initcalls() when initramfs has not been
uncompressed and mounted, surely requesting firmware will fail. While
building it as kernel module makes bnx2 probe be executed after
initramfs has been uncompressed and mounted, and firmware can be
detected.

Well, sometime code change is very easy, but it's very difficult to
write patch log to make maintainers and reviewers easily understand why
this patch is acceptable and user case is reasonalbe.

I will post a patch to add the firmware requesting code back in
bnx2_open.

Thanks for your detailed information, Paul.

> 
> ```
> $ grep BNX2 /boot/config-4.8.4.mx64.112
> # CONFIG_SCSI_BNX2_ISCSI is not set
> CONFIG_BNX2=y
> CONFIG_BNX2X=y
> CONFIG_BNX2X_SRIOV=y
> ```
> 
> Second, the filesystem driver is also built into the Linux kernel.
> 
> On our system, there is a systemd service unit, which sets up the network
> device.
> 
> ```
> $ more /etc/systemd/system/network.service
> [Unit]
> Description=Network Connectivity
> DefaultDependencies=no
> 
> [Service]
> Type=oneshot
> RemainAfterExit=yes
> ExecStart=/usr/sbin/mxnetctl start
> ExecStart=/sbin/ip addr add XXX broadcast XXX dev net00
> ExecStart=/sbin/ip link set up dev net00
> ExecStop=/sbin/ip addr del XXX dev net00
> StandardOutput=syslog
> 
> [Install]
> WantedBy=network.target
> ```
> 
> During that time, the hard drive has been detected, and the filesystem has
> been mounted.
> 
> 
> Kind regards,
> 
> Paul

Re: [bnx2] [Regression 4.8] Driver loading fails without firmware

[Baoquan He]

Hi Paul,

On 10/26/16 at 12:31pm, Paul Menzel wrote:
> Baoquan, could you please fix this regression. My suggestion is, that you
> add the old code back, but check if the firmware has been loaded. If it
> hasn’t, load it again.
> 
> That way, people can update their Linux kernel, and it continues working
> without changing the initramfs, or anything else.

I checked code and this looks good to me. I can post a patch with this
change to upstream, see what maintainers and other reviewers say.

The thing is I don't understand quite well about your requirement. With
my understanding, you just didn't add bnx2 firmware into initramfs, but
later opening the interface can still request that firmware with "ifup
eth-xxx" command. Is that correct? If yes, requeting firmware twice in
probing path and opening path looks good.


However I am wondering what's your exact steps to do this. 

What I tried to do is I execute command "dracut --add-drivers bnx2 -f
/boot/initramfs-4.9.0-rc3+.img 4.9.0-rc3+" to build a new initramfs,
meanwhile make sure bnx2.ko is included, then uncompressed initramfs and
deleted bnx2 folder under lib/firmware/ of uncompressed initramfs. Then
pack them to be /boot/initramfs-4.9.0-rc3+.img and restart. I did saw
below failure message. But later how did you really make the bnx2
network interface up? Could you say it more specifically?

[7.364186] bnx2: QLogic bnx2 Gigabit Ethernet Driver v2.2.6 (January 29, 
2014)
[7.371706] ACPI: PCI Interrupt Link [LN44] enabled at IRQ 44
[7.378128] bnx2 :01:00.0: Direct firmware load for 
bnx2/bnx2-mips-09-6.2.1b.fw failed with error -2
[7.387619] bnx2: Can't load firmware file "bnx2/bnx2-mips-09-6.2.1b.fw"
[7.387888] bnx2: probe of :01:00.0 failed with error -2
[7.388990] ACPI: PCI Interrupt Link [LN45] enabled at IRQ 45
[7.389370] bnx2 :01:00.1: Direct firmware load for 
bnx2/bnx2-mips-09-6.2.1b.fw failed with error -2
[7.389371] bnx2: Can't load firmware file "bnx2/bnx2-mips-09-6.2.1b.fw"
[7.389475] bnx2: probe of :01:00.1 failed with error -2

Thanks
Baoquan

Re: [bnx2] [Regression 4.8] Driver loading fails without firmware

[Baoquan He]

On 10/31/16 at 11:59am, Baoquan He wrote:
> Hi Paul,
> 
> On 10/30/16 at 12:05pm, Paul Menzel wrote:
> > Dear Baoquan,
> > 
> > 
> > Am Samstag, den 29.10.2016, 10:55 +0800 schrieb Baoquan He:
> > > On 10/27/16 at 03:21pm, Paul Menzel wrote:
> > 
> > > > > > Baoquan, could you please fix this regression. My suggestion is, 
> > > > > > that you
> > > > > > add the old code back, but check if the firmware has been loaded. 
> > > > > > If it
> > > > > > hasn’t, load it again.
> > > > > > 
> > > > > > That way, people can update their Linux kernel, and it continues 
> > > > > > working
> > > > > > without changing the initramfs, or anything else.
> > > > > 
> > > > > I saw your mail but I am also not familiar with bnx2 driver. As the
> > > > > commit log says I just tried to make bnx2 driver reset itself earlier.
> > > > > 
> > > > > So you did a git bisect and found this commit caused the regression,
> > > > > right? If yes, and network developers have no action, I will look into
> > > > > the code and see if I have idea to fix it.
> > > > 
> > > > Well, I looked through the commits and found that one, which would 
> > > > explain
> > > > the changed behavior.
> > > > 
> > > > To be sure, and to follow your request, I took Linux 4.8.4 and reverted 
> > > > your
> > > > commit (attached). Then I deleted the firmware again from the 
> > > > initramfs, and
> > > > rebooted. The devices showed up just fine as before.
> > > > 
> > > > So to summarize, the commit is indeed the culprit.
> > 
> > > Sorry for this.
> > > 
> > > Could you tell the steps to reproduce? I will find a machine with bnx2
> > > NIC and check if there's other ways.
> > 
> > Well, delete the bnx2 firmware files from the initramfs, and start the
> > system.
> > 
> > Did you read my proposal, to try to load the firmware twice, that means,
> > basically revert only the deleted lines of your commit, and add an
> > additional check?
> 
Please ignore this one, I have reproduced it. Will post a fix after
test.
> 
> I got a x86_64 system with bnx2 NIC, and clone Linus's git tree into
> that system. Then building a new kernel 4.9.0-rc3+ with new initramfs.
> But when I uncompressed the new initramfs, didn't find bnx2 related
> firmware, no bnx2 files under lib/firmware of uncompressed initramfs
> folder. While I did see them in /lib/firmware/bnx2/bnx2-x.fw. Could
> you please say it more specifically how I should do to reproduce the
> failure you encountered? I think your proposal looks good, just need a
> test before post.
> 
> Thanks
> Baoquan

Re: [bnx2] [Regression 4.8] Driver loading fails without firmware

[Baoquan He]

Hi Paul,

On 10/30/16 at 12:05pm, Paul Menzel wrote:
> Dear Baoquan,
> 
> 
> Am Samstag, den 29.10.2016, 10:55 +0800 schrieb Baoquan He:
> > On 10/27/16 at 03:21pm, Paul Menzel wrote:
> 
> > > > > Baoquan, could you please fix this regression. My suggestion is, that 
> > > > > you
> > > > > add the old code back, but check if the firmware has been loaded. If 
> > > > > it
> > > > > hasn’t, load it again.
> > > > > 
> > > > > That way, people can update their Linux kernel, and it continues 
> > > > > working
> > > > > without changing the initramfs, or anything else.
> > > > 
> > > > I saw your mail but I am also not familiar with bnx2 driver. As the
> > > > commit log says I just tried to make bnx2 driver reset itself earlier.
> > > > 
> > > > So you did a git bisect and found this commit caused the regression,
> > > > right? If yes, and network developers have no action, I will look into
> > > > the code and see if I have idea to fix it.
> > > 
> > > Well, I looked through the commits and found that one, which would explain
> > > the changed behavior.
> > > 
> > > To be sure, and to follow your request, I took Linux 4.8.4 and reverted 
> > > your
> > > commit (attached). Then I deleted the firmware again from the initramfs, 
> > > and
> > > rebooted. The devices showed up just fine as before.
> > > 
> > > So to summarize, the commit is indeed the culprit.
> 
> > Sorry for this.
> > 
> > Could you tell the steps to reproduce? I will find a machine with bnx2
> > NIC and check if there's other ways.
> 
> Well, delete the bnx2 firmware files from the initramfs, and start the
> system.
> 
> Did you read my proposal, to try to load the firmware twice, that means,
> basically revert only the deleted lines of your commit, and add an
> additional check?

Thanks for your information!

I got a x86_64 system with bnx2 NIC, and clone Linus's git tree into
that system. Then building a new kernel 4.9.0-rc3+ with new initramfs.
But when I uncompressed the new initramfs, didn't find bnx2 related
firmware, no bnx2 files under lib/firmware of uncompressed initramfs
folder. While I did see them in /lib/firmware/bnx2/bnx2-x.fw. Could
you please say it more specifically how I should do to reproduce the
failure you encountered? I think your proposal looks good, just need a
test before post.

Thanks
Baoquan

Re: [bnx2] [Regression 4.8] Driver loading fails without firmware

[Baoquan He]

On 10/27/16 at 03:21pm, Paul Menzel wrote:
> Dear Baoquan,
> > > Baoquan, could you please fix this regression. My suggestion is, that you
> > > add the old code back, but check if the firmware has been loaded. If it
> > > hasn’t, load it again.
> > > 
> > > That way, people can update their Linux kernel, and it continues working
> > > without changing the initramfs, or anything else.
> > 
> > I saw your mail but I am also not familiar with bnx2 driver. As the
> > commit log says I just tried to make bnx2 driver reset itself earlier.
> > 
> > So you did a git bisect and found this commit caused the regression,
> > right? If yes, and network developers have no action, I will look into
> > the code and see if I have idea to fix it.
> 
> Well, I looked through the commits and found that one, which would explain
> the changed behavior.
> 
> To be sure, and to follow your request, I took Linux 4.8.4 and reverted your
> commit (attached). Then I deleted the firmware again from the initramfs, and
> rebooted. The devices showed up just fine as before.
> 
> So to summarize, the commit is indeed the culprit.

Hi Paul,

Sorry for this.

Could you tell the steps to reproduce? I will find a machine with bnx2
NIC and check if there's other ways.

Thanks
Baoquan

> From 61b8dac8796343a797858b4a2eb0a59a0cfcd735 Mon Sep 17 00:00:00 2001
> From: Paul Menzel 
> Date: Thu, 27 Oct 2016 11:34:52 +0200
> Subject: [PATCH] Revert "bnx2: Reset device during driver initialization"
> 
> This reverts commit 3e1be7ad2d38c6bd6aeef96df9bd0a7822f4e51c.
> ---
>  drivers/net/ethernet/broadcom/bnx2.c | 12 +---
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2.c 
> b/drivers/net/ethernet/broadcom/bnx2.c
> index 27f11a5..ecd357d 100644
> --- a/drivers/net/ethernet/broadcom/bnx2.c
> +++ b/drivers/net/ethernet/broadcom/bnx2.c
> @@ -6356,6 +6356,10 @@ bnx2_open(struct net_device *dev)
>   struct bnx2 *bp = netdev_priv(dev);
>   int rc;
>  
> + rc = bnx2_request_firmware(bp);
> + if (rc < 0)
> + goto out;
> +
>   netif_carrier_off(dev);
>  
>   bnx2_disable_int(bp);
> @@ -6424,6 +6428,7 @@ bnx2_open(struct net_device *dev)
>   bnx2_free_irq(bp);
>   bnx2_free_mem(bp);
>   bnx2_del_napi(bp);
> + bnx2_release_firmware(bp);
>   goto out;
>  }
>  
> @@ -8570,12 +8575,6 @@ bnx2_init_one(struct pci_dev *pdev, const struct 
> pci_device_id *ent)
>  
>   pci_set_drvdata(pdev, dev);
>  
> - rc = bnx2_request_firmware(bp);
> - if (rc < 0)
> - goto error;
> -
> -
> - bnx2_reset_chip(bp, BNX2_DRV_MSG_CODE_RESET);
>   memcpy(dev->dev_addr, bp->mac_addr, ETH_ALEN);
>  
>   dev->hw_features = NETIF_F_IP_CSUM | NETIF_F_SG |
> @@ -8608,7 +8607,6 @@ bnx2_init_one(struct pci_dev *pdev, const struct 
> pci_device_id *ent)
>   return 0;
>  
>  error:
> - bnx2_release_firmware(bp);
>   pci_iounmap(pdev, bp->regview);
>   pci_release_regions(pdev);
>   pci_disable_device(pdev);
> -- 
> 2.4.1
> 

Re: [bnx2] [Regression 4.8] Driver loading fails without firmware

[Baoquan He]

Hi Paul,

Sorry for this.


On 10/26/16 at 12:31pm, Paul Menzel wrote:
> > > dev->hw_features = NETIF_F_IP_CSUM | NETIF_F_SG |
> > > @@ -8607,6 +8608,7 @@ bnx2_init_one(struct pci_dev *pdev, const struct
> > > pci_device_id *ent)
> > > return 0;
> > > 
> > >  error:
> > > +   bnx2_release_firmware(bp);
> > > pci_iounmap(pdev, bp->regview);
> > > pci_release_regions(pdev);
> > > pci_disable_device(pdev);
> 
> Baoquan, could you please fix this regression. My suggestion is, that you
> add the old code back, but check if the firmware has been loaded. If it
> hasn’t, load it again.
> 
> That way, people can update their Linux kernel, and it continues working
> without changing the initramfs, or anything else.

I saw your mail but I am also not familiar with bnx2 driver. As the
commit log says I just tried to make bnx2 driver reset itself earlier.

So you did a git bisect and found this commit caused the regression,
right? If yes, and network developers have no action, I will look into
the code and see if I have idea to fix it.

Thanks
Baoquan

Re: [PATCH v2] bnx2: Reset device during driver initialization

[Baoquan He]

On 09/13/16 at 11:25am, David Miller wrote:
> 
> Just to be clear, I did actually apply this v2 of the patch
> rather than the initial version.:)

Thanks a lot!

[PATCH v2] bnx2: Reset device during driver initialization

[Baoquan He]

When system enters into kdump kernel because of kernel panic, it won't
shutdown devices. On-flight DMA will continue transferring data until
device driver initializes. All devices are supposed to reset during
driver initialization. And this property is used to fix the kdump
failure in system with intel iommu. Other systems with hardware iommu
should be similar. Please check commit 091d42e ("iommu/vt-d: Copy
translation tables from old kernel") and those commits around.

But bnx2 driver doesn't reset device during driver initialization. The
device resetting is deferred to net device up stage. This will cause
hardware iommu handling failure on bnx2 device. And its resetting relies
on firmware. So in this patch move the firmware requesting code to earlier
bnx2_init_one(), then next call bnx2_reset_chip to reset device.

Signed-off-by: Baoquan He <b...@redhat.com>
---
v1->v2:
Forget removing bnx2_release_firmware which is used for error handling
of bnx2_request_firmware.

 drivers/net/ethernet/broadcom/bnx2.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2.c 
b/drivers/net/ethernet/broadcom/bnx2.c
index 8fc3f3c..505ceaf 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -6356,10 +6356,6 @@ bnx2_open(struct net_device *dev)
struct bnx2 *bp = netdev_priv(dev);
int rc;
 
-   rc = bnx2_request_firmware(bp);
-   if (rc < 0)
-   goto out;
-
netif_carrier_off(dev);
 
bnx2_disable_int(bp);
@@ -6428,7 +6424,6 @@ open_err:
bnx2_free_irq(bp);
bnx2_free_mem(bp);
bnx2_del_napi(bp);
-   bnx2_release_firmware(bp);
goto out;
 }
 
@@ -8575,6 +8570,12 @@ bnx2_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
 
pci_set_drvdata(pdev, dev);
 
+   rc = bnx2_request_firmware(bp);
+   if (rc < 0)
+   goto error;
+
+
+   bnx2_reset_chip(bp, BNX2_DRV_MSG_CODE_RESET);
memcpy(dev->dev_addr, bp->mac_addr, ETH_ALEN);
 
dev->hw_features = NETIF_F_IP_CSUM | NETIF_F_SG |
@@ -8607,6 +8608,7 @@ bnx2_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
return 0;
 
 error:
+   bnx2_release_firmware(bp);
pci_iounmap(pdev, bp->regview);
pci_release_regions(pdev);
pci_disable_device(pdev);
-- 
2.5.5

Re: [PATCH] bnx2: Reset device during driver initialization

[Baoquan He]

On 09/09/16 at 04:11pm, Baoquan He wrote:
> When system enters into kdump kernel because of kernel panic, it won't
> shutdown devices. On-flight DMA will continue transferring data until
> device driver initializes. All devices are supposed to reset during
> driver initialization. And this property is used to fix the kdump
> failure in system with intel iommu. Other systems with hardware iommu
> should be similar. Please check commit 091d42e ("iommu/vt-d: Copy
> translation tables from old kernel") and those commits around it.
> 
> But bnx2 driver doesn't reset device during driver initialization. The
> device resetting is deferred to net device up stage. This will cause
> hardware iommu handling failure on bnx2 device. And its resetting relies
> on firmware. So in this patch move the firmware requesting code to earlier
> bnx2_init_one(), then next call bnx2_reset_chip to reset device.
> 
> Signed-off-by: Baoquan He <b...@redhat.com>
> ---
>  drivers/net/ethernet/broadcom/bnx2.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2.c 
> b/drivers/net/ethernet/broadcom/bnx2.c
> index 8fc3f3c..d68a487 100644
> --- a/drivers/net/ethernet/broadcom/bnx2.c
> +++ b/drivers/net/ethernet/broadcom/bnx2.c
> @@ -6356,10 +6356,6 @@ bnx2_open(struct net_device *dev)
>   struct bnx2 *bp = netdev_priv(dev);
>   int rc;
>  
> - rc = bnx2_request_firmware(bp);
> - if (rc < 0)
> - goto out;

Sorry, here the corresponding bnx2_release_firmware need be removed too.
Will post v2 to update this.

> -
>   netif_carrier_off(dev);
>  
>   bnx2_disable_int(bp);
> @@ -8575,6 +8571,12 @@ bnx2_init_one(struct pci_dev *pdev, const struct 
> pci_device_id *ent)
>  
>   pci_set_drvdata(pdev, dev);
>  
> + rc = bnx2_request_firmware(bp);
> + if (rc < 0)
> + goto error;
> +
> +
> + bnx2_reset_chip(bp, BNX2_DRV_MSG_CODE_RESET);
>   memcpy(dev->dev_addr, bp->mac_addr, ETH_ALEN);
>  
>   dev->hw_features = NETIF_F_IP_CSUM | NETIF_F_SG |
> @@ -8607,6 +8609,7 @@ bnx2_init_one(struct pci_dev *pdev, const struct 
> pci_device_id *ent)
>   return 0;
>  
>  error:
> + bnx2_release_firmware(bp);
>   pci_iounmap(pdev, bp->regview);
>   pci_release_regions(pdev);
>   pci_disable_device(pdev);
> -- 
> 2.5.5
> 

Re: [PATCH] bnx2: Reset device during driver initialization

[Baoquan He]

Hi Joerg,

On 09/09/16 at 10:41am, Joerg Roedel wrote:
> 
> Hi Baoquan,
> 
> On Fri, Sep 09, 2016 at 04:22:25PM +0800, Baoquan He wrote:
> > Recently I tried to fix the kdump failure in amd iommu system again, and
> > now the latest code works, IO_PAGE_FAULT can't be seen any more. But on
> > several amd iommu system with bnx2 NIC, always IO_PAGE_FAULT will be
> > printed out. After investegating I found out bnx2 driver doesn't reset
> > hardware/reg like other pci device, it does the reset job in bnx2_open
> > which is the net device up stage. So with this patch the IO_PAGE_FAULT
> > is away too on the system with bnx2 NIC. I will 
> > 
> > However when I got a intel system with vt-d and bnx2 NIC, kdump works
> > well, and no any error message can be seen. From code it clearly shows
> > the domain assignment is done in __intel_map_single, at this time bnx2
> > driver hasn't reset device, the on-flight DMA should still exist. Do you
> > have any idea on this? Or I missed anything? I also deferred the
> > set_dte_entry calling to __map_single calling, the principal should be
> > similar.
> 
> Did you make sure that all unity-mappings are in place in the newly
> assigned domain for the bnx2 device before domains are switched?

What I am doing is that in iommu driver init stage anything will keep
going forware as it does in normal kernel except for set_dte_entry
calling. If in kdump kernel and in iommu init stage, just return
directly at the beginning of set_dte_entry.

For identity mapping, the pass through handling will do everything but
return from the beginning of set_dte_entry too. Since it need install
pte_root into dev table entry too though its pte_root is NULL. The unity
mapping range only does iova reservation. It doesn't do dev table entry
handling before device driver init.

So unity mappings should be OK.

Thanks a lot!

Baoquan

Re: [PATCH] bnx2: Reset device during driver initialization

[Baoquan He]

On 09/09/16 at 10:41am, Joerg Roedel wrote:
> 
> Hi Baoquan,
> 
> On Fri, Sep 09, 2016 at 04:22:25PM +0800, Baoquan He wrote:
> > Recently I tried to fix the kdump failure in amd iommu system again, and
> > now the latest code works, IO_PAGE_FAULT can't be seen any more. But on
> > several amd iommu system with bnx2 NIC, always IO_PAGE_FAULT will be
> > printed out. After investegating I found out bnx2 driver doesn't reset
> > hardware/reg like other pci device, it does the reset job in bnx2_open
> > which is the net device up stage. So with this patch the IO_PAGE_FAULT
> > is away too on the system with bnx2 NIC. I will 
> > 
> > However when I got a intel system with vt-d and bnx2 NIC, kdump works
> > well, and no any error message can be seen. From code it clearly shows
> > the domain assignment is done in __intel_map_single, at this time bnx2
> > driver hasn't reset device, the on-flight DMA should still exist. Do you
> > have any idea on this? Or I missed anything? I also deferred the
> > set_dte_entry calling to __map_single calling, the principal should be
> > similar.
> 
> Did you make sure that all unity-mappings are in place in the newly
> assigned domain for the bnx2 device before domains are switched?

About unity-mappings, are you saying it for intel iommu or amd iommu?
For amd iommu, I just skip call set_dte_entry calling during iommu
device init stage. Then in device init stage, namely __map_single, judge
and call set_dte_entry there to install the new pt_root into the related
dev table entry.

Thought you mention it, let me look into the unity-mappings of amd iommu
again.

Thanks a lot!

Re: [PATCH] bnx2: Reset device during driver initialization

[Baoquan He]

Hi Joreg,

Sorry, forget ccing to you.

Recently I tried to fix the kdump failure in amd iommu system again, and
now the latest code works, IO_PAGE_FAULT can't be seen any more. But on
several amd iommu system with bnx2 NIC, always IO_PAGE_FAULT will be
printed out. After investegating I found out bnx2 driver doesn't reset
hardware/reg like other pci device, it does the reset job in bnx2_open
which is the net device up stage. So with this patch the IO_PAGE_FAULT
is away too on the system with bnx2 NIC. I will 

However when I got a intel system with vt-d and bnx2 NIC, kdump works
well, and no any error message can be seen. From code it clearly shows
the domain assignment is done in __intel_map_single, at this time bnx2
driver hasn't reset device, the on-flight DMA should still exist. Do you
have any idea on this? Or I missed anything? I also deferred the
set_dte_entry calling to __map_single calling, the principal should be
similar.

Thanks
Baoquan

On 09/09/16 at 04:11pm, Baoquan He wrote:
> When system enters into kdump kernel because of kernel panic, it won't
> shutdown devices. On-flight DMA will continue transferring data until
> device driver initializes. All devices are supposed to reset during
> driver initialization. And this property is used to fix the kdump
> failure in system with intel iommu. Other systems with hardware iommu
> should be similar. Please check commit 091d42e ("iommu/vt-d: Copy
> translation tables from old kernel") and those commits around it.
> 
> But bnx2 driver doesn't reset device during driver initialization. The
> device resetting is deferred to net device up stage. This will cause
> hardware iommu handling failure on bnx2 device. And its resetting relies
> on firmware. So in this patch move the firmware requesting code to earlier
> bnx2_init_one(), then next call bnx2_reset_chip to reset device.
> 
> Signed-off-by: Baoquan He <b...@redhat.com>
> ---
>  drivers/net/ethernet/broadcom/bnx2.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2.c 
> b/drivers/net/ethernet/broadcom/bnx2.c
> index 8fc3f3c..d68a487 100644
> --- a/drivers/net/ethernet/broadcom/bnx2.c
> +++ b/drivers/net/ethernet/broadcom/bnx2.c
> @@ -6356,10 +6356,6 @@ bnx2_open(struct net_device *dev)
>   struct bnx2 *bp = netdev_priv(dev);
>   int rc;
>  
> - rc = bnx2_request_firmware(bp);
> - if (rc < 0)
> - goto out;
> -
>   netif_carrier_off(dev);
>  
>   bnx2_disable_int(bp);
> @@ -8575,6 +8571,12 @@ bnx2_init_one(struct pci_dev *pdev, const struct 
> pci_device_id *ent)
>  
>   pci_set_drvdata(pdev, dev);
>  
> + rc = bnx2_request_firmware(bp);
> + if (rc < 0)
> + goto error;
> +
> +
> + bnx2_reset_chip(bp, BNX2_DRV_MSG_CODE_RESET);
>   memcpy(dev->dev_addr, bp->mac_addr, ETH_ALEN);
>  
>   dev->hw_features = NETIF_F_IP_CSUM | NETIF_F_SG |
> @@ -8607,6 +8609,7 @@ bnx2_init_one(struct pci_dev *pdev, const struct 
> pci_device_id *ent)
>   return 0;
>  
>  error:
> + bnx2_release_firmware(bp);
>   pci_iounmap(pdev, bp->regview);
>   pci_release_regions(pdev);
>   pci_disable_device(pdev);
> -- 
> 2.5.5
> 

[PATCH] bnx2: Reset device during driver initialization

[Baoquan He]

When system enters into kdump kernel because of kernel panic, it won't
shutdown devices. On-flight DMA will continue transferring data until
device driver initializes. All devices are supposed to reset during
driver initialization. And this property is used to fix the kdump
failure in system with intel iommu. Other systems with hardware iommu
should be similar. Please check commit 091d42e ("iommu/vt-d: Copy
translation tables from old kernel") and those commits around it.

But bnx2 driver doesn't reset device during driver initialization. The
device resetting is deferred to net device up stage. This will cause
hardware iommu handling failure on bnx2 device. And its resetting relies
on firmware. So in this patch move the firmware requesting code to earlier
bnx2_init_one(), then next call bnx2_reset_chip to reset device.

Signed-off-by: Baoquan He <b...@redhat.com>
---
 drivers/net/ethernet/broadcom/bnx2.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2.c 
b/drivers/net/ethernet/broadcom/bnx2.c
index 8fc3f3c..d68a487 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -6356,10 +6356,6 @@ bnx2_open(struct net_device *dev)
struct bnx2 *bp = netdev_priv(dev);
int rc;
 
-   rc = bnx2_request_firmware(bp);
-   if (rc < 0)
-   goto out;
-
netif_carrier_off(dev);
 
bnx2_disable_int(bp);
@@ -8575,6 +8571,12 @@ bnx2_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
 
pci_set_drvdata(pdev, dev);
 
+   rc = bnx2_request_firmware(bp);
+   if (rc < 0)
+   goto error;
+
+
+   bnx2_reset_chip(bp, BNX2_DRV_MSG_CODE_RESET);
memcpy(dev->dev_addr, bp->mac_addr, ETH_ALEN);
 
dev->hw_features = NETIF_F_IP_CSUM | NETIF_F_SG |
@@ -8607,6 +8609,7 @@ bnx2_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
return 0;
 
 error:
+   bnx2_release_firmware(bp);
pci_iounmap(pdev, bp->regview);
pci_release_regions(pdev);
pci_disable_device(pdev);
-- 
2.5.5