Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1556418804-10266-1-git-send-email-hofrat@osadl.org> <1556418804-10266-2-git-send-email-hofrat@osadl.org>
In-Reply-To: <1556418804-10266-2-git-send-email-hofrat@osadl.org>
From:   Sven Van Asbroeck <thesven73@gmail.com>
Date:   Sun, 28 Apr 2019 10:32:19 -0400
Message-ID: <CAGngYiV6ykYFuq458tWaLqDGLOo41aL5JE2OZ7uFTp3T4KOYkw@mail.gmail.com>
Subject: Re: [PATCH] staging: fieldbus: anybus-s: force endiannes annotation
To:     Nicholas Mc Guire <hofrat@osadl.org>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        devel@driverdev.osuosl.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Thanks for the contibution! See inline.

On Sat, Apr 27, 2019 at 10:39 PM Nicholas Mc Guire <hofrat@osadl.org> wrote:
>
> While the endiannes is being handled correctly sparse was unhappy with
> the missing annotation as be16_to_cpu() expects a __be16.

Your commit message has room for improvement here. See my remarks
on your other patch:
https://lkml.org/lkml/2019/4/28/95

>
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> ---
>
> Problem reported by sparse
>
> As far as I understand sparse here the __force is actually the only
> solution possible to inform sparse that the endiannes handling is ok
>
> Patch was compile-tested with. x86_64_defconfig + FIELDBUS_DEV=m,
> HMS_ANYBUSS_BUS=m
>
> Patch is against 5.1-rc6 (localversion-next is next-20190426)
>
>  drivers/staging/fieldbus/anybuss/host.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/fieldbus/anybuss/host.c b/drivers/staging/fieldbus/anybuss/host.c
> index 6227daf..278acac 100644
> --- a/drivers/staging/fieldbus/anybuss/host.c
> +++ b/drivers/staging/fieldbus/anybuss/host.c
> @@ -1348,7 +1348,7 @@ anybuss_host_common_probe(struct device *dev,
>         add_device_randomness(&val, 4);
>         regmap_bulk_read(cd->regmap, REG_FIELDBUS_TYPE, &fieldbus_type,
>                          sizeof(fieldbus_type));
> -       fieldbus_type = be16_to_cpu(fieldbus_type);
> +       fieldbus_type = be16_to_cpu((__force __be16)fieldbus_type);

Hmm... that would be cheating :)
what if you create a new local variable of type __be16?
Like so:

__be16 fieldbus_type_be;
<...>
regmap_bulk_read(cd->regmap, REG_FIELDBUS_TYPE, &fieldbus_type_be,
                          sizeof(fieldbus_type_be));
fieldbus_type = be16_to_cpu(fieldbus_type_be);

would that get sparse to shut up?

>         dev_info(dev, "Fieldbus type: %04X", fieldbus_type);
>         regmap_bulk_read(cd->regmap, REG_MODULE_SW_V, val, 2);
>         dev_info(dev, "Module SW version: %02X%02X",
> --
> 2.1.4
>