On Mon, Nov 7, 2016 at 4:44 AM, Cao jin <caoj.f...@cn.fujitsu.com> wrote:
> Under certain condition, I find guest will oops on writel() in
> igb_configure_tx_ring(), because hw->hw_address is NULL. While other
> register access won't oops kernel because they use wr32/rd32 which have
> a defense against NULL pointer. The oops message are as following:
>
> [ 141.225449] pcieport 0000:00:1c.0: AER: Multiple Uncorrected (Fatal)
> error received: id=0101
> [ 141.225523] igb 0000:01:00.1: PCIe Bus Error: severity=Uncorrected
> (Fatal), type=Unaccessible, id=0101(Unregistered Agent ID)
> [ 141.299442] igb 0000:01:00.1: broadcast error_detected message
> [ 141.300539] igb 0000:01:00.0 enp1s0f0: PCIe link lost, device now
> detached
> [ 141.351019] igb 0000:01:00.1 enp1s0f1: PCIe link lost, device now
> detached
> [ 143.465904] pcieport 0000:00:1c.0: Root Port link has been reset
> [ 143.465994] igb 0000:01:00.1: broadcast slot_reset message
> [ 143.466039] igb 0000:01:00.0: enabling device (0000 -> 0002)
> [ 144.389078] igb 0000:01:00.1: enabling device (0000 -> 0002)
> [ 145.312078] igb 0000:01:00.1: broadcast resume message
> [ 145.322211] BUG: unable to handle kernel paging request at
> 0000000000003818
> [ 145.361275] IP: [<ffffffffa02fd38d>] igb_configure_tx_ring+0x14d/0x280
> [igb]
> [ 145.438007] Oops: 0002 [#1] SMP
>
> On the other hand, commit 238ac817 does some optimization which
> dropped the field "head". So I think it is time to drop "tail" as well.
There is a bug here, but removing tail isn't the fix.
> Signed-off-by: Cao jin <caoj.f...@cn.fujitsu.com>
> ---
> drivers/net/ethernet/intel/igb/igb.h | 1 -
> drivers/net/ethernet/intel/igb/igb_main.c | 16 +++++++++-------
> 2 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb.h
> b/drivers/net/ethernet/intel/igb/igb.h
> index d11093d..0df06bc 100644
> --- a/drivers/net/ethernet/intel/igb/igb.h
> +++ b/drivers/net/ethernet/intel/igb/igb.h
> @@ -247,7 +247,6 @@ struct igb_ring {
> };
> void *desc; /* descriptor ring memory */
> unsigned long flags; /* ring specific flags */
> - void __iomem *tail; /* pointer to ring tail register */
> dma_addr_t dma; /* phys address of the ring */
> unsigned int size; /* length of desc. ring in bytes */
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
> b/drivers/net/ethernet/intel/igb/igb_main.c
> index edc9a6a..e177d0e 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -3390,9 +3390,8 @@ void igb_configure_tx_ring(struct igb_adapter *adapter,
> tdba & 0x00000000ffffffffULL);
> wr32(E1000_TDBAH(reg_idx), tdba >> 32);
>
> - ring->tail = hw->hw_addr + E1000_TDT(reg_idx);
This line is where the bug is. This should be adapter->io_addr, not
hw->hw_addr.
> wr32(E1000_TDH(reg_idx), 0);
> - writel(0, ring->tail);
> + wr32(E1000_TDT(reg_idx), 0);
>
> txdctl |= IGB_TX_PTHRESH;
> txdctl |= IGB_TX_HTHRESH << 8;
> @@ -3729,9 +3728,8 @@ void igb_configure_rx_ring(struct igb_adapter *adapter,
> ring->count * sizeof(union e1000_adv_rx_desc));
>
> /* initialize head and tail */
> - ring->tail = hw->hw_addr + E1000_RDT(reg_idx);
Same thing here. It looks like the wrong values where used.
> wr32(E1000_RDH(reg_idx), 0);
> - writel(0, ring->tail);
> + wr32(E1000_RDT(reg_idx), 0);
>
> /* set descriptor configuration */
Would you prefer to submit the patch for this or should I? Basically
all you need to do is change the two lines where ring->tail is
populated so that you use adapter->io_addr instead of hw->hw_addr.
Thanks.
- Alex
