linux-kernel - Re: [PATCH V2] staging: fieldbus: anybus-s: consolidate wait_for_completion

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGngYiXMMKz2AesEAVO_wdXw0ixsnjNo0o920evZPL9CM0cdJQ@mail.gmail.com>
Date:   Sun, 28 Apr 2019 10:23:43 -0400
From:   Sven Van Asbroeck <thesven73@...il.com>
To:     Nicholas Mc Guire <hofrat@...dl.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        devel@...verdev.osuosl.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2] staging: fieldbus: anybus-s: consolidate
 wait_for_completion_timeout return handling

Looking good, but see inline.

On Sat, Apr 27, 2019 at 10:39 PM Nicholas Mc Guire <hofrat@...dl.org> wrote:
>
> wait_for_completion_timeout() returns unsigned long (0 on timeout or
> remaining jiffies) not int - so rather than introducing an additional
> variable simply wrap the completion into an if().

Your commit message could be improved:

- the headline should make clear what this is, e.g. add functionality,
bugfix, shutting up sparse, etc. Using the verb 'fix' would be
good here.

- in case of a bugfix, it would make sense to write a short
paragraph about what can go wrong, followed by a short
paragraph outlining what the patch does to fix it.

>
> Signed-off-by: Nicholas Mc Guire <hofrat@...dl.org>
> ---
>
> Problem located with experimental API conformance checking cocci script
>
> V2: The original patch's logic was wrong as it was skipping the
>     fall-through if so using the fix proposed by Sven Van Asbroeck
>     <thesven73@...il.com> - This solution also eliminates the need
>     to introduce an additional variable - Thanks !
>
> Patch was compile-tested with. x86_64_defconfig + FIELDBUS_DEV=m,
> HMS_ANYBUSS_BUS=m
> (with an unrelated sparse warnings (cast to restricted __be16))
>
> Patch is against 5.1-rc6 (localversion-next is next-20190426)
>
>  drivers/staging/fieldbus/anybuss/host.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/fieldbus/anybuss/host.c b/drivers/staging/fieldbus/anybuss/host.c
> index e34d424..6227daf 100644
> --- a/drivers/staging/fieldbus/anybuss/host.c
> +++ b/drivers/staging/fieldbus/anybuss/host.c
> @@ -1325,11 +1325,11 @@ 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)
> +       if (!wait_for_completion_timeout(&cd->card_boot, TIMEOUT)) {
>                 ret = -ETIMEDOUT;
> -       if (ret < 0)
>                 goto err_reset;
> +       }
> +

Nit: why add a blank line here?

>         /*
>          * according to the anybus docs, we're allowed to read these
>          * without handshaking / reserving the area
> --
> 2.1.4
>