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 <[email protected]>
---
Problem reported by sparse
V2: As requested by Sven Van Asbroeck <[email protected]> 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
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 <[email protected]>
---
Problem located with experimental API conformance checking cocci script
V3: As requested by Sven Van Asbroeck <[email protected]> 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
<[email protected]> - 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
On Mon, Apr 29, 2019 at 2:11 AM Nicholas Mc Guire <[email protected]> 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 <[email protected]>
> ---
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
On Mon, Apr 29, 2019 at 2:11 AM Nicholas Mc Guire <[email protected]> wrote:
>
> V2: As requested by Sven Van Asbroeck <[email protected]> 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
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 <[email protected]> wrote:
> >
> > V2: As requested by Sven Van Asbroeck <[email protected]> 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
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 <[email protected]> wrote:
> > >
> > > V2: As requested by Sven Van Asbroeck <[email protected]> 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.
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 <[email protected]> wrote:
> > > >
> > > > V2: As requested by Sven Van Asbroeck <[email protected]> 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
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.
On Tue, Apr 30, 2019 at 12:19 AM Al Viro <[email protected]> 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?
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 <[email protected]> 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
On Tue, Apr 30, 2019 at 10:03 AM Greg Kroah-Hartman
<[email protected]> 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.
On Tue, Apr 30, 2019 at 10:22 AM Sven Van Asbroeck <[email protected]> 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
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 <[email protected]> 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...