Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1516459

Re: [Intel-wired-lan] [PATCH] igb: drop field "tail" of struct igb_ring

Show all headers | View raw

On Mon, Nov 7, 2016 at 4:44 AM, Cao jin <caoj.fnst@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.fnst@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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH] igb: drop field "tail" of struct igb_ring Cao jin <caoj.fnst@cn.fujitsu.com> - 2016-11-07 13:50 +0100
  Re: [Intel-wired-lan] [PATCH] igb: drop field "tail" of struct igb_ring Alexander Duyck <alexander.duyck@gmail.com> - 2016-11-07 19:50 +0100
    Re: [Intel-wired-lan] [PATCH] igb: drop field "tail" of struct  igb_ring Cao jin <caoj.fnst@cn.fujitsu.com> - 2016-11-08 04:00 +0100
      Re: [Intel-wired-lan] [PATCH] igb: drop field "tail" of struct  igb_ring Cao jin <caoj.fnst@cn.fujitsu.com> - 2016-11-08 07:10 +0100

csiph-web