diff mbox

[net-next-2.6,RFC,v2,01/13] ethtool: allow custom interval for physical identification

Message ID 20110413195851.25901.8139.stgit@gitlad.jf.intel.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Allan, Bruce W April 13, 2011, 7:58 p.m. UTC 
When physical identification of an adapter is done by toggling the
mechanism on and off through software utilizing the set_phys_id operation,
it is done with a fixed duration for both on and off states.  Some drivers
may want to set a custom duration for the on/off intervals.  This patch
changes the API so the return code from the driver's entry point when it
is called with ETHTOOL_ID_ACTIVE can specify the frequency at which to
cycle the on/off states.

Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
Cc: Ben Hutchings <bhutchings@solarflare.com>
---

 include/linux/ethtool.h |    6 ++++--
 net/core/ethtool.c      |   13 ++++++++-----
 2 files changed, 12 insertions(+), 7 deletions(-)


--
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

Comments

Ben Hutchings April 13, 2011, 8:25 p.m. UTC | #1 
On Wed, 2011-04-13 at 12:58 -0700, Bruce Allan wrote:
> When physical identification of an adapter is done by toggling the
> mechanism on and off through software utilizing the set_phys_id operation,
> it is done with a fixed duration for both on and off states.  Some drivers
> may want to set a custom duration for the on/off intervals.  This patch
> changes the API so the return code from the driver's entry point when it
> is called with ETHTOOL_ID_ACTIVE can specify the frequency at which to
> cycle the on/off states.
[...]
> @@ -1655,23 +1655,26 @@ static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
>  		schedule_timeout_interruptible(
>  			id.data ? (id.data * HZ) : MAX_SCHEDULE_TIMEOUT);
>  	} else {
> -		/* Driver expects to be called periodically */
> +		/* Driver expects to be called using the frequency in rc */
> +		int i = 0, interval = (HZ / (rc * 2));
> +
>  		do {
>  			rtnl_lock();
>  			rc = dev->ethtool_ops->set_phys_id(dev, ETHTOOL_ID_ON);
>  			rtnl_unlock();
>  			if (rc)
>  				break;
> -			schedule_timeout_interruptible(HZ / 2);
> +			schedule_timeout_interruptible(interval);
>  
>  			rtnl_lock();
>  			rc = dev->ethtool_ops->set_phys_id(dev, ETHTOOL_ID_OFF);
>  			rtnl_unlock();
>  			if (rc)
>  				break;
> -			schedule_timeout_interruptible(HZ / 2);
> +			schedule_timeout_interruptible(interval);
>  		} while (!signal_pending(current) &&
> -			 (id.data == 0 || --id.data != 0));
> +			 (id.data == 0 ||
> +			  (++i * 2 * interval) < (id.data * HZ)));
[...]

I'm sure there ought to be a clearer way to do this, and to avoid any
weird effects from integer overflow in the multiplication.  How about
using an inner loop for each second:

		/* Driver expects to be called at twice the frequency in rc */
		int n = rc * 2, i, interval = HZ / n;

		do {
			i = n;
			do {
	 			rtnl_lock();
 				rc = dev->ethtool_ops->set_phys_id(
					dev, (i & 1) ? ETHTOOL_ID_OFF : ETHTOOL_ID_ON);
	 			rtnl_unlock();
 				if (rc)
 					break;
				schedule_timeout_interruptible(interval);
			} while (!signal_pending(current) && --i != 0);
 		} while (!signal_pending(current) &&
			 (id.data == 0 || --id.data != 0));

Ben.
Allan, Bruce W April 13, 2011, 10:39 p.m. UTC | #2 
>-----Original Message-----

>From: Ben Hutchings [mailto:bhutchings@solarflare.com]

>Sent: Wednesday, April 13, 2011 1:25 PM

>To: Allan, Bruce W

>Cc: netdev@vger.kernel.org

>Subject: Re: [net-next-2.6 RFC PATCH v2 01/13] ethtool: allow custom interval

>for physical identification

>

>I'm sure there ought to be a clearer way to do this, and to avoid any

>weird effects from integer overflow in the multiplication.  How about

>using an inner loop for each second:

>

>		/* Driver expects to be called at twice the frequency in rc */

>		int n = rc * 2, i, interval = HZ / n;

>

>		do {

>			i = n;

>			do {

>	 			rtnl_lock();

> 				rc = dev->ethtool_ops->set_phys_id(

>					dev, (i & 1) ? ETHTOOL_ID_OFF : ETHTOOL_ID_ON);

>	 			rtnl_unlock();

> 				if (rc)

> 					break;

>				schedule_timeout_interruptible(interval);

>			} while (!signal_pending(current) && --i != 0);

> 		} while (!signal_pending(current) &&

>			 (id.data == 0 || --id.data != 0));

>

>Ben.


OK, if that is clearer to you...v3 forthcoming.

Thanks,
Bruce.
Ben Hutchings April 13, 2011, 10:44 p.m. UTC | #3 
On Wed, 2011-04-13 at 15:39 -0700, Allan, Bruce W wrote:
> 
> >-----Original Message-----
> >From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> >Sent: Wednesday, April 13, 2011 1:25 PM
> >To: Allan, Bruce W
> >Cc: netdev@vger.kernel.org
> >Subject: Re: [net-next-2.6 RFC PATCH v2 01/13] ethtool: allow custom interval
> >for physical identification
> >
> >I'm sure there ought to be a clearer way to do this, and to avoid any
> >weird effects from integer overflow in the multiplication.  How about
> >using an inner loop for each second:
> >
> >		/* Driver expects to be called at twice the frequency in rc */
> >		int n = rc * 2, i, interval = HZ / n;
> >

		/* Count down seconds */
> >		do {
			/* Count down iterations per second */
> >			i = n;
> >			do {
> >	 			rtnl_lock();
> > 				rc = dev->ethtool_ops->set_phys_id(
> >					dev, (i & 1) ? ETHTOOL_ID_OFF : ETHTOOL_ID_ON);
> >	 			rtnl_unlock();
> > 				if (rc)
> > 					break;
> >				schedule_timeout_interruptible(interval);
> >			} while (!signal_pending(current) && --i != 0);
> > 		} while (!signal_pending(current) &&
> >			 (id.data == 0 || --id.data != 0));
> >
> >Ben.
> 
> OK, if that is clearer to you...v3 forthcoming.

I guess it wouldn't hurt to add comemnts too.  Would you agree that it's
clear with the additions above?

Ben.
Allan, Bruce W April 13, 2011, 10:55 p.m. UTC | #4 
>-----Original Message-----

>From: Ben Hutchings [mailto:bhutchings@solarflare.com]

>Sent: Wednesday, April 13, 2011 3:45 PM

>To: Allan, Bruce W

>Cc: netdev@vger.kernel.org

>Subject: RE: [net-next-2.6 RFC PATCH v2 01/13] ethtool: allow custom interval

>for physical identification

>

>On Wed, 2011-04-13 at 15:39 -0700, Allan, Bruce W wrote:

>>

>> >-----Original Message-----

>> >From: Ben Hutchings [mailto:bhutchings@solarflare.com]

>> >Sent: Wednesday, April 13, 2011 1:25 PM

>> >To: Allan, Bruce W

>> >Cc: netdev@vger.kernel.org

>> >Subject: Re: [net-next-2.6 RFC PATCH v2 01/13] ethtool: allow custom interval

>> >for physical identification

>> >

>> >I'm sure there ought to be a clearer way to do this, and to avoid any

>> >weird effects from integer overflow in the multiplication.  How about

>> >using an inner loop for each second:

>> >

>> >		/* Driver expects to be called at twice the frequency in rc */

>> >		int n = rc * 2, i, interval = HZ / n;

>> >

>

>		/* Count down seconds */

>> >		do {

>			/* Count down iterations per second */

>> >			i = n;

>> >			do {

>> >	 			rtnl_lock();

>> > 				rc = dev->ethtool_ops->set_phys_id(

>> >					dev, (i & 1) ? ETHTOOL_ID_OFF : ETHTOOL_ID_ON);

>> >	 			rtnl_unlock();

>> > 				if (rc)

>> > 					break;

>> >				schedule_timeout_interruptible(interval);

>> >			} while (!signal_pending(current) && --i != 0);

>> > 		} while (!signal_pending(current) &&

>> >			 (id.data == 0 || --id.data != 0));

>> >

>> >Ben.

>>

>> OK, if that is clearer to you...v3 forthcoming.

>

>I guess it wouldn't hurt to add comemnts too.  Would you agree that it's

>clear with the additions above?

>

>Ben.


Sure, makes sense to me.

Thanks,
Bruce.
diff mbox

Patch

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 12cfbd0..6191a84 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -770,8 +770,10 @@  bool ethtool_invalid_flags(struct net_device *dev, u32 data, u32 supported);
  *	attached to it.  The implementation may update the indicator
  *	asynchronously or synchronously, but in either case it must return
  *	quickly.  It is initially called with the argument %ETHTOOL_ID_ACTIVE,
- *	and must either activate asynchronous updates or return -%EINVAL.
- *	If it returns -%EINVAL then it will be called again at intervals with
+ *	and must either activate asynchronous updates and return zero, return
+ *	a negative error or return a positive frequency for synchronous
+ *	indication (e.g. 1 for one on/off cycle per second).  If it returns
+ *	a frequency then it will be called again at intervals with the
  *	argument %ETHTOOL_ID_ON or %ETHTOOL_ID_OFF and should set the state of
  *	the indicator accordingly.  Finally, it is called with the argument
  *	%ETHTOOL_ID_INACTIVE and must deactivate the indicator.  Returns a
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 43ef09f..2dd7bde 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1640,7 +1640,7 @@  static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
 		return dev->ethtool_ops->phys_id(dev, id.data);
 
 	rc = dev->ethtool_ops->set_phys_id(dev, ETHTOOL_ID_ACTIVE);
-	if (rc && rc != -EINVAL)
+	if (rc < 0)
 		return rc;
 
 	/* Drop the RTNL lock while waiting, but prevent reentry or
@@ -1655,23 +1655,26 @@  static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
 		schedule_timeout_interruptible(
 			id.data ? (id.data * HZ) : MAX_SCHEDULE_TIMEOUT);
 	} else {
-		/* Driver expects to be called periodically */
+		/* Driver expects to be called using the frequency in rc */
+		int i = 0, interval = (HZ / (rc * 2));
+
 		do {
 			rtnl_lock();
 			rc = dev->ethtool_ops->set_phys_id(dev, ETHTOOL_ID_ON);
 			rtnl_unlock();
 			if (rc)
 				break;
-			schedule_timeout_interruptible(HZ / 2);
+			schedule_timeout_interruptible(interval);
 
 			rtnl_lock();
 			rc = dev->ethtool_ops->set_phys_id(dev, ETHTOOL_ID_OFF);
 			rtnl_unlock();
 			if (rc)
 				break;
-			schedule_timeout_interruptible(HZ / 2);
+			schedule_timeout_interruptible(interval);
 		} while (!signal_pending(current) &&
-			 (id.data == 0 || --id.data != 0));
+			 (id.data == 0 ||
+			  (++i * 2 * interval) < (id.data * HZ)));
 	}
 
 	rtnl_lock();