| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20110411201225.GA9249@rere.qmqm.pl>
Date: Mon, 11 Apr 2011 22:12:25 +0200
From: Michał Mirosław <mirq-linux@...e.qmqm.pl>
To: Vladislav Zolotarov <vladz@...adcom.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Eilon Greenstein <eilong@...adcom.com>
Subject: Re: [PATCH v2] net: bnx2x: convert to hw_features
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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists