[PATCH] igb: drop field "tail" of struct igb_ring

[PATCH] igb: drop field "tail" of struct igb_ring

[Cao jin]

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.

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);
 	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);
 	wr32(E1000_RDH(reg_idx), 0);
-	writel(0, ring->tail);
+	wr32(E1000_RDT(reg_idx), 0);
 
 	/* set descriptor configuration */
 	srrctl = IGB_RX_HDR_LEN << E1000_SRRCTL_BSIZEHDRSIZE_SHIFT;
@@ -5130,6 +5128,8 @@ static void igb_tx_map(struct igb_ring *tx_ring,
 	u32 tx_flags = first->tx_flags;
 	u32 cmd_type = igb_tx_cmd_type(skb, tx_flags);
 	u16 i = tx_ring->next_to_use;
+        struct igb_adapter *adapter = netdev_priv(tx_ring->netdev);
+        struct e1000_hw *hw = &adapter->hw;
 
 	tx_desc = IGB_TX_DESC(tx_ring, i);
 
@@ -5223,7 +5223,7 @@ static void igb_tx_map(struct igb_ring *tx_ring,
 	igb_maybe_stop_tx(tx_ring, DESC_NEEDED);
 
 	if (netif_xmit_stopped(txring_txq(tx_ring)) || !skb->xmit_more) {
-		writel(i, tx_ring->tail);
+                wr32(E1000_TDT(tx_ring->reg_idx), i);
 
 		/* we need this if more than one processor can write to our tail
 		 * at a time, it synchronizes IO on IA64/Altix systems
@@ -6756,7 +6756,7 @@ static bool igb_clean_tx_irq(struct igb_q_vector *q_vector, int napi_budget)
 				"  desc.status          <%x>\n",
 				tx_ring->queue_index,
 				rd32(E1000_TDH(tx_ring->reg_idx)),
-				readl(tx_ring->tail),
+				rd32(E1000_TDT(tx_ring->reg_idx)),
 				tx_ring->next_to_use,
 				tx_ring->next_to_clean,
 				tx_buffer->time_stamp,
@@ -7265,6 +7265,8 @@ void igb_alloc_rx_buffers(struct igb_ring *rx_ring, u16 cleaned_count)
 	union e1000_adv_rx_desc *rx_desc;
 	struct igb_rx_buffer *bi;
 	u16 i = rx_ring->next_to_use;
+        struct igb_adapter *adapter = netdev_priv(rx_ring->netdev);
+        struct e1000_hw *hw = &adapter->hw;
 
 	/* nothing to do */
 	if (!cleaned_count)
@@ -7313,7 +7315,7 @@ void igb_alloc_rx_buffers(struct igb_ring *rx_ring, u16 cleaned_count)
 		 * such as IA-64).
 		 */
 		wmb();
-		writel(i, rx_ring->tail);
+                wr32(E1000_RDT(rx_ring->reg_idx), i);
 	}
 }
 
-- 
2.1.0

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

[Alexander Duyck]

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

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

[Cao jin]

On 11/08/2016 02:49 AM, Alexander Duyck wrote:
> 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.
>

Yes, totally agree with you. The oops issue just drive me find that 
"tail" may should be dropped as "head", at least won't oops kernel when 
this driver's bug come out.

Couldn't we remove "tail"? Removed "head" while left "tail" untouched 
seems weird, and all the other register's access go via rd32/wr32, 
except "tail", it also seems weird, isn't it?

>> 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.

hw->hw_addr could be alterred to NULL(in igb_rd32), this is why writel 
oops the kernel, you give a fine solution.

But from the oops message, we can find, register reading returns all 
F's, I also have a question want to consult: when igb device is reset, 
would reading register(no matter config space or non-PCIe configuration 
registers) during reset returns all F's? (I guess this is the core of my 
issue)

>
>>          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
>
>
> .
>

-- 
Yours Sincerely,

Cao jin

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

[Cao jin]

On 11/08/2016 12:12 PM, Alexander Duyck wrote:
>
>
> On Monday, November 7, 2016, Cao jin <caoj.fnst@cn.fujitsu.com
> <mailto:caoj.fnst@cn.fujitsu.com>> wrote:
>
>

>
> We removed head because it isn't really accessed very often, it is only
> really used for when the ring is configured.  Tail is accessed every
> time we add a descriptor to a ring.  The pointer chasing from ring to
> netdev to adapter to hw is expensive.  That is one of the rasons why
> we cache the pointer to the tail register.

I see. I can submit the patch as you suggested.

>
>             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
>             +++++++++-------

>
>     hw->hw_addr could be alterred to NULL(in igb_rd32), this is why
>     writel oops the kernel, you give a fine solution.
>
>     But from the oops message, we can find, register reading returns all
>     F's, I also have a question want to consult: when igb device is
>     reset, would reading register(no matter config space or non-PCIe
>     configuration registers) during reset returns all F's? (I guess this
>     is the core of my issue)
>
>
> An all F's value means the read failed.  The device is likely off of the
> bus and the hw_addr may not have been repopulated after the reset.
>
> You might want to check the mailing list as I thought someone had
> submitted a patch recently for one of the drivers to repopulate hw_addr
> after a reset.
>

I guess you are saying this one:
   http://patchwork.ozlabs.org/patch/689592/

Seems they have a similar issue with me.


-- 
Yours Sincerely,

Cao jin