[PATCH 01/11] time: add *_to_jiffies_min helpers to guarantee a minimum duration

[PATCH 01/11] time: add *_to_jiffies_min helpers to guarantee a minimum duration

[Imre Deak]

The *_to_jiffies(x) macros return a jiffy value, which if used as a
delta to wait for a specific amount of time, may result in a wait-time
that is less than x. Many callers already compensate for this by adding
one to the returned value. To document why we need to add one and to get
rid of some code duplication add a helper that does the same.

Later patches will convert the currently open-coded call sites to use
the new helpers.

Also this can serve as a basis for auditing those users of *_to_jiffies
that most likely do the wrong thing - for example set a timeout value of
msecs_to_jiffies(1) - and converting them to use the new helpers.

Kudos to Daniel Vetter for the idea of the new helpers.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 include/linux/jiffies.h |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/linux/jiffies.h b/include/linux/jiffies.h
index 82ed068..d12509b 100644
--- a/include/linux/jiffies.h
+++ b/include/linux/jiffies.h
@@ -312,6 +312,19 @@ extern u64 nsec_to_clock_t(u64 x);
 extern u64 nsecs_to_jiffies64(u64 n);
 extern unsigned long nsecs_to_jiffies(u64 n);
 
+#define __define_time_to_jiffies_min(tname, ttype)			\
+static inline unsigned long						\
+tname ## _to_jiffies_min(const ttype m)					\
+{									\
+	return max_t(long, MAX_JIFFY_OFFSET, tname ## _to_jiffies(m) + 1);\
+}
+
+__define_time_to_jiffies_min(msecs, unsigned int)
+__define_time_to_jiffies_min(usecs, unsigned int)
+__define_time_to_jiffies_min(nsecs, u64)
+__define_time_to_jiffies_min(timespec, struct timespec *)
+__define_time_to_jiffies_min(timeval, struct timeval *)
+
 #define TIMESTAMP_SIZE	30
 
 #endif
-- 
1.7.10.4

[PATCH 02/11] sched: msleep: take msecs_to_jiffies_min into use

[Imre Deak]

Use msecs_to_jiffies_min instead of open-coding the same.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 kernel/timer.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/timer.c b/kernel/timer.c
index dbf7a78..dcdc8bd 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -1799,7 +1799,7 @@ void __init init_timers(void)
  */
 void msleep(unsigned int msecs)
 {
-	unsigned long timeout = msecs_to_jiffies(msecs) + 1;
+	unsigned long timeout = msecs_to_jiffies_min(msecs);
 
 	while (timeout)
 		timeout = schedule_timeout_uninterruptible(timeout);
@@ -1813,7 +1813,7 @@ EXPORT_SYMBOL(msleep);
  */
 unsigned long msleep_interruptible(unsigned int msecs)
 {
-	unsigned long timeout = msecs_to_jiffies(msecs) + 1;
+	unsigned long timeout = msecs_to_jiffies_min(msecs);
 
 	while (timeout && !signal_pending(current))
 		timeout = schedule_timeout_interruptible(timeout);
-- 
1.7.10.4

[PATCH 03/11] drm/i915: take msecs_to_jiffies_min into use

[Imre Deak]

Use msecs_to_jiffies_min instead of open-coding the same.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  |    2 +-
 drivers/gpu/drm/i915/intel_drv.h |    2 +-
 drivers/gpu/drm/i915/intel_i2c.c |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 92d1334..8be304d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -276,7 +276,7 @@ intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
 #define C (((status = I915_READ_NOTRACE(ch_ctl)) & DP_AUX_CH_CTL_SEND_BUSY) == 0)
 	if (has_aux_irq)
 		done = wait_event_timeout(dev_priv->gmbus_wait_queue, C,
-					  msecs_to_jiffies(10) + 1);
+					  msecs_to_jiffies_min(10));
 	else
 		done = wait_for_atomic(C, 10) == 0;
 	if (!done && !(C))
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 388a394..f2e3409 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -42,7 +42,7 @@
  * we've never had a chance to check the condition before the timeout.
  */
 #define _wait_for(COND, MS, W) ({ \
-	unsigned long timeout__ = jiffies + msecs_to_jiffies(MS) + 1;	\
+	unsigned long timeout__ = jiffies + msecs_to_jiffies_min(MS);	\
 	int ret__ = 0;							\
 	while (!(COND)) {						\
 		if (time_after(jiffies, timeout__)) {			\
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 5d24503..447b4b0 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -228,7 +228,7 @@ gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
 	 * need to wake up periodically and check that ourselves. */
 	I915_WRITE(GMBUS4 + reg_offset, gmbus4_irq_en);
 
-	for (i = 0; i < msecs_to_jiffies(50) + 1; i++) {
+	for (i = 0; i < msecs_to_jiffies_min(50); i++) {
 		prepare_to_wait(&dev_priv->gmbus_wait_queue, &wait,
 				TASK_UNINTERRUPTIBLE);
 
-- 
1.7.10.4

[PATCH 04/11] hwmon/lm63,lm90: take msecs_to_jiffies_min into use

[Imre Deak]

Use msecs_to_jiffies_min instead of open-coding the same.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/hwmon/lm63.c |    2 +-
 drivers/hwmon/lm90.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
index f644a2e..db44bcb 100644
--- a/drivers/hwmon/lm63.c
+++ b/drivers/hwmon/lm63.c
@@ -248,7 +248,7 @@ static struct lm63_data *lm63_update_device(struct device *dev)
 	mutex_lock(&data->update_lock);
 
 	next_update = data->last_updated
-	  + msecs_to_jiffies(data->update_interval) + 1;
+	  + msecs_to_jiffies_min(data->update_interval);
 
 	if (time_after(jiffies, next_update) || !data->valid) {
 		if (data->config & 0x04) { /* tachometer enabled  */
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 8eeb141..314f9d3 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -471,7 +471,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
 	mutex_lock(&data->update_lock);
 
 	next_update = data->last_updated
-	  + msecs_to_jiffies(data->update_interval) + 1;
+	  + msecs_to_jiffies_min(data->update_interval);
 	if (time_after(jiffies, next_update) || !data->valid) {
 		u8 h, l;
 		u8 alarms;
-- 
1.7.10.4

Re: [PATCH 04/11] hwmon/lm63,lm90: take msecs_to_jiffies_min into use

[Guenter Roeck]

On Fri, May 10, 2013 at 03:13:22PM +0300, Imre Deak wrote:
> Use msecs_to_jiffies_min instead of open-coding the same.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---

Function name could be better. Other than that,

Acked-by: Guenter Roeck <linux@roeck-us.net>

Re: [PATCH 04/11] hwmon/lm63,lm90: take msecs_to_jiffies_min into use

[Jean Delvare]

Hi Imre,

On Fri, 10 May 2013 15:13:22 +0300, Imre Deak wrote:
> Use msecs_to_jiffies_min instead of open-coding the same.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/hwmon/lm63.c |    2 +-
>  drivers/hwmon/lm90.c |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> index f644a2e..db44bcb 100644
> --- a/drivers/hwmon/lm63.c
> +++ b/drivers/hwmon/lm63.c
> @@ -248,7 +248,7 @@ static struct lm63_data *lm63_update_device(struct device *dev)
>  	mutex_lock(&data->update_lock);
>  
>  	next_update = data->last_updated
> -	  + msecs_to_jiffies(data->update_interval) + 1;
> +	  + msecs_to_jiffies_min(data->update_interval);
>  
>  	if (time_after(jiffies, next_update) || !data->valid) {
>  		if (data->config & 0x04) { /* tachometer enabled  */
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 8eeb141..314f9d3 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -471,7 +471,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
>  	mutex_lock(&data->update_lock);
>  
>  	next_update = data->last_updated
> -	  + msecs_to_jiffies(data->update_interval) + 1;
> +	  + msecs_to_jiffies_min(data->update_interval);
>  	if (time_after(jiffies, next_update) || !data->valid) {
>  		u8 h, l;
>  		u8 alarms;

I don't like this. The + 1 aren't there because msecs_to_jiffies() may
return less than the required time, as you claim. I don't think this is
the case (see the doubts expressed in my reply to patch 1/11.)

These + 1 are there because the chip may need exactly
data->update_interval for the data sampling and the datasheet isn't
completely clear about what would happen if the host polls for results
at exactly the same frequency the chip is sampling.

Most likely it is just paranoia from me and the + 1 can be removed.
Especially when time_after (as opposed to time_after_eq) already adds
some margin - probably exactly what we need here. I have a few chips
here I could test this on, BTW (ADM1032 and LM64.)

And even if this was actually needed, then it is written the wrong way.
We don't need one more jiffy, the SMBus slave doesn't even know what a
jiffy is. We need an arbitrary additional amount of time, expressed in
a standard time unit like ms. So
  msecs_to_jiffies(data->update_interval + 1)
would be the right way to write it.

On top of that, your proposed change will make the resulting binary
larger, the compilation longer, the future code reviews harder, and
backporting these drivers harder. Each of these points only by a very
small fraction, of course, but for a benefit which is even smaller
IMHO, if it even exists (which I doubt.)

So I'm not going to apply this patch, sorry.

-- 
Jean Delvare

Re: [PATCH 04/11] hwmon/lm63,lm90: take msecs_to_jiffies_min into use

[Imre Deak]

On Mon, 2013-05-13 at 09:47 +0200, Jean Delvare wrote:
> Hi Imre,
> 
> On Fri, 10 May 2013 15:13:22 +0300, Imre Deak wrote:
> > Use msecs_to_jiffies_min instead of open-coding the same.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/hwmon/lm63.c |    2 +-
> >  drivers/hwmon/lm90.c |    2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> > index f644a2e..db44bcb 100644
> > --- a/drivers/hwmon/lm63.c
> > +++ b/drivers/hwmon/lm63.c
> > @@ -248,7 +248,7 @@ static struct lm63_data *lm63_update_device(struct device *dev)
> >  	mutex_lock(&data->update_lock);
> >  
> >  	next_update = data->last_updated
> > -	  + msecs_to_jiffies(data->update_interval) + 1;
> > +	  + msecs_to_jiffies_min(data->update_interval);
> >  
> >  	if (time_after(jiffies, next_update) || !data->valid) {
> >  		if (data->config & 0x04) { /* tachometer enabled  */
> > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> > index 8eeb141..314f9d3 100644
> > --- a/drivers/hwmon/lm90.c
> > +++ b/drivers/hwmon/lm90.c
> > @@ -471,7 +471,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
> >  	mutex_lock(&data->update_lock);
> >  
> >  	next_update = data->last_updated
> > -	  + msecs_to_jiffies(data->update_interval) + 1;
> > +	  + msecs_to_jiffies_min(data->update_interval);
> >  	if (time_after(jiffies, next_update) || !data->valid) {
> >  		u8 h, l;
> >  		u8 alarms;
> 
> I don't like this. The + 1 aren't there because msecs_to_jiffies() may
> return less than the required time, as you claim. I don't think this is
> the case (see the doubts expressed in my reply to patch 1/11.)
> 
> These + 1 are there because the chip may need exactly
> data->update_interval for the data sampling and the datasheet isn't
> completely clear about what would happen if the host polls for results
> at exactly the same frequency the chip is sampling.
> 
> Most likely it is just paranoia from me and the + 1 can be removed.
> Especially when time_after (as opposed to time_after_eq) already adds
> some margin - probably exactly what we need here. I have a few chips
> here I could test this on, BTW (ADM1032 and LM64.)

Yes I realize now that time_after() vs. time_after_eq() guarantees
already that we wait at least data->update_interval. Moreover +1 was
there to wait more than this, so clearly my change here was incorrect.

> And even if this was actually needed, then it is written the wrong way.
> We don't need one more jiffy, the SMBus slave doesn't even know what a
> jiffy is. We need an arbitrary additional amount of time, expressed in
> a standard time unit like ms. So
>   msecs_to_jiffies(data->update_interval + 1)
> would be the right way to write it.

Agreed, this describes the intention better.

> On top of that, your proposed change will make the resulting binary
> larger, the compilation longer,

True. This could be solved by turning the macros into uninlined
functions.

> the future code reviews harder,

Not sure what you mean by this. Having a properly named helper instead
of just writing +1 makes it more obvious what the intention was and
easier to spot places that are doing the wrong thing.

> and backporting these drivers harder.

This would be the case for places where the adjustment is already there
and we would just make this more explicit with the new helper. But for
places we actually fix in the current code backporting the fix would be
a benefit.

A was planning to send a patchset with the actual fixes after there is
some consensus that the approach is viable at all..

> Each of these points only by a very
> small fraction, of course, but for a benefit which is even smaller
> IMHO, if it even exists (which I doubt.)

I think there is a clear benefit.

> So I'm not going to apply this patch, sorry.

Ok, this particular change was done without enough thought and we should
drop it, but I hope you agree we need a generic solution for this
problem.

Thanks a lot for the review and thoughts,
Imre

Re: [PATCH 04/11] hwmon/lm63,lm90: take msecs_to_jiffies_min into use

[Jean Delvare]

On Mon, 13 May 2013 14:56:33 +0300, Imre Deak wrote:
> On Mon, 2013-05-13 at 09:47 +0200, Jean Delvare wrote:
> > Hi Imre,
> > 
> > On Fri, 10 May 2013 15:13:22 +0300, Imre Deak wrote:
> > > Use msecs_to_jiffies_min instead of open-coding the same.
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/hwmon/lm63.c |    2 +-
> > >  drivers/hwmon/lm90.c |    2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> > > index f644a2e..db44bcb 100644
> > > --- a/drivers/hwmon/lm63.c
> > > +++ b/drivers/hwmon/lm63.c
> > > @@ -248,7 +248,7 @@ static struct lm63_data *lm63_update_device(struct device *dev)
> > >  	mutex_lock(&data->update_lock);
> > >  
> > >  	next_update = data->last_updated
> > > -	  + msecs_to_jiffies(data->update_interval) + 1;
> > > +	  + msecs_to_jiffies_min(data->update_interval);
> > >  
> > >  	if (time_after(jiffies, next_update) || !data->valid) {
> > >  		if (data->config & 0x04) { /* tachometer enabled  */
> > > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> > > index 8eeb141..314f9d3 100644
> > > --- a/drivers/hwmon/lm90.c
> > > +++ b/drivers/hwmon/lm90.c
> > > @@ -471,7 +471,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
> > >  	mutex_lock(&data->update_lock);
> > >  
> > >  	next_update = data->last_updated
> > > -	  + msecs_to_jiffies(data->update_interval) + 1;
> > > +	  + msecs_to_jiffies_min(data->update_interval);
> > >  	if (time_after(jiffies, next_update) || !data->valid) {
> > >  		u8 h, l;
> > >  		u8 alarms;
> > 
> > I don't like this. The + 1 aren't there because msecs_to_jiffies() may
> > return less than the required time, as you claim. I don't think this is
> > the case (see the doubts expressed in my reply to patch 1/11.)
> > 
> > These + 1 are there because the chip may need exactly
> > data->update_interval for the data sampling and the datasheet isn't
> > completely clear about what would happen if the host polls for results
> > at exactly the same frequency the chip is sampling.
> > 
> > Most likely it is just paranoia from me and the + 1 can be removed.
> > Especially when time_after (as opposed to time_after_eq) already adds
> > some margin - probably exactly what we need here. I have a few chips
> > here I could test this on, BTW (ADM1032 and LM64.)
> 
> Yes I realize now that time_after() vs. time_after_eq() guarantees
> already that we wait at least data->update_interval. Moreover +1 was
> there to wait more than this, so clearly my change here was incorrect.
> 
> > And even if this was actually needed, then it is written the wrong way.
> > We don't need one more jiffy, the SMBus slave doesn't even know what a
> > jiffy is. We need an arbitrary additional amount of time, expressed in
> > a standard time unit like ms. So
> >   msecs_to_jiffies(data->update_interval + 1)
> > would be the right way to write it.
> 
> Agreed, this describes the intention better.

I'll look into it and send a patch.

> > On top of that, your proposed change will make the resulting binary
> > larger, the compilation longer,
> 
> True. This could be solved by turning the macros into uninlined
> functions.
> 
> > the future code reviews harder,
> 
> Not sure what you mean by this. Having a properly named helper instead
> of just writing +1 makes it more obvious what the intention was and
> easier to spot places that are doing the wrong thing.

I mean that this is one more thing to check when reviewing a new
driver: did the author use msecs_to_jiffies or msecs_to_jiffies_min.
But that wasn't a good point from me, I'll admit, as we already have to
check if the timeout value is set properly.

> > and backporting these drivers harder.
> 
> This would be the case for places where the adjustment is already there
> and we would just make this more explicit with the new helper. But for
> places we actually fix in the current code backporting the fix would be
> a benefit.

My point is that a backported driver can't use a function which was
added in a later kernel version than the backport target. Either the
missing function must be backported as well (but most often this isn't
an option) or some compatibility code must be thrown in. Even though I
agree that backporting isn't a primary concern and should never prevent
good changes from happening upstream, I think it is a gentle reminder
that changes should only be done when we're sure they are worth the
effort. I am maintaining two dozens of standalone hwmon drivers to be
used with older kernels, and the less work I have to do on these, the
more time I can devote to more interesting tasks.

> (...)
> Thanks a lot for the review and thoughts,

You're welcome.

-- 
Jean Delvare

[PATCH 05/11] media/si4713-i2c: take usecs_to_jiffies_min into use

[Imre Deak]

Use usecs_to_jiffies_min instead of open-coding the same.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/media/radio/si4713-i2c.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/radio/si4713-i2c.c b/drivers/media/radio/si4713-i2c.c
index bd61b3b..09eb6b9 100644
--- a/drivers/media/radio/si4713-i2c.c
+++ b/drivers/media/radio/si4713-i2c.c
@@ -252,7 +252,7 @@ static int si4713_send_command(struct si4713_device *sdev, const u8 command,
 
 	/* Wait response from interrupt */
 	if (!wait_for_completion_timeout(&sdev->work,
-				usecs_to_jiffies(usecs) + 1))
+				usecs_to_jiffies_min(usecs)))
 		v4l2_warn(&sdev->sd,
 				"(%s) Device took too much time to answer.\n",
 				__func__);
@@ -494,7 +494,7 @@ static int si4713_wait_stc(struct si4713_device *sdev, const int usecs)
 
 	/* Wait response from STC interrupt */
 	if (!wait_for_completion_timeout(&sdev->work,
-			usecs_to_jiffies(usecs) + 1))
+			usecs_to_jiffies_min(usecs)))
 		v4l2_warn(&sdev->sd,
 			"%s: device took too much time to answer (%d usec).\n",
 				__func__, usecs);
-- 
1.7.10.4

[PATCH 06/11] net/bonding: take msecs_to_jiffies_min into use

[Imre Deak]

Use msecs_to_jiffies_min instead of open-coding the same.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/net/bonding/bond_main.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 07401a3..0c2de73 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1751,7 +1751,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 	read_lock(&bond->lock);
 
 	new_slave->last_arp_rx = jiffies -
-		(msecs_to_jiffies(bond->params.arp_interval) + 1);
+		(msecs_to_jiffies_min(bond->params.arp_interval));
 
 	if (bond->params.miimon && !bond->params.use_carrier) {
 		link_reporting = bond_check_dev_link(bond, slave_dev, 1);
-- 
1.7.10.4

Re: [PATCH 06/11] net/bonding: take msecs_to_jiffies_min into use

[Michal Kubecek]

On Fri, May 10, 2013 at 03:13:24PM +0300, Imre Deak wrote:
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1751,7 +1751,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>  	read_lock(&bond->lock);
>  
>  	new_slave->last_arp_rx = jiffies -
> -		(msecs_to_jiffies(bond->params.arp_interval) + 1);
> +		(msecs_to_jiffies_min(bond->params.arp_interval));
>  
>  	if (bond->params.miimon && !bond->params.use_carrier) {
>  		link_reporting = bond_check_dev_link(bond, slave_dev, 1);

This "+ 1" was actually meant as "plus one". We need to ensure that

  slave->last_arp_rx + msecs_to_jiffies(bond->params.arp_interval)

is strictly less than current value of jiffies. So with proposed
definition of msecs_to_jiffies_min() it works correctly but if the
implementation ever changes in such way that

  msecs_to_jiffies_min(x) >= msecs_to_jiffies(x)

for some value of x, the code would be incorrect.

                                                       Michal Kubeček

Re: [PATCH 06/11] net/bonding: take msecs_to_jiffies_min into use

[Imre Deak]

On Fri, 2013-05-10 at 15:58 +0200, Michal Kubecek wrote:
> On Fri, May 10, 2013 at 03:13:24PM +0300, Imre Deak wrote:
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -1751,7 +1751,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
> >  	read_lock(&bond->lock);
> >  
> >  	new_slave->last_arp_rx = jiffies -
> > -		(msecs_to_jiffies(bond->params.arp_interval) + 1);
> > +		(msecs_to_jiffies_min(bond->params.arp_interval));
> >  
> >  	if (bond->params.miimon && !bond->params.use_carrier) {
> >  		link_reporting = bond_check_dev_link(bond, slave_dev, 1);
> 
> This "+ 1" was actually meant as "plus one". We need to ensure that
> 
>   slave->last_arp_rx + msecs_to_jiffies(bond->params.arp_interval)
> 
> is strictly less than current value of jiffies.

Ok, I see, the adjustment here is for a different reason and
msecs_to_jiffies_min wouldn't express this properly. So we should drop
this patch. Perhaps it'd be nice to add something like the above
explanation as a code comment, to make it clear that the adjustment is
not for guaranteeing a minimum duration as it is in many other places.

--Imre

> So with proposed
> definition of msecs_to_jiffies_min() it works correctly but if the
> implementation ever changes in such way that
> 
>   msecs_to_jiffies_min(x) >= msecs_to_jiffies(x)
> 
> for some value of x, the code would be incorrect.
> 
>                                                        Michal Kubeček
> 

[PATCH 07/11] net/peak_pcmcia: take msecs_to_jiffies_min into use

[Imre Deak]

Use msecs_to_jiffies_min instead of open-coding the same.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/net/can/sja1000/peak_pcmcia.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/sja1000/peak_pcmcia.c b/drivers/net/can/sja1000/peak_pcmcia.c
index 1a7020b..3e45e16 100644
--- a/drivers/net/can/sja1000/peak_pcmcia.c
+++ b/drivers/net/can/sja1000/peak_pcmcia.c
@@ -253,7 +253,7 @@ static inline int pcan_pccard_present(struct pcan_pccard *card)
 static int pcan_wait_spi_busy(struct pcan_pccard *card)
 {
 	unsigned long timeout = jiffies +
-				msecs_to_jiffies(PCC_SPI_MAX_BUSY_WAIT_MS) + 1;
+				msecs_to_jiffies_min(PCC_SPI_MAX_BUSY_WAIT_MS);
 
 	/* be sure to read status at least once after sleeping */
 	while (pcan_read_reg(card, PCC_CSR) & PCC_CSR_SPI_BUSY) {
-- 
1.7.10.4

Re: [PATCH 07/11] net/peak_pcmcia: take msecs_to_jiffies_min into use

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 484 bytes --]

On 05/10/2013 02:13 PM, Imre Deak wrote:
> Use msecs_to_jiffies_min instead of open-coding the same.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Tnx, applied to linux-can-next/master.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 07/11] net/peak_pcmcia: take msecs_to_jiffies_min into use

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 726 bytes --]

On 05/15/2013 11:12 AM, Marc Kleine-Budde wrote:
> On 05/10/2013 02:13 PM, Imre Deak wrote:
>> Use msecs_to_jiffies_min instead of open-coding the same.
>>
>> Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> Tnx, applied to linux-can-next/master.

Removed, as requested by Imre:

> The reason is that in that driver I wasn't sure about why +1 was
> added, since the time_after() check already guarantees a minimum wait
> time.

Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

[PATCH 08/11] usb/isp116x-hcd: take msecs_to_jiffies_min into use

[Imre Deak]

Use msecs_to_jiffies_min instead of open-coding the same.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/usb/host/isp116x-hcd.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/isp116x-hcd.c b/drivers/usb/host/isp116x-hcd.c
index b64e661..6cc4660 100644
--- a/drivers/usb/host/isp116x-hcd.c
+++ b/drivers/usb/host/isp116x-hcd.c
@@ -620,7 +620,7 @@ static irqreturn_t isp116x_irq(struct usb_hcd *hcd)
 			   to come out of suspend, it may take more
 			   than 10ms for status bits to stabilize. */
 			mod_timer(&hcd->rh_timer, jiffies
-				  + msecs_to_jiffies(20) + 1);
+				  + msecs_to_jiffies_min(20));
 		if (intstat & HCINT_RD) {
 			DBG("---- remote wakeup\n");
 			usb_hcd_resume_root_hub(hcd);
-- 
1.7.10.4

[PATCH 09/11] net/sunrpc: take msecs_to_jiffies_min into use

[Imre Deak]

Use msecs_to_jiffies_min instead of open-coding the same.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 net/sunrpc/xprtrdma/verbs.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c
index 93726560..9cac2c8 100644
--- a/net/sunrpc/xprtrdma/verbs.c
+++ b/net/sunrpc/xprtrdma/verbs.c
@@ -404,7 +404,7 @@ rpcrdma_create_id(struct rpcrdma_xprt *xprt,
 		goto out;
 	}
 	wait_for_completion_interruptible_timeout(&ia->ri_done,
-				msecs_to_jiffies(RDMA_RESOLVE_TIMEOUT) + 1);
+				msecs_to_jiffies_min(RDMA_RESOLVE_TIMEOUT));
 	rc = ia->ri_async_rc;
 	if (rc)
 		goto out;
@@ -417,7 +417,7 @@ rpcrdma_create_id(struct rpcrdma_xprt *xprt,
 		goto out;
 	}
 	wait_for_completion_interruptible_timeout(&ia->ri_done,
-				msecs_to_jiffies(RDMA_RESOLVE_TIMEOUT) + 1);
+				msecs_to_jiffies_min(RDMA_RESOLVE_TIMEOUT));
 	rc = ia->ri_async_rc;
 	if (rc)
 		goto out;
-- 
1.7.10.4

[PATCH 10/11] net/tipc: take msecs_to_jiffies_min into use

[Imre Deak]

Use msecs_to_jiffies_min instead of open-coding the same.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 net/tipc/core.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tipc/core.h b/net/tipc/core.h
index 0207db0..3f23e22 100644
--- a/net/tipc/core.h
+++ b/net/tipc/core.h
@@ -133,7 +133,7 @@ static inline void k_init_timer(struct timer_list *timer, Handler routine,
  */
 static inline void k_start_timer(struct timer_list *timer, unsigned long msec)
 {
-	mod_timer(timer, jiffies + msecs_to_jiffies(msec) + 1);
+	mod_timer(timer, jiffies + msecs_to_jiffies_min(msec));
 }
 
 /**
-- 
1.7.10.4

[PATCH 11/11] sound/oxygen_io: take msecs_to_jiffies_min into use

[Imre Deak]

Use msecs_to_jiffies_min instead of open-coding the same.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 sound/pci/oxygen/oxygen_io.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/pci/oxygen/oxygen_io.c b/sound/pci/oxygen/oxygen_io.c
index 521eae4..132ecbe 100644
--- a/sound/pci/oxygen/oxygen_io.c
+++ b/sound/pci/oxygen/oxygen_io.c
@@ -108,7 +108,7 @@ static int oxygen_ac97_wait(struct oxygen *chip, unsigned int mask)
 	wait_event_timeout(chip->ac97_waitqueue,
 			   ({ status |= oxygen_read8(chip, OXYGEN_AC97_INTERRUPT_STATUS);
 			      status & mask; }),
-			   msecs_to_jiffies(1) + 1);
+			   msecs_to_jiffies_min(1));
 	/*
 	 * Check even after a timeout because this function should not require
 	 * the AC'97 interrupt to be enabled.
-- 
1.7.10.4

Re: [PATCH 11/11] sound/oxygen_io: take msecs_to_jiffies_min into use

[Takashi Iwai]

At Fri, 10 May 2013 15:13:29 +0300,
Imre Deak wrote:
> 
> Use msecs_to_jiffies_min instead of open-coding the same.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  sound/pci/oxygen/oxygen_io.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/pci/oxygen/oxygen_io.c b/sound/pci/oxygen/oxygen_io.c
> index 521eae4..132ecbe 100644
> --- a/sound/pci/oxygen/oxygen_io.c
> +++ b/sound/pci/oxygen/oxygen_io.c
> @@ -108,7 +108,7 @@ static int oxygen_ac97_wait(struct oxygen *chip, unsigned int mask)
>  	wait_event_timeout(chip->ac97_waitqueue,
>  			   ({ status |= oxygen_read8(chip, OXYGEN_AC97_INTERRUPT_STATUS);
>  			      status & mask; }),
> -			   msecs_to_jiffies(1) + 1);
> +			   msecs_to_jiffies_min(1));

This would change the behavior, I guess.  (Though, I'm not sure
whether the original code was intentional.)

And, isn't msecs_to_jiffies_min(1) identical with msecs_to_jiffies(1)?


thanks,

Takashi

Re: [PATCH 11/11] sound/oxygen_io: take msecs_to_jiffies_min into use

[Imre Deak]

On Mon, 2013-05-13 at 16:00 +0200, Takashi Iwai wrote:
> At Fri, 10 May 2013 15:13:29 +0300,
> Imre Deak wrote:
> > 
> > Use msecs_to_jiffies_min instead of open-coding the same.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  sound/pci/oxygen/oxygen_io.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/sound/pci/oxygen/oxygen_io.c b/sound/pci/oxygen/oxygen_io.c
> > index 521eae4..132ecbe 100644
> > --- a/sound/pci/oxygen/oxygen_io.c
> > +++ b/sound/pci/oxygen/oxygen_io.c
> > @@ -108,7 +108,7 @@ static int oxygen_ac97_wait(struct oxygen *chip, unsigned int mask)
> >  	wait_event_timeout(chip->ac97_waitqueue,
> >  			   ({ status |= oxygen_read8(chip, OXYGEN_AC97_INTERRUPT_STATUS);
> >  			      status & mask; }),
> > -			   msecs_to_jiffies(1) + 1);
> > +			   msecs_to_jiffies_min(1));
> 
> This would change the behavior, I guess.  

Not to my understanding, the new macro should end up doing the same
thing.

> (Though, I'm not sure whether the original code was intentional.)

Well, I only assumed that.. But using wait_event_timeout() without the
+1 would make little sense to me. In that case we may not wait at all
for the condition to become true, if we are close to the next scheduling
clock tick.

> And, isn't msecs_to_jiffies_min(1) identical with msecs_to_jiffies(1)?

No, it should be one more in value.

--Imre

Re: [PATCH 11/11] sound/oxygen_io: take msecs_to_jiffies_min into use

[Takashi Iwai]

At Mon, 13 May 2013 17:24:38 +0300,
Imre Deak wrote:
> 
> On Mon, 2013-05-13 at 16:00 +0200, Takashi Iwai wrote:
> > At Fri, 10 May 2013 15:13:29 +0300,
> > Imre Deak wrote:
> > > 
> > > Use msecs_to_jiffies_min instead of open-coding the same.
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  sound/pci/oxygen/oxygen_io.c |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/sound/pci/oxygen/oxygen_io.c b/sound/pci/oxygen/oxygen_io.c
> > > index 521eae4..132ecbe 100644
> > > --- a/sound/pci/oxygen/oxygen_io.c
> > > +++ b/sound/pci/oxygen/oxygen_io.c
> > > @@ -108,7 +108,7 @@ static int oxygen_ac97_wait(struct oxygen *chip, unsigned int mask)
> > >  	wait_event_timeout(chip->ac97_waitqueue,
> > >  			   ({ status |= oxygen_read8(chip, OXYGEN_AC97_INTERRUPT_STATUS);
> > >  			      status & mask; }),
> > > -			   msecs_to_jiffies(1) + 1);
> > > +			   msecs_to_jiffies_min(1));
> > 
> > This would change the behavior, I guess.  
> 
> Not to my understanding, the new macro should end up doing the same
> thing.

Ah, OK, I just saw your patch 01/11.

But then msecs_to_jiffies_min() sounds confusing, if it plus one
implicitly.


Takashi

> > (Though, I'm not sure whether the original code was intentional.)
> 
> Well, I only assumed that.. But using wait_event_timeout() without the
> +1 would make little sense to me. In that case we may not wait at all
> for the condition to become true, if we are close to the next scheduling
> clock tick.
> 
> > And, isn't msecs_to_jiffies_min(1) identical with msecs_to_jiffies(1)?
> 
> No, it should be one more in value.

Re: [PATCH 11/11] sound/oxygen_io: take msecs_to_jiffies_min into use

[Clemens Ladisch]

Takashi Iwai wrote:
> Imre Deak wrote:
>> Use msecs_to_jiffies_min instead of open-coding the same.
>>
>> +++ b/sound/pci/oxygen/oxygen_io.c
>> @@ -108,7 +108,7 @@ static int oxygen_ac97_wait(struct oxygen *chip, unsigned int mask)
>>  	wait_event_timeout(chip->ac97_waitqueue,
>>  			   ({ status |= oxygen_read8(chip, OXYGEN_AC97_INTERRUPT_STATUS);
>>  			      status & mask; }),
>> -			   msecs_to_jiffies(1) + 1);
>> +			   msecs_to_jiffies_min(1));
>
> This would change the behavior, I guess.

No, that's exactly how msecs_to_jiffies_min would be implemented.

> (Though, I'm not sure whether the original code was intentional.)

When I wrote this, I was not able to prove that msecs_to_jiffies always
rounds up.  (And I guess it doesn't; otherwise the _min variant would
not be needed.)


Regards,
Clemens

Re: [PATCH 01/11] time: add *_to_jiffies_min helpers to guarantee a minimum duration

[Daniel Vetter]

On Fri, May 10, 2013 at 2:13 PM, Imre Deak <imre.deak@intel.com> wrote:
> The *_to_jiffies(x) macros return a jiffy value, which if used as a
> delta to wait for a specific amount of time, may result in a wait-time
> that is less than x. Many callers already compensate for this by adding
> one to the returned value. To document why we need to add one and to get
> rid of some code duplication add a helper that does the same.
>
> Later patches will convert the currently open-coded call sites to use
> the new helpers.
>
> Also this can serve as a basis for auditing those users of *_to_jiffies
> that most likely do the wrong thing - for example set a timeout value of
> msecs_to_jiffies(1) - and converting them to use the new helpers.
>
> Kudos to Daniel Vetter for the idea of the new helpers.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  include/linux/jiffies.h |   13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/include/linux/jiffies.h b/include/linux/jiffies.h
> index 82ed068..d12509b 100644
> --- a/include/linux/jiffies.h
> +++ b/include/linux/jiffies.h
> @@ -312,6 +312,19 @@ extern u64 nsec_to_clock_t(u64 x);
>  extern u64 nsecs_to_jiffies64(u64 n);
>  extern unsigned long nsecs_to_jiffies(u64 n);

Some piece of kerneldoc is missing here ... Also I'm a bit confused
about the _min suffix, I kinda expect something called _min to be less
than the normal one thing. A few ideas:
- to_jiffy_timeout (since pretty much all of the values computed like
this will end up in schedule_timeout eventually),
- to_jiffies_relative (iirc relative is already established in other parts)
- ...

In any case I'd like to have this + the i915 patch go into 3.10 cc:
stable if possible, since it fixes bugs (the i915 patch is missing
bugzilla links, too).

Cheers, Daniel

> +#define __define_time_to_jiffies_min(tname, ttype)                     \
> +static inline unsigned long                                            \
> +tname ## _to_jiffies_min(const ttype m)                                        \
> +{                                                                      \
> +       return max_t(long, MAX_JIFFY_OFFSET, tname ## _to_jiffies(m) + 1);\
> +}
> +
> +__define_time_to_jiffies_min(msecs, unsigned int)
> +__define_time_to_jiffies_min(usecs, unsigned int)
> +__define_time_to_jiffies_min(nsecs, u64)
> +__define_time_to_jiffies_min(timespec, struct timespec *)
> +__define_time_to_jiffies_min(timeval, struct timeval *)
> +
>  #define TIMESTAMP_SIZE 30
>
>  #endif
> --
> 1.7.10.4
>



--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 01/11] time: add *_to_jiffies_min helpers to guarantee a minimum duration

[Imre Deak]

On Fri, 2013-05-10 at 14:24 +0200, Daniel Vetter wrote:
> On Fri, May 10, 2013 at 2:13 PM, Imre Deak <imre.deak@intel.com> wrote:
> > The *_to_jiffies(x) macros return a jiffy value, which if used as a
> > delta to wait for a specific amount of time, may result in a wait-time
> > that is less than x. Many callers already compensate for this by adding
> > one to the returned value. To document why we need to add one and to get
> > rid of some code duplication add a helper that does the same.
> >
> > Later patches will convert the currently open-coded call sites to use
> > the new helpers.
> >
> > Also this can serve as a basis for auditing those users of *_to_jiffies
> > that most likely do the wrong thing - for example set a timeout value of
> > msecs_to_jiffies(1) - and converting them to use the new helpers.
> >
> > Kudos to Daniel Vetter for the idea of the new helpers.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  include/linux/jiffies.h |   13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/include/linux/jiffies.h b/include/linux/jiffies.h
> > index 82ed068..d12509b 100644
> > --- a/include/linux/jiffies.h
> > +++ b/include/linux/jiffies.h
> > @@ -312,6 +312,19 @@ extern u64 nsec_to_clock_t(u64 x);
> >  extern u64 nsecs_to_jiffies64(u64 n);
> >  extern unsigned long nsecs_to_jiffies(u64 n);
> 
> Some piece of kerneldoc is missing here ...

True, I'll add it.

>  Also I'm a bit confused
> about the _min suffix, I kinda expect something called _min to be less
> than the normal one thing. A few ideas:
> - to_jiffy_timeout (since pretty much all of the values computed like
> this will end up in schedule_timeout eventually),
> - to_jiffies_relative (iirc relative is already established in other parts)
> - ...

_min as in minimum guaranteed. As for _timeout, the returned value can
be used in other ways too for example to directly compare against
jiffies. I don't have a strong preference, so I can use _timeout
instead.

> In any case I'd like to have this + the i915 patch go into 3.10 cc:
> stable if possible, since it fixes bugs
> (the i915 patch is missing bugzilla links, too).

Ok, I will CC stable. I planned to post the patchset with the actual
fixes as a follow-up, so haven't included any bugzilla links here.

> Cheers, Daniel
> 
> > +#define __define_time_to_jiffies_min(tname, ttype)                     \
> > +static inline unsigned long                                            \
> > +tname ## _to_jiffies_min(const ttype m)                                        \
> > +{                                                                      \
> > +       return max_t(long, MAX_JIFFY_OFFSET, tname ## _to_jiffies(m) + 1);\

Here I meant min_t :/

--Imre

> > +}
> > +
> > +__define_time_to_jiffies_min(msecs, unsigned int)
> > +__define_time_to_jiffies_min(usecs, unsigned int)
> > +__define_time_to_jiffies_min(nsecs, u64)
> > +__define_time_to_jiffies_min(timespec, struct timespec *)
> > +__define_time_to_jiffies_min(timeval, struct timeval *)
> > +
> >  #define TIMESTAMP_SIZE 30
> >
> >  #endif
> > --
> > 1.7.10.4

Re: [PATCH 01/11] time: add *_to_jiffies_min helpers to guarantee a minimum duration

[Jean Delvare]

Hi Imre,

On Fri, 10 May 2013 15:13:19 +0300, Imre Deak wrote:
> The *_to_jiffies(x) macros return a jiffy value, which if used as a
> delta to wait for a specific amount of time, may result in a wait-time
> that is less than x.

Are you sure? I have always considered that *_to_jiffies(x) macros
rounded up, and reading the code seems to confirm that:

	/*
	 * Generic case - multiply, round and divide. (...)
	 */
	(...)
	return (MSEC_TO_HZ_MUL32 * m + MSEC_TO_HZ_ADJ32)
		>> MSEC_TO_HZ_SHR32;

What makes you think the resulting wait time can be less that requested?

If this really is the case then the proper way to address the issue is
to fix the original macros, not introducing new ones.

> Many callers already compensate for this by adding
> one to the returned value.

This is an assumption from you, and I am afraid it is wrong in many
cases. I see that Michal Kubecek already pointed out one case where it
was indeed wrong, and I am about to make a similar reply to another
post of yours.

> To document why we need to add one and to get
> rid of some code duplication add a helper that does the same.

I'm sorry but you can't call "+ 1" code duplication.

> Later patches will convert the currently open-coded call sites to use
> the new helpers.
> 
> Also this can serve as a basis for auditing those users of *_to_jiffies
> that most likely do the wrong thing - for example set a timeout value of
> msecs_to_jiffies(1) - and converting them to use the new helpers.

You should be very, very careful before claiming that the code is wrong
and you're fixing it. It might as well be that the code is right but
you did not understand it, and you're actually breaking it. Or the code
was already wrong and you're making it worse ;)

As a summary, I don't like the idea, sorry.

-- 
Jean Delvare

Re: [PATCH 01/11] time: add *_to_jiffies_min helpers to guarantee a minimum duration

[Imre Deak]

On Mon, 2013-05-13 at 09:29 +0200, Jean Delvare wrote:
> Hi Imre,
> 
> On Fri, 10 May 2013 15:13:19 +0300, Imre Deak wrote:
> > The *_to_jiffies(x) macros return a jiffy value, which if used as a
> > delta to wait for a specific amount of time, may result in a wait-time
> > that is less than x.
> 
> Are you sure? I have always considered that *_to_jiffies(x) macros
> rounded up, and reading the code seems to confirm that:
> 
> 	/*
> 	 * Generic case - multiply, round and divide. (...)
> 	 */
> 	(...)
> 	return (MSEC_TO_HZ_MUL32 * m + MSEC_TO_HZ_ADJ32)
> 		>> MSEC_TO_HZ_SHR32;
> 
> What makes you think the resulting wait time can be less that requested?

Yes the above does a round-up, but for another reason. It makes only
sure you won't wait less than the requested time because you have a too
coarse HZ value. So for example with HZ=1000 it won't do any adjustment,
but with HZ=100 it'll round up durations not dividable by 10 msec.

What the proposed change wants to solve is how - or rather what point in
time - the returned value is used. For example in the following loop to
wait for some condition to become true:

timeout = msecs_to_jiffies(1);
while (!condition && timeout) {
	prepare_to_wait(wq, ...);
	timeout = schedule_timeout(timeout);
}

it would seem we'll wait at least 1 msec for the condition to become
true. In fact with HZ=1000 and an initial timeout value of 1 we may wait
less, since schedule_timeout() will return with 0 already at the next
scheduling clock tick which is most probably less than 1 msec ahead in
time.

> If this really is the case then the proper way to address the issue is
> to fix the original macros, not introducing new ones.

I'm not sure if we need the adjustment in all cases. For example in the
following polling loop we'd like to wake up every msec (to check for
something not signaled through the wq) and time out after 50 iterations:

for (i = 0; i < 50; i++) {
	prepare_to_wait(wq, ...);
	if (condition)
		break;
	schedule_timeout(msecs_to_jiffies(1));
}

Having the +1 adjustment in msecs_to_jiffies() would result in waking up
close to every 2 msec.

> > Many callers already compensate for this by adding
> > one to the returned value.
> 
> This is an assumption from you, and I am afraid it is wrong in many
> cases. I see that Michal Kubecek already pointed out one case where it
> was indeed wrong, and I am about to make a similar reply to another
> post of yours.

Agreed, I was wrong in those cases and we shouldn't change them. There
are places on the other hand where the adjustment is made to guarantee a
minimum waiting time and thus better expressed with a properly named
helper.

> > To document why we need to add one and to get
> > rid of some code duplication add a helper that does the same.
> 
> I'm sorry but you can't call "+ 1" code duplication.

You're right, the change doesn't reduce code duplication.. It only
documents why the +1 is needed, which is very much called for imo.

> > Later patches will convert the currently open-coded call sites to use
> > the new helpers.
> > 
> > Also this can serve as a basis for auditing those users of *_to_jiffies
> > that most likely do the wrong thing - for example set a timeout value of
> > msecs_to_jiffies(1) - and converting them to use the new helpers.
> 
> You should be very, very careful before claiming that the code is wrong
> and you're fixing it. It might as well be that the code is right but
> you did not understand it, and you're actually breaking it. Or the code
> was already wrong and you're making it worse ;)

Agreed and I should've been more careful with the converting patches.
But the fact is that there is currently code out there that uses
msecs_to_jiffies() incorrectly. Fixing these and making it less likely
for similar problems to appear in the future is worth the trouble
though.

--Imre

> As a summary, I don't like the idea, sorry.

Re: [PATCH 01/11] time: add *_to_jiffies_min helpers to guarantee a minimum duration

[Jean Delvare]

Hi Imre,

On Mon, 13 May 2013 14:27:28 +0300, Imre Deak wrote:
> On Mon, 2013-05-13 at 09:29 +0200, Jean Delvare wrote:
> > Hi Imre,
> > 
> > On Fri, 10 May 2013 15:13:19 +0300, Imre Deak wrote:
> > > The *_to_jiffies(x) macros return a jiffy value, which if used as a
> > > delta to wait for a specific amount of time, may result in a wait-time
> > > that is less than x.
> > 
> > Are you sure? I have always considered that *_to_jiffies(x) macros
> > rounded up, and reading the code seems to confirm that:
> > 
> > 	/*
> > 	 * Generic case - multiply, round and divide. (...)
> > 	 */
> > 	(...)
> > 	return (MSEC_TO_HZ_MUL32 * m + MSEC_TO_HZ_ADJ32)
> > 		>> MSEC_TO_HZ_SHR32;
> > 
> > What makes you think the resulting wait time can be less that requested?
> 
> Yes the above does a round-up, but for another reason. It makes only
> sure you won't wait less than the requested time because you have a too
> coarse HZ value. So for example with HZ=1000 it won't do any adjustment,
> but with HZ=100 it'll round up durations not dividable by 10 msec.

For HZ=1000 the code above is never reached, the code which is executed
instead is:

	/*
	 * HZ is equal to or smaller than 1000, and 1000 is a nice
	 * round multiple of HZ, divide with the factor between them,
	 * but round upwards:
	 */
	return (m + (MSEC_PER_SEC / HZ) - 1) / (MSEC_PER_SEC / HZ);

which simplifies to just:

	return m;

So indeed no round up of any kind. Thanks for the clarification.

> What the proposed change wants to solve is how - or rather what point in
> time - the returned value is used. For example in the following loop to
> wait for some condition to become true:
> 
> timeout = msecs_to_jiffies(1);
> while (!condition && timeout) {
> 	prepare_to_wait(wq, ...);
> 	timeout = schedule_timeout(timeout);
> }
> 
> it would seem we'll wait at least 1 msec for the condition to become
> true. In fact with HZ=1000 and an initial timeout value of 1 we may wait
> less, since schedule_timeout() will return with 0 already at the next
> scheduling clock tick which is most probably less than 1 msec ahead in
> time.

OK, I see your point now.

But maybe your example code is not good in the first place. I don't
think you should use schedule_timeout() for such a small wait time.
Aren't you supposed to use HR timers instead?

> > If this really is the case then the proper way to address the issue is
> > to fix the original macros, not introducing new ones.
> 
> I'm not sure if we need the adjustment in all cases. For example in the
> following polling loop we'd like to wake up every msec (to check for
> something not signaled through the wq) and time out after 50 iterations:
> 
> for (i = 0; i < 50; i++) {
> 	prepare_to_wait(wq, ...);
> 	if (condition)
> 		break;
> 	schedule_timeout(msecs_to_jiffies(1));
> }
> 
> Having the +1 adjustment in msecs_to_jiffies() would result in waking up
> close to every 2 msec.

To be honest I thought it was already the case, but I was wrong. What
confused me is that I mostly work on hwmon drivers and the typical use
case of msecs_to_jiffies() in these drivers is in conjunction with
time_after(). It's time_after() which does "round up", in that it
always completes the current jiffy before it starts counting.

So there may be a need for what you're doing, just not in the drivers
I'm taking care of. So I'll keep quiet about it from now on ;)

-- 
Jean Delvare

Re: [PATCH 01/11] time: add *_to_jiffies_min helpers to guarantee a minimum duration

[Imre Deak]

On Mon, 2013-05-13 at 14:28 +0200, Jean Delvare wrote:
> Hi Imre,
> 
> On Mon, 13 May 2013 14:27:28 +0300, Imre Deak wrote:
> > On Mon, 2013-05-13 at 09:29 +0200, Jean Delvare wrote:
> > > Hi Imre,
> > > 
> > > On Fri, 10 May 2013 15:13:19 +0300, Imre Deak wrote:
> > > > The *_to_jiffies(x) macros return a jiffy value, which if used as a
> > > > delta to wait for a specific amount of time, may result in a wait-time
> > > > that is less than x.
> > > 
> > > Are you sure? I have always considered that *_to_jiffies(x) macros
> > > rounded up, and reading the code seems to confirm that:
> > > 
> > > 	/*
> > > 	 * Generic case - multiply, round and divide. (...)
> > > 	 */
> > > 	(...)
> > > 	return (MSEC_TO_HZ_MUL32 * m + MSEC_TO_HZ_ADJ32)
> > > 		>> MSEC_TO_HZ_SHR32;
> > > 
> > > What makes you think the resulting wait time can be less that requested?
> > 
> > Yes the above does a round-up, but for another reason. It makes only
> > sure you won't wait less than the requested time because you have a too
> > coarse HZ value. So for example with HZ=1000 it won't do any adjustment,
> > but with HZ=100 it'll round up durations not dividable by 10 msec.
> 
> For HZ=1000 the code above is never reached, the code which is executed
> instead is:
> 
> 	/*
> 	 * HZ is equal to or smaller than 1000, and 1000 is a nice
> 	 * round multiple of HZ, divide with the factor between them,
> 	 * but round upwards:
> 	 */
> 	return (m + (MSEC_PER_SEC / HZ) - 1) / (MSEC_PER_SEC / HZ);
> 
> which simplifies to just:
> 
> 	return m;
> 
> So indeed no round up of any kind. Thanks for the clarification.
> 
> > What the proposed change wants to solve is how - or rather what point in
> > time - the returned value is used. For example in the following loop to
> > wait for some condition to become true:
> > 
> > timeout = msecs_to_jiffies(1);
> > while (!condition && timeout) {
> > 	prepare_to_wait(wq, ...);
> > 	timeout = schedule_timeout(timeout);
> > }
> > 
> > it would seem we'll wait at least 1 msec for the condition to become
> > true. In fact with HZ=1000 and an initial timeout value of 1 we may wait
> > less, since schedule_timeout() will return with 0 already at the next
> > scheduling clock tick which is most probably less than 1 msec ahead in
> > time.
> 
> OK, I see your point now.
> 
> But maybe your example code is not good in the first place. I don't
> think you should use schedule_timeout() for such a small wait time.
> Aren't you supposed to use HR timers instead?

The problem would be still there even with a longer wait time.
msecs_to_jiffies(n) above only guarantees n-1 msecs minimum wait time.
This kind of loop - and the wait_for_event_timeout() family of functions
where it is embedded - care only about a lower bound to the wait time,
and since HR timers are costlier they would only add unneeded overhead
here.

> > > If this really is the case then the proper way to address the issue is
> > > to fix the original macros, not introducing new ones.
> > 
> > I'm not sure if we need the adjustment in all cases. For example in the
> > following polling loop we'd like to wake up every msec (to check for
> > something not signaled through the wq) and time out after 50 iterations:
> > 
> > for (i = 0; i < 50; i++) {
> > 	prepare_to_wait(wq, ...);
> > 	if (condition)
> > 		break;
> > 	schedule_timeout(msecs_to_jiffies(1));
> > }
> > 
> > Having the +1 adjustment in msecs_to_jiffies() would result in waking up
> > close to every 2 msec.
> 
> To be honest I thought it was already the case, but I was wrong. What
> confused me is that I mostly work on hwmon drivers and the typical use
> case of msecs_to_jiffies() in these drivers is in conjunction with
> time_after(). It's time_after() which does "round up", in that it
> always completes the current jiffy before it starts counting.

Right. It's good that you raised this point, it wasn't clear for me
either.

--Imre

> So there may be a need for what you're doing, just not in the drivers
> I'm taking care of. So I'll keep quiet about it from now on ;)

Re: [PATCH 01/11] time: add *_to_jiffies_min helpers to guarantee a minimum duration

[Jean Delvare]

BTW...

On Fri, 10 May 2013 15:13:19 +0300, Imre Deak wrote:
> +#define __define_time_to_jiffies_min(tname, ttype)			\
> +static inline unsigned long						\
> +tname ## _to_jiffies_min(const ttype m)					\
> +{									\
> +	return max_t(long, MAX_JIFFY_OFFSET, tname ## _to_jiffies(m) + 1);\
> +}

How would this work at all? Isn't it going to just return
MAX_JIFFY_OFFSET all the time?

-- 
Jean Delvare

Re: [PATCH 01/11] time: add *_to_jiffies_min helpers to guarantee a minimum duration

[Imre Deak]

On Mon, 2013-05-13 at 10:17 +0200, Jean Delvare wrote:
> BTW...
> 
> On Fri, 10 May 2013 15:13:19 +0300, Imre Deak wrote:
> > +#define __define_time_to_jiffies_min(tname, ttype)			\
> > +static inline unsigned long						\
> > +tname ## _to_jiffies_min(const ttype m)					\
> > +{									\
> > +	return max_t(long, MAX_JIFFY_OFFSET, tname ## _to_jiffies(m) + 1);\
> > +}
> 
> How would this work at all? Isn't it going to just return
> MAX_JIFFY_OFFSET all the time?

Yes, that's a mistake, I meant here min_t.

--Imre

[PATCH v2 0/8] add *_to_jiffies_timeout helpers to guarantee a minimum duration

[Imre Deak]

For a motivation of this pachset see the commit log of patch 1/8.

v2:
- s/max_t/min_t/ when clamping to MAX_JIFFY_OFFSET
- change the _min suffix to _timeout (Daniel Vetter)
- add kerneldoc (Daniel Vetter)
- turn the macros into uninlined functions
- don't convert msecs_to_jiffies() users where the intention of +1 was
  something else (Jean Delvare, Michal Kubecek)

Imre Deak (8):
  time: add *_to_jiffies_timeout helpers to guarantee a minimum
    duration
  sched: msleep: take msecs_to_jiffies_timeout into use
  drm/i915: take msecs_to_jiffies_timeout into use
  media/si4713-i2c: take usecs_to_jiffies_timeout into use
  usb/isp116x-hcd: take msecs_to_jiffies_timeout into use
  net/sunrpc: take msecs_to_jiffies_timeout into use
  net/tipc: take msecs_to_jiffies_timeout into use
  sound/oxygen_io: take msecs_to_jiffies_timeout into use

 drivers/gpu/drm/i915/intel_drv.h |    2 +-
 drivers/gpu/drm/i915/intel_i2c.c |    2 +-
 drivers/media/radio/si4713-i2c.c |    4 ++--
 drivers/usb/host/isp116x-hcd.c   |    2 +-
 include/linux/jiffies.h          |    6 ++++++
 kernel/time.c                    |   27 +++++++++++++++++++++++++++
 kernel/timer.c                   |    4 ++--
 net/sunrpc/xprtrdma/verbs.c      |    4 ++--
 net/tipc/core.h                  |    2 +-
 sound/pci/oxygen/oxygen_io.c     |    2 +-
 10 files changed, 44 insertions(+), 11 deletions(-)

-- 
1.7.10.4

[PATCH v2 1/8] time: add *_to_jiffies_timeout helpers to guarantee a minimum duration

[Imre Deak]

The *_to_jiffies(x) macros return a jiffy value, which if used as a
delta to wait for a specific amount of time, may result in a wait time
that is less than x. Many callers already compensate for this by adding
one to the returned value. Instead of having this adjustment open-coded
at every call site add helpers that return the adjusted value. This will
make the intention for the adjustment more explicit and also provide
documentation for why it is needed.

Later patches will convert the currently open-coded call sites to use
the new helpers.

Also this can serve as a basis for auditing those users of *_to_jiffies
that most likely do the wrong thing - for example set a timeout value of
msecs_to_jiffies(1) - and converting them to use the new helpers.

Kudos to Daniel Vetter for the idea of the new helpers.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 include/linux/jiffies.h |    6 ++++++
 kernel/time.c           |   27 +++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/include/linux/jiffies.h b/include/linux/jiffies.h
index 8fb8edf..e77a5a0 100644
--- a/include/linux/jiffies.h
+++ b/include/linux/jiffies.h
@@ -311,6 +311,12 @@ extern u64 nsec_to_clock_t(u64 x);
 extern u64 nsecs_to_jiffies64(u64 n);
 extern unsigned long nsecs_to_jiffies(u64 n);
 
+extern unsigned long msecs_to_jiffies_timeout(const unsigned int m);
+extern unsigned long usecs_to_jiffies_timeout(const unsigned int u);
+extern unsigned long nsecs_to_jiffies_timeout(const u64 n);
+extern unsigned long timespec_to_jiffies_timeout(const struct timespec *value);
+extern unsigned long timeval_to_jiffies_timeout(const struct timeval *value);
+
 #define TIMESTAMP_SIZE	30
 
 #endif
diff --git a/kernel/time.c b/kernel/time.c
index d3617db..759d897 100644
--- a/kernel/time.c
+++ b/kernel/time.c
@@ -712,3 +712,30 @@ struct timespec timespec_add_safe(const struct timespec lhs,
 
 	return res;
 }
+
+/**
+ * msecs_to_jiffies_timeout - Convert msecs to jiffies with guaranteed minimum
+ *
+ * Works similarly to msecs_to_jiffies(), but returns a value that represents
+ * a wait time that is guaranteed to be at least the given msecs duration. In
+ * contrast the value returned by msecs_to_jiffies() represents a wait time
+ * that can be up to 1 jiffy less than the specified msecs duration, depending
+ * on which point in time between two scheduling clock ticks we use the
+ * returned jiffy value.
+ *
+ * Similar helpers are available corresponding to the rest of *_to_jiffies()
+ * functions.
+ */
+#define __define_time_to_jiffies_timeout(tname, ttype)			\
+unsigned long tname ## _to_jiffies_timeout(const ttype v)		\
+{									\
+	unsigned long j = tname ## _to_jiffies(v);			\
+	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);		\
+}									\
+EXPORT_SYMBOL(tname ## _to_jiffies_timeout);
+
+__define_time_to_jiffies_timeout(msecs, unsigned int)
+__define_time_to_jiffies_timeout(usecs, unsigned int)
+__define_time_to_jiffies_timeout(nsecs, u64)
+__define_time_to_jiffies_timeout(timespec, struct timespec *)
+__define_time_to_jiffies_timeout(timeval, struct timeval *)
-- 
1.7.10.4

Re: [PATCH v2 1/8] time: add *_to_jiffies_timeout helpers to guarantee a minimum duration

[Arnd Bergmann]

On Tuesday 14 May 2013, Imre Deak wrote:
> The *_to_jiffies(x) macros return a jiffy value, which if used as a
> delta to wait for a specific amount of time, may result in a wait time
> that is less than x. Many callers already compensate for this by adding
> one to the returned value. Instead of having this adjustment open-coded
> at every call site add helpers that return the adjusted value. This will
> make the intention for the adjustment more explicit and also provide
> documentation for why it is needed.
> 
> Later patches will convert the currently open-coded call sites to use
> the new helpers.
> 
> Also this can serve as a basis for auditing those users of *_to_jiffies
> that most likely do the wrong thing - for example set a timeout value of
> msecs_to_jiffies(1) - and converting them to use the new helpers.
> 
> Kudos to Daniel Vetter for the idea of the new helpers.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

This certainly looks like a reasonable change, but I wonder if we could take
it one step further and add milisecond based interfaces for some of those
functions that currently take a jiffies value, something like

int timer_set_msecs(struct timer_list *timer, unsigned long msecs)
{
	unsigned long j = msec_to_jiffies(msecs);
	return mod_timer(timer, min_t(msecs, MAX_JIFFY_OFFSET));
}

> +#define __define_time_to_jiffies_timeout(tname, ttype)                 \
> +unsigned long tname ## _to_jiffies_timeout(const ttype v)              \
> +{                                                                      \
> +       unsigned long j = tname ## _to_jiffies(v);                      \
> +       return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);           \
> +}                                                                      \
> +EXPORT_SYMBOL(tname ## _to_jiffies_timeout);

The macro has a few disadvantages:

* It's impossible to grep for the function or use tags if you generate
  the identifier using the macro.

* msecs_to_jiffies is what puts MAX_JIFFY_OFFSET there in the first
  place, which means you add an extra comparison here that should
  not really be needed.

	Arnd

Re: [PATCH v2 1/8] time: add *_to_jiffies_timeout helpers to guarantee a minimum duration

[Imre Deak]

On Wed, 2013-05-15 at 17:26 +0200, Arnd Bergmann wrote:
> On Tuesday 14 May 2013, Imre Deak wrote:
> > The *_to_jiffies(x) macros return a jiffy value, which if used as a
> > delta to wait for a specific amount of time, may result in a wait time
> > that is less than x. Many callers already compensate for this by adding
> > one to the returned value. Instead of having this adjustment open-coded
> > at every call site add helpers that return the adjusted value. This will
> > make the intention for the adjustment more explicit and also provide
> > documentation for why it is needed.
> > 
> > Later patches will convert the currently open-coded call sites to use
> > the new helpers.
> > 
> > Also this can serve as a basis for auditing those users of *_to_jiffies
> > that most likely do the wrong thing - for example set a timeout value of
> > msecs_to_jiffies(1) - and converting them to use the new helpers.
> > 
> > Kudos to Daniel Vetter for the idea of the new helpers.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> This certainly looks like a reasonable change, but I wonder if we could take
> it one step further and add milisecond based interfaces for some of those
> functions that currently take a jiffies value, something like
> 
> int timer_set_msecs(struct timer_list *timer, unsigned long msecs)
> {
> 	unsigned long j = msec_to_jiffies(msecs);
> 	return mod_timer(timer, min_t(msecs, MAX_JIFFY_OFFSET));
> }

Ok, but I think we'd still need the *_to_jiffies_timeout helpers, so
that we don't have to open code the +1 magic anywhere.

> > +#define __define_time_to_jiffies_timeout(tname, ttype)                 \
> > +unsigned long tname ## _to_jiffies_timeout(const ttype v)              \
> > +{                                                                      \
> > +       unsigned long j = tname ## _to_jiffies(v);                      \
> > +       return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);           \
> > +}                                                                      \
> > +EXPORT_SYMBOL(tname ## _to_jiffies_timeout);
> 
> The macro has a few disadvantages:
> 
> * It's impossible to grep for the function or use tags if you generate
>   the identifier using the macro.

They are fully spelled in include/linux/jiffies.h . Would it be ok if I
moved the kernel doc there with a reference to kernel/time.c?

> * msecs_to_jiffies is what puts MAX_JIFFY_OFFSET there in the first
>   place, which means you add an extra comparison here that should
>   not really be needed.

Yes, but that allows us to keep things simple across all the helpers. I
haven't checked but I'd assume compiler inlining/optimization should
make this a non-issue anyway.

--Imre

Re: [PATCH v2 1/8] time: add *_to_jiffies_timeout helpers to guarantee a minimum duration

[Arnd Bergmann]

On Wednesday 15 May 2013, Imre Deak wrote:
> On Wed, 2013-05-15 at 17:26 +0200, Arnd Bergmann wrote:
>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > 
> > This certainly looks like a reasonable change, but I wonder if we could take
> > it one step further and add milisecond based interfaces for some of those
> > functions that currently take a jiffies value, something like
> > 
> > int timer_set_msecs(struct timer_list *timer, unsigned long msecs)
> > {
> > 	unsigned long j = msec_to_jiffies(msecs);
> > 	return mod_timer(timer, min_t(msecs, MAX_JIFFY_OFFSET));
> > }
> 
> Ok, but I think we'd still need the *_to_jiffies_timeout helpers, so
> that we don't have to open code the +1 magic anywhere.

Yes, but we would not change any driver code to use those.
We probably would not need all of the helpers and add only the
ones that are required by other infrastructure.

> > > +#define __define_time_to_jiffies_timeout(tname, ttype)                 \
> > > +unsigned long tname ## _to_jiffies_timeout(const ttype v)              \
> > > +{                                                                      \
> > > +       unsigned long j = tname ## _to_jiffies(v);                      \
> > > +       return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);           \
> > > +}                                                                      \
> > > +EXPORT_SYMBOL(tname ## _to_jiffies_timeout);
> > 
> > The macro has a few disadvantages:
> > 
> > * It's impossible to grep for the function or use tags if you generate
> >   the identifier using the macro.
> 
> They are fully spelled in include/linux/jiffies.h . Would it be ok if I
> moved the kernel doc there with a reference to kernel/time.c?

Yes, I guess that's ok. I would prefer to have them open-coded, but
it's not a big issue.

> > * msecs_to_jiffies is what puts MAX_JIFFY_OFFSET there in the first
> >   place, which means you add an extra comparison here that should
> >   not really be needed.
> 
> Yes, but that allows us to keep things simple across all the helpers. I
> haven't checked but I'd assume compiler inlining/optimization should
> make this a non-issue anyway.

msecs_to_jiffies is a global function, so it won't normally get inlined.

	Arnd.

Re: [PATCH v2 1/8] time: add *_to_jiffies_timeout helpers to guarantee a minimum duration

[Imre Deak]

On Fri, 2013-05-17 at 15:35 +0200, Arnd Bergmann wrote:
> On Wednesday 15 May 2013, Imre Deak wrote:
> > On Wed, 2013-05-15 at 17:26 +0200, Arnd Bergmann wrote:
> >
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > 
> > > This certainly looks like a reasonable change, but I wonder if we could take
> > > it one step further and add milisecond based interfaces for some of those
> > > functions that currently take a jiffies value, something like
> > > 
> > > int timer_set_msecs(struct timer_list *timer, unsigned long msecs)
> > > {
> > > 	unsigned long j = msec_to_jiffies(msecs);
> > > 	return mod_timer(timer, min_t(msecs, MAX_JIFFY_OFFSET));
> > > }
> > 
> > Ok, but I think we'd still need the *_to_jiffies_timeout helpers, so
> > that we don't have to open code the +1 magic anywhere.
> 
> Yes, but we would not change any driver code to use those.

I don't know which would be a better API for drivers. We have quite a
few more functions that accept a jiffies timeout parameter, for example:

wait_event_timeout
wait_event_interruptible_timeout
wait_for_completion_timeout
wait_for_completion_interruptible_timeout
schedule_timeout
...

For all of these we'd have to add an msec and perhaps also an usec
variant. I find it simpler to have a set of helpers that can convert
from any time format to jiffies, which can be used then with any of
these functions.

Also we would still need to expose these helpers to drivers in case they
want to compare against the current jiffies directly.

> We probably would not need all of the helpers and add only the
> ones that are required by other infrastructure.
>
> > > > +#define __define_time_to_jiffies_timeout(tname, ttype)                 \
> > > > +unsigned long tname ## _to_jiffies_timeout(const ttype v)              \
> > > > +{                                                                      \
> > > > +       unsigned long j = tname ## _to_jiffies(v);                      \
> > > > +       return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);           \
> > > > +}                                                                      \
> > > > +EXPORT_SYMBOL(tname ## _to_jiffies_timeout);
> > > 
> > > The macro has a few disadvantages:
> > > 
> > > * It's impossible to grep for the function or use tags if you generate
> > >   the identifier using the macro.
> > 
> > They are fully spelled in include/linux/jiffies.h . Would it be ok if I
> > moved the kernel doc there with a reference to kernel/time.c?
> 
> Yes, I guess that's ok. I would prefer to have them open-coded, but
> it's not a big issue.
> 
> > > * msecs_to_jiffies is what puts MAX_JIFFY_OFFSET there in the first
> > >   place, which means you add an extra comparison here that should
> > >   not really be needed.
> > 
> > Yes, but that allows us to keep things simple across all the helpers. I
> > haven't checked but I'd assume compiler inlining/optimization should
> > make this a non-issue anyway.
> 
> msecs_to_jiffies is a global function, so it won't normally get inlined.

Actually you were right about this. Now that I checked msecs_to_jiffies
does get inlined into msecs_to_jiffies_timeout but the comparison won't
get optimized away.

How about the following to solve the two issues you raised? Admittedly
somewhat bigger diff than the previous version :/

diff --git a/include/linux/jiffies.h b/include/linux/jiffies.h
index 8fb8edf..270a012 100644
--- a/include/linux/jiffies.h
+++ b/include/linux/jiffies.h
@@ -272,16 +272,17 @@ extern unsigned long preset_lpj;
  */
 #define USEC_ROUND (u64)(((u64)1 << USEC_JIFFIE_SC) - 1)
 /*
- * The maximum jiffie value is (MAX_INT >> 1).  Here we translate that
+ * The passed jiffy value can be at most (MAX_INT >> 1).  We translate
that
  * into seconds.  The 64-bit case will overflow if we are not careful,
- * so use the messy SH_DIV macro to do it.  Still all constants.
+ * so use the messy SH_DIV macro to do it.  Still all constants if the
+ * parameter is constant.
  */
 #if BITS_PER_LONG < 64
-# define MAX_SEC_IN_JIFFIES \
-	(long)((u64)((u64)MAX_JIFFY_OFFSET * TICK_NSEC) / NSEC_PER_SEC)
+# define JIFFIES_TO_SEC(j) \
+	(long)((u64)((u64)(j) * TICK_NSEC) / NSEC_PER_SEC)
 #else	/* take care of overflow on 64 bits machines */
-# define MAX_SEC_IN_JIFFIES \
-	(SH_DIV((MAX_JIFFY_OFFSET >> SEC_JIFFIE_SC) * TICK_NSEC, NSEC_PER_SEC,
1) - 1)
+# define JIFFIES_TO_SEC(j) \
+	(SH_DIV(((j) >> SEC_JIFFIE_SC) * TICK_NSEC, NSEC_PER_SEC, 1) - 1)
 
 #endif
 
@@ -311,6 +312,11 @@ extern u64 nsec_to_clock_t(u64 x);
 extern u64 nsecs_to_jiffies64(u64 n);
 extern unsigned long nsecs_to_jiffies(u64 n);
 
+extern unsigned long msecs_to_jiffies_timeout(const unsigned int m);
+extern unsigned long usecs_to_jiffies_timeout(const unsigned int u);
+extern unsigned long timespec_to_jiffies_timeout(const struct timespec
*value);
+extern unsigned long timeval_to_jiffies_timeout(const struct timeval
*value);
+
 #define TIMESTAMP_SIZE	30
 
 #endif
diff --git a/kernel/time.c b/kernel/time.c
index d3617db..00f9f7c 100644
--- a/kernel/time.c
+++ b/kernel/time.c
@@ -434,7 +434,8 @@ EXPORT_SYMBOL(ns_to_timeval);
  *
  * We must also be careful about 32-bit overflows.
  */
-unsigned long msecs_to_jiffies(const unsigned int m)
+static unsigned long __msecs_to_jiffies_delta(const unsigned int m,
+					      const int delta)
 {
 	/*
 	 * Negative value, means infinite timeout:
@@ -448,7 +449,7 @@ unsigned long msecs_to_jiffies(const unsigned int m)
 	 * round multiple of HZ, divide with the factor between them,
 	 * but round upwards:
 	 */
-	return (m + (MSEC_PER_SEC / HZ) - 1) / (MSEC_PER_SEC / HZ);
+	return (m + (MSEC_PER_SEC / HZ) - 1) / (MSEC_PER_SEC / HZ) + delta;
 #elif HZ > MSEC_PER_SEC && !(HZ % MSEC_PER_SEC)
 	/*
 	 * HZ is larger than 1000, and HZ is a nice round multiple of
@@ -457,40 +458,78 @@ unsigned long msecs_to_jiffies(const unsigned int
m)
 	 * But first make sure the multiplication result cannot
 	 * overflow:
 	 */
-	if (m > jiffies_to_msecs(MAX_JIFFY_OFFSET))
+	if (m > jiffies_to_msecs(MAX_JIFFY_OFFSET - delta))
 		return MAX_JIFFY_OFFSET;
 
-	return m * (HZ / MSEC_PER_SEC);
+	return m * (HZ / MSEC_PER_SEC) + delta;
 #else
 	/*
 	 * Generic case - multiply, round and divide. But first
 	 * check that if we are doing a net multiplication, that
 	 * we wouldn't overflow:
 	 */
-	if (HZ > MSEC_PER_SEC && m > jiffies_to_msecs(MAX_JIFFY_OFFSET))
+	if (HZ > MSEC_PER_SEC && m > jiffies_to_msecs(MAX_JIFFY_OFFSET -
delta))
 		return MAX_JIFFY_OFFSET;
 
-	return (MSEC_TO_HZ_MUL32 * m + MSEC_TO_HZ_ADJ32)
-		>> MSEC_TO_HZ_SHR32;
+	return ((MSEC_TO_HZ_MUL32 * m + MSEC_TO_HZ_ADJ32)
+		>> MSEC_TO_HZ_SHR32) + delta;
 #endif
 }
+
+unsigned long msecs_to_jiffies(const unsigned int m)
+{
+	return __msecs_to_jiffies_delta(m, 0);
+}
 EXPORT_SYMBOL(msecs_to_jiffies);
 
-unsigned long usecs_to_jiffies(const unsigned int u)
+/**
+ * msecs_to_jiffies_timeout - Convert msecs to jiffies with guaranteed
minimum duration
+ *
+ * Works similarly to msecs_to_jiffies(), but returns a value that
represents
+ * a wait time that is guaranteed to be at least the given msecs
duration. In
+ * contrast the value returned by msecs_to_jiffies() represents a wait
time
+ * that can be up to 1 jiffy less than the specified msecs duration,
depending
+ * on which point in time between two scheduling clock ticks we use the
+ * returned jiffy value.
+ */
+unsigned long msecs_to_jiffies_timeout(const unsigned int m)
+{
+	return __msecs_to_jiffies_delta(m, 1);
+}
+EXPORT_SYMBOL(msecs_to_jiffies_timeout);
+
+static unsigned long __usecs_to_jiffies_delta(const unsigned int u,
+					      const int delta)
 {
-	if (u > jiffies_to_usecs(MAX_JIFFY_OFFSET))
+	if (u > jiffies_to_usecs(MAX_JIFFY_OFFSET - delta))
 		return MAX_JIFFY_OFFSET;
 #if HZ <= USEC_PER_SEC && !(USEC_PER_SEC % HZ)
-	return (u + (USEC_PER_SEC / HZ) - 1) / (USEC_PER_SEC / HZ);
+	return (u + (USEC_PER_SEC / HZ) - 1) / (USEC_PER_SEC / HZ) + delta;
 #elif HZ > USEC_PER_SEC && !(HZ % USEC_PER_SEC)
-	return u * (HZ / USEC_PER_SEC);
+	return u * (HZ / USEC_PER_SEC) + delta;
 #else
-	return (USEC_TO_HZ_MUL32 * u + USEC_TO_HZ_ADJ32)
-		>> USEC_TO_HZ_SHR32;
+	return ((USEC_TO_HZ_MUL32 * u + USEC_TO_HZ_ADJ32)
+		>> USEC_TO_HZ_SHR32) + delta;
 #endif
 }
+
+unsigned long usecs_to_jiffies(const unsigned int u)
+{
+	return __usecs_to_jiffies_delta(u, 0);
+}
 EXPORT_SYMBOL(usecs_to_jiffies);
 
+/**
+ * usecs_to_jiffies_timeout - Convert usecs to jiffies with guaranteed
minimum duration
+ *
+ * Works like msecs_to_jiffies_timeout(), but takes usecs.
+ */
+unsigned long usecs_to_jiffies_timeout(const unsigned int u)
+{
+	return __usecs_to_jiffies_delta(u, 1);
+}
+EXPORT_SYMBOL(usecs_to_jiffies_timeout);
+
 /*
  * The TICK_NSEC - 1 rounds up the value to the next resolution.  Note
  * that a remainder subtract here would not do the right thing as the
@@ -502,23 +541,41 @@ EXPORT_SYMBOL(usecs_to_jiffies);
  * The >> (NSEC_JIFFIE_SC - SEC_JIFFIE_SC) converts the scaled nsec
  * value to a scaled second value.
  */
-unsigned long
-timespec_to_jiffies(const struct timespec *value)
+static unsigned long
+__timespec_to_jiffies_delta(const struct timespec *value, const int
delta)
 {
 	unsigned long sec = value->tv_sec;
 	long nsec = value->tv_nsec + TICK_NSEC - 1;
 
-	if (sec >= MAX_SEC_IN_JIFFIES){
-		sec = MAX_SEC_IN_JIFFIES;
+	if (sec >= JIFFIES_TO_SEC(MAX_JIFFY_OFFSET - delta)) {
+		sec = JIFFIES_TO_SEC(MAX_JIFFY_OFFSET - delta);
 		nsec = 0;
 	}
-	return (((u64)sec * SEC_CONVERSION) +
+	return ((((u64)sec * SEC_CONVERSION) +
 		(((u64)nsec * NSEC_CONVERSION) >>
-		 (NSEC_JIFFIE_SC - SEC_JIFFIE_SC))) >> SEC_JIFFIE_SC;
+		 (NSEC_JIFFIE_SC - SEC_JIFFIE_SC))) >> SEC_JIFFIE_SC) + delta;
+
+}
 
+unsigned long
+timespec_to_jiffies(const struct timespec *value)
+{
+	return __timespec_to_jiffies_delta(value, 0);
 }
 EXPORT_SYMBOL(timespec_to_jiffies);
 
+/**
+ * timespec_to_jiffies_timeout - Convert timespec to jiffies with
guaranteed minimum duration
+ *
+ * Works like msecs_to_jiffies_timeout(), but takes a timespec.
+ */
+unsigned long
+timespec_to_jiffies_timeout(const struct timespec *value)
+{
+	return __timespec_to_jiffies_delta(value, 1);
+}
+EXPORT_SYMBOL(timespec_to_jiffies_timeout);
+
 void
 jiffies_to_timespec(const unsigned long jiffies, struct timespec
*value)
 {
@@ -545,22 +602,40 @@ EXPORT_SYMBOL(jiffies_to_timespec);
  * Instruction wise, this should cost only an additional add with carry
  * instruction above the way it was done above.
  */
-unsigned long
-timeval_to_jiffies(const struct timeval *value)
+static unsigned long
+__timeval_to_jiffies_delta(const struct timeval *value, const int
delta)
 {
 	unsigned long sec = value->tv_sec;
 	long usec = value->tv_usec;
 
-	if (sec >= MAX_SEC_IN_JIFFIES){
-		sec = MAX_SEC_IN_JIFFIES;
+	if (sec >= JIFFIES_TO_SEC(MAX_JIFFY_OFFSET - delta)) {
+		sec = JIFFIES_TO_SEC(MAX_JIFFY_OFFSET - delta);
 		usec = 0;
 	}
-	return (((u64)sec * SEC_CONVERSION) +
+	return ((((u64)sec * SEC_CONVERSION) +
 		(((u64)usec * USEC_CONVERSION + USEC_ROUND) >>
-		 (USEC_JIFFIE_SC - SEC_JIFFIE_SC))) >> SEC_JIFFIE_SC;
+		 (USEC_JIFFIE_SC - SEC_JIFFIE_SC))) >> SEC_JIFFIE_SC) + delta;
+}
+
+unsigned long
+timeval_to_jiffies(const struct timeval *value)
+{
+	return __timeval_to_jiffies_delta(value, 0);
 }
 EXPORT_SYMBOL(timeval_to_jiffies);
 
+/**
+ * timeval_to_jiffies_timeout - Convert timeval to jiffies with
guaranteed minimum duration
+ *
+ * Works like msecs_to_jiffies_timeout(), but takes a timeval.
+ */
+unsigned long
+timeval_to_jiffies_timeout(const struct timeval *value)
+{
+	return __timeval_to_jiffies_delta(value, 1);
+}
+EXPORT_SYMBOL(timeval_to_jiffies_timeout);
+
 void jiffies_to_timeval(const unsigned long jiffies, struct timeval
*value)
 {
 	/*

[PATCH v2 2/8] sched: msleep: take msecs_to_jiffies_timeout into use

[Imre Deak]

Use msecs_to_jiffies_timeout instead of open-coding the same.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 kernel/timer.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/timer.c b/kernel/timer.c
index a860bba..f397d3b 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -1660,7 +1660,7 @@ void __init init_timers(void)
  */
 void msleep(unsigned int msecs)
 {
-	unsigned long timeout = msecs_to_jiffies(msecs) + 1;
+	unsigned long timeout = msecs_to_jiffies_timeout(msecs);
 
 	while (timeout)
 		timeout = schedule_timeout_uninterruptible(timeout);
@@ -1674,7 +1674,7 @@ EXPORT_SYMBOL(msleep);
  */
 unsigned long msleep_interruptible(unsigned int msecs)
 {
-	unsigned long timeout = msecs_to_jiffies(msecs) + 1;
+	unsigned long timeout = msecs_to_jiffies_timeout(msecs);
 
 	while (timeout && !signal_pending(current))
 		timeout = schedule_timeout_interruptible(timeout);
-- 
1.7.10.4

[PATCH v2 3/8] drm/i915: take msecs_to_jiffies_timeout into use

[Imre Deak]

Use msecs_to_jiffies_timeout instead of open-coding the same.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h |    2 +-
 drivers/gpu/drm/i915/intel_i2c.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 624a9e6..13a46b7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -42,7 +42,7 @@
  * we've never had a chance to check the condition before the timeout.
  */
 #define _wait_for(COND, MS, W) ({ \
-	unsigned long timeout__ = jiffies + msecs_to_jiffies(MS) + 1;	\
+	unsigned long timeout__ = jiffies + msecs_to_jiffies_timeout(MS);\
 	int ret__ = 0;							\
 	while (!(COND)) {						\
 		if (time_after(jiffies, timeout__)) {			\
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 5d24503..98cd8535 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -228,7 +228,7 @@ gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
 	 * need to wake up periodically and check that ourselves. */
 	I915_WRITE(GMBUS4 + reg_offset, gmbus4_irq_en);
 
-	for (i = 0; i < msecs_to_jiffies(50) + 1; i++) {
+	for (i = 0; i < msecs_to_jiffies_timeout(50); i++) {
 		prepare_to_wait(&dev_priv->gmbus_wait_queue, &wait,
 				TASK_UNINTERRUPTIBLE);
 
-- 
1.7.10.4

[PATCH v2 4/8] media/si4713-i2c: take usecs_to_jiffies_timeout into use

[Imre Deak]

Use usecs_to_jiffies_timeout instead of open-coding the same.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/media/radio/si4713-i2c.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/radio/si4713-i2c.c b/drivers/media/radio/si4713-i2c.c
index fe16088..e12f058 100644
--- a/drivers/media/radio/si4713-i2c.c
+++ b/drivers/media/radio/si4713-i2c.c
@@ -233,7 +233,7 @@ static int si4713_send_command(struct si4713_device *sdev, const u8 command,
 
 	/* Wait response from interrupt */
 	if (!wait_for_completion_timeout(&sdev->work,
-				usecs_to_jiffies(usecs) + 1))
+				usecs_to_jiffies_timeout(usecs)))
 		v4l2_warn(&sdev->sd,
 				"(%s) Device took too much time to answer.\n",
 				__func__);
@@ -470,7 +470,7 @@ static int si4713_wait_stc(struct si4713_device *sdev, const int usecs)
 
 	/* Wait response from STC interrupt */
 	if (!wait_for_completion_timeout(&sdev->work,
-			usecs_to_jiffies(usecs) + 1))
+			usecs_to_jiffies_timeout(usecs)))
 		v4l2_warn(&sdev->sd,
 			"%s: device took too much time to answer (%d usec).\n",
 				__func__, usecs);
-- 
1.7.10.4

Re: [PATCH v2 4/8] media/si4713-i2c: take usecs_to_jiffies_timeout into use

[edubezval]

Imre,

On Tue, May 14, 2013 at 10:48 AM, Imre Deak <imre.deak@intel.com> wrote:
> Use usecs_to_jiffies_timeout instead of open-coding the same.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Acked-by: Eduardo Valentin <edubezval@gmail.com>

> ---
>  drivers/media/radio/si4713-i2c.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/radio/si4713-i2c.c b/drivers/media/radio/si4713-i2c.c
> index fe16088..e12f058 100644
> --- a/drivers/media/radio/si4713-i2c.c
> +++ b/drivers/media/radio/si4713-i2c.c
> @@ -233,7 +233,7 @@ static int si4713_send_command(struct si4713_device *sdev, const u8 command,
>
>         /* Wait response from interrupt */
>         if (!wait_for_completion_timeout(&sdev->work,
> -                               usecs_to_jiffies(usecs) + 1))
> +                               usecs_to_jiffies_timeout(usecs)))
>                 v4l2_warn(&sdev->sd,
>                                 "(%s) Device took too much time to answer.\n",
>                                 __func__);
> @@ -470,7 +470,7 @@ static int si4713_wait_stc(struct si4713_device *sdev, const int usecs)
>
>         /* Wait response from STC interrupt */
>         if (!wait_for_completion_timeout(&sdev->work,
> -                       usecs_to_jiffies(usecs) + 1))
> +                       usecs_to_jiffies_timeout(usecs)))
>                 v4l2_warn(&sdev->sd,
>                         "%s: device took too much time to answer (%d usec).\n",
>                                 __func__, usecs);
> --
> 1.7.10.4
>



-- 
Eduardo Bezerra Valentin

[PATCH v2 5/8] usb/isp116x-hcd: take msecs_to_jiffies_timeout into use

[Imre Deak]

Use msecs_to_jiffies_timeout instead of open-coding the same.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/usb/host/isp116x-hcd.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/isp116x-hcd.c b/drivers/usb/host/isp116x-hcd.c
index b64e661..de5807a 100644
--- a/drivers/usb/host/isp116x-hcd.c
+++ b/drivers/usb/host/isp116x-hcd.c
@@ -620,7 +620,7 @@ static irqreturn_t isp116x_irq(struct usb_hcd *hcd)
 			   to come out of suspend, it may take more
 			   than 10ms for status bits to stabilize. */
 			mod_timer(&hcd->rh_timer, jiffies
-				  + msecs_to_jiffies(20) + 1);
+				  + msecs_to_jiffies_timeout(20));
 		if (intstat & HCINT_RD) {
 			DBG("---- remote wakeup\n");
 			usb_hcd_resume_root_hub(hcd);
-- 
1.7.10.4

[PATCH v2 6/8] net/sunrpc: take msecs_to_jiffies_timeout into use

[Imre Deak]

Use msecs_to_jiffies_timeout instead of open-coding the same.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 net/sunrpc/xprtrdma/verbs.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c
index 93726560..8a6575d 100644
--- a/net/sunrpc/xprtrdma/verbs.c
+++ b/net/sunrpc/xprtrdma/verbs.c
@@ -404,7 +404,7 @@ rpcrdma_create_id(struct rpcrdma_xprt *xprt,
 		goto out;
 	}
 	wait_for_completion_interruptible_timeout(&ia->ri_done,
-				msecs_to_jiffies(RDMA_RESOLVE_TIMEOUT) + 1);
+				msecs_to_jiffies_timeout(RDMA_RESOLVE_TIMEOUT));
 	rc = ia->ri_async_rc;
 	if (rc)
 		goto out;
@@ -417,7 +417,7 @@ rpcrdma_create_id(struct rpcrdma_xprt *xprt,
 		goto out;
 	}
 	wait_for_completion_interruptible_timeout(&ia->ri_done,
-				msecs_to_jiffies(RDMA_RESOLVE_TIMEOUT) + 1);
+				msecs_to_jiffies_timeout(RDMA_RESOLVE_TIMEOUT));
 	rc = ia->ri_async_rc;
 	if (rc)
 		goto out;
-- 
1.7.10.4

[PATCH v2 7/8] net/tipc: take msecs_to_jiffies_timeout into use

[Imre Deak]

Use msecs_to_jiffies_timeout instead of open-coding the same.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 net/tipc/core.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tipc/core.h b/net/tipc/core.h
index 0207db0..b24e2fc 100644
--- a/net/tipc/core.h
+++ b/net/tipc/core.h
@@ -133,7 +133,7 @@ static inline void k_init_timer(struct timer_list *timer, Handler routine,
  */
 static inline void k_start_timer(struct timer_list *timer, unsigned long msec)
 {
-	mod_timer(timer, jiffies + msecs_to_jiffies(msec) + 1);
+	mod_timer(timer, jiffies + msecs_to_jiffies_timeout(msec));
 }
 
 /**
-- 
1.7.10.4

[PATCH v2 8/8] sound/oxygen_io: take msecs_to_jiffies_timeout into use

[Imre Deak]

Use msecs_to_jiffies_timeout instead of open-coding the same.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 sound/pci/oxygen/oxygen_io.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/pci/oxygen/oxygen_io.c b/sound/pci/oxygen/oxygen_io.c
index 521eae4..51bfcd0 100644
--- a/sound/pci/oxygen/oxygen_io.c
+++ b/sound/pci/oxygen/oxygen_io.c
@@ -108,7 +108,7 @@ static int oxygen_ac97_wait(struct oxygen *chip, unsigned int mask)
 	wait_event_timeout(chip->ac97_waitqueue,
 			   ({ status |= oxygen_read8(chip, OXYGEN_AC97_INTERRUPT_STATUS);
 			      status & mask; }),
-			   msecs_to_jiffies(1) + 1);
+			   msecs_to_jiffies_timeout(1));
 	/*
 	 * Check even after a timeout because this function should not require
 	 * the AC'97 interrupt to be enabled.
-- 
1.7.10.4

Re: [PATCH v2 8/8] sound/oxygen_io: take msecs_to_jiffies_timeout into use

[Takashi Iwai]

At Tue, 14 May 2013 17:48:38 +0300,
Imre Deak wrote:
> 
> Use msecs_to_jiffies_timeout instead of open-coding the same.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Acked-by: Takashi Iwai <tiwai@suse.de>


Takashi

> ---
>  sound/pci/oxygen/oxygen_io.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/pci/oxygen/oxygen_io.c b/sound/pci/oxygen/oxygen_io.c
> index 521eae4..51bfcd0 100644
> --- a/sound/pci/oxygen/oxygen_io.c
> +++ b/sound/pci/oxygen/oxygen_io.c
> @@ -108,7 +108,7 @@ static int oxygen_ac97_wait(struct oxygen *chip, unsigned int mask)
>  	wait_event_timeout(chip->ac97_waitqueue,
>  			   ({ status |= oxygen_read8(chip, OXYGEN_AC97_INTERRUPT_STATUS);
>  			      status & mask; }),
> -			   msecs_to_jiffies(1) + 1);
> +			   msecs_to_jiffies_timeout(1));
>  	/*
>  	 * Check even after a timeout because this function should not require
>  	 * the AC'97 interrupt to be enabled.
> -- 
> 1.7.10.4
>