* Re: [PATCH] sky2: fix checksum bit management on some chips
2012-06-06 20:01 ` [PATCH] sky2: fix checksum bit management on some chips Stephen Hemminger
@ 2012-06-06 20:14 ` Eric Dumazet
2012-06-06 20:23 ` Stephen Hemminger
2012-06-07 12:40 ` Kirill Smelkov
2012-06-07 20:07 ` David Miller
2 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2012-06-06 20:14 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Kirill Smelkov, David Miller, Mirko Lindner, netdev
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 ?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] sky2: fix checksum bit management on some chips
2012-06-06 20:14 ` Eric Dumazet
@ 2012-06-06 20:23 ` Stephen Hemminger
0 siblings, 0 replies; 6+ messages in thread
From: Stephen Hemminger @ 2012-06-06 20:23 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Kirill Smelkov, David Miller, Mirko Lindner, netdev
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sky2: fix checksum bit management on some chips
2012-06-06 20:01 ` [PATCH] sky2: fix checksum bit management on some chips Stephen Hemminger
2012-06-06 20:14 ` Eric Dumazet
@ 2012-06-07 12:40 ` Kirill Smelkov
2012-06-07 20:07 ` David Miller
2 siblings, 0 replies; 6+ messages in thread
From: Kirill Smelkov @ 2012-06-07 12:40 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David Miller, Mirko Lindner, netdev
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] sky2: fix checksum bit management on some chips
2012-06-06 20:01 ` [PATCH] sky2: fix checksum bit management on some chips Stephen Hemminger
2012-06-06 20:14 ` Eric Dumazet
2012-06-07 12:40 ` Kirill Smelkov
@ 2012-06-07 20:07 ` David Miller
2 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2012-06-07 20:07 UTC (permalink / raw)
To: shemminger; +Cc: kirr, mlindner, netdev
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.
^ permalink raw reply [flat|nested] 6+ messages in thread