Message ID | 1302531021.21065.79.camel@lb-tlvb-vladz |
---|---|
State | Superseded, archived |
Delegated to: | David Miller |
Headers | show |
The v3 patch fixes missing LRO flag and ensures that netdev_update_features() won't be called after failed bnx2x_nic_load(). More comments below. On Mon, Apr 11, 2011 at 05:10:21PM +0300, Vladislav Zolotarov wrote: > On Sun, 2011-04-10 at 08:35 -0700, Michał Mirosław wrote: > > Since ndo_fix_features callback is postponing features change when > > bp->recovery_state != BNX2X_RECOVERY_DONE, netdev_update_features() > > has to be called again when this condition changes. > Unfortunately, NACK again. See below, pls. [...] > > diff --git a/drivers/net/bnx2x/bnx2x_cmn.c b/drivers/net/bnx2x/bnx2x_cmn.c > > index e83ac6d..9691b67 100644 > > --- a/drivers/net/bnx2x/bnx2x_cmn.c > > +++ b/drivers/net/bnx2x/bnx2x_cmn.c > > @@ -2443,11 +2443,21 @@ alloc_err: > > > > } > > > > +static int bnx2x_reload_if_running(struct net_device *dev) > > +{ > > + struct bnx2x *bp = netdev_priv(dev); > > + > > + if (unlikely(!netif_running(dev))) > > + return 0; > > + > > + bnx2x_nic_unload(bp, UNLOAD_NORMAL); > > + return bnx2x_nic_load(bp, LOAD_NORMAL); > > +} > > + > > /* called with rtnl_lock */ > > int bnx2x_change_mtu(struct net_device *dev, int new_mtu) > > { [...] > > +u32 bnx2x_fix_features(struct net_device *dev, u32 features) > > +{ > > + struct bnx2x *bp = netdev_priv(dev); > > + > > + if (bp->recovery_state != BNX2X_RECOVERY_DONE) { > > + netdev_err(dev, "Handling parity error recovery. Try again later\n"); > > + > > + /* Don't allow bnx2x_set_features() to be called now. */ > > + return dev->features; > > + } > > + > > + /* TPA requires Rx CSUM offloading */ > > + if (!(features & NETIF_F_RXCSUM) || bp->disable_tpa) > > + features &= ~NETIF_F_LRO; > Shouldn't it be (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM) and not > NETIF_F_RXCSUM? [...] > In addition this function should ensure NETIF_F_IP_CSUM and > NETIF_F_IPV6_CSUM are changed together. [...] > > +int bnx2x_set_features(struct net_device *dev, u32 features) [...] > Since there is no set_rx_csum() anymore the above function has to handle > bp->rx_csum namely correlate it with (NETIF_F_IP_CSUM | > NETIF_F_IPV6_CSUM) bits in the 'features'. You seem to confuse TX checksum offloads (IP_CSUM,IPV6_CSUM) with RX checksum offload (RXCSUM). The driver doesn't touch hardware state on changes to checksum offloads so they are independent - there's no point in adding artificial dependencies here. [...] > > diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c > > index f3cf889..ffa0611 100644 > > --- a/drivers/net/bnx2x/bnx2x_main.c > > +++ b/drivers/net/bnx2x/bnx2x_main.c > > @@ -7661,6 +7661,7 @@ exit_leader_reset: > > bp->is_leader = 0; > > bnx2x_release_hw_lock(bp, HW_LOCK_RESOURCE_RESERVED_08); > > smp_wmb(); > > + netdev_update_features(bp->dev); > > return rc; > > } > > Before I continue I'd like to clarify one thing: there is no sense to > call for netdev_update_features() if bnx2x_nic_load(), called right > before it, has failed as long as the following bnx2x_nic_load() that > will be called from the netdev_update_features() flow will also fail > (for the same reasons as the previous one). If bnx2x_nic_load() fails > for the certain NIC we actually shut this NIC down. So, the following > remarks will be based on the above statement. In all those cases, bnx2x_reload_if_running() will be called only when LRO state is changed while there's a recovery in progress. [...] > U shouldn't call for netdev_update_features(bp->dev) if bnx2x_nic_load() > has failed. It would also be nice if netdev_update_features() would > propagate the exit status of ndo_set_features() when ndo_set_features() > fails in the __netdev_update_features(). That's fixed in v3. > See the patch for the bnx2x below: > > @@ -8993,7 +8995,14 @@ static int bnx2x_open(struct net_device *dev) > > bp->recovery_state = BNX2X_RECOVERY_DONE; > > - return bnx2x_nic_load(bp, LOAD_OPEN); > + rc = bnx2x_nic_load(bp, LOAD_OPEN); > + if (!rc) > + netdev_update_features(bp->dev); > + > + if (bp->state == BNX2X_STATE_OPEN) > + return 0; > + else > + return -EBUSY; > } Hmm. I missed this part in the v3 patch. This clobbers bnx2x_nic_load()'s error return, though. > > /* called with rtnl_lock */ > > @@ -9304,6 +9309,8 @@ static const struct net_device_ops bnx2x_netdev_ops = { > > .ndo_validate_addr = eth_validate_addr, > > .ndo_do_ioctl = bnx2x_ioctl, > > .ndo_change_mtu = bnx2x_change_mtu, > > + .ndo_fix_features = bnx2x_fix_features, > > + .ndo_set_features = bnx2x_set_features, > > .ndo_tx_timeout = bnx2x_tx_timeout, > > #ifdef CONFIG_NET_POLL_CONTROLLER > > .ndo_poll_controller = poll_bnx2x, > > @@ -9430,20 +9437,18 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev, > > > > dev->netdev_ops = &bnx2x_netdev_ops; > > bnx2x_set_ethtool_ops(dev); > > - dev->features |= NETIF_F_SG; > > - dev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM; > > + > > if (bp->flags & USING_DAC_FLAG) > > dev->features |= NETIF_F_HIGHDMA; > > - dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN); > > - dev->features |= NETIF_F_TSO6; > > - dev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX); > > > > - dev->vlan_features |= NETIF_F_SG; > > - dev->vlan_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM; > > - if (bp->flags & USING_DAC_FLAG) > > - dev->vlan_features |= NETIF_F_HIGHDMA; > > - dev->vlan_features |= (NETIF_F_TSO | NETIF_F_TSO_ECN); > > - dev->vlan_features |= NETIF_F_TSO6; > > + dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | > > + NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 | > > + NETIF_F_HW_VLAN_TX; > hw_features are missing NETIF_F_GRO and NETIF_F_LRO flags that are > currently configured in bnx2x_init_bp(). GRO is enabled by core now. LRO is fixed in v3. > > + dev->features |= dev->hw_features | NETIF_F_HW_VLAN_RX; > > + > > + dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | > > + NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_HIGHDMA; > I'm not sure if it's safe to set NETIF_F_HIGHDMA unconditionally. I > think it's better to correlate it with the USING_DAC_FLAG which is set > according to what is returned by > dma_set_mask(&pdev->dev, DMA_BIT_MASK(64)). dev->vlan_features get masked with dev->features and only then applied to VLAN device. Best Regards, Michał Mirosław -- 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 Mon, 2011-04-11 at 13:12 -0700, Michał Mirosław wrote: > The v3 patch fixes missing LRO flag and ensures that netdev_update_features() > won't be called after failed bnx2x_nic_load(). More comments below. As long as there is v4 already I'll comment it and skip v3. See a few comments on your comments below. ;) > > On Mon, Apr 11, 2011 at 05:10:21PM +0300, Vladislav Zolotarov wrote: > > On Sun, 2011-04-10 at 08:35 -0700, Michał Mirosław wrote: > > > Since ndo_fix_features callback is postponing features change when > > > bp->recovery_state != BNX2X_RECOVERY_DONE, netdev_update_features() > > > has to be called again when this condition changes. > > Unfortunately, NACK again. See below, pls. > [...] > > > diff --git a/drivers/net/bnx2x/bnx2x_cmn.c b/drivers/net/bnx2x/bnx2x_cmn.c > > > index e83ac6d..9691b67 100644 > > > --- a/drivers/net/bnx2x/bnx2x_cmn.c > > > +++ b/drivers/net/bnx2x/bnx2x_cmn.c > > > @@ -2443,11 +2443,21 @@ alloc_err: > > > > > > } > > > > > > +static int bnx2x_reload_if_running(struct net_device *dev) > > > +{ > > > + struct bnx2x *bp = netdev_priv(dev); > > > + > > > + if (unlikely(!netif_running(dev))) > > > + return 0; > > > + > > > + bnx2x_nic_unload(bp, UNLOAD_NORMAL); > > > + return bnx2x_nic_load(bp, LOAD_NORMAL); > > > +} > > > + > > > /* called with rtnl_lock */ > > > int bnx2x_change_mtu(struct net_device *dev, int new_mtu) > > > { > [...] > > > +u32 bnx2x_fix_features(struct net_device *dev, u32 features) > > > +{ > > > + struct bnx2x *bp = netdev_priv(dev); > > > + > > > + if (bp->recovery_state != BNX2X_RECOVERY_DONE) { > > > + netdev_err(dev, "Handling parity error recovery. Try again later\n"); > > > + > > > + /* Don't allow bnx2x_set_features() to be called now. */ > > > + return dev->features; > > > + } > > > + > > > + /* TPA requires Rx CSUM offloading */ > > > + if (!(features & NETIF_F_RXCSUM) || bp->disable_tpa) > > > + features &= ~NETIF_F_LRO; > > Shouldn't it be (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM) and not > > NETIF_F_RXCSUM? > [...] > > In addition this function should ensure NETIF_F_IP_CSUM and > > NETIF_F_IPV6_CSUM are changed together. > [...] > > > +int bnx2x_set_features(struct net_device *dev, u32 features) > [...] > > Since there is no set_rx_csum() anymore the above function has to handle > > bp->rx_csum namely correlate it with (NETIF_F_IP_CSUM | > > NETIF_F_IPV6_CSUM) bits in the 'features'. > > You seem to confuse TX checksum offloads (IP_CSUM,IPV6_CSUM) with > RX checksum offload (RXCSUM). U r right. My bad. However u forgot to add RXCSUM to hw_features in v2 but I see it fixed in v4. > > The driver doesn't touch hardware state on changes to checksum offloads > so they are independent - there's no point in adding artificial > dependencies here. Considering Tx csum offloads u are right but this is not true regarding the Rx csum offload and this is what I meant above. I see that v4 properly handles it now. Sorry for a confusion. > > [...] > > > diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c > > > index f3cf889..ffa0611 100644 > > > --- a/drivers/net/bnx2x/bnx2x_main.c > > > +++ b/drivers/net/bnx2x/bnx2x_main.c > > > @@ -7661,6 +7661,7 @@ exit_leader_reset: > > > bp->is_leader = 0; > > > bnx2x_release_hw_lock(bp, HW_LOCK_RESOURCE_RESERVED_08); > > > smp_wmb(); > > > + netdev_update_features(bp->dev); > > > return rc; > > > } > > > > Before I continue I'd like to clarify one thing: there is no sense to > > call for netdev_update_features() if bnx2x_nic_load(), called right > > before it, has failed as long as the following bnx2x_nic_load() that > > will be called from the netdev_update_features() flow will also fail > > (for the same reasons as the previous one). If bnx2x_nic_load() fails > > for the certain NIC we actually shut this NIC down. So, the following > > remarks will be based on the above statement. > > In all those cases, bnx2x_reload_if_running() will be called only when > LRO state is changed while there's a recovery in progress. Hmmm... And what about all other features from hw_features? What if they have changed (in wanted_features) while recovery was in progress? According to the __netdev_update_features() code it will invoke ndo_set_features() in these cases either. Do I miss something here? > > [...] > > U shouldn't call for netdev_update_features(bp->dev) if bnx2x_nic_load() > > has failed. It would also be nice if netdev_update_features() would > > propagate the exit status of ndo_set_features() when ndo_set_features() > > fails in the __netdev_update_features(). > > That's fixed in v3. Not everything. See below. > > > See the patch for the bnx2x below: > > > > @@ -8993,7 +8995,14 @@ static int bnx2x_open(struct net_device *dev) > > > > bp->recovery_state = BNX2X_RECOVERY_DONE; > > > > - return bnx2x_nic_load(bp, LOAD_OPEN); > > + rc = bnx2x_nic_load(bp, LOAD_OPEN); > > + if (!rc) > > + netdev_update_features(bp->dev); > > + > > + if (bp->state == BNX2X_STATE_OPEN) > > + return 0; > > + else > > + return -EBUSY; > > } > > Hmm. I missed this part in the v3 patch. This clobbers bnx2x_nic_load()'s > error return, though. Exactly! Quoting my remark above: "It would also be nice if netdev_update_features() would propagate the exit status of ndo_set_features() when ndo_set_features() fails in the __netdev_update_features()." Could u comment on this, pls. > > > > /* called with rtnl_lock */ > > > @@ -9304,6 +9309,8 @@ static const struct net_device_ops bnx2x_netdev_ops = { > > > .ndo_validate_addr = eth_validate_addr, > > > .ndo_do_ioctl = bnx2x_ioctl, > > > .ndo_change_mtu = bnx2x_change_mtu, > > > + .ndo_fix_features = bnx2x_fix_features, > > > + .ndo_set_features = bnx2x_set_features, > > > .ndo_tx_timeout = bnx2x_tx_timeout, > > > #ifdef CONFIG_NET_POLL_CONTROLLER > > > .ndo_poll_controller = poll_bnx2x, > > > @@ -9430,20 +9437,18 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev, > > > > > > dev->netdev_ops = &bnx2x_netdev_ops; > > > bnx2x_set_ethtool_ops(dev); > > > - dev->features |= NETIF_F_SG; > > > - dev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM; > > > + > > > if (bp->flags & USING_DAC_FLAG) > > > dev->features |= NETIF_F_HIGHDMA; > > > - dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN); > > > - dev->features |= NETIF_F_TSO6; > > > - dev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX); > > > > > > - dev->vlan_features |= NETIF_F_SG; > > > - dev->vlan_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM; > > > - if (bp->flags & USING_DAC_FLAG) > > > - dev->vlan_features |= NETIF_F_HIGHDMA; > > > - dev->vlan_features |= (NETIF_F_TSO | NETIF_F_TSO_ECN); > > > - dev->vlan_features |= NETIF_F_TSO6; > > > + dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | > > > + NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 | > > > + NETIF_F_HW_VLAN_TX; > > hw_features are missing NETIF_F_GRO and NETIF_F_LRO flags that are > > currently configured in bnx2x_init_bp(). > > GRO is enabled by core now. LRO is fixed in v3. Got it. Thanks. > > > > + dev->features |= dev->hw_features | NETIF_F_HW_VLAN_RX; > > > + > > > + dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | > > > + NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_HIGHDMA; > > I'm not sure if it's safe to set NETIF_F_HIGHDMA unconditionally. I > > think it's better to correlate it with the USING_DAC_FLAG which is set > > according to what is returned by > > dma_set_mask(&pdev->dev, DMA_BIT_MASK(64)). > > dev->vlan_features get masked with dev->features and only then applied > to VLAN device. Ok. However, could, pls., quote the above sentence of yours as a comment for this code line? ;) See my further comments for v4. thanks, vlad > > Best Regards, > Michał Mirosław > -- > 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 > -- 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
> > In all those cases, bnx2x_reload_if_running() will be called only when > > LRO state is changed while there's a recovery in progress. > > Hmmm... And what about all other features from hw_features? What if they > have changed (in wanted_features) while recovery was in progress? > According to the __netdev_update_features() code it will invoke > ndo_set_features() in these cases either. Do I miss something here? I think I understood what u meant. So, yes, if the bnx2x_nic_load() called only if TPA_ENABLED_FLAG in bp->flags has changed. And this can happen if either NETIF_F_LRO has changed while NETIF_F_RXCSUM was set or if NETIF_F_LRO was set and NETIF_F_RXCSUM is being cleared. thanks, vlad -- 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
diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c index f3cf889..71e7818 100644 --- a/drivers/net/bnx2x/bnx2x_main.c +++ b/drivers/net/bnx2x/bnx2x_main.c @@ -7728,6 +7728,8 @@ static void bnx2x_parity_recover(struct bnx2x *bp) return; } + netdev_update_features(bp->dev); + return; }