* [PATCH RFC] staging: fieldbus: anybus-s: use proper type for wait_for_completion_timeout
@ 2019-04-27 4:26 Nicholas Mc Guire
2019-04-27 6:17 ` Sven Van Asbroeck
0 siblings, 1 reply; 6+ messages in thread
From: Nicholas Mc Guire @ 2019-04-27 4:26 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Sven Van Asbroeck, devel, linux-kernel, Nicholas Mc Guire
wait_for_completion_timeout() returns unsigned long (0 on timeout or
remaining jiffies) not int. thus there is no negative case to check for
here.
Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---
Problem located with experimental API conformance checking cocci script
Q: It is not really clear if the proposed fix is the right thing here or if
this should not be using wait_for_completion_interruptible - which would
return -ERESTARTSYS on interruption. Someone that knows the details of
this driver would need to check what condition should initiate the
goto err_reset; which was actually unreachable in the current code.
Patch was compile-tested with. x86_64_defconfig + FIELDBUS_DEV=m,
HMS_ANYBUSS_BUS=m
(some unrelated sparse warnings (cast to restricted __be16))
Patch is against 5.1-rc6 (localversion-next is next-20190426)
drivers/staging/fieldbus/anybuss/host.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/fieldbus/anybuss/host.c b/drivers/staging/fieldbus/anybuss/host.c
index e34d424..c52d715 100644
--- a/drivers/staging/fieldbus/anybuss/host.c
+++ b/drivers/staging/fieldbus/anybuss/host.c
@@ -1263,6 +1263,7 @@ anybuss_host_common_probe(struct device *dev,
const struct anybuss_ops *ops)
{
int ret, i;
+ unsigned long time_left;
u8 val[4];
u16 fieldbus_type;
struct anybuss_host *cd;
@@ -1325,11 +1326,9 @@ anybuss_host_common_probe(struct device *dev,
* interrupt came in: ready to go !
*/
reset_deassert(cd);
- ret = wait_for_completion_timeout(&cd->card_boot, TIMEOUT);
- if (ret == 0)
+ time_left = wait_for_completion_timeout(&cd->card_boot, TIMEOUT);
+ if (time_left == 0)
ret = -ETIMEDOUT;
- if (ret < 0)
- goto err_reset;
/*
* according to the anybus docs, we're allowed to read these
* without handshaking / reserving the area
--
2.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] staging: fieldbus: anybus-s: use proper type for wait_for_completion_timeout
2019-04-27 4:26 [PATCH RFC] staging: fieldbus: anybus-s: use proper type for wait_for_completion_timeout Nicholas Mc Guire
@ 2019-04-27 6:17 ` Sven Van Asbroeck
2019-04-27 7:00 ` Nicholas Mc Guire
0 siblings, 1 reply; 6+ messages in thread
From: Sven Van Asbroeck @ 2019-04-27 6:17 UTC (permalink / raw)
To: Nicholas Mc Guire; +Cc: Greg Kroah-Hartman, devel, Linux Kernel Mailing List
Hello Nicholas, thank you for your contribution, I really appreciate it !
See inline comments below.
On Sat, Apr 27, 2019 at 12:32 AM Nicholas Mc Guire <hofrat@osadl.org> wrote:
>
> wait_for_completion_timeout() returns unsigned long (0 on timeout or
> remaining jiffies) not int.
Nice catch !
> thus there is no negative case to check for
> here.
The current code can only break if wait_for_completion_timeout()
returns an unsigned long large enough to make the "int ret" turn
negative. This could result in probe() returning a nonsensical error
value, even though the wait did not time out.
Fortunately that currently cannot happen, because TIMEOUT
(2*HZ) won't overflow an int.
That being said, this return value type mismatch should definitely
be fixed up.
>
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> ---
>
> Problem located with experimental API conformance checking cocci script
>
> Q: It is not really clear if the proposed fix is the right thing here or if
> this should not be using wait_for_completion_interruptible - which would
> return -ERESTARTSYS on interruption. Someone that knows the details of
> this driver would need to check what condition should initiate the
> goto err_reset; which was actually unreachable in the current code.
It's used in probe(), so no need for an interruptible wait, AFAIK.
It can stay as-is.
>
> Patch was compile-tested with. x86_64_defconfig + FIELDBUS_DEV=m,
> HMS_ANYBUSS_BUS=m
> (some unrelated sparse warnings (cast to restricted __be16))
That sounds interesting too. Could you provide more details?
<snip>
> - ret = wait_for_completion_timeout(&cd->card_boot, TIMEOUT);
> - if (ret == 0)
> + time_left = wait_for_completion_timeout(&cd->card_boot, TIMEOUT);
> + if (time_left == 0)
> ret = -ETIMEDOUT;
> - if (ret < 0)
> - goto err_reset;
NAK. This does not jump to err_reset on timeout.
May I suggest the following instead ? (manual formatting)
- ret = wait_for_completion_timeout(&cd->card_boot, TIMEOUT);
- if (ret == 0)
- ret = -ETIMEDOUT;
- if (ret < 0)
- goto err_reset;
+ if (!wait_for_completion_timeout(&cd->card_boot, TIMEOUT)) {
+ ret = -ETIMEDOUT;
+ goto err_reset;
+ }
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] staging: fieldbus: anybus-s: use proper type for wait_for_completion_timeout
2019-04-27 6:17 ` Sven Van Asbroeck
@ 2019-04-27 7:00 ` Nicholas Mc Guire
2019-04-27 7:20 ` Sven Van Asbroeck
0 siblings, 1 reply; 6+ messages in thread
From: Nicholas Mc Guire @ 2019-04-27 7:00 UTC (permalink / raw)
To: Sven Van Asbroeck
Cc: Nicholas Mc Guire, Greg Kroah-Hartman, devel, Linux Kernel Mailing List
On Sat, Apr 27, 2019 at 02:17:42AM -0400, Sven Van Asbroeck wrote:
> Hello Nicholas, thank you for your contribution, I really appreciate it !
> See inline comments below.
>
> On Sat, Apr 27, 2019 at 12:32 AM Nicholas Mc Guire <hofrat@osadl.org> wrote:
> >
> > wait_for_completion_timeout() returns unsigned long (0 on timeout or
> > remaining jiffies) not int.
>
> Nice catch !
>
> > thus there is no negative case to check for
> > here.
>
> The current code can only break if wait_for_completion_timeout()
> returns an unsigned long large enough to make the "int ret" turn
> negative. This could result in probe() returning a nonsensical error
> value, even though the wait did not time out.
>
> Fortunately that currently cannot happen, because TIMEOUT
> (2*HZ) won't overflow an int.
ok - thats benign then - never the less since code tends to get copied
it would be good to make it comply with API spec
>
> That being said, this return value type mismatch should definitely
> be fixed up.
>
> >
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > ---
> >
> > Problem located with experimental API conformance checking cocci script
> >
> > Q: It is not really clear if the proposed fix is the right thing here or if
> > this should not be using wait_for_completion_interruptible - which would
> > return -ERESTARTSYS on interruption. Someone that knows the details of
> > this driver would need to check what condition should initiate the
> > goto err_reset; which was actually unreachable in the current code.
>
> It's used in probe(), so no need for an interruptible wait, AFAIK.
> It can stay as-is.
>
> >
> > Patch was compile-tested with. x86_64_defconfig + FIELDBUS_DEV=m,
> > HMS_ANYBUSS_BUS=m
> > (some unrelated sparse warnings (cast to restricted __be16))
>
> That sounds interesting too. Could you provide more details?
make C=1
drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted __be16
drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted __be16
drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted __be16
drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted __be16
drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted
>
> <snip>
>
> > - ret = wait_for_completion_timeout(&cd->card_boot, TIMEOUT);
> > - if (ret == 0)
> > + time_left = wait_for_completion_timeout(&cd->card_boot, TIMEOUT);
> > + if (time_left == 0)
> > ret = -ETIMEDOUT;
> > - if (ret < 0)
> > - goto err_reset;
>
> NAK. This does not jump to err_reset on timeout.
>
> May I suggest the following instead ? (manual formatting)
>
> - ret = wait_for_completion_timeout(&cd->card_boot, TIMEOUT);
> - if (ret == 0)
> - ret = -ETIMEDOUT;
> - if (ret < 0)
> - goto err_reset;
> + if (!wait_for_completion_timeout(&cd->card_boot, TIMEOUT)) {
> + ret = -ETIMEDOUT;
> + goto err_reset;
> + }
Ah - ok - that was the bit missing for me !
how that goto branch would be reached or if it should be
dropped as it had not been reachable in the past.
I'll send the V2 later today then.
thx!
hofrat
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] staging: fieldbus: anybus-s: use proper type for wait_for_completion_timeout
2019-04-27 7:00 ` Nicholas Mc Guire
@ 2019-04-27 7:20 ` Sven Van Asbroeck
2019-04-27 11:17 ` Nicholas Mc Guire
0 siblings, 1 reply; 6+ messages in thread
From: Sven Van Asbroeck @ 2019-04-27 7:20 UTC (permalink / raw)
To: Nicholas Mc Guire
Cc: Nicholas Mc Guire, Greg Kroah-Hartman, devel, Linux Kernel Mailing List
On Sat, Apr 27, 2019 at 3:01 AM Nicholas Mc Guire <der.herr@hofr.at> wrote:
> > > (some unrelated sparse warnings (cast to restricted __be16))
> >
> > That sounds interesting too. Could you provide more details?
>
> make C=1
> drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted __be16
> drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted __be16
> drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted __be16
> drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted __be16
> drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted
regmap_bulk_read(cd->regmap, REG_FIELDBUS_TYPE, &fieldbus_type,
sizeof(fieldbus_type));
fieldbus_type = be16_to_cpu(fieldbus_type);
Probably because the parameter to be16_to_cpu() should be __be16.
Would you like to spin a separate patch for this too? Or shall I?
> I'll send the V2 later today then.
Thank you !
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] staging: fieldbus: anybus-s: use proper type for wait_for_completion_timeout
2019-04-27 7:20 ` Sven Van Asbroeck
@ 2019-04-27 11:17 ` Nicholas Mc Guire
2019-04-27 14:14 ` Sven Van Asbroeck
0 siblings, 1 reply; 6+ messages in thread
From: Nicholas Mc Guire @ 2019-04-27 11:17 UTC (permalink / raw)
To: Sven Van Asbroeck
Cc: Nicholas Mc Guire, Greg Kroah-Hartman, devel, Linux Kernel Mailing List
On Sat, Apr 27, 2019 at 03:20:54AM -0400, Sven Van Asbroeck wrote:
> On Sat, Apr 27, 2019 at 3:01 AM Nicholas Mc Guire <der.herr@hofr.at> wrote:
> > > > (some unrelated sparse warnings (cast to restricted __be16))
> > >
> > > That sounds interesting too. Could you provide more details?
> >
> > make C=1
> > drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted __be16
> > drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted __be16
> > drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted __be16
> > drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted __be16
> > drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted
>
> regmap_bulk_read(cd->regmap, REG_FIELDBUS_TYPE, &fieldbus_type,
i> sizeof(fieldbus_type));
> fieldbus_type = be16_to_cpu(fieldbus_type);
>
> Probably because the parameter to be16_to_cpu() should be __be16.
> Would you like to spin a separate patch for this too? Or shall I?
so the issue is simply that the endiannes anotatoin is missing even
though the conversion is being done - with other words there is no code
lvel funcitonal bug here but rather sparse needs the anotation to verify
correctness and that is missing. Just want to make sure I understand
this before I try to "fix" a sparse warning.
thx!
hofrat
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] staging: fieldbus: anybus-s: use proper type for wait_for_completion_timeout
2019-04-27 11:17 ` Nicholas Mc Guire
@ 2019-04-27 14:14 ` Sven Van Asbroeck
0 siblings, 0 replies; 6+ messages in thread
From: Sven Van Asbroeck @ 2019-04-27 14:14 UTC (permalink / raw)
To: Nicholas Mc Guire
Cc: Nicholas Mc Guire, Greg Kroah-Hartman, devel, Linux Kernel Mailing List
On Sat, Apr 27, 2019 at 7:18 AM Nicholas Mc Guire <der.herr@hofr.at> wrote:
>
> so the issue is simply that the endiannes anotatoin is missing even
> though the conversion is being done - with other words there is no code
> lvel funcitonal bug here but rather sparse needs the anotation to verify
> correctness and that is missing. Just want to make sure I understand
> this before I try to "fix" a sparse warning.
Correct.
Thanks,
Sven
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-04-27 14:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-27 4:26 [PATCH RFC] staging: fieldbus: anybus-s: use proper type for wait_for_completion_timeout Nicholas Mc Guire
2019-04-27 6:17 ` Sven Van Asbroeck
2019-04-27 7:00 ` Nicholas Mc Guire
2019-04-27 7:20 ` Sven Van Asbroeck
2019-04-27 11:17 ` Nicholas Mc Guire
2019-04-27 14:14 ` Sven Van Asbroeck
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).