| Message ID | 20120606130130.5a86f94a@nehalam.linuxnetplumber.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
--- 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)
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