Re: [PATCH RFC] staging: fieldbus: anybus-s: use proper type for wait_for_completion_timeout

[Nicholas Mc Guire]

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@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> >
> > 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