Patchwork sky2: fix checksum bit management on some chips

login
register
mail settings
Submitter stephen hemminger
Date June 6, 2012, 8:01 p.m.
Message ID <20120606130130.5a86f94a@nehalam.linuxnetplumber.net>
Download mbox | patch
Permalink /patch/163422/
State Accepted
Delegated to: David Miller
Headers show

Comments

stephen hemminger - June 6, 2012, 8:01 p.m.
The newer flavors of Yukon II use a different method for receive
checksum offload. This is indicated in the driver by the SKY2_HW_NEW_LE
flag. On these newer chips, the BMU_ENA_RX_CHKSUM should not be set.

The driver would get incorrectly toggle the bit, enabling the old
checksum logic on these chips and cause a BUG_ON() assertion. If
receive checksum was toggled via ethtool.

Reported-by: Kirill Smelkov <kirr@mns.spb.ru>

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
Patch against net-next, please apply to net and stable kernels.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - June 6, 2012, 8:14 p.m.
On Wed, 2012-06-06 at 13:01 -0700, Stephen Hemminger wrote:
> The newer flavors of Yukon II use a different method for receive
> checksum offload. This is indicated in the driver by the SKY2_HW_NEW_LE
> flag. On these newer chips, the BMU_ENA_RX_CHKSUM should not be set.
> 
> The driver would get incorrectly toggle the bit, enabling the old
> checksum logic on these chips and cause a BUG_ON() assertion. If
> receive checksum was toggled via ethtool.
> 
> Reported-by: Kirill Smelkov <kirr@mns.spb.ru>
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> ---
> Patch against net-next, please apply to net and stable kernels.
> 
> --- a/drivers/net/ethernet/marvell/sky2.c	2012-06-06 11:09:38.288440819 -0700
> +++ b/drivers/net/ethernet/marvell/sky2.c	2012-06-06 11:25:01.275782462 -0700
> @@ -4381,10 +4381,12 @@ static int sky2_set_features(struct net_
>  	struct sky2_port *sky2 = netdev_priv(dev);
>  	netdev_features_t changed = dev->features ^ features;
>  
> -	if (changed & NETIF_F_RXCSUM) {
> -		bool on = features & NETIF_F_RXCSUM;
> -		sky2_write32(sky2->hw, Q_ADDR(rxqaddr[sky2->port], Q_CSR),
> -			     on ? BMU_ENA_RX_CHKSUM : BMU_DIS_RX_CHKSUM);
> +	if ((changed & NETIF_F_RXCSUM) &&
> +	    !(sky2->hw->flags & SKY2_HW_NEW_LE)) {
> +		sky2_write32(sky2->hw,
> +			     Q_ADDR(rxqaddr[sky2->port], Q_CSR),
> +			     (features & NETIF_F_RXCSUM)
> +			     ? BMU_ENA_RX_CHKSUM : BMU_DIS_RX_CHKSUM);

Don't you need to return an error if NETIF_F_RXCSUM could not be
changed ?


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
stephen hemminger - June 6, 2012, 8:23 p.m.
On Wed, 06 Jun 2012 22:14:04 +0200
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On Wed, 2012-06-06 at 13:01 -0700, Stephen Hemminger wrote:
> > The newer flavors of Yukon II use a different method for receive
> > checksum offload. This is indicated in the driver by the SKY2_HW_NEW_LE
> > flag. On these newer chips, the BMU_ENA_RX_CHKSUM should not be set.
> > 
> > The driver would get incorrectly toggle the bit, enabling the old
> > checksum logic on these chips and cause a BUG_ON() assertion. If
> > receive checksum was toggled via ethtool.
> > 
> > Reported-by: Kirill Smelkov <kirr@mns.spb.ru>
> > 
> > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> > 
> > ---
> > Patch against net-next, please apply to net and stable kernels.
> > 
> > --- a/drivers/net/ethernet/marvell/sky2.c	2012-06-06 11:09:38.288440819 -0700
> > +++ b/drivers/net/ethernet/marvell/sky2.c	2012-06-06 11:25:01.275782462 -0700
> > @@ -4381,10 +4381,12 @@ static int sky2_set_features(struct net_
> >  	struct sky2_port *sky2 = netdev_priv(dev);
> >  	netdev_features_t changed = dev->features ^ features;
> >  
> > -	if (changed & NETIF_F_RXCSUM) {
> > -		bool on = features & NETIF_F_RXCSUM;
> > -		sky2_write32(sky2->hw, Q_ADDR(rxqaddr[sky2->port], Q_CSR),
> > -			     on ? BMU_ENA_RX_CHKSUM : BMU_DIS_RX_CHKSUM);
> > +	if ((changed & NETIF_F_RXCSUM) &&
> > +	    !(sky2->hw->flags & SKY2_HW_NEW_LE)) {
> > +		sky2_write32(sky2->hw,
> > +			     Q_ADDR(rxqaddr[sky2->port], Q_CSR),
> > +			     (features & NETIF_F_RXCSUM)
> > +			     ? BMU_ENA_RX_CHKSUM : BMU_DIS_RX_CHKSUM);
> 
> Don't you need to return an error if NETIF_F_RXCSUM could not be
> changed ?
> 
> 

No, what happens is that on the new chips, the feature flag is already checked
in the receive status processing
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kirill Smelkov - June 7, 2012, 12:40 p.m.
On Wed, Jun 06, 2012 at 01:01:30PM -0700, Stephen Hemminger wrote:
> The newer flavors of Yukon II use a different method for receive
> checksum offload. This is indicated in the driver by the SKY2_HW_NEW_LE
> flag. On these newer chips, the BMU_ENA_RX_CHKSUM should not be set.
> 
> The driver would get incorrectly toggle the bit, enabling the old
> checksum logic on these chips and cause a BUG_ON() assertion. If
> receive checksum was toggled via ethtool.
> 
> Reported-by: Kirill Smelkov <kirr@mns.spb.ru>
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> ---
> Patch against net-next, please apply to net and stable kernels.
> 
> --- a/drivers/net/ethernet/marvell/sky2.c	2012-06-06 11:09:38.288440819 -0700
> +++ b/drivers/net/ethernet/marvell/sky2.c	2012-06-06 11:25:01.275782462 -0700
> @@ -4381,10 +4381,12 @@ static int sky2_set_features(struct net_
>  	struct sky2_port *sky2 = netdev_priv(dev);
>  	netdev_features_t changed = dev->features ^ features;
>  
> -	if (changed & NETIF_F_RXCSUM) {
> -		bool on = features & NETIF_F_RXCSUM;
> -		sky2_write32(sky2->hw, Q_ADDR(rxqaddr[sky2->port], Q_CSR),
> -			     on ? BMU_ENA_RX_CHKSUM : BMU_DIS_RX_CHKSUM);
> +	if ((changed & NETIF_F_RXCSUM) &&
> +	    !(sky2->hw->flags & SKY2_HW_NEW_LE)) {
> +		sky2_write32(sky2->hw,
> +			     Q_ADDR(rxqaddr[sky2->port], Q_CSR),
> +			     (features & NETIF_F_RXCSUM)
> +			     ? BMU_ENA_RX_CHKSUM : BMU_DIS_RX_CHKSUM);
>  	}
>  
>  	if (changed & NETIF_F_RXHASH)


Thanks Stephen, now that BUG_ON is gone.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - June 7, 2012, 8:07 p.m.
From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 6 Jun 2012 13:01:30 -0700

> The newer flavors of Yukon II use a different method for receive
> checksum offload. This is indicated in the driver by the SKY2_HW_NEW_LE
> flag. On these newer chips, the BMU_ENA_RX_CHKSUM should not be set.
> 
> The driver would get incorrectly toggle the bit, enabling the old
> checksum logic on these chips and cause a BUG_ON() assertion. If
> receive checksum was toggled via ethtool.
> 
> Reported-by: Kirill Smelkov <kirr@mns.spb.ru>
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Applied and queued up for -stable.

> Patch against net-next, please apply to net and stable kernels.

Stephen, please don't do this, generate your patches always
against the correct tree even if they are likely to still
apply when generated against the wrong tree.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

--- a/drivers/net/ethernet/marvell/sky2.c	2012-06-06 11:09:38.288440819 -0700
+++ b/drivers/net/ethernet/marvell/sky2.c	2012-06-06 11:25:01.275782462 -0700
@@ -4381,10 +4381,12 @@  static int sky2_set_features(struct net_
 	struct sky2_port *sky2 = netdev_priv(dev);
 	netdev_features_t changed = dev->features ^ features;
 
-	if (changed & NETIF_F_RXCSUM) {
-		bool on = features & NETIF_F_RXCSUM;
-		sky2_write32(sky2->hw, Q_ADDR(rxqaddr[sky2->port], Q_CSR),
-			     on ? BMU_ENA_RX_CHKSUM : BMU_DIS_RX_CHKSUM);
+	if ((changed & NETIF_F_RXCSUM) &&
+	    !(sky2->hw->flags & SKY2_HW_NEW_LE)) {
+		sky2_write32(sky2->hw,
+			     Q_ADDR(rxqaddr[sky2->port], Q_CSR),
+			     (features & NETIF_F_RXCSUM)
+			     ? BMU_ENA_RX_CHKSUM : BMU_DIS_RX_CHKSUM);
 	}
 
 	if (changed & NETIF_F_RXHASH)