On Mon, Nov 01, 2010 at 09:25:54PM +0000, Ben Hutchings wrote:
> On Sat, 2010-10-30 at 10:44 +0200, Michał Mirosław wrote:
> [...]
> > @@ -1670,7 +1668,7 @@ struct net_device *nes_netdev_init(struct nes_device
> > *nesdev,
> > netdev->type = ARPHRD_ETHER;
> > netdev->features = NETIF_F_HIGHDMA;
> > netdev->netdev_ops = &nes_netdev_ops;
> > - netdev->hw_features |= NETIF_F_SG;
> > + netdev->hw_features |= NETIF_F_SG|NETIF_F_TSO;
>
> There should be spaces on either side of the '|' operator.
The style varies among drivers' sources. I don't have any preference
whatsoever.
> [...]
> > diff --git a/drivers/net/atlx/atl1.c b/drivers/net/atlx/atl1.c
> > index 9e27bd6..814a06c 100644
> > --- a/drivers/net/atlx/atl1.c
> > +++ b/drivers/net/atlx/atl1.c
> > @@ -2992,6 +2992,7 @@ static int __devinit atl1_probe(struct pci_dev *pdev,
> > netdev->watchdog_timeo = 5 * HZ;
> >
> > netdev->hw_features |= NETIF_F_SG;
> > + netdev->hw_features |= NETIF_F_TSO;
>
> Why not set both flags in the same statement? You might as well make
> the drivers consistent in this regard.
Will fix. The compiler should combine those anyway.
> [...]
> > diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> > index 017667c..9b0e598 100644
> > --- a/net/core/ethtool.c
> > +++ b/net/core/ethtool.c
> [...]
> > @@ -1065,10 +1048,13 @@ static int __ethtool_set_sg(struct net_device *dev,
> > u32 data)
> > {
> > int err;
> >
> > - if (!data && dev->ethtool_ops->set_tso) {
> > - err = dev->ethtool_ops->set_tso(dev, 0);
> > - if (err)
> > - return err;
> > + if (!data && (dev->hw_features & NETIF_F_ALL_TSO)) {
> > + if (dev->ethtool_ops->hw_set_tso) {
> > + err = dev->ethtool_ops->hw_set_tso(dev, 0);
> > + if (err < 0)
> > + return err;
> > + }
> > + dev->features &= dev->hw_features & NETIF_F_ALL_TSO;
>
> Surely this should be:
> dev->features &= ~NETIF_F_ALL_TSO;
I originally thought of hw_features as a bitfield of features changable
by ethtool. So if driver writer wanted to have TSO permanently on but
allow toggling of TSO6 for debug than that would work. There is actually
a bug in original code: it doesn't clear NETIF_F_TSO if there is no set_tso
function, but clears NETIF_F_SG anyway (the bug is preserved). This is
a material for successive patches.
BTW, why is scatter-gather needed for TSO and TX checksumming? I see
no interdependence on s-g for those offloads.
> [...]
> > @@ -1158,7 +1149,18 @@ static int ethtool_set_tso(struct net_device *dev,
> > char __user *useraddr)
> > if (edata.data && !(dev->features & NETIF_F_SG))
> > return -EINVAL;
> >
> > - return dev->ethtool_ops->set_tso(dev, edata.data);
> > + if (dev->ethtool_ops->hw_set_tso) {
> > + int err = dev->ethtool_ops->hw_set_tso(dev, edata.data);
> > + if (err)
> > + return min(err, 0);
> [...]
>
> Again, the odd semantics of a positive value need to be documented.
Of course.
Best Regards,
Michał Mirosław
------------------------------------------------------------------------------
Nokia and AT&T present the 2010 Calling All Innovators-North America contest
Create new apps & games for the Nokia N8 for consumers in U.S. and Canada
$10 million total in prizes - $4M cash, 500 devices, nearly $6M in marketing
Develop with Nokia Qt SDK, Web Runtime, or Java and Publish to Ovi Store
http://p.sf.net/sfu/nokia-dev2dev
_______________________________________________
E1000-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit
http://communities.intel.com/community/wired
