* [PATCH V2] staging: fieldbus: anybus-s: consolidate wait_for_completion_timeout return handling
@ 2019-04-28 2:33 Nicholas Mc Guire
2019-04-28 2:33 ` [PATCH] staging: fieldbus: anybus-s: force endiannes annotation Nicholas Mc Guire
2019-04-28 14:23 ` [PATCH V2] staging: fieldbus: anybus-s: consolidate wait_for_completion_timeout return handling Sven Van Asbroeck
0 siblings, 2 replies; 4+ messages in thread
From: Nicholas Mc Guire @ 2019-04-28 2:33 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Sven Van Asbroeck, devel, linux-kernel, Nicholas Mc Guire
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().
Signed-off-by: Nicholas Mc Guire <hofrat@osadl.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@gmail.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;
+ }
+
/*
* according to the anybus docs, we're allowed to read these
* without handshaking / reserving the area
--
2.1.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH] staging: fieldbus: anybus-s: force endiannes annotation
2019-04-28 2:33 [PATCH V2] staging: fieldbus: anybus-s: consolidate wait_for_completion_timeout return handling Nicholas Mc Guire
@ 2019-04-28 2:33 ` Nicholas Mc Guire
2019-04-28 14:32 ` Sven Van Asbroeck
2019-04-28 14:23 ` [PATCH V2] staging: fieldbus: anybus-s: consolidate wait_for_completion_timeout return handling Sven Van Asbroeck
1 sibling, 1 reply; 4+ messages in thread
From: Nicholas Mc Guire @ 2019-04-28 2:33 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Sven Van Asbroeck, devel, linux-kernel, Nicholas Mc Guire
While the endiannes is being handled correctly sparse was unhappy with
the missing annotation as be16_to_cpu() expects a __be16.
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);
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH V2] staging: fieldbus: anybus-s: consolidate wait_for_completion_timeout return handling
2019-04-28 2:33 [PATCH V2] staging: fieldbus: anybus-s: consolidate wait_for_completion_timeout return handling Nicholas Mc Guire
2019-04-28 2:33 ` [PATCH] staging: fieldbus: anybus-s: force endiannes annotation Nicholas Mc Guire
@ 2019-04-28 14:23 ` Sven Van Asbroeck
1 sibling, 0 replies; 4+ messages in thread
From: Sven Van Asbroeck @ 2019-04-28 14:23 UTC (permalink / raw)
To: Nicholas Mc Guire; +Cc: Greg Kroah-Hartman, devel, Linux Kernel Mailing List
Looking good, but see inline.
On Sat, Apr 27, 2019 at 10:39 PM Nicholas Mc Guire <hofrat@osadl.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@osadl.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@gmail.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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] staging: fieldbus: anybus-s: force endiannes annotation
2019-04-28 2:33 ` [PATCH] staging: fieldbus: anybus-s: force endiannes annotation Nicholas Mc Guire
@ 2019-04-28 14:32 ` Sven Van Asbroeck
0 siblings, 0 replies; 4+ messages in thread
From: Sven Van Asbroeck @ 2019-04-28 14:32 UTC (permalink / raw)
To: Nicholas Mc Guire; +Cc: Greg Kroah-Hartman, devel, Linux Kernel Mailing List
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-04-28 14:32 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-28 2:33 [PATCH V2] staging: fieldbus: anybus-s: consolidate wait_for_completion_timeout return handling Nicholas Mc Guire
2019-04-28 2:33 ` [PATCH] staging: fieldbus: anybus-s: force endiannes annotation Nicholas Mc Guire
2019-04-28 14:32 ` Sven Van Asbroeck
2019-04-28 14:23 ` [PATCH V2] staging: fieldbus: anybus-s: consolidate wait_for_completion_timeout return handling Sven Van Asbroeck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.