On Wed, Nov 03, 2010 at 03:42:47PM -0700, Matt Carlson wrote:

On Wed, Nov 03, 2010 at 03:29:10PM -0700, Micha?? Miros??aw wrote:

quoted

On Mon, Nov 01, 2010 at 07:24:38PM -0700, Matt Carlson wrote:

quoted

On Fri, Oct 29, 2010 at 09:28:26PM -0700, Micha?? Miros??aw wrote:

quoted

diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 30ccbb6..b07e2d1 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c

@@ -11306,7 +11306,6 @@ static const struct ethtool_ops tg3_ethtool_ops = {
        .get_rx_csum            = tg3_get_rx_csum,
        .set_rx_csum            = tg3_set_rx_csum,
        .set_tx_csum            = tg3_set_tx_csum,
-       .set_sg                 = ethtool_op_set_sg,

This is exchanged ...

quoted

quoted

quoted

        .set_tso                = tg3_set_tso,
        .self_test              = tg3_self_test,
        .get_strings            = tg3_get_strings,

@@ -14681,6 +14680,7 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
        tp->rx_pending = TG3_DEF_RX_RING_PENDING;
        tp->rx_jumbo_pending = TG3_DEF_RX_JUMBO_RING_PENDING;

+       dev->hw_features |= NETIF_F_SG;

... for this. (This introduces no functional changes, whatsoever.)

quoted

quoted

Scatter-gather should not be enabled if TG3_FLAG_BROKEN_CHECKSUMS is set.  I
would do the following instead:

	if (!(tp->tg3_flags & TG3_FLAG_BROKEN_CHECKSUMS))
		dev->hw_features |= NETIF_F_SG;

TG3_FLAG_BROKEN_CHECKSUMS is set in tg3_get_invariants(), so this code
would need to be placed later than that function call.

This bug is there now, so I'll queue this as all other hints of existent
bugs that this patch series "uncovers".

How so?  This patch would be introducing the bug.  From tg3_get_invariants:

	if (tp->pci_chip_rev_id == CHIPREV_ID_5700_B0)
		tp->tg3_flags |= TG3_FLAG_BROKEN_CHECKSUMS;
	else {
		unsigned long features = NETIF_F_IP_CSUM | NETIF_F_SG | NETIF_F_GRO;

		tp->tg3_flags |= TG3_FLAG_RX_CHECKSUMS;
		if (tp->tg3_flags3 & TG3_FLG3_5755_PLUS)
			features |= NETIF_F_IPV6_CSUM;
		tp->dev->features |= features;
		vlan_features_add(tp->dev, features);
	}

Actually this is hidden anyway, because currently scatter-gather depends
on checksumming offload. So the SG won't be enabled based on check left
in ethtool_set_sg().

Note, that hw_features flags enable toggling of the offloads but don't
enable them unless requested by a user later.

Best Regards,
Michał Mirosław