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 <[email protected]>
---
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
<[email protected]> - 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
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 <[email protected]>
---
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
Looking good, but see inline.
On Sat, Apr 27, 2019 at 10:39 PM Nicholas Mc Guire <[email protected]> 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 <[email protected]>
> ---
>
> 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
> <[email protected]> - 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
>
Thanks for the contibution! See inline.
On Sat, Apr 27, 2019 at 10:39 PM Nicholas Mc Guire <[email protected]> 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 <[email protected]>
> ---
>
> 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
>