[PATCH V2] staging: fieldbus: anybus-s: force endiannes annotation

[PATCH V2] staging: fieldbus: anybus-s: force endiannes annotation

[Nicholas Mc Guire]

While the endiannes is being handled correctly sparse was unhappy with
the missing annotation as be16_to_cpu() expects a __be16. The __force
cast to __be16 makes sparse happy but has no impact on the generated 
binary.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

Problem reported by sparse

V2: As requested by Sven Van Asbroeck <thesven73@gmail.com> make the
    impact of the patch clear in the commit message.

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 + OF=y, FIELDBUS_DEV=m,
HMS_ANYBUSS_BUS=m

Verification that the patch has no impact on the binary being generated
was done by verifying that the diff of the binaries before and after
applying the patch is empty.


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);
 	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

[PATCH V3] staging: fieldbus: anybus-s: consolidate wait_for_completion_timeout return handling

[Nicholas Mc Guire]

wait_for_completion_timeout() returns unsigned long (0 on timeout or
remaining jiffies) not int - so this type error allows for a
theoretically int overflow - though not in this case where TIMEOUT is
only HZ*2). To fix this type inconsistency the completion is wrapped
into the if() rather than introducing an additional unsigned long
variable. 

Along with fixing this type inconsistency the fall-through if is
consolidated to a single if-block.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

Problem located with experimental API conformance checking cocci script

V3: As requested by Sven Van Asbroeck <thesven73@gmail.com> cleanup
    the commit message to make it more clear what the impact of the
    proposed change is....lets see if this is any better.

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@gmail.com> - This solution also eliminates the need
    to introduce an additional variable - Thanks !

Patch was compile-tested with. x86_64_defconfig + OF=y, 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;
+	}
+
 	/*
 	 * according to the anybus docs, we're allowed to read these
 	 * without handshaking / reserving the area
-- 
2.1.4

Re: [PATCH V3] staging: fieldbus: anybus-s: consolidate wait_for_completion_timeout return handling

[Sven Van Asbroeck]

On Mon, Apr 29, 2019 at 2:11 AM Nicholas Mc Guire <hofrat@osadl.org> wrote:
>
> wait_for_completion_timeout() returns unsigned long (0 on timeout or
> remaining jiffies) not int - so this type error allows for a
> theoretically int overflow - though not in this case where TIMEOUT is
> only HZ*2). To fix this type inconsistency the completion is wrapped
> into the if() rather than introducing an additional unsigned long
> variable.
>
> Along with fixing this type inconsistency the fall-through if is
> consolidated to a single if-block.
>
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> ---

Queued to https://gitlab.com/TheSven73/linux branch: fieldbus-dev-testing,
with my Reviewed-by tag, and the following fix-ups applied:
- tweaked commit subject and message slightly (less is more)
- removed spurious newline

Thank you Nicholas, I really appreciate it !
Sven

Re: [PATCH V2] staging: fieldbus: anybus-s: force endiannes annotation

[Sven Van Asbroeck]

On Mon, Apr 29, 2019 at 2:11 AM Nicholas Mc Guire <hofrat@osadl.org> wrote:
>
> V2: As requested by Sven Van Asbroeck <thesven73@gmail.com> make the
>     impact of the patch clear in the commit message.

Thank you, but did you miss my comment about creating a local variable
instead? See:
https://lkml.org/lkml/2019/4/28/97

Re: [PATCH V2] staging: fieldbus: anybus-s: force endiannes annotation

[Nicholas Mc Guire]

On Mon, Apr 29, 2019 at 10:03:36AM -0400, Sven Van Asbroeck wrote:
> On Mon, Apr 29, 2019 at 2:11 AM Nicholas Mc Guire <hofrat@osadl.org> wrote:
> >
> > V2: As requested by Sven Van Asbroeck <thesven73@gmail.com> make the
> >     impact of the patch clear in the commit message.
> 
> Thank you, but did you miss my comment about creating a local variable
> instead? See:
> https://lkml.org/lkml/2019/4/28/97

Did not miss it - I just don't think that makes it any more
understandable - the __force __be16 makes it clear I believe
that this is correct, sparse does not like this though - so tell
sparse. The local variable would need to be explained as it is
functionally not necessary - therefor I find it more confusing
that using  __force here.

If that rational is wrong let me know.

thx!
hofrat

Re: [PATCH V2] staging: fieldbus: anybus-s: force endiannes annotation

[Al Viro]

On Tue, Apr 30, 2019 at 04:22:38AM +0200, Nicholas Mc Guire wrote:
> On Mon, Apr 29, 2019 at 10:03:36AM -0400, Sven Van Asbroeck wrote:
> > On Mon, Apr 29, 2019 at 2:11 AM Nicholas Mc Guire <hofrat@osadl.org> wrote:
> > >
> > > V2: As requested by Sven Van Asbroeck <thesven73@gmail.com> make the
> > >     impact of the patch clear in the commit message.
> > 
> > Thank you, but did you miss my comment about creating a local variable
> > instead? See:
> > https://lkml.org/lkml/2019/4/28/97
> 
> Did not miss it - I just don't think that makes it any more
> understandable - the __force __be16 makes it clear I believe
> that this is correct, sparse does not like this though - so tell
> sparse.

... to STFU, 'cause you know better.  The trouble is, how do we
(or yourself a year or two later) know *why* it is correct?
Worse, how do we (or yourself, etc.) know if a change about to be
done to the code won't invalidate the proof of yours?

> The local variable would need to be explained as it is
> functionally not necessary - therefor I find it more confusing
> that using  __force here.

What's confusing is mixing host- and fixed-endian values in the
same variable at different times.  Treat those as unrelated
types that happen to have the same sizeof.

Quite a few of __force instances in the tree should be taken out
and shot.  Don't add to their number.

Re: [PATCH V2] staging: fieldbus: anybus-s: force endiannes annotation

[Nicholas Mc Guire]

On Tue, Apr 30, 2019 at 04:02:23AM +0100, Al Viro wrote:
> On Tue, Apr 30, 2019 at 04:22:38AM +0200, Nicholas Mc Guire wrote:
> > On Mon, Apr 29, 2019 at 10:03:36AM -0400, Sven Van Asbroeck wrote:
> > > On Mon, Apr 29, 2019 at 2:11 AM Nicholas Mc Guire <hofrat@osadl.org> wrote:
> > > >
> > > > V2: As requested by Sven Van Asbroeck <thesven73@gmail.com> make the
> > > >     impact of the patch clear in the commit message.
> > > 
> > > Thank you, but did you miss my comment about creating a local variable
> > > instead? See:
> > > https://lkml.org/lkml/2019/4/28/97
> > 
> > Did not miss it - I just don't think that makes it any more
> > understandable - the __force __be16 makes it clear I believe
> > that this is correct, sparse does not like this though - so tell
> > sparse.
> 
> ... to STFU, 'cause you know better.  The trouble is, how do we
> (or yourself a year or two later) know *why* it is correct?
> Worse, how do we (or yourself, etc.) know if a change about to be
> done to the code won't invalidate the proof of yours?
> 
> > The local variable would need to be explained as it is
> > functionally not necessary - therefor I find it more confusing
> > that using  __force here.
> 
> What's confusing is mixing host- and fixed-endian values in the
> same variable at different times.  Treat those as unrelated
> types that happen to have the same sizeof.
> 
> Quite a few of __force instances in the tree should be taken out
> and shot.  Don't add to their number.

ok - my bad thn - I had assumed that using __force is reasonable
if the handling is correct and its a localized conversoin only 
like var = be16_to_cpu(var) which evaded introducing additinal
variables just to have different types but no different function.
But the long-term issue of hiding bugs by __force makes sesne to
me - will give it another shot at scripting this in coccinelle.

thx!
hofrat

Re: [PATCH V2] staging: fieldbus: anybus-s: force endiannes annotation

[Al Viro]

On Tue, Apr 30, 2019 at 05:33:10AM +0200, Nicholas Mc Guire wrote:

> ok - my bad thn - I had assumed that using __force is reasonable
> if the handling is correct and its a localized conversoin only 
> like var = be16_to_cpu(var) which evaded introducing additinal
> variables just to have different types but no different function.

If compiler can't recognize that in

	T1 v1;
	T2 v2;

	code using v1, but not v2
	v2 = f(v1);
	code using v2, but not v1

it can use the same memory for v1 and v2, file a bug against the
compiler.  Or stop using that toy altogether - that kind of
optimizations is early 60s stuff and any real compiler will
handle that.  Both gcc and clang certainly do handle that.

Another thing they handle is figuring out that be16_to_cpu()
et.al. are pure functions, so

	f(be16_to_cpu(n));
	no modifications of n
	g(be16_to_cpu(n));

doesn't need to have le16_to_cpu recalculated.  IOW, that particular
code could as well have been
	dev_info(dev, "Fieldbus type: %04X", be16_to_cpu(fieldbus_type));
	...
	cd->client->fieldbus_type = be16_to_cpu(fieldbus_type);

... not that there's much sense keeping ->fieldbus_type in host-endian,
while we are at it.

Re: [PATCH V2] staging: fieldbus: anybus-s: force endiannes annotation

[Sven Van Asbroeck]

On Tue, Apr 30, 2019 at 12:19 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> ... not that there's much sense keeping ->fieldbus_type in host-endian,
> while we are at it.

Interesting! Suppose we make device->fieldbus_type bus-endian.
Then the endinan-ness conversion either needs to happen in
bus_match() (and we'd have to convert endianness each time
this function is called).
Or, we make driver->fieldbus_type bus-endian also, then there
is no need for conversion... but the driver writer has to remember
to specify this in bus endianness:

static struct anybuss_client_driver profinet_driver = {
        .probe = ...,
        .fieldbus_type = endian convert?? (0x0089),
};

Which pushes bus implementation details onto the
client driver writer? Also, how to convert a constant
to a specific endianness in a static initializer?

You never make a remark without good reason, so what
am I overlooking?

Re: [PATCH V2] staging: fieldbus: anybus-s: force endiannes annotation

[Greg Kroah-Hartman]

On Tue, Apr 30, 2019 at 09:32:20AM -0400, Sven Van Asbroeck wrote:
> On Tue, Apr 30, 2019 at 12:19 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > ... not that there's much sense keeping ->fieldbus_type in host-endian,
> > while we are at it.
> 
> Interesting! Suppose we make device->fieldbus_type bus-endian.

Keep it bus-endian, as that's the "normal" way bus structures work (like
PCI and USB for example), and that should be in a documented, and
consistent, form, right?

Then do the conversion when you access the field from within the kernel.
Again, examples of USB and PCI can be used if you want to copy what they
do.

thanks,

greg k-h

Re: [PATCH V2] staging: fieldbus: anybus-s: force endiannes annotation

[Sven Van Asbroeck]

On Tue, Apr 30, 2019 at 10:03 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> Keep it bus-endian, as that's the "normal" way bus structures work (like
> PCI and USB for example), and that should be in a documented, and
> consistent, form, right?
>
> Then do the conversion when you access the field from within the kernel.

Ah ok, it's like a standard way of implementing a bus. Sounds good, I'll
spin a patch to conform to it. And while I'm at it, I'll rename fieldbus_type
because it can be confused with another fieldbus_type within the
fieldbus_dev core.

Re: [PATCH V2] staging: fieldbus: anybus-s: force endiannes annotation

[Sven Van Asbroeck]

On Tue, Apr 30, 2019 at 10:22 AM Sven Van Asbroeck <thesven73@gmail.com> wrote:
>
> Ah ok, it's like a standard way of implementing a bus. Sounds good, I'll
> spin a patch to conform to it. And while I'm at it, I'll rename fieldbus_type
> because it can be confused with another fieldbus_type within the
> fieldbus_dev core.

Nicholas, this future patch will silence sparse. So you can drop the
patch you proposed at the beginning of this email thread.

Thanks for your help, really appreciate it.
Sven

Re: [PATCH V2] staging: fieldbus: anybus-s: force endiannes annotation

[Al Viro]

On Tue, Apr 30, 2019 at 09:32:20AM -0400, Sven Van Asbroeck wrote:
> On Tue, Apr 30, 2019 at 12:19 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > ... not that there's much sense keeping ->fieldbus_type in host-endian,
> > while we are at it.
> 
> Interesting! Suppose we make device->fieldbus_type bus-endian.
> Then the endinan-ness conversion either needs to happen in
> bus_match() (and we'd have to convert endianness each time
> this function is called).
> Or, we make driver->fieldbus_type bus-endian also, then there
> is no need for conversion... but the driver writer has to remember
> to specify this in bus endianness:
> 
> static struct anybuss_client_driver profinet_driver = {
>         .probe = ...,
>         .fieldbus_type = endian convert?? (0x0089),
> };
> 
> Which pushes bus implementation details onto the
> client driver writer? Also, how to convert a constant
> to a specific endianness in a static initializer?

cpu_to_be16() or htons() - either will be fine there.
On little-endian you'll get
	htons(0x0089) =>
	___htons(0x0089) =>
	__cpu_to_be16(0x0089) =>
	((__force __be16)__swab16((0x0089))) =>
	((__be16)(__builtin_constant_p((__u16)((0x0089))) ?
		___constant_swab16((0x0089)) : __fswab16((0x0089))) =>
	((__be16)(__builtin_constant_p((__u16)((0x0089))) ?
		((__u16)((((__u16)((0x0089)) & (__u16)0x00ffU) << 8) |
			 (((__u16)((0x0089)) & (__u16)0xff00U) >> 8))) :
		__fswab16((0x0089)))
and once the preprocessor has produced that, from compiler POV we have
a constant expression as argument of __builtin_constant_p(), so it
evaluates as true, reducing the whole thing to
	((__be16)(((__u16)((((__u16)((0x0089)) & (__u16)0x00ffU) << 8) |
			 (((__u16)((0x0089)) & (__u16)0xff00U) >> 8))) )
i.e. (__be16)0x8900.  On big-endian expansion will be different,
resulting in (__be16)0x0089...

IOW, you can use endianness convertors in static initializers; things
like
struct sockaddr_in addr = {.sin_addr.s_addr = htonl(0x7f000001),
			 .sin_port = htons(25),
			 .sin_family = AF_INET};
are fine kernel-side (from the compiler POV, that is - something
trying to speak SMTP in the kernel code would obviously be a bad sign).

As for having to remember - sparse will complain about endianness mismatches
in initializer...