Messages in this thread | | | From | Sven Van Asbroeck <> | Date | Sat, 27 Apr 2019 02:17:42 -0400 | Subject | Re: [PATCH RFC] staging: fieldbus: anybus-s: use proper type for wait_for_completion_timeout |
| |
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; + }
| |