[net-next-2.6 RFC PATCH] ethtool: allow custom interval for physical identification

[net-next-2.6 RFC PATCH] ethtool: allow custom interval for physical identification

[Bruce Allan]

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 can
specify the interval duration as a positive number; -EINVAL is still
allowed in order to use the default single on/off interval per second.

Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
---

 net/core/ethtool.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 43ef09f..02db945 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 && rc != -EINVAL)
 		return rc;
 
 	/* Drop the RTNL lock while waiting, but prevent reentry or
@@ -1656,22 +1656,25 @@ static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
 			id.data ? (id.data * HZ) : MAX_SCHEDULE_TIMEOUT);
 	} else {
 		/* Driver expects to be called periodically */
+		int i = 0, interval = ((rc > 0) ? rc : (HZ / 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();

Re: [net-next-2.6 RFC PATCH] ethtool: allow custom interval for physical identification

[Stephen Hemminger]

On Mon, 11 Apr 2011 16:16:35 -0700
Bruce Allan <bruce.w.allan@intel.com> 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 can
> specify the interval duration as a positive number; -EINVAL is still
> allowed in order to use the default single on/off interval per second.
> 
> Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>

IMHO this is -EOVERKILL.

-- 

RE: [net-next-2.6 RFC PATCH] ethtool: allow custom interval for physical identification

[Allan, Bruce W]

>-----Original Message-----
>From: Stephen Hemminger [mailto:shemminger@vyatta.com]
>Sent: Monday, April 11, 2011 4:26 PM
>To: Allan, Bruce W
>Cc: netdev@vger.kernel.org
>Subject: Re: [net-next-2.6 RFC PATCH] ethtool: allow custom interval for
>physical identification
>
>On Mon, 11 Apr 2011 16:16:35 -0700
>Bruce Allan <bruce.w.allan@intel.com> 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 can
>> specify the interval duration as a positive number; -EINVAL is still
>> allowed in order to use the default single on/off interval per second.
>>
>> Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
>
>IMHO this is -EOVERKILL.

I realize it does seem like that, but we have OEMs that expect the LEDs to
blink a certain way during a physical identification (twice a second vs.
once a second per the original .set_phys_id patchset).  There may be other
drivers from different hardware vendors that have similar but different
requirements.

RE: [net-next-2.6 RFC PATCH] ethtool: allow custom interval for physical identification

[Ben Hutchings]

On Mon, 2011-04-11 at 16:30 -0700, Allan, Bruce W wrote:
> >-----Original Message-----
> >From: Stephen Hemminger [mailto:shemminger@vyatta.com]
> >Sent: Monday, April 11, 2011 4:26 PM
> >To: Allan, Bruce W
> >Cc: netdev@vger.kernel.org
> >Subject: Re: [net-next-2.6 RFC PATCH] ethtool: allow custom interval for
> >physical identification
> >
> >On Mon, 11 Apr 2011 16:16:35 -0700
> >Bruce Allan <bruce.w.allan@intel.com> 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 can
> >> specify the interval duration as a positive number; -EINVAL is still
> >> allowed in order to use the default single on/off interval per second.
> >>
> >> Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
> >
> >IMHO this is -EOVERKILL.
> 
> I realize it does seem like that, but we have OEMs that expect the LEDs to
> blink a certain way during a physical identification (twice a second vs.
> once a second per the original .set_phys_id patchset).  There may be other
> drivers from different hardware vendors that have similar but different
> requirements.

I noticed that some drivers did this.  Do you know if these OEMs expect
this of all hardware, or do they actually want different vendors'
hardware to blink in different ways?  If it's a common requirement to
blink at 2 Hz then let's use that frequency for all the drivers that
want to be called periodically.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

RE: [net-next-2.6 RFC PATCH] ethtool: allow custom interval for physical identification

[Allan, Bruce W]

>-----Original Message-----
>From: Ben Hutchings [mailto:bhutchings@solarflare.com]
>Sent: Monday, April 11, 2011 5:01 PM
>To: Allan, Bruce W
>Cc: Stephen Hemminger; netdev@vger.kernel.org
>Subject: RE: [net-next-2.6 RFC PATCH] ethtool: allow custom interval for
>physical identification
>
>I noticed that some drivers did this.  Do you know if these OEMs expect
>this of all hardware, or do they actually want different vendors'
>hardware to blink in different ways?  If it's a common requirement to
>blink at 2 Hz then let's use that frequency for all the drivers that
>want to be called periodically.
>
>Ben.

Sorry, I don't know.  I'll ask around, but doubt I will get a definitive
answer.

FWIW, without digging too deep into how other drivers identify their
respective ports through software, it appears it was split:
* bnx2*, cxgb3, niu, s2io, sfc, sky2, tg3 - once per second
* e100*, igb, ixgb*, pcnet32, ewrk3, cxgb4 - approx. twice per second

AFAIK for parts that can set the physical identification through hardware,
the Intel drivers set the on/off intervals to approximately twice/second;
I don't know what other drivers do in that situation.

So, I would guess it is not a common requirement to blink at a specific Hz.
I have no problem with changing the hard-coded blink frequency to what our
OEMs expect, but that might be an issue for those other vendors; I was just
trying to make it flexible.

Thanks,
Bruce.

RE: [net-next-2.6 RFC PATCH] ethtool: allow custom interval for physical identification

[Ben Hutchings]

On Mon, 2011-04-11 at 18:07 -0700, Allan, Bruce W wrote:
> >-----Original Message-----
> >From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> >Sent: Monday, April 11, 2011 5:01 PM
> >To: Allan, Bruce W
> >Cc: Stephen Hemminger; netdev@vger.kernel.org
> >Subject: RE: [net-next-2.6 RFC PATCH] ethtool: allow custom interval for
> >physical identification
> >
> >I noticed that some drivers did this.  Do you know if these OEMs expect
> >this of all hardware, or do they actually want different vendors'
> >hardware to blink in different ways?  If it's a common requirement to
> >blink at 2 Hz then let's use that frequency for all the drivers that
> >want to be called periodically.
> >
> >Ben.
> 
> Sorry, I don't know.  I'll ask around, but doubt I will get a definitive
> answer.

I enquired here and found that we do have an OEM specifying 1 Hz.

> FWIW, without digging too deep into how other drivers identify their
> respective ports through software, it appears it was split:
> * bnx2*, cxgb3, niu, s2io, sfc, sky2, tg3 - once per second
> * e100*, igb, ixgb*, pcnet32, ewrk3, cxgb4 - approx. twice per second
>
> AFAIK for parts that can set the physical identification through hardware,
> the Intel drivers set the on/off intervals to approximately twice/second;
> I don't know what other drivers do in that situation.
> 
> So, I would guess it is not a common requirement to blink at a specific Hz.
> I have no problem with changing the hard-coded blink frequency to what our
> OEMs expect, but that might be an issue for those other vendors; I was just
> trying to make it flexible.

Sadly it appears this is necessary.

Let's define the return value for drivers wanting periodic callbacks to
be the blink frequency in Hz (normally 1 or 2), and get rid of the
special case of -EINVAL.  This also removes the rather inelegant
semantic that drivers may need to change their state despite returning
an error code.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

RE: [net-next-2.6 RFC PATCH] ethtool: allow custom interval for physical identification

[Allan, Bruce W]

>-----Original Message-----
>From: Ben Hutchings [mailto:bhutchings@solarflare.com]
>Sent: Tuesday, April 12, 2011 9:28 AM
>To: Allan, Bruce W
>Cc: Stephen Hemminger; netdev@vger.kernel.org
>Subject: RE: [net-next-2.6 RFC PATCH] ethtool: allow custom interval for
>physical identification
>
>I enquired here and found that we do have an OEM specifying 1 Hz.
>
>> FWIW, without digging too deep into how other drivers identify their
>> respective ports through software, it appears it was split:
>> * bnx2*, cxgb3, niu, s2io, sfc, sky2, tg3 - once per second
>> * e100*, igb, ixgb*, pcnet32, ewrk3, cxgb4 - approx. twice per second
>>
>> AFAIK for parts that can set the physical identification through hardware,
>> the Intel drivers set the on/off intervals to approximately twice/second;
>> I don't know what other drivers do in that situation.
>>
>> So, I would guess it is not a common requirement to blink at a specific Hz.
>> I have no problem with changing the hard-coded blink frequency to what our
>> OEMs expect, but that might be an issue for those other vendors; I was just
>> trying to make it flexible.
>
>Sadly it appears this is necessary.
>
>Let's define the return value for drivers wanting periodic callbacks to
>be the blink frequency in Hz (normally 1 or 2), and get rid of the
>special case of -EINVAL.  This also removes the rather inelegant
>semantic that drivers may need to change their state despite returning
>an error code.
>
>Ben.

OK.  Would you like me to send an updated patch?

Bruce.

RE: [net-next-2.6 RFC PATCH] ethtool: allow custom interval for physical identification

[Ben Hutchings]

On Tue, 2011-04-12 at 11:17 -0700, Allan, Bruce W wrote:
> >-----Original Message-----
> >From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> >Sent: Tuesday, April 12, 2011 9:28 AM
> >To: Allan, Bruce W
> >Cc: Stephen Hemminger; netdev@vger.kernel.org
> >Subject: RE: [net-next-2.6 RFC PATCH] ethtool: allow custom interval for
> >physical identification
> >
> >I enquired here and found that we do have an OEM specifying 1 Hz.
> >
> >> FWIW, without digging too deep into how other drivers identify their
> >> respective ports through software, it appears it was split:
> >> * bnx2*, cxgb3, niu, s2io, sfc, sky2, tg3 - once per second
> >> * e100*, igb, ixgb*, pcnet32, ewrk3, cxgb4 - approx. twice per second
> >>
> >> AFAIK for parts that can set the physical identification through hardware,
> >> the Intel drivers set the on/off intervals to approximately twice/second;
> >> I don't know what other drivers do in that situation.
> >>
> >> So, I would guess it is not a common requirement to blink at a specific Hz.
> >> I have no problem with changing the hard-coded blink frequency to what our
> >> OEMs expect, but that might be an issue for those other vendors; I was just
> >> trying to make it flexible.
> >
> >Sadly it appears this is necessary.
> >
> >Let's define the return value for drivers wanting periodic callbacks to
> >be the blink frequency in Hz (normally 1 or 2), and get rid of the
> >special case of -EINVAL.  This also removes the rather inelegant
> >semantic that drivers may need to change their state despite returning
> >an error code.
> >
> >Ben.
> 
> OK.  Would you like me to send an updated patch?

Please.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.