Message ID | 20110413195851.25901.8139.stgit@gitlad.jf.intel.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
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.
>-----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.
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.
>-----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 --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();
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