diff mbox

Make possible speeds known to ethtool

Message ID 4966DABD.1000203@pobox.com
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Jeff Garzik Jan. 9, 2009, 5:03 a.m. UTC 
Herbert Xu wrote:
> Jeff Garzik <jgarzik@pobox.com> wrote:
>> For generic net stack flags outside the driver's control, that can 
>> easily be added to ethtool_{get,set}_flags() overriding or ignoring 
>> whatever the driver may have done.
> 
> To use the flags interface as is, you'd have to go through every
> single driver to get them to call ethtool_op_set_flags.  I'm sorry
> but I'm sticking with the current interface.

I think you misunderstand.  You don't have touch any drivers at all... 
see attached demonstration patch.

The more general point is that it is silly to add two ethtool ioctls 
each time you want to twiddle a single boolean flag (whatever that flag 
may be, generic or driver-specific or whatnot).

If you still desire separation from ->{get,set}_flags() ops, then at 
least create an ETHTOOL_[GS]STACK_FLAGS.

	Jeff

Comments

Herbert Xu Jan. 9, 2009, 5:15 a.m. UTC | #1 
On Fri, Jan 09, 2009 at 12:03:57AM -0500, Jeff Garzik wrote:
>
> I think you misunderstand.  You don't have touch any drivers at all...  
> see attached demonstration patch.
>
> The more general point is that it is silly to add two ethtool ioctls  
> each time you want to twiddle a single boolean flag (whatever that flag  
> may be, generic or driver-specific or whatnot).
>
> If you still desire separation from ->{get,set}_flags() ops, then at  
> least create an ETHTOOL_[GS]STACK_FLAGS.

OK, however I'm still not convinced that this is a good idea.
First of all we don't have a shortage in the ethtool name space,
we've only used up two hex digits worth of a 32-bit integer field.

More importantly, making multiple bit changes at the same time
may create semantic nightmares in future.

For example, imagine if we started out with this generic flag
function and TX checksum offload, SG, TSO were done using it.
Now the user issues a request changing all of these bits, we'd
then have to either validate it for contradictory settings, or
devise some ad-hoc ordering in which the settings are applied.

This just seems to be unnecessary penny-pinching that doesn't
save much but may end up costing us down the road.

Cheers,
Jeff Garzik Jan. 9, 2009, 5:30 a.m. UTC | #2 
Herbert Xu wrote:
> On Fri, Jan 09, 2009 at 12:03:57AM -0500, Jeff Garzik wrote:
>> I think you misunderstand.  You don't have touch any drivers at all...  
>> see attached demonstration patch.
>>
>> The more general point is that it is silly to add two ethtool ioctls  
>> each time you want to twiddle a single boolean flag (whatever that flag  
>> may be, generic or driver-specific or whatnot).
>>
>> If you still desire separation from ->{get,set}_flags() ops, then at  
>> least create an ETHTOOL_[GS]STACK_FLAGS.
> 
> OK, however I'm still not convinced that this is a good idea.
> First of all we don't have a shortage in the ethtool name space,
> we've only used up two hex digits worth of a 32-bit integer field.
> 
> More importantly, making multiple bit changes at the same time
> may create semantic nightmares in future.
> 
> For example, imagine if we started out with this generic flag
> function and TX checksum offload, SG, TSO were done using it.
> Now the user issues a request changing all of these bits, we'd
> then have to either validate it for contradictory settings, or
> devise some ad-hoc ordering in which the settings are applied.

Actually it's just the opposite -- _the_ most common complaint from 
users and driver developers of the ethtool interface, over the years, 
has been that there is no way to collect all the modifications and then 
commit it to hardware all in one go.

Each new ethtool command added often winds up pausing and resetting the 
hardware completely, and ETHTOOL_SGRO is no exception.

Now consider what happens when you have a lot of settings to tweak... 
reset, reset, reset, reset, reset.  That's a lot of needless hardware 
banging.

If I could redesign everything from scratch, I would make the interface 
flexible enough to input a list of changed settings, and then commit 
those all at once.  Lists of flags make that possible to a limited 
extent, at least.


> This just seems to be unnecessary penny-pinching that doesn't
> save much but may end up costing us down the road.

This sort of interface was added because the current "multi-reset" 
interface has already cost us.

A multi-flag interface is also more flexible and future-proof, as you 
can deploy new flags without having to update any userland utility... 
ethtool can permit input of the raw value as well as a more refined 
command interface.

That's another facet of the driver-private flag interfaces, too.  The 
ETHTOOL_[GS]PFLAGS interface is flexible enough to permit a wide variety 
of feature flags to be exported by various drivers; userland ethtool 
automatically supports all flags a driver supports, and never needs 
updating even if the list of supported driver-private flags change.

	Jeff



--
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
Herbert Xu Jan. 9, 2009, 5:35 a.m. UTC | #3 
On Fri, Jan 09, 2009 at 12:30:21AM -0500, Jeff Garzik wrote:
>
> Actually it's just the opposite -- _the_ most common complaint from  
> users and driver developers of the ethtool interface, over the years,  
> has been that there is no way to collect all the modifications and then  
> commit it to hardware all in one go.

Yes that's a problem for flags that require the drivers to reset
itself.

> Each new ethtool command added often winds up pausing and resetting the  
> hardware completely, and ETHTOOL_SGRO is no exception.

But as I explained before, GRO (like GSO) is purely a software
setting, it has nothing to do with the driver at all.  In other
words we don't need the driver to reset or do anything.

If anything by going into the driver's set_flags function as you
suggested may cause a spurious reset that wouldn't have happened
otherwise.

So for software flags like GSO/GRO at least, I don't see any
benefit in going to a multi-bit interface.  On the flip side,
I see potential complications with a multi-bit interfaces that
simply don't exist with a single-bit interface.

Cheers,
Jeff Garzik Jan. 9, 2009, 6:28 a.m. UTC | #4 
Herbert Xu wrote:
> On Fri, Jan 09, 2009 at 12:30:21AM -0500, Jeff Garzik wrote:
>> Actually it's just the opposite -- _the_ most common complaint from  
>> users and driver developers of the ethtool interface, over the years,  
>> has been that there is no way to collect all the modifications and then  
>> commit it to hardware all in one go.
> 
> Yes that's a problem for flags that require the drivers to reset
> itself.
> 
>> Each new ethtool command added often winds up pausing and resetting the  
>> hardware completely, and ETHTOOL_SGRO is no exception.
> 
> But as I explained before, GRO (like GSO) is purely a software
> setting, it has nothing to do with the driver at all.  In other

Not quite true...  it touches the driver's rx-csum hook.


> If anything by going into the driver's set_flags function as you
> suggested may cause a spurious reset that wouldn't have happened
> otherwise.
> 
> So for software flags like GSO/GRO at least, I don't see any
> benefit in going to a multi-bit interface.  On the flip side,
> I see potential complications with a multi-bit interfaces that
> simply don't exist with a single-bit interface.

Well, whichever.  Overall, if [GS]GRO remains I am happy to take patches 
supporting it in the userspace utility...

	Jeff


--
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
Herbert Xu Jan. 9, 2009, 6:30 a.m. UTC | #5 
On Fri, Jan 09, 2009 at 01:28:22AM -0500, Jeff Garzik wrote:
>
> Not quite true...  it touches the driver's rx-csum hook.

Only because we never put the RX offload flag in a place that's
independent of the driver.  Also GRO never sets the RX offload
flag (or clears it).

> Well, whichever.  Overall, if [GS]GRO remains I am happy to take patches  
> supporting it in the userspace utility...

Thanks,
diff mbox

Patch

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 27c67a5..75fab70 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -285,6 +285,7 @@  struct ethtool_perm_addr {
  */
 enum ethtool_flags {
 	ETH_FLAG_LRO		= (1 << 15),	/* LRO is enabled */
+	ETH_FLAG_GRO		= (1 << 14),	/* GRO is enabled */
 };
 
 struct ethtool_rxnfc {
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 947710a..a114afa 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -864,6 +864,60 @@  static int ethtool_set_value(struct net_device *dev, char __user *useraddr,
 	return actor(dev, edata.data);
 }
 
+static int ethtool_get_generic_flags(struct net_device *dev, u32 *val_out)
+{
+	if (dev->features & NETIF_F_GRO)
+		*val_out |= ETH_FLAG_GRO;
+
+	return 0;
+}
+
+static int ethtool_set_generic_flags(struct net_device *dev, u32 val_in)
+{
+	if (val_in & ETH_FLAG_GRO) {
+		if (!dev->ethtool_ops->get_rx_csum ||
+		    !dev->ethtool_ops->get_rx_csum(dev))
+			return -EINVAL;
+		dev->features |= NETIF_F_GRO;
+	} else
+		dev->features &= ~NETIF_F_GRO;
+
+	return 0;
+}
+
+static int ethtool_get_flags(struct net_device *dev, char __user *useraddr)
+{
+	struct ethtool_value edata = { ETHTOOL_GFLAGS };
+	int rc;
+
+	if (dev->ethtool_ops->get_flags)
+		edata.data = dev->ethtool_ops->get_flags(dev);
+
+	rc = ethtool_get_generic_flags(dev, &edata.data);
+	if (rc)
+		return rc;
+
+	if (copy_to_user(useraddr, &edata, sizeof(edata)))
+		return -EFAULT;
+	return 0;
+}
+
+static int ethtool_set_flags(struct net_device *dev, char __user *useraddr)
+{
+	struct ethtool_value edata;
+	int rc;
+
+	if (copy_from_user(&edata, useraddr, sizeof(edata)))
+		return -EFAULT;
+
+	rc = ethtool_set_generic_flags(dev, edata.data);
+
+	if (rc == 0 && dev->ethtool_ops->set_flags)
+		rc = dev->ethtool_ops->set_flags(dev, edata.data);
+
+	return rc;
+}
+
 /* The main entry point in this file.  Called from net/core/dev.c */
 
 int dev_ethtool(struct net *net, struct ifreq *ifr)
@@ -1036,12 +1090,10 @@  int dev_ethtool(struct net *net, struct ifreq *ifr)
 		rc = ethtool_set_gso(dev, useraddr);
 		break;
 	case ETHTOOL_GFLAGS:
-		rc = ethtool_get_value(dev, useraddr, ethcmd,
-				       dev->ethtool_ops->get_flags);
+		rc = ethtool_get_flags(dev, useraddr);
 		break;
 	case ETHTOOL_SFLAGS:
-		rc = ethtool_set_value(dev, useraddr,
-				       dev->ethtool_ops->set_flags);
+		rc = ethtool_set_flags(dev, useraddr);
 		break;
 	case ETHTOOL_GPFLAGS:
 		rc = ethtool_get_value(dev, useraddr, ethcmd,