* [PATCH] wan: dscc4: use msecs_to_jiffies for conversions
@ 2015-06-06 8:41 Nicholas Mc Guire
2015-06-07 7:13 ` David Miller
0 siblings, 1 reply; 4+ messages in thread
From: Nicholas Mc Guire @ 2015-06-06 8:41 UTC (permalink / raw)
To: Francois Romieu; +Cc: netdev, linux-kernel, Nicholas Mc Guire
API compliance scanning with coccinelle flagged:
./drivers/net/wan/dscc4.c:1036:1-33:
WARNING: timeout (10) seems HZ dependent
./drivers/net/wan/dscc4.c:554:2-34:
WARNING: timeout (10) seems HZ dependent
./drivers/net/wan/dscc4.c:599:2-34:
WARNING: timeout (10) seems HZ dependent
Numeric constants passed to schedule_timeout_*() make the effective
timeout HZ dependent which does not seem to be the intent here.
Fixed up by converting the constant to jiffies with msecs_to_jiffies()
Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---
As the intended timeout is not documented and msecs_to_jiffies(10) may
be a factor 10 off from constant 10, this needs to be checked by
someone that knows the details of this driver, in any case it should
be passed in a HZ independent way.
Patch was compile tested with x86_64_defconfig + CONFIG_WAN=y
CONFIG_HDLC=m, CONFIG_DSCC4=m
Patch is against 4.1-rc6 (localversion-next is -next-20150605)
drivers/net/wan/dscc4.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wan/dscc4.c b/drivers/net/wan/dscc4.c
index 0822356..2ce5249 100644
--- a/drivers/net/wan/dscc4.c
+++ b/drivers/net/wan/dscc4.c
@@ -551,7 +551,7 @@ static int dscc4_wait_ack_cec(struct dscc4_dev_priv *dpriv,
msg, i);
goto done;
}
- schedule_timeout_uninterruptible(10);
+ schedule_timeout_uninterruptible(msecs_to_jiffies(10));
rmb();
} while (++i > 0);
netdev_err(dev, "%s timeout\n", msg);
@@ -596,7 +596,7 @@ static inline int dscc4_xpr_ack(struct dscc4_dev_priv *dpriv)
(dpriv->iqtx[cur] & cpu_to_le32(Xpr)))
break;
smp_rmb();
- schedule_timeout_uninterruptible(10);
+ schedule_timeout_uninterruptible(msecs_to_jiffies(10));
} while (++i > 0);
return (i >= 0 ) ? i : -EAGAIN;
@@ -1033,7 +1033,7 @@ static void dscc4_pci_reset(struct pci_dev *pdev, void __iomem *ioaddr)
/* Flush posted writes */
readl(ioaddr + GSTAR);
- schedule_timeout_uninterruptible(10);
+ schedule_timeout_uninterruptible(msecs_to_jiffies(10));
for (i = 0; i < 16; i++)
pci_write_config_dword(pdev, i << 2, dscc4_pci_config_store[i]);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] wan: dscc4: use msecs_to_jiffies for conversions
2015-06-06 8:41 [PATCH] wan: dscc4: use msecs_to_jiffies for conversions Nicholas Mc Guire
@ 2015-06-07 7:13 ` David Miller
2015-06-07 8:31 ` Nicholas Mc Guire
2015-06-07 8:32 ` Francois Romieu
0 siblings, 2 replies; 4+ messages in thread
From: David Miller @ 2015-06-07 7:13 UTC (permalink / raw)
To: hofrat; +Cc: romieu, netdev, linux-kernel
From: Nicholas Mc Guire <hofrat@osadl.org>
Date: Sat, 6 Jun 2015 10:41:06 +0200
> API compliance scanning with coccinelle flagged:
> ./drivers/net/wan/dscc4.c:1036:1-33:
> WARNING: timeout (10) seems HZ dependent
> ./drivers/net/wan/dscc4.c:554:2-34:
> WARNING: timeout (10) seems HZ dependent
> ./drivers/net/wan/dscc4.c:599:2-34:
> WARNING: timeout (10) seems HZ dependent
>
> Numeric constants passed to schedule_timeout_*() make the effective
> timeout HZ dependent which does not seem to be the intent here.
> Fixed up by converting the constant to jiffies with msecs_to_jiffies()
>
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
Whoever wrote these things probably wanted whatever this amounts
to when HZ=100, so that is the only valid transformation you can
make to fix this up here.
Otherwise you seriously risk breaking the driver.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] wan: dscc4: use msecs_to_jiffies for conversions
2015-06-07 7:13 ` David Miller
@ 2015-06-07 8:31 ` Nicholas Mc Guire
2015-06-07 8:32 ` Francois Romieu
1 sibling, 0 replies; 4+ messages in thread
From: Nicholas Mc Guire @ 2015-06-07 8:31 UTC (permalink / raw)
To: David Miller; +Cc: hofrat, romieu, netdev, linux-kernel
On Sun, 07 Jun 2015, David Miller wrote:
> From: Nicholas Mc Guire <hofrat@osadl.org>
> Date: Sat, 6 Jun 2015 10:41:06 +0200
>
> > API compliance scanning with coccinelle flagged:
> > ./drivers/net/wan/dscc4.c:1036:1-33:
> > WARNING: timeout (10) seems HZ dependent
> > ./drivers/net/wan/dscc4.c:554:2-34:
> > WARNING: timeout (10) seems HZ dependent
> > ./drivers/net/wan/dscc4.c:599:2-34:
> > WARNING: timeout (10) seems HZ dependent
> >
> > Numeric constants passed to schedule_timeout_*() make the effective
> > timeout HZ dependent which does not seem to be the intent here.
> > Fixed up by converting the constant to jiffies with msecs_to_jiffies()
> >
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
>
> Whoever wrote these things probably wanted whatever this amounts
> to when HZ=100, so that is the only valid transformation you can
> make to fix this up here.
>
> Otherwise you seriously risk breaking the driver.
I did not find dscc4 in the 2.2 series of kernels so it does not predate
configurable HZ - or do you mean simply that increasing the timeout
here should be side-effect free and thus the larger of the values
should be taken ? - though that would imply that it is possibly now
broken for HZ=1000.
Will fix it up to 10 == 100ms and fix the patch documentation.
thx!
hofrat
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] wan: dscc4: use msecs_to_jiffies for conversions
2015-06-07 7:13 ` David Miller
2015-06-07 8:31 ` Nicholas Mc Guire
@ 2015-06-07 8:32 ` Francois Romieu
1 sibling, 0 replies; 4+ messages in thread
From: Francois Romieu @ 2015-06-07 8:32 UTC (permalink / raw)
To: David Miller; +Cc: hofrat, netdev, linux-kernel
David Miller <davem@davemloft.net> :
[...]
> Whoever wrote these things probably wanted whatever this amounts
> to when HZ=100, so that is the only valid transformation you can
> make to fix this up here.
It's linux kernel illiterate style from 13 years ago but I commented
dscc4_pci_reset.
wait_ack_cec and xpr_ack are dumb busy loops that break on short integer
sign change. Hackish hint.
> Otherwise you seriously risk breaking the driver.
How seriously is hard to tell. I've never tried it at really low line
rates though.
Please apply.
Thanks.
--
Ueimor
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-06-07 8:33 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-06 8:41 [PATCH] wan: dscc4: use msecs_to_jiffies for conversions Nicholas Mc Guire
2015-06-07 7:13 ` David Miller
2015-06-07 8:31 ` Nicholas Mc Guire
2015-06-07 8:32 ` Francois Romieu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).