Here is the function in question:

static void igb_cache_ring_register(struct igb_adapter *adapter)
{
        int i = 0, j = 0;
        u32 rbase_offset = adapter->vfs_allocated_count;

        switch (adapter->hw.mac.type) {
        case e1000_82576:
                /* The queues are allocated for virtualization such that VF 0
                 * is allocated queues 0 and 8, VF 1 queues 1 and 9, etc.
                 * In order to avoid collision we start at the first free queue
                 * and continue consuming queues in the same sequence
                 */
                if ((adapter->rss_queues > 1) && adapter->vmdq_pools) {
                        for (; i < adapter->rss_queues; i++)
                                adapter->rx_ring[i]->reg_idx = rbase_offset +
                                                                Q_IDX_82576(i);
                }
                *** Q: should we break or fall through here? ***
        case e1000_82575:
        case e1000_82580:
        case e1000_i350:
        case e1000_i354:
        case e1000_i210:
        case e1000_i211:
                /* Fall through */
        default:
                for (; i < adapter->num_rx_queues; i++)
                        adapter->rx_ring[i]->reg_idx = rbase_offset + i;
                for (; j < adapter->num_tx_queues; j++)
                        adapter->tx_ring[j]->reg_idx = rbase_offset + j;
                break;
        }
}

The thing to note is that "i" and "j" are set to 0 at the top of the
function rather than being initialized in the for loops.  So it might
make sense to run all three for loops:

top loop: init rx_ring[0 .. adapter->rss_queues - 1]
fall through
middle loop: init rx_ring[adapter->rss_queues .. adapter->num_rx_queues - 1]
bottom loop: init tx_ring[0 .. adapter->num_tx_queues - 1]

So the middle loop starts out with "i = adapter->rss_queues" rather than
"i = 0".  If the loops started out with the more typical "for (i = 0;
...", then it would make less sense because then the middle loop would
undo the top loop.

But I am not familiar with this code, so if you still think that the fix
is incorrect, then I will submit the requested information to sourceforge.

Tony

On 01/07/2015 11:36 AM, Fujinaka, Todd wrote:
> NACK. The fix is incorrect and just invalidates the case statement in general.
>
> The TX hang message really doesn't tell you anything and it would be more 
> useful to see the full logs. We'll need a lot more information to debug this 
> issue. I would suggest filing the issue on sourceforge with all the usual 
> full info:
>
> OS version
> Kernel version
> CPU information (model number and speed)
> Motherboard information (model number)
> Memory (how much, what speed, what size)
> dmesg
> ethtool -S ethX
> ethtool -i ethX
> cat /proc/interrupts
> cat /var/log/messages
> lspci -vvv
>
> Todd Fujinaka
> Software Application Engineer
> Networking Division (ND)
> Intel Corporation
> todd.fujin...@intel.com
> (503) 712-4565
>
> -----Original Message-----
> From: Tony Battersby [mailto:to...@cybernetics.com] 
> Sent: Wednesday, January 07, 2015 6:59 AM
> To: e1000-devel@lists.sourceforge.net
> Subject: [E1000-devel] [PATCH] igb: fix Tx Unit Hang on 82576 with RSS > 1
>
> On my Supermicro X8DTH-6F motherboard, the onboard 82576 ethernet ports are 
> completely inoperable with the latest igb from sourceforge when using RSS > 
> 1.  This is the error message:
>
> igb 0000:01:00.0: Detected Tx Unit Hang
>   Tx Queue             <0>
>   TDH                  <0>
>   TDT                  <1>
>   next_to_use          <1>
>   next_to_clean        <0>
> buffer_info[next_to_clean]
>   time_stamp           <10000b131>
>   next_to_watch        <ffff88032f1f4000>
>   jiffies              <10000b3b4>
>   desc.status          <a8000>
>
> Known-good versions:
>   sourceforge igb-5.2.5.tar.gz and earlier
>   upstream in-kernel igb
>
> Known-bad versions:
>   sourceforge igb-5.2.9.2.tar.gz and later
>
> The patch below reverts the change that is causing the problem.
>
> Signed-off-by: Tony Battersby <to...@cybernetics.com>
> ---
>
> --- igb-5.2.15/src/igb_main.c.orig    2014-09-18 12:12:17.000000000 -0400
> +++ igb-5.2.15/src/igb_main.c 2015-01-06 17:47:07.000000000 -0500
> @@ -408,7 +408,7 @@ static void igb_cache_ring_register(stru
>                               adapter->rx_ring[i]->reg_idx = rbase_offset +
>                                                               Q_IDX_82576(i);
>               }
> -     break;
> +             /* Fall through */
>       case e1000_82575:
>       case e1000_82580:
>       case e1000_i350:
>
>


------------------------------------------------------------------------------
Dive into the World of Parallel Programming! The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit 
http://communities.intel.com/community/wired

Reply via email to