Re: [PATCH] staging: bcm2835-camera: Avoid apotential sleep while holding a spin_lock

[Nicholas Mc Guire]

On Mon, Jun 24, 2019 at 07:33:51AM +0200, Christophe JAILLET wrote:
> Do not allocate memory with GFP_KERNEL when holding a spin_lock, it may
> sleep. Use GFP_NOWAIT instead.
>

checking for this in the rest of the kernel with a cocci spatch

virtual report

@nonatomic@
position p;
identifier var;
@@

  spin_lock(...)
  ... when != spin_unlock(...)
* var = idr_alloc@p(...,GFP_KERNEL);
  ... when != spin_unlock(...)
  spin_unlock(...);

this seems to be the only instance of this specific problem.

> Fixes: 950fd867c635 ("staging: bcm2835-camera: Replace open-coded idr with a 
> struct idr.")

The GFP_KERNEL actually was there befor this patch so not sure if this Fixes
ref is correct - I think the GFP_KERNEL was introduced in:
4e6bafdfb9f3 ("staging: bcm2835_camera: Use a mapping table for context field 
of mmal_msg_header")

> Signed-off-by: Christophe JAILLET 
Reviewed-by: Nicholas Mc Guire 

> ---
>  drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c 
> b/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c
> index 16af735af5c3..438d548c6e24 100644
> --- a/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c
> +++ b/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c
> @@ -186,7 +186,7 @@ get_msg_context(struct vchiq_mmal_instance *instance)
>*/
>   spin_lock(>context_map_lock);
>   handle = idr_alloc(>context_map, msg_context,
> -0, 0, GFP_KERNEL);
> +0, 0, GFP_NOWAIT);
>   spin_unlock(>context_map_lock);
>  
>   if (handle < 0) {
> -- 
> 2.20.1
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: greybus: use proper return type for wait_for_completion_timeout

[Nicholas Mc Guire]

On Tue, Apr 30, 2019 at 12:58:21PM +0300, Dan Carpenter wrote:
> On Sat, Apr 27, 2019 at 05:27:25AM +0200, Nicholas Mc Guire wrote:
> > wait_for_completion_timeout() returns unsigned long (0 on timeout or
> > remaining jiffies) not int. 
> > 
> 
> Yeah, but it's fine though because 1 / 256 fits into int without a
> problem.
> 
> I'm not sure this sort of patch is worth it when it's just a style
> debate instead of a bugfix.  I'm a little bit torn about this.  In
> Smatch, I run into this issue one in a while where Smatch doesn't know
> if the timeout is less than int.  Right now I hacked the DB to say that
> these functions always return < INT_MAX.
> 
> Anyway, for sure the commit message should say that it's just a cleanup
> and not a bugfix.
>
I know its not a functional bug its "only" an API violation - the problem
is more that code is often cut and at some point it may be a 
problem or someoe expects a negative return value without that this evef
can occure.

But yes - the commit message should have stated that this non-conformance
in this case has no effect - will resend.

thx!
hofrat
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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  
> > > wrote:
> > > >
> > > > V2: As requested by Sven Van Asbroeck  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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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  wrote:
> >
> > V2: As requested by Sven Van Asbroeck  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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[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 
---

Problem reported by sparse

V2: As requested by Sven Van Asbroeck  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(, 4);
regmap_bulk_read(cd->regmap, REG_FIELDBUS_TYPE, _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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[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 
---

Problem located with experimental API conformance checking cocci script

V3: As requested by Sven Van Asbroeck  cleanup
the commit message to make it more clear what the impact of the
proposed change islets 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 
 - 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(>card_boot, TIMEOUT);
-   if (ret == 0)
+   if (!wait_for_completion_timeout(>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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] 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. 

Signed-off-by: Nicholas Mc Guire 
---

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(, 4);
regmap_bulk_read(cd->regmap, REG_FIELDBUS_TYPE, _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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH V2] 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 rather than introducing an additional
variable simply wrap the completion into an if().

Signed-off-by: Nicholas Mc Guire 
---

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 
 - 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(>card_boot, TIMEOUT);
-   if (ret == 0)
+   if (!wait_for_completion_timeout(>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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RFC] staging: fieldbus: anybus-s: use proper type for wait_for_completion_timeout

[Nicholas Mc Guire]

On Sat, Apr 27, 2019 at 03:20:54AM -0400, Sven Van Asbroeck wrote:
> On Sat, Apr 27, 2019 at 3:01 AM Nicholas Mc Guire  wrote:
> > > > (some unrelated sparse warnings (cast to restricted __be16))
> > >
> > > That sounds interesting too. Could you provide more details?
> >
> > make C=1
> > drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to 
> > restricted __be16
> > drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to 
> > restricted __be16
> > drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to 
> > restricted __be16
> > drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to 
> > restricted __be16
> > drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted
> 
> regmap_bulk_read(cd->regmap, REG_FIELDBUS_TYPE, _type,
i> sizeof(fieldbus_type));
> fieldbus_type = be16_to_cpu(fieldbus_type);
> 
> Probably because the parameter to be16_to_cpu() should be __be16.
> Would you like to spin a separate patch for this too? Or shall I?

so the issue is simply that the endiannes anotatoin is missing even 
though the conversion is being done - with other words there is no code
lvel funcitonal bug here but rather sparse needs the anotation to verify
correctness and that is missing. Just want to make sure I understand
this before I try to "fix" a sparse warning.

thx!
hofrat
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH RFC V2] staging: kpc2000: use int for wait_for_completion_interruptible

[Nicholas Mc Guire]

weit_for_completion_interruptible returns in (0 on completion and 
-ERESTARTSYS on interruption) - so use an int not long for API conformance
and simplify the logic here a bit: need not check explicitly for == 0 as
this is either -ERESTARTSYS or 0.

Signed-off-by: Nicholas Mc Guire 
---

Problem located with experimental API conformance checking cocci script

V2: kbuild reported a missing closing comment - seems that I managed
send out the the initial version before compile testing/checkpatching
sorry for the noise.

Not sure if making such point-wise fixes makes much sense - this driver has
a number of issues both style-wise and API compliance wise.

Note that kpc_dma_transfer() returns int not long - currently rv (long) is
being returned in most places (some places do return int) - so the return
handling here is a bit inconsistent.

Patch was compile-tested with: x86_64_defconfig + KPC2000=y, KPC2000_DMA=y
(with a number of unrelated sparse warnings about non-declared symbols, and
 smatch warnings about overflowing constants as well as coccicheck warning
 about usless casting)

Patch is against 5.1-rc6 (localversion-next is next-20190426)

 drivers/staging/kpc2000/kpc_dma/fileops.c | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c 
b/drivers/staging/kpc2000/kpc_dma/fileops.c
index 5741d2b..66f0d5a 100644
--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
@@ -38,6 +38,7 @@ int  kpc_dma_transfer(struct dev_private_data *priv, struct 
kiocb *kcb, unsigned
 {
unsigned int i = 0;
long rv = 0;
+   int ret = 0;
struct kpc_dma_device *ldev;
struct aio_cb_data *acd;
DECLARE_COMPLETION_ONSTACK(done);
@@ -180,16 +181,17 @@ int  kpc_dma_transfer(struct dev_private_data *priv, 
struct kiocb *kcb, unsigned

// If this is a synchronous kiocb, we need to put the calling process 
to sleep until the transfer is complete
if (kcb == NULL || is_sync_kiocb(kcb)){
-   rv = wait_for_completion_interruptible();
-   // If the user aborted (rv == -ERESTARTSYS), we're no longer 
responsible for cleaning up the acd
-   if (rv == -ERESTARTSYS){
+   ret = wait_for_completion_interruptible();
+   /* If the user aborted (ret == -ERESTARTSYS), we're
+* no longer responsible for cleaning up the acd
+*/
+   if (ret) {
acd->cpl = NULL;
-   }
-   if (rv == 0){
-   rv = acd->len;
+   } else {
+   ret = acd->len;
kfree(acd);
}
-   return rv;
+   return ret;
}

return -EIOCBQUEUED;
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RFC] staging: fieldbus: anybus-s: use proper type for wait_for_completion_timeout

[Nicholas Mc Guire]

On Sat, Apr 27, 2019 at 02:17:42AM -0400, Sven Van Asbroeck wrote:
> Hello Nicholas, thank you for your contribution, I really appreciate it !
> See inline comments below.
> 
> On Sat, Apr 27, 2019 at 12:32 AM Nicholas Mc Guire  wrote:
> >
> > wait_for_completion_timeout() returns unsigned long (0 on timeout or
> > remaining jiffies) not int.
> 
> Nice catch !
> 
> > thus there is no negative case to check for
> > here.
> 
> The current code can only break if wait_for_completion_timeout()
> returns an unsigned long large enough to make the "int ret" turn
> negative. This could result in probe() returning a nonsensical error
> value, even though the wait did not time out.
> 
> Fortunately that currently cannot happen, because TIMEOUT
> (2*HZ) won't overflow an int.

ok - thats benign then -  never the less since code tends to get copied 
it would be good to make it comply with API spec

> 
> That being said, this return value type mismatch should definitely
> be fixed up.
> 
> >
> > Signed-off-by: Nicholas Mc Guire 
> > ---
> >
> > Problem located with experimental API conformance checking cocci script
> >
> > Q: It is not really clear if the proposed fix is the right thing here or if
> >this should not be using wait_for_completion_interruptible - which would
> >return -ERESTARTSYS on interruption. Someone that knows the details of
> >this driver would need to check what condition should initiate the
> >goto err_reset; which was actually unreachable in the current code.
> 
> It's used in probe(), so no need for an interruptible wait, AFAIK.
> It can stay as-is.
> 
> >
> > Patch was compile-tested with. x86_64_defconfig + FIELDBUS_DEV=m,
> > HMS_ANYBUSS_BUS=m
> > (some unrelated sparse warnings (cast to restricted __be16))
> 
> That sounds interesting too. Could you provide more details?

make C=1
drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted 
__be16
drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted 
__be16
drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted 
__be16
drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted 
__be16
drivers/staging/fieldbus/anybuss/host.c:1350:25: warning: cast to restricted


> 
> 
> 
> > -   ret = wait_for_completion_timeout(>card_boot, TIMEOUT);
> > -   if (ret == 0)
> > +   time_left = wait_for_completion_timeout(>card_boot, TIMEOUT);
> > +   if (time_left == 0)
> > ret = -ETIMEDOUT;
> > -   if (ret < 0)
> > -   goto err_reset;
> 
> NAK. This does not jump to err_reset on timeout.
> 
> May I suggest the following instead ? (manual formatting)
> 
> - ret = wait_for_completion_timeout(>card_boot, TIMEOUT);
> - if (ret == 0)
> - ret = -ETIMEDOUT;
> - if (ret < 0)
> - goto err_reset;
> + if (!wait_for_completion_timeout(>card_boot, TIMEOUT)) {
> + ret = -ETIMEDOUT;
> + goto err_reset;
> + }

Ah - ok - that was the bit missing for me !
how that goto branch would be reached or if it should be
dropped as it had not been reachable in the past.

I'll send the V2 later today then.

thx!
hofrat

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH RFC] staging: fieldbus: anybus-s: use proper type for wait_for_completion_timeout

[Nicholas Mc Guire]

wait_for_completion_timeout() returns unsigned long (0 on timeout or
remaining jiffies) not int. thus there is no negative case to check for 
here.

Signed-off-by: Nicholas Mc Guire 
---

Problem located with experimental API conformance checking cocci script

Q: It is not really clear if the proposed fix is the right thing here or if
   this should not be using wait_for_completion_interruptible - which would
   return -ERESTARTSYS on interruption. Someone that knows the details of
   this driver would need to check what condition should initiate the
   goto err_reset; which was actually unreachable in the current code.

Patch was compile-tested with. x86_64_defconfig + FIELDBUS_DEV=m,
HMS_ANYBUSS_BUS=m
(some unrelated sparse warnings (cast to restricted __be16))

Patch is against 5.1-rc6 (localversion-next is next-20190426)

 drivers/staging/fieldbus/anybuss/host.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/fieldbus/anybuss/host.c 
b/drivers/staging/fieldbus/anybuss/host.c
index e34d424..c52d715 100644
--- a/drivers/staging/fieldbus/anybuss/host.c
+++ b/drivers/staging/fieldbus/anybuss/host.c
@@ -1263,6 +1263,7 @@ anybuss_host_common_probe(struct device *dev,
  const struct anybuss_ops *ops)
 {
int ret, i;
+   unsigned long time_left;
u8 val[4];
u16 fieldbus_type;
struct anybuss_host *cd;
@@ -1325,11 +1326,9 @@ anybuss_host_common_probe(struct device *dev,
 *   interrupt came in: ready to go !
 */
reset_deassert(cd);
-   ret = wait_for_completion_timeout(>card_boot, TIMEOUT);
-   if (ret == 0)
+   time_left = wait_for_completion_timeout(>card_boot, TIMEOUT);
+   if (time_left == 0)
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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: greybus: use proper return type for wait_for_completion_timeout

[Nicholas Mc Guire]

wait_for_completion_timeout() returns unsigned long (0 on timeout or
remaining jiffies) not int. 

Signed-off-by: Nicholas Mc Guire 
---
Problem located with experimental API conformance checking cocci script

Patch was compile-tested with: x86_64_defconfig + GREYBUS=m

Patch is against 5.1-rc6 (localversion-next is next-20190426)

 drivers/staging/greybus/uart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c
index b3bffe9..ff18112 100644
--- a/drivers/staging/greybus/uart.c
+++ b/drivers/staging/greybus/uart.c
@@ -323,7 +323,7 @@ static int send_break(struct gb_tty *gb_tty, u8 state)
 
 static int gb_uart_wait_for_all_credits(struct gb_tty *gb_tty)
 {
-   int ret;
+   unsigned long ret;
 
if (gb_tty->credits == GB_UART_FIRMWARE_CREDITS)
return 0;
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH RFC] staging: kpc2000: use int for wait_for_completion_interruptible

[Nicholas Mc Guire]

weit_for_completion_interruptible returns in (0 on completion and 
-ERESTARTSYS on interruption) - so use an int not long for API conformance
and simplify the logic here a bit: need not check explicitly for == 0 as
this is either -ERESTARTSYS or 0.

Signed-off-by: Nicholas Mc Guire 
---

Problem located with experimental API conformance checking cocci script

Not sure if making such point-wise fixes makes much sense - this driver has
a number of issues both style-wise and API compliance wise.

Note that kpc_dma_transfer() returns int not long - currently rv (long) is
being returned in most places (some places do return int) - so the return
handling here is a bit inconsistent.

Patch was compile-tested with: x86_64_defconfig + KPC2000=y, KPC2000_DMA=y
(with a number of unrelated sparse warnings about non-declared symbols, and
 smatch warnings about overflowing constants as well as coccicheck warning
 about usless casting)

Patch is against 5.1-rc6 (localversion-next is next-20190426)

 drivers/staging/kpc2000/kpc_dma/fileops.c | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c 
b/drivers/staging/kpc2000/kpc_dma/fileops.c
index 5741d2b..66f0d5a 100644
--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
@@ -38,6 +38,7 @@ int  kpc_dma_transfer(struct dev_private_data *priv, struct 
kiocb *kcb, unsigned
 {
unsigned int i = 0;
long rv = 0;
+   int ret = 0;
struct kpc_dma_device *ldev;
struct aio_cb_data *acd;
DECLARE_COMPLETION_ONSTACK(done);
@@ -180,16 +181,17 @@ int  kpc_dma_transfer(struct dev_private_data *priv, 
struct kiocb *kcb, unsigned

// If this is a synchronous kiocb, we need to put the calling process 
to sleep until the transfer is complete
if (kcb == NULL || is_sync_kiocb(kcb)){
-   rv = wait_for_completion_interruptible();
-   // If the user aborted (rv == -ERESTARTSYS), we're no longer 
responsible for cleaning up the acd
-   if (rv == -ERESTARTSYS){
+   ret = wait_for_completion_interruptible();
+   /* If the user aborted (ret == -ERESTARTSYS), we're
+* no longer responsible for cleaning up the acd
+*
+   if (ret){
acd->cpl = NULL;
-   }
-   if (rv == 0){
-   rv = acd->len;
+   } else {
+   ret = acd->len;
kfree(acd);
}
-   return rv;
+   return ret
}

return -EIOCBQUEUED;
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH V2] staging: vc04_services: handle kzalloc failure

[Nicholas Mc Guire]

The kzalloc here was being used without checking the return - if the
kzalloc fails return VCHIQ_ERROR. The call-site of
vchiq_platform_init_state() vchiq_init_state() was not responding
to an allocation failure so checks for != VCHIQ_SUCCESS
and pass VCHIQ_ERROR up to vchiq_platform_init() which then
will fail with -EINVAL.

Signed-off-by: Nicholas Mc Guire 
Reported-by: kbuild test robot 
---

Problem located with experimental coccinelle script

V2: The != VCHIQ_SUCCES went unnoticed not clear how I did that as
building drivers/staging/vc04_services/vchiq.o seemed to have succeeded
anyway kbuild test found it. This is one of the disadvantages if one
can not make path/file.o due to missing Makefiles and is required to
build entire directories for compile-testing, I suspect that I did not
clean the directory before compile-testing the patch.


Patch was compile-tested with: bcm2835_defconfig (implies BCM2835_VCHIQ=m)
(with sparse warning:
  CHECK   drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c:410:29: 
warning: incorrect type in argument 1 (different address spaces)
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c:410:29:
expected void const *x
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c:410:29:
got char [noderef]  *buf
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c:416:63: 
warning: incorrect type in argument 1 (different address spaces)
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c:416:63:
expected void const *addr
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c:416:63:
got char [noderef]  *
  CC [M]  drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.o
Q: should this buf have been remapped to user space e.g. remap_vmalloc_range ?

Patch is against 5.1-rc5 (localversion next is 20190417)

 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c | 3 +++
 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c 
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index 31eb7d5..a9a2291 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -179,6 +179,9 @@ vchiq_platform_init_state(struct vchiq_state *state)
struct vchiq_2835_state *platform_state;
 
state->platform_state = kzalloc(sizeof(*platform_state), GFP_KERNEL);
+   if (!state->platform_state)
+   return VCHIQ_ERROR;
+
platform_state = (struct vchiq_2835_state *)state->platform_state;
 
platform_state->inited = 1;
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c 
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 6057a90..92e6221 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -2209,6 +2209,8 @@ vchiq_init_state(struct vchiq_state *state, struct 
vchiq_slot_zero *slot_zero)
local->debug[DEBUG_ENTRIES] = DEBUG_MAX;
 
status = vchiq_platform_init_state(state);
+   if (status != VCHIQ_SUCCESS)
+   return VCHIQ_ERROR;
 
/*
bring up slot handler thread
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH RFC] staging: vc04_services: handle kzalloc failure

[Nicholas Mc Guire]

The kzalloc here was being used without checking the return - if the
kzalloc fails return VCHIQ_ERROR. The call-site of
vchiq_platform_init_state() vchiq_init_state() was not responding
to an allocation failure so checks for != VCHIQ_SUCCESS
and pass VCHIQ_ERROR up to vchiq_platform_init() which then
will fail with -EINVAL.

Signed-off-by: Nicholas Mc Guire 
---

Problem located with experimental coccinelle script

Patch was compile-tested with: bcm2835_defconfig (implies BCM2835_VCHIQ=m)
(with sparse warning:
  CHECK   drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c:410:29: 
warning: incorrect type in argument 1 (different address spaces)
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c:410:29:
expected void const *x
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c:410:29:
got char [noderef]  *buf
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c:416:63: 
warning: incorrect type in argument 1 (different address spaces)
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c:416:63:
expected void const *addr
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c:416:63:
got char [noderef]  *
  CC [M]  drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.o
Q: should this buf have been remapped to user space e.g. remap_vmalloc_range ?

Patch is against 5.1-rc5 (localversion next is 20190417)

 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c | 3 +++
 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c 
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index 31eb7d5..a9a2291 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -179,6 +179,9 @@ vchiq_platform_init_state(struct vchiq_state *state)
struct vchiq_2835_state *platform_state;
 
state->platform_state = kzalloc(sizeof(*platform_state), GFP_KERNEL);
+   if (!state->platform_state)
+   return VCHIQ_ERROR;
+
platform_state = (struct vchiq_2835_state *)state->platform_state;
 
platform_state->inited = 1;
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c 
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 6057a90..92e6221 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -2209,6 +2209,8 @@ vchiq_init_state(struct vchiq_state *state, struct 
vchiq_slot_zero *slot_zero)
local->debug[DEBUG_ENTRIES] = DEBUG_MAX;
 
status = vchiq_platform_init_state(state);
+   if (status != VCHIQ_SUCCES)
+   return VCHIQ_ERROR;
 
/*
bring up slot handler thread
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH V2] staging: wilc1000: give usleep_range a range

[Nicholas Mc Guire]

From: Nicholas Mc Guire 

usleep_range() is called in non-atomic context so there is little point
in setting min==max as the jitter of hrtimer is determined by interruptions
anyway. usleep_range can only perform the intended coalescence if some
room for placing the hrtimer is provided. Given the range of milliseconds
the delay will be 2+ anyway - so make it 2-2.5 ms which gives hrtimers
space to optimize without negatively impacting performance.

Signed-off-by: Nicholas Mc Guire 
---

Problem located with an experimental coccinelle script
./drivers/staging/wilc1000/wilc_wlan.c:411:4-16: WARNING: inefficient 
usleep_range with range 0 (min==max)
./drivers/staging/wilc1000/wilc_wlan.c:426:4-16: WARNING: inefficient 
usleep_range with range 0 (min==max)

V2: Based on feedback from Adham Abozaeid  that
since this is in the transmit path 5ms could impact throughput but adding
500 microseconds should be tolerable.

Patch was compile tested with: x86_64_defconfig + Staging=y,
WILC1000_SDIO=m, WILC1000_SPI=m, WILC1000=m

Patch is against 5.1-rc4 (localversion-next is -next-20190410)

 drivers/staging/wilc1000/wilc_wlan.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_wlan.c 
b/drivers/staging/wilc1000/wilc_wlan.c
index c238969..42da533 100644
--- a/drivers/staging/wilc1000/wilc_wlan.c
+++ b/drivers/staging/wilc1000/wilc_wlan.c
@@ -408,7 +408,7 @@ void chip_wakeup(struct wilc *wilc)
wilc->hif_func->hif_write_reg(wilc, 1, reg & ~BIT(1));
 
do {
-   usleep_range(2 * 1000, 2 * 1000);
+   usleep_range(2000, 2500);
wilc_get_chipid(wilc, true);
} while (wilc_get_chipid(wilc, true) == 0);
} while (wilc_get_chipid(wilc, true) == 0);
@@ -423,7 +423,7 @@ void chip_wakeup(struct wilc *wilc)
 _status_reg);
 
while ((clk_status_reg & 0x1) == 0) {
-   usleep_range(2 * 1000, 2 * 1000);
+   usleep_range(2000, 2500);
 
wilc->hif_func->hif_read_reg(wilc, 0xf1,
 _status_reg);
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RFC] staging: wilc1000: give usleep_range a range

[Nicholas Mc Guire]

On Wed, Apr 10, 2019 at 06:31:21PM +, adham.aboza...@microchip.com wrote:
> Hi Nicolas
> 
> On 4/8/19 6:36 PM, Nicholas Mc Guire wrote:
> > On Mon, Apr 08, 2019 at 09:10:00PM +, adham.aboza...@microchip.com 
> > wrote:
> >> Hi Nicholas
> >>
> >> On 4/6/19 5:01 AM, Nicholas Mc Guire wrote:
> >>> External E-Mail
> >>>
> >>>
> >>> Someone that knows the motivation for setting the time to 2 millisecond
> >>> might need to check if the 2 milliseconds where seen as tollerable max or
> >>> min - I'm assuming it was the min so extending.
> >> 2 msec is the time the chip takes to wake up from sleep.
> >>
> >> Increasing the maximum to 5 msec will impact the throughput since this 
> >> call is on the transmit path.
> >>
> > ok - would it be tollerable to make it 2 - 2.5 ms ?
> > even that would allow for the hrtimer subsystem to optimize
> > a lot. In any case the min==max case gives you very little
> > if you run a test-case with usleep_range(1000,1000) and
> > a loop with usleep_range(1000,2000) and look at the distribution
> > you will have a hard time seeing any difference.
> 
> yes, I believe 2.5 shouldn't be a problem.
>
thanks - will send out a V2 then shortly.

thx!
hofrat 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RFC] staging: wilc1000: give usleep_range a range

[Nicholas Mc Guire]

On Mon, Apr 08, 2019 at 09:10:00PM +, adham.aboza...@microchip.com wrote:
> Hi Nicholas
> 
> On 4/6/19 5:01 AM, Nicholas Mc Guire wrote:
> > External E-Mail
> >
> >
> > Someone that knows the motivation for setting the time to 2 millisecond
> > might need to check if the 2 milliseconds where seen as tollerable max or
> > min - I'm assuming it was the min so extending.
> 
> 2 msec is the time the chip takes to wake up from sleep.
> 
> Increasing the maximum to 5 msec will impact the throughput since this call 
> is on the transmit path.
> 

ok - would it be tollerable to make it 2 - 2.5 ms ?
even that would allow for the hrtimer subsystem to optimize
a lot. In any case the min==max case gives you very little
if you run a test-case with usleep_range(1000,1000) and
a loop with usleep_range(1000,2000) and look at the distribution
you will have a hard time seeing any difference.

I doubt you would readily see the change from usleep_range(2000,2000)
to usleep_range(2000,3000) in benchmarks - maybe (2000,5000) would
be visible.

My assumption (I have not analyzed it in detail) is that if
you have a high re-use of existing timers that the setup of the timer
is faster and thats why increasing the range > 0 can actually result
in better jitter distribution.

thx!
hofrat

> > diff --git a/drivers/staging/wilc1000/wilc_wlan.c 
> > b/drivers/staging/wilc1000/wilc_wlan.c
> > index c238969..42da533 100644
> > --- a/drivers/staging/wilc1000/wilc_wlan.c
> > +++ b/drivers/staging/wilc1000/wilc_wlan.c
> > @@ -408,7 +408,7 @@ void chip_wakeup(struct wilc *wilc)
> > wilc->hif_func->hif_write_reg(wilc, 1, reg & ~BIT(1));
> >  
> > do {
> > -   usleep_range(2 * 1000, 2 * 1000);
> > +   usleep_range(2 * 1000, 5 * 1000);
> > wilc_get_chipid(wilc, true);
> > } while (wilc_get_chipid(wilc, true) == 0);
> > } while (wilc_get_chipid(wilc, true) == 0);
> > @@ -423,7 +423,7 @@ void chip_wakeup(struct wilc *wilc)
> >  _status_reg);
> >  
> > while ((clk_status_reg & 0x1) == 0) {
> > -   usleep_range(2 * 1000, 2 * 1000);
> > +   usleep_range(2 * 1000, 5 * 1000);
> >  
> > wilc->hif_func->hif_read_reg(wilc, 0xf1,
> >  _status_reg);
> 
> 
> Thanks,
> 
> Adham
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH RFC] staging: wilc1000: give usleep_range a range

[Nicholas Mc Guire]

usleep_range() is called in non-atomic context so there is little point
in setting min==max the jitter of hrtimer is determined by interruptions
anyway. usleep_range can only perform the intended coalescence if some
room for placing the hrtimer is provided. Given the range of milliseconds
the delay will be anything from 2 to a few anyway - so make it 2-5 ms.

Signed-off-by: Nicholas Mc Guire 
---

Problem located with an experimental coccinelle script
./drivers/staging/wilc1000/wilc_wlan.c:411:4-16: WARNING: inefficient 
usleep_range with range 0 (min==max)
./drivers/staging/wilc1000/wilc_wlan.c:426:4-16: WARNING: inefficient 
usleep_range with range 0 (min==max)

Someone that knows the motivation for setting the time to 2 millisecond
might need to check if the 2 milliseconds where seen as tollerable max or
min - I'm assuming it was the min so extending.

Patch was compile tested with: x86_64_defconfig + Staging=y,
WILC1000_SDIO=m, WILC1000_SPI=m, WILC1000=m

Patch is against 5.1-rc3 (localversion-next is -next-20190405)

 drivers/staging/wilc1000/wilc_wlan.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_wlan.c 
b/drivers/staging/wilc1000/wilc_wlan.c
index c238969..42da533 100644
--- a/drivers/staging/wilc1000/wilc_wlan.c
+++ b/drivers/staging/wilc1000/wilc_wlan.c
@@ -408,7 +408,7 @@ void chip_wakeup(struct wilc *wilc)
wilc->hif_func->hif_write_reg(wilc, 1, reg & ~BIT(1));
 
do {
-   usleep_range(2 * 1000, 2 * 1000);
+   usleep_range(2 * 1000, 5 * 1000);
wilc_get_chipid(wilc, true);
} while (wilc_get_chipid(wilc, true) == 0);
} while (wilc_get_chipid(wilc, true) == 0);
@@ -423,7 +423,7 @@ void chip_wakeup(struct wilc *wilc)
 _status_reg);
 
while ((clk_status_reg & 0x1) == 0) {
-   usleep_range(2 * 1000, 2 * 1000);
+   usleep_range(2 * 1000, 5 * 1000);
 
wilc->hif_func->hif_read_reg(wilc, 0xf1,
 _status_reg);
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH V3] staging: wilc1000: drop explicit conversion to bool

[Nicholas Mc Guire]

As the expression evaluates to a boolean anyway (relational and logical
operators) conversion with the ternary operator is not needed here as
coccinelle notes (boolconv.cocci)

Signed-off-by: Nicholas Mc Guire 
Reviewed-by: Julian Calaby  
---

V2: sent out the wrong version - the commit message was longer than 75
chars - only change here is the commit message wrapping.

V3: dropping the superfluous outer () as suggested by
Julian Calaby 

scripts/coccinelle/misc/boolconv.cocci warned about:
drivers/staging/wilc1000/wilc_wlan.c:14:48-53: WARNING: conversion to bool not 
needed here

Patch was compile tested with: x86_64_defconfig + Staging=y,
WILC1000_SDIO=m, WILC1000_SPI=m, WILC1000=m
(checkpatch, sparse and cocci clean otherwise)

Patch is against 5.1-rc3 (localversion-next is -next-20190405)

 drivers/staging/wilc1000/wilc_wlan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/wilc1000/wilc_wlan.c 
b/drivers/staging/wilc1000/wilc_wlan.c
index c238969..6c9fd3a 100644
--- a/drivers/staging/wilc1000/wilc_wlan.c
+++ b/drivers/staging/wilc1000/wilc_wlan.c
@@ -11,7 +11,7 @@
 
 static inline bool is_wilc1000(u32 id)
 {
-   return ((id & 0xf000) == 0x10 ? true : false);
+   return (id & 0xf000) == 0x10;
 }
 
 static inline void acquire_bus(struct wilc *wilc, enum bus_acquire acquire)
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH V2] staging: wilc1000: drop explicit conversion to bool

[Nicholas Mc Guire]

On Sat, Apr 06, 2019 at 08:11:55PM +1100, Julian Calaby wrote:
> Hi Nicholas,
> 
> On Sat, Apr 6, 2019 at 7:48 PM Nicholas Mc Guire  wrote:
> >
> > As the expression evaluates to a boolean anyway (relational and logical
> > operators) conversion with the ternary operator is not needed here as
> > coccinelle notes (boolconv.cocci)
> >
> > Signed-off-by: Nicholas Mc Guire 
> > ---
> >
> > V2: sent out the wrong version - the commit message was longer than 75
> > chars - only change here is the commit message wrapping.
> >
> > scripts/coccinelle/misc/boolconv.cocci warned about:
> > drivers/staging/wilc1000/wilc_wlan.c:14:48-53: WARNING: conversion to bool 
> > not needed here
> >
> > Patch was compile tested with: x86_64_defconfig + Staging=y,
> > WILC1000_SDIO=m, WILC1000_SPI=m, WILC1000=m
> > (checkpatch, sparse and cocci clean otherwise)
> >
> > Patch is against 5.1-rc3 (localversion-next is -next-20190403)
> >
> >  drivers/staging/wilc1000/wilc_wlan.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/wilc1000/wilc_wlan.c 
> > b/drivers/staging/wilc1000/wilc_wlan.c
> > index c238969..6c9fd3a 100644
> > --- a/drivers/staging/wilc1000/wilc_wlan.c
> > +++ b/drivers/staging/wilc1000/wilc_wlan.c
> > @@ -11,7 +11,7 @@
> >
> >  static inline bool is_wilc1000(u32 id)
> >  {
> > -   return ((id & 0xf000) == 0x10 ? true : false);
> > +   return ((id & 0xf000) == 0x10);
> 
> Whilst you're here, you might as well remove the superfluous parentheses.
>

fine - there are a few other places though that this would need to be cleaned
up to be consistent e.g. entries = ((reg >> 3) & 0x3f); wilc_wlan.c
 
> Other than that,
> 
> Reviewed-by: Julian Calaby 
> 

will resend with outer () dropped and Reviewed-by: added

thx!
hofrat

> Thanks,
> 
> -- 
> Julian Calaby
> 
> Email: julian.cal...@gmail.com
> Profile: http://www.google.com/profiles/julian.calaby/
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH V2] staging: wilc1000: drop explicit conversion to bool

[Nicholas Mc Guire]

As the expression evaluates to a boolean anyway (relational and logical
operators) conversion with the ternary operator is not needed here as
coccinelle notes (boolconv.cocci)

Signed-off-by: Nicholas Mc Guire 
---

V2: sent out the wrong version - the commit message was longer than 75
chars - only change here is the commit message wrapping.

scripts/coccinelle/misc/boolconv.cocci warned about:
drivers/staging/wilc1000/wilc_wlan.c:14:48-53: WARNING: conversion to bool not 
needed here

Patch was compile tested with: x86_64_defconfig + Staging=y,
WILC1000_SDIO=m, WILC1000_SPI=m, WILC1000=m
(checkpatch, sparse and cocci clean otherwise)

Patch is against 5.1-rc3 (localversion-next is -next-20190403)

 drivers/staging/wilc1000/wilc_wlan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/wilc1000/wilc_wlan.c 
b/drivers/staging/wilc1000/wilc_wlan.c
index c238969..6c9fd3a 100644
--- a/drivers/staging/wilc1000/wilc_wlan.c
+++ b/drivers/staging/wilc1000/wilc_wlan.c
@@ -11,7 +11,7 @@
 
 static inline bool is_wilc1000(u32 id)
 {
-   return ((id & 0xf000) == 0x10 ? true : false);
+   return ((id & 0xf000) == 0x10);
 }
 
 static inline void acquire_bus(struct wilc *wilc, enum bus_acquire acquire)
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: wilc1000: drop explicit conversion to bool

[Nicholas Mc Guire]

As the expression evaluates to a boolean anyway (relational and logical 
operators) conversion
with the ternary operator is not needed here as coccinelle notes 
(boolconv.cocci)

Signed-off-by: Nicholas Mc Guire 
---

scripts/coccinelle/misc/boolconv.cocci warned about:
drivers/staging/wilc1000/wilc_wlan.c:14:48-53: WARNING: conversion to bool not 
needed here

Patch was compile tested with: x86_64_defconfig + Staging=y,
WILC1000_SDIO=m, WILC1000_SPI=m, WILC1000=m
(checkpatch, sparse and cocci clean otherwise)

Patch is against 5.1-rc3 (localversion-next is -next-20190403)

 drivers/staging/wilc1000/wilc_wlan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/wilc1000/wilc_wlan.c 
b/drivers/staging/wilc1000/wilc_wlan.c
index c238969..6c9fd3a 100644
--- a/drivers/staging/wilc1000/wilc_wlan.c
+++ b/drivers/staging/wilc1000/wilc_wlan.c
@@ -11,7 +11,7 @@
 
 static inline bool is_wilc1000(u32 id)
 {
-   return ((id & 0xf000) == 0x10 ? true : false);
+   return ((id & 0xf000) == 0x10);
 }
 
 static inline void acquire_bus(struct wilc *wilc, enum bus_acquire acquire)
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/2] staging:iio:accel:adis16203: add SPDX license identifier tag

[Nicholas Mc Guire]

Pop in the SPDX tag as the license is clearly indicated
as GPL V2 or later this should also be indicated with a SPDX license
identifier tag.

Signed-off-by: Nicholas Mc Guire 
---

chackpatch.pl was warning:
WARNING: Missing or malformed SPDX-License-Identifier tag in line 1

Patch was compile tested with: X86_64_defconfig + SPI=y, IIO=m
STAGING=y, ADIS16203=m

Patch is against 5.1-rc3 (localversion-next is next-20190403)

 drivers/staging/iio/accel/adis16203.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/iio/accel/adis16203.c 
b/drivers/staging/iio/accel/adis16203.c
index 5cc96c80..cf9d41a 100644
--- a/drivers/staging/iio/accel/adis16203.c
+++ b/drivers/staging/iio/accel/adis16203.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
 /*
  * ADIS16203 Programmable 360 Degrees Inclinometer
  *
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/2] staging:iio:accel:adis16240: add SPDX license identifier tag

[Nicholas Mc Guire]

Pop in the SPDX tag as the license is clearly indicated
as GPL V2 or later this should also be indicated with a SPDX license
identifier tag.

Signed-off-by: Nicholas Mc Guire 
---

chackpatch.pl was warning:
WARNING: Missing or malformed SPDX-License-Identifier tag in line 1

Patch was compile tested with: x86_64_defconfig + SPI=y, IIO=m
STAGING=y, ADIS16240=m

Patch is against 5.1-rc3 (localversion-next is next-20190403)

 drivers/staging/iio/accel/adis16240.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/iio/accel/adis16240.c 
b/drivers/staging/iio/accel/adis16240.c
index 24e525f..b2d71ae 100644
--- a/drivers/staging/iio/accel/adis16240.c
+++ b/drivers/staging/iio/accel/adis16240.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
 /*
  * ADIS16240 Programmable Impact Sensor and Recorder driver
  *
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtlwifi: Use proper enum for return in halmac_parse_psd_data_88xx

[Nicholas Mc Guire]

On Wed, Feb 20, 2019 at 10:25:24PM -0700, Nathan Chancellor wrote:
> Clang warns:
> 
> drivers/staging/rtlwifi/halmac/halmac_88xx/halmac_func_88xx.c:2472:11:
> warning: implicit conversion from enumeration type 'enum
> halmac_cmd_process_status' to different enumeration type 'enum
> halmac_ret_status' [-Wenum-conversion]
> return HALMAC_CMD_PROCESS_ERROR;
> ~~ ^~~~
> 1 warning generated.
> 
yup - my bad I somehow managed to end up in halmac_cmd_process_status
rather than halmac_ret_status - HALMAC_RET_MALLOC_FAIL makes sense here.

interesting that gcc did not fuss at this.

thx!
hofrat

> Fix this by using the proper enum for allocation failures,
> HALMAC_RET_MALLOC_FAIL, which is used in the rest of this file.
> 
> Fixes: e4b08e16b7d9 ("staging: r8822be: check kzalloc return or bail")
> Link: https://github.com/ClangBuiltLinux/linux/issues/375
> Signed-off-by: Nathan Chancellor 

Reviewed-by: Nicholas Mc Guire 

> ---
>  drivers/staging/rtlwifi/halmac/halmac_88xx/halmac_func_88xx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rtlwifi/halmac/halmac_88xx/halmac_func_88xx.c 
> b/drivers/staging/rtlwifi/halmac/halmac_88xx/halmac_func_88xx.c
> index ec742da030db..ddbeff8224ab 100644
> --- a/drivers/staging/rtlwifi/halmac/halmac_88xx/halmac_func_88xx.c
> +++ b/drivers/staging/rtlwifi/halmac/halmac_88xx/halmac_func_88xx.c
> @@ -2469,7 +2469,7 @@ halmac_parse_psd_data_88xx(struct halmac_adapter 
> *halmac_adapter, u8 *c2h_buf,
>   if (!psd_set->data) {
>   psd_set->data = kzalloc(psd_set->data_size, GFP_KERNEL);
>   if (!psd_set->data)
> - return HALMAC_CMD_PROCESS_ERROR;
> + return HALMAC_RET_MALLOC_FAIL;
>   }
>  
>   if (segment_id == 0)
> -- 
> 2.21.0.rc1
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: r8822be: check kzalloc return or bail

[Nicholas Mc Guire]

The kzalloc() in halmac_parse_psd_data_88xx() can fail and return NULL
so check the psd_set->data after allocation and if allocation failed
return HALMAC_CMD_PROCESS_ERROR.

Signed-off-by: Nicholas Mc Guire 
Fixes: 938a0447f094 ("staging: r8822be: Add code for halmac sub-drive")
---

Problem was located with an experimental coccinelle script

Patch was compile tested with: x86_64_defconfig + STAGING=y,
R8822BE=m
(with a smatch error that looks like a false-positive

  CHECK   drivers/staging/rtlwifi/halmac/halmac_88xx/halmac_func_88xx.c
drivers/staging/rtlwifi/halmac/halmac_88xx/halmac_func_88xx.c:624 
halmac_func_write_logical_efuse_88xx() error: uninitialized symbol 
'pg_efuse_header2'.
  CC [M]  drivers/staging/rtlwifi/halmac/halmac_88xx/halmac_func_88xx.o

as the initialization of pg_efuse_header2 is under the same if condition (line 
592) as the
use at line 624 it is initialized)

Patch is agaisnt 5.0-rc6 (localversion-next is next-20190215)

 drivers/staging/rtlwifi/halmac/halmac_88xx/halmac_func_88xx.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/rtlwifi/halmac/halmac_88xx/halmac_func_88xx.c 
b/drivers/staging/rtlwifi/halmac/halmac_88xx/halmac_func_88xx.c
index 53f55f12..bf783a0 100644
--- a/drivers/staging/rtlwifi/halmac/halmac_88xx/halmac_func_88xx.c
+++ b/drivers/staging/rtlwifi/halmac/halmac_88xx/halmac_func_88xx.c
@@ -2466,8 +2466,11 @@ halmac_parse_psd_data_88xx(struct halmac_adapter 
*halmac_adapter, u8 *c2h_buf,
segment_size = (u8)PSD_DATA_GET_SEGMENT_SIZE(c2h_buf);
psd_set->data_size = total_size;
 
-   if (!psd_set->data)
+   if (!psd_set->data) {
psd_set->data = kzalloc(psd_set->data_size, GFP_KERNEL);
+   if (!psd_set->data)
+   return HALMAC_CMD_PROCESS_ERROR;
+   }
 
if (segment_id == 0)
psd_set->segment_size = segment_size;
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH V2] staging: iio: adc: ad7280a: check for devm_kasprint() failure

[Nicholas Mc Guire]

devm_kasprintf() may return NULL on failure of internal allocation thus
the assignments to  attr.name  are not safe if not checked. On error
ad7280_attr_init() returns a negative return so -ENOMEM should be
OK here (passed on as return value of the probe function). To make the
error case more readable a temporary  iio_attr  is introduced and the code
refactored.

Signed-off-by: Nicholas Mc Guire 
Fixes: 2051f25d2a26 ("iio: adc: New driver for AD7280A Lithium Ion Battery 
Monitoring System2")
---

V2: use a tmp pointer to iio_attr to make the error check more readable
as proposed by Dan Carpenter 

Problem located with an experimental coccinelle script

Patch was compile tested with: x86_64_defconfig + STAGING=y
SPI=y, IIO=y, AD7280=m

Patch is against 4.20-rc4 (localversion-next is next-20181127)

 drivers/staging/iio/adc/ad7280a.c | 43 +++
 1 file changed, 21 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7280a.c 
b/drivers/staging/iio/adc/ad7280a.c
index 0bb9ab1..7a0ba26 100644
--- a/drivers/staging/iio/adc/ad7280a.c
+++ b/drivers/staging/iio/adc/ad7280a.c
@@ -561,6 +561,7 @@ static int ad7280_attr_init(struct ad7280_state *st)
 {
int dev, ch, cnt;
unsigned int index;
+   struct iio_dev_attr *iio_attr;
 
st->iio_attr = devm_kcalloc(>spi->dev, 2, sizeof(*st->iio_attr) *
(st->slave_num + 1) * AD7280A_CELLS_PER_DEV,
@@ -571,37 +572,35 @@ static int ad7280_attr_init(struct ad7280_state *st)
for (dev = 0, cnt = 0; dev <= st->slave_num; dev++)
for (ch = AD7280A_CELL_VOLTAGE_1; ch <= AD7280A_CELL_VOLTAGE_6;
ch++, cnt++) {
+   iio_attr = >iio_attr[cnt];
index = dev * AD7280A_CELLS_PER_DEV + ch;
-   st->iio_attr[cnt].address =
-   ad7280a_devaddr(dev) << 8 | ch;
-   st->iio_attr[cnt].dev_attr.attr.mode =
-   0644;
-   st->iio_attr[cnt].dev_attr.show =
-   ad7280_show_balance_sw;
-   st->iio_attr[cnt].dev_attr.store =
-   ad7280_store_balance_sw;
-   st->iio_attr[cnt].dev_attr.attr.name =
+   iio_attr->address = ad7280a_devaddr(dev) << 8 | ch;
+   iio_attr->dev_attr.attr.mode = 0644;
+   iio_attr->dev_attr.show = ad7280_show_balance_sw;
+   iio_attr->dev_attr.store = ad7280_store_balance_sw;
+   iio_attr->dev_attr.attr.name =
devm_kasprintf(>spi->dev, GFP_KERNEL,
   "in%d-in%d_balance_switch_en",
   index, index + 1);
-   ad7280_attributes[cnt] =
-   >iio_attr[cnt].dev_attr.attr;
+   if (!iio_attr->dev_attr.attr.name)
+   return -ENOMEM;
+
+   ad7280_attributes[cnt] = _attr->dev_attr.attr;
cnt++;
-   st->iio_attr[cnt].address =
-   ad7280a_devaddr(dev) << 8 |
+   iio_attr = >iio_attr[cnt];
+   iio_attr->address = ad7280a_devaddr(dev) << 8 |
(AD7280A_CB1_TIMER + ch);
-   st->iio_attr[cnt].dev_attr.attr.mode =
-   0644;
-   st->iio_attr[cnt].dev_attr.show =
-   ad7280_show_balance_timer;
-   st->iio_attr[cnt].dev_attr.store =
-   ad7280_store_balance_timer;
-   st->iio_attr[cnt].dev_attr.attr.name =
+   iio_attr->dev_attr.attr.mode = 0644;
+   iio_attr->dev_attr.show = ad7280_show_balance_timer;
+   iio_attr->dev_attr.store = ad7280_store_balance_timer;
+   iio_attr->dev_attr.attr.name =
devm_kasprintf(>spi->dev, GFP_KERNEL,
   "in%d-in%d_balance_timer",
   index, index + 1);
-   ad7280_attributes[cnt] =
-   >iio_attr[cnt].dev_attr.attr;
+   if (!iio_attr->dev_attr.attr.name)
+   return -ENOMEM;
+
+   ad7280_attributes[cnt] = _attr->dev_attr.attr;
}
 
ad7280_attributes[cnt] = NULL;
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: iio: adc: ad7280a: check for devm_kasprint() failure

[Nicholas Mc Guire]

On Mon, Nov 26, 2018 at 04:00:32PM +0300, Dan Carpenter wrote:
> On Mon, Nov 26, 2018 at 10:39:04AM +0100, Nicholas Mc Guire wrote:
> > devm_kasprintf() may return NULL on failure of internal allocation thus
> > the assignments to  attr.name  are not safe if not checked. On error
> > ad7280_attr_init() returns a negative return so -ENOMEM should be
> > OK here (passed on as return value of the probe function).
> > 
> > Signed-off-by: Nicholas Mc Guire 
> > Fixes: 2051f25d2a26 ("iio: adc: New driver for AD7280A Lithium Ion Battery 
> > Monitoring System2")
> > ---
> > 
> > Problem located with an experimental coccinelle script
> > 
> > As using   if(!st->iio_attr[cnt].dev_attr.attr.name)  seamed quite
> > unreadable in this case the  (var  == NULL)  variant was used. Not
>^^
> Why two spaces?

just a typo 

> 
> > sure if there are objections against this (checkpatch.pl issues
> > a CHECK on this).
> > 
> 
> You should just follow checkpatch rules here.  If you don't, someone
> else will just send a patch to make it checkpatch compliant.  One thing
> you could do is at the start of the loop do:
> 
>   struct iio_dev_attr *attr = >iio_attr[cnt];
> 
> Then it becomes:
> 
>   if (!attr->dev_attr.attr.name)
> 
> It's slightly more readable that way.  Keep in mind that we increment
> cnt++ in the middle of the loop so you'll have to update attr as well.
>
My understanding was that CHECK: notes are not strict rules but
those that may vary from case to case - anyway you solution
sounds reasonable and in any case better than:

   if (!st->iio_attr[cnt].dev_attr.attr.name)

which just looked bad to me.

thx!
hofrat
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: iio: adc: ad7280a: check for devm_kasprint() failure

[Nicholas Mc Guire]

devm_kasprintf() may return NULL on failure of internal allocation thus
the assignments to  attr.name  are not safe if not checked. On error
ad7280_attr_init() returns a negative return so -ENOMEM should be
OK here (passed on as return value of the probe function).

Signed-off-by: Nicholas Mc Guire 
Fixes: 2051f25d2a26 ("iio: adc: New driver for AD7280A Lithium Ion Battery 
Monitoring System2")
---

Problem located with an experimental coccinelle script

As using   if(!st->iio_attr[cnt].dev_attr.attr.name)  seamed quite
unreadable in this case the  (var  == NULL)  variant was used. Not
sure if there are objections against this (checkpatch.pl issues
a CHECK on this).

Patch was compile tested with: x86_64_defconfig + STAGING=y
SPI=y, IIO=y, AD7280=m

Patch is against 4.20-rc4 (localversion-next is next-20181126)

 drivers/staging/iio/adc/ad7280a.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/staging/iio/adc/ad7280a.c 
b/drivers/staging/iio/adc/ad7280a.c
index 0bb9ab1..5b87530 100644
--- a/drivers/staging/iio/adc/ad7280a.c
+++ b/drivers/staging/iio/adc/ad7280a.c
@@ -584,6 +584,9 @@ static int ad7280_attr_init(struct ad7280_state *st)
devm_kasprintf(>spi->dev, GFP_KERNEL,
   "in%d-in%d_balance_switch_en",
   index, index + 1);
+   if (st->iio_attr[cnt].dev_attr.attr.name  == NULL)
+   return -ENOMEM;
+
ad7280_attributes[cnt] =
>iio_attr[cnt].dev_attr.attr;
cnt++;
@@ -600,6 +603,9 @@ static int ad7280_attr_init(struct ad7280_state *st)
devm_kasprintf(>spi->dev, GFP_KERNEL,
   "in%d-in%d_balance_timer",
   index, index + 1);
+   if (st->iio_attr[cnt].dev_attr.attr.name == NULL)
+   return -ENOMEM;
+
ad7280_attributes[cnt] =
>iio_attr[cnt].dev_attr.attr;
}
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] media: davinci_vpfe: bail out if kmalloc failed

[Nicholas Mc Guire]

 The kmalloc is passed indirectly to  from  but with an offset
which if not 0 will cause the null check if (to && from && size) 
to succeed. An explicit !NULL check is thus added for params here.

 ipipe_s_config and ipipe_g_config - both fail to check kmalloc
are called from ipipe_ioctl where a negative return is a valid
indication of error so simply setting rval = -ENOMEM seems ok.

Signed-off-by: Nicholas Mc Guire 
Fixes: da43b6ccadcf ("[media] davinci: vpfe: dm365: add IPIPE support for media 
controller driver")
---

Problem located with experimental coccinelle patch

Patch was compile tested with: davinci_all_defconfig + SAGING=y,
STAGING_MEDIA=y, MEDIA_SUPPORT=m, MEDIA_CONTROLLER=y,
VIDEO_V4L2_SUBDEV_API=y, VIDEO_DAVINCI_VPBE_DISPLAY=m,
VIDEO_DM365_VPFE=m
(with some coccicheck findings unrelated to the proposed change)

Patch is against 4.20-rc3 (localversion-next is next-20181120)

 drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c 
b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
index 3d910b8..0150aed 100644
--- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
+++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
@@ -1266,6 +1266,11 @@ static int ipipe_s_config(struct v4l2_subdev *sd, struct 
vpfe_ipipe_config *cfg)
 
params = kmalloc(sizeof(struct ipipe_module_params),
 GFP_KERNEL);
+   if (!params) {
+   rval = -ENOMEM;
+   goto error;
+   }
+
to = (void *)params + module_if->param_offset;
size = module_if->param_size;
 
@@ -1308,6 +1313,11 @@ static int ipipe_g_config(struct v4l2_subdev *sd, struct 
vpfe_ipipe_config *cfg)
 
params = kmalloc(sizeof(struct ipipe_module_params),
 GFP_KERNEL);
+   if (!params) {
+   rval = -ENOMEM;
+   goto error;
+   }
+
from = (void *)params + module_if->param_offset;
size = module_if->param_size;
 
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working

[Nicholas Mc Guire]

On Tue, Sep 11, 2018 at 10:20:41AM +0300, Dan Carpenter wrote:
> On Tue, Sep 11, 2018 at 06:48:27AM +0000, Nicholas Mc Guire wrote:
> > On Mon, Sep 10, 2018 at 08:30:38PM +0200, Hans de Goede wrote:
> > > Commit 1daddbc8dec5 ("staging: vboxvideo: Update driver to use
> > > drm_dev_register.") replaced the obsolere drm_get_pci_dev() with
> > > normal pci probe and remove functions.
> > > 
> > > But the new vbox_pci_probe() is missing a pci_enable_device() call,
> > > causing interrupts to not be delivered. This causes resizes of the
> > > vm window to not get seen by the drm/kms code.
> > > 
> > > This commit adds the missing pci_enable_device() call, fixing this.
> > 
> > pci_enable_device is the wrapper to pci_enable_device_flags() the later
> > return < 0 on error - so while the check for if(ret) will do the right
> > think here I think it would be prefereable to explicidly use if (ret < 0)
> > as all error values pci_enable_device_flags() returns are negative.
> > 
> 
> It could go either way, I think.  "if (ret) " is sort of as explicit as
> "if (ret < 0) " when you consider the false side.  When I see "if (ret)"
> then I know the code returns negative error codes and zero, but when I
> see "if (ret < 0)" then I think maybe this returns positive non-zero
> values as well.
> 
> As a static analysis person, the "if (ret)" style is easier for me.
> Sometimes Smatch doesn't know what a function returns.  Maybe the error
> code comes from a different thread and Smatch doesn't understand
> threads.  So then when people use "if (ret)" Smatch knows that non-zero
> means *param1 is not initialized.  Then the caller does "if (ret < 0)"
> that means that positive non-zero values are not handled so let's print
> an uninitialized variable warning.  Just to spell it out a little more,
> the error code won't be printed for "if (ret)" because negatives are a
> subset of non-zero.
> 
> Of course, if you do it consistently there won't be a warning message.
> I never see the consistent subsystems, so I don't know if they exist.
>
Probably true - there is quite a bit of incorrect type issues in the
kernel and there are a cases of comparing to e.g. <= 0 for signed
types is used, so I personally prefere if the check allows type
inference - if I see a "ret < 0" it can be infered that the type must
be signed and an unsigned is an error while for !0 case does not allow
such inference.

Anyway - as noted the patch seems correct with respect to the intent and
if the general preference is for "if (ret)" then no objections.

thanks for the clarification !

hofrat
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working

[Nicholas Mc Guire]

On Tue, Sep 11, 2018 at 08:53:35AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 11-09-18 08:48, Nicholas Mc Guire wrote:
> >On Mon, Sep 10, 2018 at 08:30:38PM +0200, Hans de Goede wrote:
> >>Commit 1daddbc8dec5 ("staging: vboxvideo: Update driver to use
> >>drm_dev_register.") replaced the obsolere drm_get_pci_dev() with
> >>normal pci probe and remove functions.
> >>
> >>But the new vbox_pci_probe() is missing a pci_enable_device() call,
> >>causing interrupts to not be delivered. This causes resizes of the
> >>vm window to not get seen by the drm/kms code.
> >>
> >>This commit adds the missing pci_enable_device() call, fixing this.
> >
> >pci_enable_device is the wrapper to pci_enable_device_flags() the later
> >return < 0 on error - so while the check for if(ret) will do the right
> >think here I think it would be prefereable to explicidly use if (ret < 0)
> >as all error values pci_enable_device_flags() returns are negative.
> 
> The use of "if (ret)" checks for functions which return 0 on success
> negative value on error is a standard pattern in the kernel, so I would
> prefer to keep this as is.
>

Well as noted it does the right thing - just checking the use of 
pci_enable_device() in the existing drivers it seems that it is roughly
balanced between checking < 0 and !0. The rational for < 0 would be that
the negative return values mandate a signed type, whilc !0 does not and
if someone then uses and unsigned type the error case would return as
success while the < 0 would be detected at compile time (or other static
code checkers). 

thx!
hofrat

> >>Fixes: 1daddbc8dec5 ("staging: vboxvideo: Update driver to use ...")
> >>Cc: Fabio Rafael da Rosa 
> >>Cc: Nicholas Mc Guire 
> >>Signed-off-by: Hans de Goede 
> >Reviewed-by: Nicholas Mc Guire 
> 
> Thanks.
> 
> Regards,
> 
> Hans
> 
> 
> >
> >>---
> >>  drivers/staging/vboxvideo/vbox_drv.c | 7 +++
> >>  1 file changed, 7 insertions(+)
> >>
> >>diff --git a/drivers/staging/vboxvideo/vbox_drv.c 
> >>b/drivers/staging/vboxvideo/vbox_drv.c
> >>index da92c493f157..69cc508af1bc 100644
> >>--- a/drivers/staging/vboxvideo/vbox_drv.c
> >>+++ b/drivers/staging/vboxvideo/vbox_drv.c
> >>@@ -59,6 +59,11 @@ static int vbox_pci_probe(struct pci_dev *pdev, const 
> >>struct pci_device_id *ent)
> >>ret = PTR_ERR(dev);
> >>goto err_drv_alloc;
> >>}
> >>+
> >>+   ret = pci_enable_device(pdev);
> >>+   if (ret)
> >>+   goto err_pci_enable;
> >>+
> >>dev->pdev = pdev;
> >>pci_set_drvdata(pdev, dev);
> >>@@ -75,6 +80,8 @@ static int vbox_pci_probe(struct pci_dev *pdev, const 
> >>struct pci_device_id *ent)
> >>   err_drv_dev_register:
> >>vbox_driver_unload(dev);
> >>   err_vbox_driver_load:
> >>+   pci_disable_device(pdev);
> >>+ err_pci_enable:
> >>drm_dev_put(dev);
> >>   err_drv_alloc:
> >>return ret;
> >>-- 
> >>2.19.0.rc0
> >>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4.19 regression fix 1/2] staging: vboxvideo: Fix IRQs no longer working

[Nicholas Mc Guire]

On Mon, Sep 10, 2018 at 08:30:38PM +0200, Hans de Goede wrote:
> Commit 1daddbc8dec5 ("staging: vboxvideo: Update driver to use
> drm_dev_register.") replaced the obsolere drm_get_pci_dev() with
> normal pci probe and remove functions.
> 
> But the new vbox_pci_probe() is missing a pci_enable_device() call,
> causing interrupts to not be delivered. This causes resizes of the
> vm window to not get seen by the drm/kms code.
> 
> This commit adds the missing pci_enable_device() call, fixing this.

pci_enable_device is the wrapper to pci_enable_device_flags() the later
return < 0 on error - so while the check for if(ret) will do the right
think here I think it would be prefereable to explicidly use if (ret < 0)
as all error values pci_enable_device_flags() returns are negative.

> 
> Fixes: 1daddbc8dec5 ("staging: vboxvideo: Update driver to use ...")
> Cc: Fabio Rafael da Rosa 
> Cc: Nicholas Mc Guire 
> Signed-off-by: Hans de Goede 
Reviewed-by: Nicholas Mc Guire 

> ---
>  drivers/staging/vboxvideo/vbox_drv.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/staging/vboxvideo/vbox_drv.c 
> b/drivers/staging/vboxvideo/vbox_drv.c
> index da92c493f157..69cc508af1bc 100644
> --- a/drivers/staging/vboxvideo/vbox_drv.c
> +++ b/drivers/staging/vboxvideo/vbox_drv.c
> @@ -59,6 +59,11 @@ static int vbox_pci_probe(struct pci_dev *pdev, const 
> struct pci_device_id *ent)
>   ret = PTR_ERR(dev);
>   goto err_drv_alloc;
>   }
> +
> + ret = pci_enable_device(pdev);
> + if (ret)
> + goto err_pci_enable;
> +
>   dev->pdev = pdev;
>   pci_set_drvdata(pdev, dev);
>  
> @@ -75,6 +80,8 @@ static int vbox_pci_probe(struct pci_dev *pdev, const 
> struct pci_device_id *ent)
>   err_drv_dev_register:
>   vbox_driver_unload(dev);
>   err_vbox_driver_load:
> + pci_disable_device(pdev);
> + err_pci_enable:
>   drm_dev_put(dev);
>   err_drv_alloc:
>   return ret;
> -- 
> 2.19.0.rc0
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: bcm2835-camera: fix timeout handling in wait_for_completion_timeout

[Nicholas Mc Guire]

wait_for_completion_timeout returns unsigned long not int so a variable of
proper type is introduced. Further the check for <= 0 is ambiguous and should
be == 0 here indicating timeout which is the only error case so no additional
check needed here.

Signed-off-by: Nicholas Mc Guire 
Fixes: 7b3ad5abf027 ("staging: Import the BCM2835 MMAL-based V4L2 camera 
driver.")
---

Patch found by experimental coccinell API conformance checker

Patch was compile tested with: bcm2835_defconfig + CONFIG_MEDIA_SUPPORT=y
CONFIG_MEDIA_CAMERA_SUPPORT=y, CONFIG_VIDEO_BCM2835=m

Patch is against 4.18-rc5 (localversion-next is next-20180720)

 drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c 
b/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c
index f5b5ead..51e5b04 100644
--- a/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c
@@ -630,6 +630,7 @@ static int send_synchronous_mmal_msg(struct 
vchiq_mmal_instance *instance,
 {
struct mmal_msg_context *msg_context;
int ret;
+   unsigned long timeout;
 
/* payload size must not cause message to exceed max size */
if (payload_len >
@@ -668,11 +669,11 @@ static int send_synchronous_mmal_msg(struct 
vchiq_mmal_instance *instance,
return ret;
}
 
-   ret = wait_for_completion_timeout(_context->u.sync.cmplt, 3 * HZ);
-   if (ret <= 0) {
-   pr_err("error %d waiting for sync completion\n", ret);
-   if (ret == 0)
-   ret = -ETIME;
+   timeout = wait_for_completion_timeout(_context->u.sync.cmplt,
+ 3 * HZ);
+   if (timeout == 0) {
+   pr_err("timed out waiting for sync completion\n");
+   ret = -ETIME;
/* todo: what happens if the message arrives after aborting */
release_msg_context(msg_context);
return ret;
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: bcm2835-camera: handle wait_for_completion_timeout return properly

[Nicholas Mc Guire]

wait_for_completion_timeout returns unsigned long not int so a variable of
proper type is introduced. Further the check for <= 0 is ambiguous and
should be == 0 here indicating timeout.

Signed-off-by: Nicholas Mc Guire 
Fixes: 7b3ad5abf027 ("staging: Import the BCM2835 MMAL-based V4L2 camera 
driver.")
---

Patch found by experimental coccinell API conformance checker

Printing the return value was dropped as there is no additional information
by including it (not an errno). 

Patch was compile tested with: bcm2835_defconfig + CONFIG_MEDIA_SUPPORT=y
CONFIG_MEDIA_CAMERA_SUPPORT=y, CONFIG_VIDEO_BCM2835=m
(with some sparse warnings - not related to the proposed change thogh)

Patch is against 4.18-rc5 (localversion-next is next-20180720)

 drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c 
b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
index 6dd0c83..c04bdf0 100644
--- a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
+++ b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
@@ -580,6 +580,7 @@ static int start_streaming(struct vb2_queue *vq, unsigned 
int count)
 static void stop_streaming(struct vb2_queue *vq)
 {
int ret;
+   unsigned long timeout;
struct bm2835_mmal_dev *dev = vb2_get_drv_priv(vq);
 
v4l2_dbg(1, bcm2835_v4l2_debug, >v4l2_dev, "%s: dev:%p\n",
@@ -605,10 +606,10 @@ static void stop_streaming(struct vb2_queue *vq)
  sizeof(dev->capture.frame_count));
 
/* wait for last frame to complete */
-   ret = wait_for_completion_timeout(>capture.frame_cmplt, HZ);
-   if (ret <= 0)
+   timeout = wait_for_completion_timeout(>capture.frame_cmplt, HZ);
+   if (timeout == 0)
v4l2_err(>v4l2_dev,
-"error %d waiting for frame completion\n", ret);
+"timed out waiting for frame completion\n");
 
v4l2_dbg(1, bcm2835_v4l2_debug, >v4l2_dev,
 "disabling connection\n");
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: board: drop refcount in success case

[Nicholas Mc Guire]

On Tue, Jun 19, 2018 at 09:51:44AM +0200, Geert Uytterhoeven wrote:
> On Tue, Jun 19, 2018 at 9:37 AM Dan Carpenter  
> wrote:
> > On Mon, Jun 18, 2018 at 08:53:19PM +0200, Nicholas Mc Guire wrote:
> > >  The call to of_find_compatible_node() returns irqc_node with refcount
> > > incremented thus it must be explicitly decremented here after it was
> > > checked for non-NULL.
> > >
> > > Signed-off-by: Nicholas Mc Guire 
> > > Fixes: commit 72ee8626eeb1 ("staging: board: Add support for translating 
> > > hwirq to virq numbers")
> > > ---
> > >
> > > Problem located with an experimental coccinelle script
> > >
> > > Patch was compile-tested with: x86_64_defconfig + STAGING=y, 
> > > STAGING_BOARD=y
> > >
> > > Patch is against 4.18-rc1 (localversion-next is next-20180618)
> > >
> > >  drivers/staging/board/board.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/staging/board/board.c b/drivers/staging/board/board.c
> > > index cb6feb3..8ee48e5 100644
> > > --- a/drivers/staging/board/board.c
> > > +++ b/drivers/staging/board/board.c
> > > @@ -64,12 +64,13 @@ int __init board_staging_gic_setup_xlate(const char 
> > > *gic_match,
> > >   irqc_node = of_find_compatible_node(NULL, NULL, gic_match);
> > >
> > >   WARN_ON(!irqc_node);
> > >   if (!irqc_node)
> > >   return -ENOENT;
> > >
> > > + of_node_put(irqc_node);
> >
> > I don't feel like this is the right thing...  We should keep the
> > reference until we're done with it.  Which apparently is never?
> 
> Indeed. The reference must not be released in this function, as it's stored in
> a global variable, and used later.

yup -  I had simply interpreted this incorrectly as checking only and
overlooked that this was a global variable.

> 
> As all users are __init, it could be released when the init section is freeed,
> in theory, but there's no callback for that.
> 
> Hence:
> NAKed-by: Geert Uytterhoeven 
>
thanks for the clarification - sorry for the noise.

thx
hofrat
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: board: drop refcount in success case

[Nicholas Mc Guire]

 The call to of_find_compatible_node() returns irqc_node with refcount
incremented thus it must be explicitly decremented here after it was
checked for non-NULL.

Signed-off-by: Nicholas Mc Guire 
Fixes: commit 72ee8626eeb1 ("staging: board: Add support for translating hwirq 
to virq numbers")
---

Problem located with an experimental coccinelle script

Patch was compile-tested with: x86_64_defconfig + STAGING=y, STAGING_BOARD=y

Patch is against 4.18-rc1 (localversion-next is next-20180618)

 drivers/staging/board/board.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/board/board.c b/drivers/staging/board/board.c
index cb6feb3..8ee48e5 100644
--- a/drivers/staging/board/board.c
+++ b/drivers/staging/board/board.c
@@ -64,12 +64,13 @@ int __init board_staging_gic_setup_xlate(const char 
*gic_match,
irqc_node = of_find_compatible_node(NULL, NULL, gic_match);
 
WARN_ON(!irqc_node);
if (!irqc_node)
return -ENOENT;
 
+   of_node_put(irqc_node);
irqc_base = base;
return 0;
 }
 
 static void __init gic_fixup_resource(struct resource *res)
 {
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

completion issues in ks7010

[Nicholas Mc Guire]

Hi !

 staging ks7010 has a a few completion calls, some of which are using
 wait_for_completion_interruptible_timeout(). The key difference 
 betweeen wait_for_completion_interruptible_timeout() and
 wait_for_completion_timeout() is, if I got it right, only that
 the former will return on any pending fatal signals while the
 later will ither timeout (return 0) or complete (retrun >= 1)
 so as most of the timeouts are relatively long lived (500ms to
 5s) the interruptible variant seems resonable and the interupted
 case probably should be treated (atleast with a DPRINTK()) as
 failure case. The short lived case (20ms timeout) might be converted 
 to wait_for_completion_timeout().

 The case in ks7010_card_init() is not clear to me, the specific 
 sequence is:
init_completion(>confirm_wait);
hostif_sme_enqueue(priv, SME_START);
   enqueue SME_START 
   call tasklet_schedule(>sme_task);
  sme_task is hostif_sme_task()
  call hostif_sme_execute()
   switch (event) {
   case SME_START:
  if (priv->dev_state == DEVICE_STATE_BOOT) {
  hostif_mib_get_request(priv, 
DOT11_MAC_ADDRESS);
  }
  break;
wait_for_completion_interruptible_timeout(...)

 but it seems that completion is never called in this path - so effectively 
 this is an interruptible wait only -> should the SME_START case be calling
 complete(>confirm_wait); here ?


 The second general issue with completion is the use of complete()
 it is called with:

/* wake_up_all(>confirm_wait); */
complete(>confirm_wait);

 or

/* wake_up_interruptible_all(>confirm_wait); */
complete(>confirm_wait);

 the comment suggest that it would be expecting to wake all
 waiters but compete() is actually passing 1 in __wake_up_common 
 for nr_exclusive so only the first waiter would be woken 
 should this be a comlplete_all() - or the comment fixed ?

thx!
hofrat
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/2 RFC] staging: ks7010: drop unused empty function

[Nicholas Mc Guire]

fix sparse warning by droping unused empty function.

Fixes: 13a9930d15b4 ("staging: ks7010: add driver from Nanonote 
extra-repository")

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

sparse warning:

drivers/staging/ks7010/ks_wlan_net.c:3525:5: warning: symbol 'ks_wlan_reset' 
 not declared. Should it be static?

but ks_wlan_reset is unused and effectively an empty function so it can be
dropped.

Compile tested with: x86_64_defconfig + CONFIG_STAGING=y
CONFIG_MMC=m, CONFIG_KS7010=m

Patch is against 4.7-rc7 (localversion-next -next-20160727)

 drivers/staging/ks7010/ks_wlan_net.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/drivers/staging/ks7010/ks_wlan_net.c 
b/drivers/staging/ks7010/ks_wlan_net.c
index 1e21eb1..2347d5a 100644
--- a/drivers/staging/ks7010/ks_wlan_net.c
+++ b/drivers/staging/ks7010/ks_wlan_net.c
@@ -3521,8 +3521,3 @@ int ks_wlan_net_stop(struct net_device *dev)
 
return ret;
 }
-
-int ks_wlan_reset(struct net_device *dev)
-{
-   return 0;
-}
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/2 RFC] staging: ks7010: fix location of declaration

[Nicholas Mc Guire]

fix sparse warning by moving the extern declaration to device scope header 
file.

Fixes: 13a9930d15b4 ("staging: ks7010: add driver from Nanonote 
extra-repository")

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

sparse warning:

drivers/staging/ks7010/ks_wlan_net.c:3392:6: warning: symbol 
'send_packet_complete' was not declared. Should it be static?

was caused by build dependencies being resolved locally rather
than at device level.

Compile tested with: x86_64_defconfig + CONFIG_STAGING=y
CONFIG_MMC=m, CONFIG_KS7010=m

Patch is against 4.7-rc7 (localversion-next -next-20160727)

 drivers/staging/ks7010/ks_hostif.c | 1 -
 drivers/staging/ks7010/ks_wlan.h   | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/ks7010/ks_hostif.c 
b/drivers/staging/ks7010/ks_hostif.c
index a8822fe..e5e43c8 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -24,7 +24,6 @@ extern int ks_wlan_hw_tx(struct ks_wlan_private *priv, void 
*p,
 unsigned long size,
 void (*complete_handler) (void *arg1, void *arg2),
 void *arg1, void *arg2);
-extern void send_packet_complete(void *, void *);
 
 extern void ks_wlan_hw_wakeup_request(struct ks_wlan_private *priv);
 extern int ks_wlan_hw_power_save(struct ks_wlan_private *priv);
diff --git a/drivers/staging/ks7010/ks_wlan.h b/drivers/staging/ks7010/ks_wlan.h
index f05dc01..b4d2f51 100644
--- a/drivers/staging/ks7010/ks_wlan.h
+++ b/drivers/staging/ks7010/ks_wlan.h
@@ -501,5 +501,6 @@ struct ks_wlan_private {
 
 extern int ks_wlan_net_start(struct net_device *dev);
 extern int ks_wlan_net_stop(struct net_device *dev);
+extern void send_packet_complete(void *, void *);
 
 #endif /* _KS_WLAN_H */
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 0/2 RFC] staging: ks7010: fix sparse warnings

[Nicholas Mc Guire]

Two proposed fixes for sparse warnings

   drivers/staging/ks7010/ks_wlan_net.c:3392:6: warning: symbol 
'send_packet_complete'
 was not declared. Should it be static?

looking at how it is used I guess it should not be static but
the extern declaration moved into ks_wlan.h 

but all the functions declared extern there seem to be named ks_wlan_*, 
indicating the layering of the interface, so I was unsure if that 
should not also be renamed rather than just the extern moved.


   drivers/staging/ks7010/ks_wlan_net.c:3525:5: warning: symbol 'ks_wlan_reset'
 not declared. Should it be static?

that function is unused and it is doing nothing. Further the use of 
ks_wlan_reset type functions seem uncommon anyway the only one I was 
able to locate that is kind of a comparable functions was:
  drivers/net/wireless/rndis_wlan.c:rndis_wlan_reset()

So I guess it really should just be dropped. 


thx!
hofrat 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: ks7010: declare private functions static

[Nicholas Mc Guire]

On Mon, Jul 25, 2016 at 11:04:18PM +0200, Wolfram Sang wrote:
> On Mon, Jul 25, 2016 at 09:22:27PM +0200, Nicholas Mc Guire wrote:
> > Private functions in ks_hostif.c can be declared static. 
> > 
> > Fixes: 13a9930d15b4 ("staging: ks7010: add driver from Nanonote 
> > extra-repository")
> > 
> > Signed-off-by: Nicholas Mc Guire <hof...@osadl.org>
> 
> Reviewed-by: Wolfram Sang <w...@the-dreams.de>
> 
> drivers/staging/ks7010/ks7010_sdio.c and
> drivers/staging/ks7010/ks_wlan_net.c have similar warnings in case you'd
> like to fix those, too.)
> 
the cases found regarding completion were:
./drivers/staging/ks7010/ks_hostif.c:80 treating signal case as success
./drivers/staging/ks7010/ks_wlan_net.c:109 treating signal case as success
./drivers/staging/ks7010/ks7010_sdio.c:901 treating signal case as success
./drivers/staging/ks7010/ks7010_sdio.c:929 treating signal case as success
./drivers/video/fbdev/exynos/exynos_mipi_dsi_common.c:383 treating signal case 
as success
./drivers/video/fbdev/exynos/exynos_mipi_dsi_common.c:247 treating signal case 
as success

will be going through all of them in the next days. 

thx!
hofrat
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: ks7010: declare private functions static

[Nicholas Mc Guire]

On Tue, Jul 26, 2016 at 08:51:14AM +0200, Wolfram Sang wrote:
> On Tue, Jul 26, 2016 at 06:48:00AM +0000, Nicholas Mc Guire wrote:
> > On Mon, Jul 25, 2016 at 11:04:18PM +0200, Wolfram Sang wrote:
> > > On Mon, Jul 25, 2016 at 09:22:27PM +0200, Nicholas Mc Guire wrote:
> > > > Private functions in ks_hostif.c can be declared static. 
> > > > 
> > > > Fixes: 13a9930d15b4 ("staging: ks7010: add driver from Nanonote 
> > > > extra-repository")
> > > > 
> > > > Signed-off-by: Nicholas Mc Guire <hof...@osadl.org>
> > > 
> > > Reviewed-by: Wolfram Sang <w...@the-dreams.de>
> > > 
> > > drivers/staging/ks7010/ks7010_sdio.c and
> > > drivers/staging/ks7010/ks_wlan_net.c have similar warnings in case you'd
> > > like to fix those, too.)
> > > 
> > the cases found regarding completion were:
> > ./drivers/staging/ks7010/ks_hostif.c:80 treating signal case as success
> > ./drivers/staging/ks7010/ks_wlan_net.c:109 treating signal case as success
> > ./drivers/staging/ks7010/ks7010_sdio.c:901 treating signal case as success
> > ./drivers/staging/ks7010/ks7010_sdio.c:929 treating signal case as success
> > ./drivers/video/fbdev/exynos/exynos_mipi_dsi_common.c:383 treating signal 
> > case as success
> > ./drivers/video/fbdev/exynos/exynos_mipi_dsi_common.c:247 treating signal 
> > case as success
> > 
> > will be going through all of them in the next days. 
> 
> Awesome, thanks!
> 
> I meant the "should it be static?" sparse warnings here, though :)
> 
well I do run sparse on all the cleanups and if that triggers 
and it is sufficiently clear from context, patches will follow.

thx!
hofrat

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: ks7010: fix wait_for_completion_interruptible_timeout return handling

[Nicholas Mc Guire]

On Mon, Jul 25, 2016 at 10:54:03PM +0200, Wolfram Sang wrote:
> On Mon, Jul 25, 2016 at 09:21:50PM +0200, Nicholas Mc Guire wrote:
> > wait_for_completion_interruptible_timeout return 0 on timeout and 
> > -ERESTARTSYS if interrupted. The check for 
> > !wait_for_completion_interruptible_timeout() would report an interrupt
> > as timeout. Further, while HZ/50 will work most of the time it could 
> 
> Wouldn't it interpret -ERESTARTSYS as *no timeout*?
>

yup - actually the current code just treats the -ERESTARTSYS 
case as success.
 
> Anyway, the plain !0 comparison for me clearly shows that
> 'interruptible' was more copy then really planned or supported.
> If it was, it would need to cancel something. Also, 20ms is pretty hard
> to cancel for a user ;) Given all that and the troubles we had with
> 'interruptible' in the I2C subsystem, I'd much vote for dropping
> interruptible here.
> 
> > fail for HZ < 50, so this is switched to msecs_to_jiffies(20).
> 
> Rest looks good, thanks!
> 

thx!
hofrat
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: ks7010: fix wait_for_completion_interruptible_timeout return handling

[Nicholas Mc Guire]

wait_for_completion_interruptible_timeout return 0 on timeout and 
-ERESTARTSYS if interrupted. The check for 
!wait_for_completion_interruptible_timeout() would report an interrupt
as timeout. Further, while HZ/50 will work most of the time it could 
fail for HZ < 50, so this is switched to msecs_to_jiffies(20).

Fixes: 13a9930d15b4 ("staging: ks7010: add driver from Nanonote 
extra-repository")

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

API non-compliance was located by coccinelle

Note: build showed some sparse warnings
CHECK   drivers/staging/ks7010/ks_hostif.c  
drivers/staging/ks7010/ks_hostif.c:72:6: warning: symbol
'ks_wlan_hw_wakeup_task' was not declared. Should it be static?
drivers/staging/ks7010/ks_hostif.c:1512:6: warning: symbol
'hostif_infrastructure_set2_request' was not declared. Should it be static?

Compile tested with: x86_64_defconfig + CONFIG_STAGING=y
CONFIG_MMC=m, CONFIG_KS7010=m

Patch is against 4.7-rc7 (localversion-next -next-20160725)

 drivers/staging/ks7010/ks_hostif.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/ks7010/ks_hostif.c 
b/drivers/staging/ks7010/ks_hostif.c
index a8822fe..4d32c98 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -74,11 +74,15 @@ void ks_wlan_hw_wakeup_task(struct work_struct *work)
struct ks_wlan_private *priv =
container_of(work, struct ks_wlan_private, ks_wlan_wakeup_task);
int ps_status = atomic_read(>psstatus.status);
+   long time_left;
 
if (ps_status == PS_SNOOZE) {
ks_wlan_hw_wakeup_request(priv);
-   if 
(!wait_for_completion_interruptible_timeout(>psstatus.wakeup_wait, HZ / 
50)) { /* 20ms timeout */
-   DPRINTK(1, "wake up timeout !!!\n");
+   time_left = wait_for_completion_interruptible_timeout(
+   >psstatus.wakeup_wait,
+   msecs_to_jiffies(20));
+   if (time_left <= 0) {
+   DPRINTK(1, "wake up timeout or interrupted !!!\n");
schedule_work(>ks_wlan_wakeup_task);
return;
}
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: ks7010: declare private functions static

[Nicholas Mc Guire]

Private functions in ks_hostif.c can be declared static. 

Fixes: 13a9930d15b4 ("staging: ks7010: add driver from Nanonote 
extra-repository")

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

Found by sparse

The build of ks_hostif.o generated the following sparse warnings:

CHECK   drivers/staging/ks7010/ks_hostif.c
drivers/staging/ks7010/ks_hostif.c:72:6: warning: symbol
'ks_wlan_hw_wakeup_task' was not declared. Should it be static?
drivers/staging/ks7010/ks_hostif.c:1512:6: warning: symbol
'hostif_infrastructure_set2_request' was not declared. Should it be static?

As both functions reported are private to this file it seems reasonable 
to declare them static.

Compile tested with: x86_64_defconfig + CONFIG_STAGING=y
CONFIG_MMC=m, CONFIG_KS7010=m

Patch is against 4.7-rc7 (localversion-next -next-20160725)

 drivers/staging/ks7010/ks_hostif.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/ks7010/ks_hostif.c 
b/drivers/staging/ks7010/ks_hostif.c
index a8822fe..71b6ba2 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -69,7 +69,7 @@ inline u32 get_DWORD(struct ks_wlan_private *priv)
return data;
 }
 
-void ks_wlan_hw_wakeup_task(struct work_struct *work)
+static void ks_wlan_hw_wakeup_task(struct work_struct *work)
 {
struct ks_wlan_private *priv =
container_of(work, struct ks_wlan_private, ks_wlan_wakeup_task);
@@ -1505,7 +1505,7 @@ void hostif_infrastructure_set_request(struct 
ks_wlan_private *priv)
ks_wlan_hw_tx(priv, pp, hif_align_size(sizeof(*pp)), NULL, NULL, NULL);
 }
 
-void hostif_infrastructure_set2_request(struct ks_wlan_private *priv)
+static void hostif_infrastructure_set2_request(struct ks_wlan_private *priv)
 {
struct hostif_infrastructure_set2_request_t *pp;
uint16_t capability;
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] media: add GFP flag to media_*() that could get called in atomic context

[Nicholas Mc Guire]

On Sat, Mar 12, 2016 at 06:48:09PM -0700, Shuah Khan wrote:
> Add GFP flags to media_create_pad_link(), media_create_intf_link(),
> media_devnode_create(), and media_add_link() that could get called
> in atomic context to allow callers to pass in the right flags for
> memory allocation.
> 
> tree-wide driver changes for media_*() GFP flags change:
> Change drivers to add gfpflags to interffaces, media_create_pad_link(),
> media_create_intf_link() and media_devnode_create().
>

in two cases (media_add_link,media_devnode_create) it is passing in 
gfpflags but then checking for in_atomic() and in case it is, dumping
stack - so does it make sense in those cases to allow GFP_ATOMIC to be
passed in ? could not figure out why that would be needed (current 
callers in media-entity.c do not seem to be under a spin_lock).

and in a few cases there seems to be a little glitch with indentation 
  dvb_create_media_graph
  __fimc_md_create_fimc_sink_links
  __fimc_md_create_fimc_is_links
  vsp1_create_sink_links
  v4l2_mc_create_media_graph
  vpfe_ipipeif_register_entities

thx!
hofrat
 
> Signed-off-by: Shuah Khan 
> Suggested-by: Mauro Carvalho Chehab 
> ---
> Ran through kbuild-all compile testing.
> Tested the changes in Win-TV HVR-950Q device
> 
>  drivers/media/dvb-core/dvbdev.c| 26 +++-
>  drivers/media/i2c/s5c73m3/s5c73m3-core.c   |  6 ++-
>  drivers/media/i2c/s5k5baf.c|  3 +-
>  drivers/media/i2c/smiapp/smiapp-core.c |  3 +-
>  drivers/media/i2c/tvp5150.c|  3 +-
>  drivers/media/media-entity.c   | 30 --
>  drivers/media/platform/exynos4-is/media-dev.c  | 19 +
>  drivers/media/platform/omap3isp/isp.c  | 47 
> ++
>  drivers/media/platform/s3c-camif/camif-core.c  |  4 +-
>  drivers/media/platform/vsp1/vsp1_drm.c |  6 +--
>  drivers/media/platform/vsp1/vsp1_drv.c |  9 +++--
>  drivers/media/platform/xilinx/xilinx-vipp.c|  4 +-
>  drivers/media/usb/au0828/au0828-core.c |  3 +-
>  drivers/media/usb/uvc/uvc_entity.c |  2 +-
>  drivers/media/v4l2-core/v4l2-dev.c |  5 ++-
>  drivers/media/v4l2-core/v4l2-device.c  |  3 +-
>  drivers/media/v4l2-core/v4l2-mc.c  | 25 +++-
>  drivers/staging/media/davinci_vpfe/dm365_ipipeif.c |  3 +-
>  drivers/staging/media/davinci_vpfe/dm365_isif.c|  2 +-
>  drivers/staging/media/davinci_vpfe/dm365_resizer.c | 10 +++--
>  .../staging/media/davinci_vpfe/vpfe_mc_capture.c   | 10 ++---
>  drivers/staging/media/omap4iss/iss.c   | 17 +---
>  drivers/staging/media/omap4iss/iss_csi2.c  |  6 ++-
>  drivers/staging/media/omap4iss/iss_ipipeif.c   |  3 +-
>  drivers/staging/media/omap4iss/iss_resizer.c   |  3 +-
>  include/media/media-entity.h   |  9 +++--
>  sound/usb/media.c  | 15 ---
>  27 files changed, 170 insertions(+), 106 deletions(-)
> 
> diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
> index e1684c5..57f3e1e 100644
> --- a/drivers/media/dvb-core/dvbdev.c
> +++ b/drivers/media/dvb-core/dvbdev.c
> @@ -399,7 +399,8 @@ static int dvb_register_media_device(struct dvb_device 
> *dvbdev,
>  
>   dvbdev->intf_devnode = media_devnode_create(dvbdev->adapter->mdev,
>   intf_type, 0,
> - DVB_MAJOR, minor);
> + DVB_MAJOR, minor,
> + GFP_KERNEL);
>  
>   if (!dvbdev->intf_devnode)
>   return -ENOMEM;
> @@ -416,7 +417,7 @@ static int dvb_register_media_device(struct dvb_device 
> *dvbdev,
>   return 0;
>  
>   link = media_create_intf_link(dvbdev->entity, 
> >intf_devnode->intf,
> -   MEDIA_LNK_FL_ENABLED);
> +   MEDIA_LNK_FL_ENABLED, GFP_KERNEL);
>   if (!link)
>   return -ENOMEM;
>  #endif
> @@ -558,7 +559,8 @@ static int dvb_create_io_intf_links(struct dvb_adapter 
> *adap,
>   if (strncmp(entity->name, name, strlen(name)))
>   continue;
>   link = media_create_intf_link(entity, intf,
> -   MEDIA_LNK_FL_ENABLED);
> +   MEDIA_LNK_FL_ENABLED,
> +   GFP_KERNEL);
>   if (!link)
>   return -ENOMEM;
>   }
> @@ -680,7 +682,8 @@ int dvb_create_media_graph(struct dvb_adapter *adap,
>   }
>   if (demux && ca) {
>   ret = media_create_pad_link(demux, 1, ca,
> -   

[PATCH V2] staging: rtl8712: consolidate kmalloc + memset 0 to kzalloc

[Nicholas Mc Guire]

This is an API consolidation only. The use of kmalloc + memset to 0
here is equivalent to kzalloc.

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

V2: use kzalloc rather than kcalloc which was not an equivalent
replacement as flaged by Joe Perches <j...@perches.com>

Patch was compile tested with: x86_64_defconfig +
CONFIG_STAGING=y, CONFIG_R8712U=m

Patch is against linux-next (localversion-next is -next-20160105)

 drivers/staging/rtl8712/rtl871x_recv.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl871x_recv.c 
b/drivers/staging/rtl8712/rtl871x_recv.c
index 4ff5301..2a8c492 100644
--- a/drivers/staging/rtl8712/rtl871x_recv.c
+++ b/drivers/staging/rtl8712/rtl871x_recv.c
@@ -72,14 +72,12 @@ sint _r8712_init_recv_priv(struct recv_priv *precvpriv,
_init_queue(>recv_pending_queue);
precvpriv->adapter = padapter;
precvpriv->free_recvframe_cnt = NR_RECVFRAME;
-   precvpriv->pallocated_frame_buf = kmalloc(NR_RECVFRAME *
+   precvpriv->pallocated_frame_buf = kzalloc(NR_RECVFRAME *
sizeof(union recv_frame) + RXFRAME_ALIGN_SZ,
GFP_ATOMIC);
if (precvpriv->pallocated_frame_buf == NULL)
return _FAIL;
kmemleak_not_leak(precvpriv->pallocated_frame_buf);
-   memset(precvpriv->pallocated_frame_buf, 0, NR_RECVFRAME *
-   sizeof(union recv_frame) + RXFRAME_ALIGN_SZ);
precvpriv->precv_frame_buf = precvpriv->pallocated_frame_buf +
RXFRAME_ALIGN_SZ -
((addr_t)(precvpriv->pallocated_frame_buf) &
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: rtl8712: consolidate kmalloc/memset 0 of array to kcalloc

[Nicholas Mc Guire]

This is an API consolidation only. The use of kmalloc of an array
of elements of equal size + memset to 0 of the same is equivalent to
kcalloc.

Signed-off-by: Nicholas Mc Guire <hof...@osadl.org>
---
Found by coccinelle script (relaxed version of
scripts/coccinelle/api/alloc/kzalloc-simple.cocci)

Patch was compile tested with: x86_64_defconfig +
CONFIG_STAGING=y, CONFIG_R8712U=m

Patch is against linux-next (localversion-next is -next-20160104)

 drivers/staging/rtl8712/rtl871x_recv.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl871x_recv.c 
b/drivers/staging/rtl8712/rtl871x_recv.c
index 4ff5301..aa841bf 100644
--- a/drivers/staging/rtl8712/rtl871x_recv.c
+++ b/drivers/staging/rtl8712/rtl871x_recv.c
@@ -72,14 +72,12 @@ sint _r8712_init_recv_priv(struct recv_priv *precvpriv,
_init_queue(>recv_pending_queue);
precvpriv->adapter = padapter;
precvpriv->free_recvframe_cnt = NR_RECVFRAME;
-   precvpriv->pallocated_frame_buf = kmalloc(NR_RECVFRAME *
+   precvpriv->pallocated_frame_buf = kcalloc(NR_RECVFRAME,
sizeof(union recv_frame) + RXFRAME_ALIGN_SZ,
GFP_ATOMIC);
if (precvpriv->pallocated_frame_buf == NULL)
return _FAIL;
kmemleak_not_leak(precvpriv->pallocated_frame_buf);
-   memset(precvpriv->pallocated_frame_buf, 0, NR_RECVFRAME *
-   sizeof(union recv_frame) + RXFRAME_ALIGN_SZ);
precvpriv->precv_frame_buf = precvpriv->pallocated_frame_buf +
RXFRAME_ALIGN_SZ -
((addr_t)(precvpriv->pallocated_frame_buf) &
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 3/3] staging/rdma/hfi1: fix build warning

[Nicholas Mc Guire]

fix the following build warning
drivers/staging/rdma/hfi1/chip.c: In function 'init_qos':
drivers/staging/rdma/hfi1/chip.c:10110:6: warning: unused variable
'rxcontext' [-Wunused-variable]

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

Patch was compile tested with: x86_64_defconfig
CONFIG_INFINIBAND=m, CONFIG_STAGING=y, CONFIG_STAGING_RDMA=m

Patch is against linux-next (localversion-next is -next-20151214)

 drivers/staging/rdma/hfi1/chip.c |1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c
index 52d2bd7..ec368a8 100644
--- a/drivers/staging/rdma/hfi1/chip.c
+++ b/drivers/staging/rdma/hfi1/chip.c
@@ -10107,7 +10107,6 @@ static void init_qos(struct hfi1_devdata *dd, u32 
first_ctxt)
unsigned qpns_per_vl, ctxt, i, qpn, n = 1, m;
u64 *rsmmap;
u64 reg;
-   u8  rxcontext = is_a0(dd) ? 0 : 0xff;  /* 0 is default if a0 ver. */
 
/* validate */
if (dd->n_krcv_queues <= MIN_KERNEL_KCTXTS ||
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/3] staging/rdma/hfi1: consolidate kmalloc_array+memset into kcalloc

[Nicholas Mc Guire]

rather than using kmalloc_array + memset it seems cleaner to simply use
kcalloc which will deliver memory set to zero.

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

Patch was compile tested with: x86_64_defconfig
CONFIG_INFINIBAND=m, CONFIG_STAGING=y, CONFIG_STAGING_RDMA=m

Patch is against linux-next (localversion-next is -next-20151214)

 drivers/staging/rdma/hfi1/chip.c |3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c
index dc69159..31eec8a 100644
--- a/drivers/staging/rdma/hfi1/chip.c
+++ b/drivers/staging/rdma/hfi1/chip.c
@@ -10128,8 +10128,7 @@ static void init_qos(struct hfi1_devdata *dd, u32 
first_ctxt)
goto bail;
if (num_vls * qpns_per_vl > dd->chip_rcv_contexts)
goto bail;
-   rsmmap = kmalloc_array(NUM_MAP_REGS, sizeof(u64), GFP_KERNEL);
-   memset(rsmmap, rxcontext, NUM_MAP_REGS * sizeof(u64));
+   rsmmap = kcalloc(NUM_MAP_REGS, sizeof(u64), GFP_KERNEL);
/* init the local copy of the table */
for (i = 0, ctxt = first_ctxt; i < num_vls; i++) {
unsigned tctxt;
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/3] staging/rdma/hfi1: check return value of kcalloc

[Nicholas Mc Guire]

Add a null check after the kcalloc call as proposed by
Mike Marciniszyn <mike.marcinis...@intel.com>.

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

Patch was compile tested with: x86_64_defconfig
CONFIG_INFINIBAND=m, CONFIG_STAGING=y, CONFIG_STAGING_RDMA=m

Patch is against linux-next (localversion-next is -next-20151214)

 drivers/staging/rdma/hfi1/chip.c |3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c
index 31eec8a..52d2bd7 100644
--- a/drivers/staging/rdma/hfi1/chip.c
+++ b/drivers/staging/rdma/hfi1/chip.c
@@ -10129,6 +10129,9 @@ static void init_qos(struct hfi1_devdata *dd, u32 
first_ctxt)
if (num_vls * qpns_per_vl > dd->chip_rcv_contexts)
goto bail;
rsmmap = kcalloc(NUM_MAP_REGS, sizeof(u64), GFP_KERNEL);
+   if (!rsmmap)
+   goto bail;
+
/* init the local copy of the table */
for (i = 0, ctxt = first_ctxt; i < num_vls; i++) {
unsigned tctxt;
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging/rdma/hfi1: Fix a possible null pointer dereference

[Nicholas Mc Guire]

On Thu, Dec 10, 2015 at 11:13:38AM -0500, Mike Marciniszyn wrote:
> From: Easwar Hariharan 
> 
> A code inspection pointed out that kmalloc_array may return NULL and
> memset doesn't check the input pointer for NULL, resulting in a possible
> NULL dereference. This patch fixes this.
> 
> Reviewed-by: Mike Marciniszyn 
> Signed-off-by: Easwar Hariharan 
> ---
>  drivers/staging/rdma/hfi1/chip.c |2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/staging/rdma/hfi1/chip.c 
> b/drivers/staging/rdma/hfi1/chip.c
> index dc69159..49d49b2 100644
> --- a/drivers/staging/rdma/hfi1/chip.c
> +++ b/drivers/staging/rdma/hfi1/chip.c
> @@ -10129,6 +10129,8 @@ static void init_qos(struct hfi1_devdata *dd, u32 
> first_ctxt)
>   if (num_vls * qpns_per_vl > dd->chip_rcv_contexts)
>   goto bail;
>   rsmmap = kmalloc_array(NUM_MAP_REGS, sizeof(u64), GFP_KERNEL);
> + if (!rsmmap)
> + goto bail;
>   memset(rsmmap, rxcontext, NUM_MAP_REGS * sizeof(u64));
>   /* init the local copy of the table */
>   for (i = 0, ctxt = first_ctxt; i < num_vls; i++) {
> 
> --

Based on this report a generalization of unchecked use turned up one more
case in the current kernel (patch sent). Probably the  when  block needs 
some cleanup, but findings like this definitely are a case for coccinelle 
scanners.


/// check for missing NULL check before use 
//
//  missing check in: 
//  ./drivers/staging/rdma/hfi1/chip.c:10131 unchecked allocation
//  in -next-20151214
//  reported-by Mike Marciniszyn  
//
//  after generalization this also found:
//  ./drivers/clk/shmobile/clk-div6.c:197 unchecked allocation

virtual context
virtual org
virtual report

@badmemset@
expression mem;
position p;
statement S;
@@

<+...
*mem = kmalloc_array@p(...);
  ... when != if (!mem || ...) S
  when != if (... && !mem) S
  when != if (mem == NULL || ...) S
  when != if (... && mem == NULL) S
  when != if (unlikely(mem == NULL)) S
  when != if (unlikely(!mem)) S
  when != if (likely(!mem)) S
  when != if (likely(mem == NULL)) S
  return;
...+>

@script:python@
p << badmemset.p;
@@

print "%s:%s unchecked allocation" % (p[0].file,p[0].line)



thx!
hofrat
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/3] staging/rdma/hfi1: consolidate kmalloc_array+memset into kcalloc

[Nicholas Mc Guire]

On Mon, Dec 14, 2015 at 03:28:46PM +, Marciniszyn, Mike wrote:
> > --- a/drivers/staging/rdma/hfi1/chip.c
> > +++ b/drivers/staging/rdma/hfi1/chip.c
> > @@ -10128,8 +10128,7 @@ static void init_qos(struct hfi1_devdata *dd,
> > u32 first_ctxt)
> > goto bail;
> > if (num_vls * qpns_per_vl > dd->chip_rcv_contexts)
> > goto bail;
> > -   rsmmap = kmalloc_array(NUM_MAP_REGS, sizeof(u64),
> > GFP_KERNEL);
> > -   memset(rsmmap, rxcontext, NUM_MAP_REGS * sizeof(u64));
> > +   rsmmap = kcalloc(NUM_MAP_REGS, sizeof(u64), GFP_KERNEL);
> > /* init the local copy of the table */
> > for (i = 0, ctxt = first_ctxt; i < num_vls; i++) {
> > unsigned tctxt;
> > --
> 
> I'm NAKing this.
> 
> There is a chip specific difference that accounts for the current code.
>
I obviously made a real mess here.
I incorrectly concluded that rxcontext is 0 which it is not in some cases

sorry for the noise.

thx!
hofrat



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [BUG ?] staging: rtl8723au: condition with no effect

[Nicholas Mc Guire]

On Sat, 13 Jun 2015, Jes Sorensen wrote:

 Nicholas Mc Guire hof...@osadl.org writes:
  scanning for trivial bug-patters with coccinelle spatches returned:
  ./drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c:1395
  WARNING: condition with no effect (if branch == else)
 
  drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c - line numbers from 
  4.1-rc7
  1395if (bWithoutHWSM) {
  1396/* value16 |= (APDM_HOST | FSM_HSUS |/PFM_ALDN); */
  1397/*  2010/08/31 According to Filen description, we need to
  1398use HW to shut down 8051 automatically. */
  1399/*  Because suspend operation need the asistance of 8051
  1400to wait for 3ms. */
  1401value16 = APDM_HOST | AFSM_HSUS | PFM_ALDN;
  1402} else {
  1403value16 = APDM_HOST | AFSM_HSUS | PFM_ALDN;
  1404}
  1405 
  1406rtl8723au_write16(padapter, REG_APS_FSMCO, value16);/* 0x4802 */
 
  Not clear what the intent is but this looks like a typo/bug in the assigment
  of value16 as the condition here has no effect.
 
 Doesn't look like a typo, looks like someone at some point had commented
 out PFM_ALDN in the bWithoutHWSM case. Why they did that and then later
 overrode it, I have no idea.

as far as its traceable in git log if == else was in there from the
very beginning (including that comment) - both the if and else were
updated in commit cffca68d7b2f (staging: rtl8723au: _DisableAnalog(): Avoid
zero-init variables unnecessaril) but without changing PFM_ALDN - as there
are no comments to the meaning of these bits in the header file there is
no way to really come up with a resonable patch. anyway its either wrong 
settings or the condition should be removed.

thx!
hofrat
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[BUG ?] staging: rtl8723au: condition with no effect

[Nicholas Mc Guire]

scanning for trivial bug-patters with coccinelle spatches returned:
./drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c:1395
WARNING: condition with no effect (if branch == else)

drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c - line numbers from 4.1-rc7
1395if (bWithoutHWSM) {
1396/* value16 |= (APDM_HOST | FSM_HSUS |/PFM_ALDN); */
1397/*  2010/08/31 According to Filen description, we need to
1398use HW to shut down 8051 automatically. */
1399/*  Because suspend operation need the asistance of 8051
1400to wait for 3ms. */
1401value16 = APDM_HOST | AFSM_HSUS | PFM_ALDN;
1402} else {
1403value16 = APDM_HOST | AFSM_HSUS | PFM_ALDN;
1404}
1405 
1406rtl8723au_write16(padapter, REG_APS_FSMCO, value16);/* 0x4802 */

Not clear what the intent is but this looks like a typo/bug in the assigment
of value16 as the condition here has no effect.

thx!
hofrat
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging: dgnc: use schedule_timeout_interruptible()

[Nicholas Mc Guire]

On Sun, 31 May 2015, Greg Kroah-Hartman wrote:

 On Fri, May 29, 2015 at 06:41:27PM +0200, Nicholas Mc Guire wrote:
  API consolidation with coccinelle found:
  ./drivers/staging/dgnc/dgnc_utils.c:16:1-17:
  consolidation with schedule_timeout_*() recommended
  
  This is a 1:1 conversion with respect to schedule_timeout() to the
  schedule_timeout_interruptible() helper only - so only an API
  consolidation to improve readability. The timeout was being passed
  as (ms * HZ) / 1000 but that is not reliable as it allows the timeout
  to become 0 for small values of ms. As this cut-off is HZ dependent
  this is most likely not intended, so the timeout is converted with 
  msecs_to_jiffies which handles all corener-cases correctly.
  
  Patch was compile tested with x86_64_defconfig + CONFIG_STAGING=y,
  CONFIG_DGNC=m
  
  Patch is against 4.1-rc5 (localversion-next is -next-20150529)
 
 Can you resend this without these two sentances?  They are not needed
 and are just implied as you should have done this for every patch
 submitted.

The config does allow for some level of variantion (e.g. what hardware
it was compile tested for) and also if it was a module or built-in.

I originally put this below the --- until I was explicitly ast to put 
it above http://lkml.org/lkml/2015/5/11/552

will fix it up and resend.

thx!
hofrat 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: unisys: use schedule_timeout_interruptible()

[Nicholas Mc Guire]

API consolidation with coccinelle found:
./drivers/staging/unisys/visorbus/periodic_work.c:196:3-19:
consolidation with schedule_timeout_*() recommended

This is a 1:1 conversion with respect to schedule_timeout() to the 
schedule_timeout_interruptible() helper only - so only an API 
consolidation to improve readability. The hard coded timeout of 10
jiffies is HZ dependent which it should not be, so it is converted
with msecs_to_jiffies.

Patch was compile tested with x86_64_defconfig + CONFIG_STAGING=y,
CONFIG_UNISYSSPAR=y, CONFIG_UNISYS_VISORBUS=m

Patch is against 4.1-rc5 (localversion-next is -next-20150529)

Signed-off-by: Nicholas Mc Guire hof...@osadl.org
---

As the actually intended timeout is not documented and msecs_to_jiffies
timeouts can be a factor 10 different from the current effective timeout
this needs to be checked by someone who knows the details of this driver
in any case it should be passed in a HZ independent manner.

 drivers/staging/unisys/visorbus/periodic_work.c |3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/periodic_work.c 
b/drivers/staging/unisys/visorbus/periodic_work.c
index 3562e8b..5e56088 100644
--- a/drivers/staging/unisys/visorbus/periodic_work.c
+++ b/drivers/staging/unisys/visorbus/periodic_work.c
@@ -192,8 +192,7 @@ bool visor_periodic_work_stop(struct periodic_work *pw)
}
if (pw-is_scheduled) {
write_unlock(pw-lock);
-   __set_current_state(TASK_INTERRUPTIBLE);
-   schedule_timeout(10);
+   schedule_timeout_interruptible(msecs_to_jiffies(10));
write_lock(pw-lock);
} else {
pw-want_to_stop = false;
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/2] staging: dgnc: use schedule_timeout_interruptible()

[Nicholas Mc Guire]

API consolidation with coccinelle found:
./drivers/staging/dgnc/dgnc_utils.c:16:1-17:
consolidation with schedule_timeout_*() recommended

This is a 1:1 conversion with respect to schedule_timeout() to the
schedule_timeout_interruptible() helper only - so only an API
consolidation to improve readability. The timeout was being passed
as (ms * HZ) / 1000 but that is not reliable as it allows the timeout
to become 0 for small values of ms. As this cut-off is HZ dependent
this is most likely not intended, so the timeout is converted with 
msecs_to_jiffies which handles all corener-cases correctly.

Patch was compile tested with x86_64_defconfig + CONFIG_STAGING=y,
CONFIG_DGNC=m

Patch is against 4.1-rc5 (localversion-next is -next-20150529)

Signed-off-by: Nicholas Mc Guire hof...@osadl.org
---
 drivers/staging/dgnc/dgnc_utils.c |3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_utils.c 
b/drivers/staging/dgnc/dgnc_utils.c
index f76de82..0cbb8a1 100644
--- a/drivers/staging/dgnc/dgnc_utils.c
+++ b/drivers/staging/dgnc/dgnc_utils.c
@@ -12,7 +12,6 @@
  */
 int dgnc_ms_sleep(ulong ms)
 {
-   __set_current_state(TASK_INTERRUPTIBLE);
-   schedule_timeout((ms * HZ) / 1000);
+   schedule_timeout_interruptible(msecs_to_jiffies(ms));
return signal_pending(current);
 }
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/2] staging: dgnc: switch timeout to signed type

[Nicholas Mc Guire]

The schedule_timeout*() helpers take the timeout as signed long, as
ch_close_delay in struct channel_t was not used for other purposes its
type was switched to signed long and the declarations fixed up.

Patch was compile tested with x86_64_defconfig + CONFIG_STAGING=y,
CONFIG_DGNC=m

Patch is against 4.1-rc5 (localversion-next is -next-20150529)

Signed-off-by: Nicholas Mc Guire hof...@osadl.org
---
Note that there is a over 80 char warning here that was not fixed as
there are quite a few in dgnc_driver.h.

 drivers/staging/dgnc/dgnc_driver.h |2 +-
 drivers/staging/dgnc/dgnc_utils.c  |2 +-
 drivers/staging/dgnc/dgnc_utils.h  |2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_driver.h 
b/drivers/staging/dgnc/dgnc_driver.h
index f77fed5..5cbeb4d 100644
--- a/drivers/staging/dgnc/dgnc_driver.h
+++ b/drivers/staging/dgnc/dgnc_driver.h
@@ -320,7 +320,7 @@ struct channel_t {
uintch_open_count;  /* open count   */
uintch_flags;   /* Channel flags*/
 
-   ulong   ch_close_delay; /* How long we should drop RTS/DTR for 
*/
+   longch_close_delay; /* How long we should drop RTS/DTR for 
*/
 
ulong   ch_cpstime; /* Time for CPS calculations*/
 
diff --git a/drivers/staging/dgnc/dgnc_utils.c 
b/drivers/staging/dgnc/dgnc_utils.c
index 0cbb8a1..4f7f86b 100644
--- a/drivers/staging/dgnc/dgnc_utils.c
+++ b/drivers/staging/dgnc/dgnc_utils.c
@@ -10,7 +10,7 @@
  *
  * Returns 0 if timed out, !0 (showing signal) if interrupted by a signal.
  */
-int dgnc_ms_sleep(ulong ms)
+int dgnc_ms_sleep(signed long ms)
 {
schedule_timeout_interruptible(msecs_to_jiffies(ms));
return signal_pending(current);
diff --git a/drivers/staging/dgnc/dgnc_utils.h 
b/drivers/staging/dgnc/dgnc_utils.h
index 1164c3a..44cb479 100644
--- a/drivers/staging/dgnc/dgnc_utils.h
+++ b/drivers/staging/dgnc/dgnc_utils.h
@@ -1,6 +1,6 @@
 #ifndef __DGNC_UTILS_H
 #define __DGNC_UTILS_H
 
-int dgnc_ms_sleep(ulong ms);
+int dgnc_ms_sleep(signed long ms);
 
 #endif
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: me_daq: use schedule_timeout_interruptible()

[Nicholas Mc Guire]

On Fri, 29 May 2015, Ian Abbott wrote:

 On 29/05/15 16:58, Nicholas Mc Guire wrote:
 API consolidation with coccinelle found:
 ./drivers/staging/comedi/drivers/me_daq.c:177:1-17:
  consolidation with schedule_timeout_*() recommended

 This is a 1:1 conversion of the current calls to an available helper
 only - so only an API consolidation to improve readability.

 Patch was compile tested with x86_64_defconfig + CONFIG_STAGING=y,
 CONFIG_COMEDI=y, CONFIG_COMEDI_PCI_DRIVERS=y CONFIG_COMEDI_ME_DAQ=m

 Patch is against 4.1-rc5 (localversion-next is -next-20150529)

 Minor niggle: you don't really need to say what version the patch is  
 against in the commit message, as the version will have changed by the  
 time the patch is committed.  It can be mentioned after the --- marker  
 line if relevant, as the stuff after the --- line does not end up in  
 the commit message.

makes sense - will move that down for the other cleanups.

thx!
hofrat
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] staging: dgnc: switch timeout to signed type

[Nicholas Mc Guire]

On Fri, 29 May 2015, Dan Carpenter wrote:

 On Fri, May 29, 2015 at 06:41:28PM +0200, Nicholas Mc Guire wrote:
  The schedule_timeout*() helpers take the timeout as signed long, as
  ch_close_delay in struct channel_t was not used for other purposes its
  type was switched to signed long and the declarations fixed up.
 
 Uh, we never pass it to schedule_timeout etc and even if we did how
 would that matter?  It's either 250 or 0.
 
 What is the bug you are trying to fix and we can help you?

static code checkers being unhappy with type mismatch
automatic type conversion is ok if necessary but in this
case it simply is not as the ch_close_delay is only being
used in this one place so why not do it type clean ?
I'll turn the question around - what reason would there be to
go through type conversion if it is not needed ?

thx!
hofrat 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: panel: use schedule_timeout_interruptible()

[Nicholas Mc Guire]

API consolidation with coccinelle found:
./drivers/staging/panel/panel.c:782:2-18:
consolidation with schedule_timeout_*() recommended

This is a 1:1 conversion with respect to schedule_timeout() to the
schedule_timeout_interruptible() helper only - so only an API
consolidation to improve readability. The timeout was being passed
as (ms * HZ + 999) / 1000 but that simply looks wrong - rather than
manual converting to jiffies, msecs_to_jiffies which handles all 
corner-cases correctly, was used.

Patch was compile tested with x86_64_defconfig + CONFIG_STAGING=y,
CONFIG_PARPORT=m, CONFIG_PANEL=m

Signed-off-by: Nicholas Mc Guire hof...@osadl.org
---
Patch is against 4.1-rc5 (localversion-next is -next-20150529)

not really clear what the intent of (ms * HZ + 999) / 1000 was - this 
is HZ dependent and does not really make sense - the comment states
sleeps that many milliseconds so it probably simply should be
msecs_to_jiffies(ms) - but someone that knows the intention of this code
needs to check this.

 drivers/staging/panel/panel.c |8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index 0046ee0..d670494 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -775,12 +775,10 @@ static void pin_to_bits(int pin, unsigned char *d_val, 
unsigned char *c_val)
 /* sleeps that many milliseconds with a reschedule */
 static void long_sleep(int ms)
 {
-   if (in_interrupt()) {
+   if (in_interrupt())
mdelay(ms);
-   } else {
-   __set_current_state(TASK_INTERRUPTIBLE);
-   schedule_timeout((ms * HZ + 999) / 1000);
-   }
+   else
+   schedule_timeout_interruptible(msecs_to_jiffies(ms));
 }
 
 /* send a serial byte to the LCD panel. The caller is responsible for locking
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] staging: dgnc: switch timeout to signed type

[Nicholas Mc Guire]

On Fri, 29 May 2015, Dan Carpenter wrote:

 On Fri, May 29, 2015 at 07:21:26PM +0200, Nicholas Mc Guire wrote:
  On Fri, 29 May 2015, Dan Carpenter wrote:
  
   On Fri, May 29, 2015 at 06:41:28PM +0200, Nicholas Mc Guire wrote:
The schedule_timeout*() helpers take the timeout as signed long, as
ch_close_delay in struct channel_t was not used for other purposes its
type was switched to signed long and the declarations fixed up.
   
   Uh, we never pass it to schedule_timeout etc and even if we did how
   would that matter?  It's either 250 or 0.
   
   What is the bug you are trying to fix and we can help you?
  
  static code checkers being unhappy with type mismatch
  automatic type conversion is ok if necessary but in this
  case it simply is not as the ch_close_delay is only being
  used in this one place so why not do it type clean ?
 
 This seems like a pointless warning.  What does the warning look like?
 We pass ms to msecs_to_jiffies() and not to schedule_timeout() so it
 seems like somewhere something is confused.

Not really - just my carelessness - the msecs_to_jiffies was not in there
and I fixed up the types first - then put the msecs_to_jiffies in there
to fix up the time conversion ...oh well took the type conversion out
just to put it back in my self...sorry thats a bit braindead.
thanks for catching that.

 
  I'll turn the question around - what reason would there be to
  go through type conversion if it is not needed ?
 
 You can go crazy if you do ever pointless change which a static analysis
 tool suggests...
 
 Btw, Smatch says that ms is always 250 here, actually.  I was guessing
 earlier when I said it could be zero.  Get a smarter static checker
 which can read code. 

wont blame it on coccinelle - its my scripts that are to blame - but in 
this case it was the cleanup after the fix for the warning that broke
it.

so 2/2 is pointless - sorry for that - pleas just toss it.

thx!
hofrat
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: rts5208: pass timeout as HZ independent value

[Nicholas Mc Guire]

schedule_timeout takes a timeout in jiffies but the code currently is
passing in a constant POLLING_INTERVAL which makes this timeout HZ
dependent, so pass it through msecs_to_jiffies() to fix this up.

patch was compile tested for x86_64_defconfig + CONFIG_STAGING=y,
CONFIG_RTS5208=m

Patch is against 4.0-rc5 (localversion-next is -next-20150527)

Signed-off-by: Nicholas Mc Guire hof...@osadl.org
---

As there is no documentation of the intended timeout it might be wrong
to convert it with msecs_to_jiffies - so someone that knows this driver
needs to check on the actual value - but in any case it needs to be
passed in a HZ independent way.

 drivers/staging/rts5208/rtsx.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
index d64b6ed..1bcadba 100644
--- a/drivers/staging/rts5208/rtsx.c
+++ b/drivers/staging/rts5208/rtsx.c
@@ -537,7 +537,7 @@ static int rtsx_polling_thread(void *__dev)
for (;;) {
 
set_current_state(TASK_INTERRUPTIBLE);
-   schedule_timeout(POLLING_INTERVAL);
+   schedule_timeout(msecs_to_jiffies(POLLING_INTERVAL));
 
/* lock the device pointers */
mutex_lock((dev-dev_mutex));
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2] staging: rtl8188eu: core: switch with redundant cases

[Nicholas Mc Guire]

A few redundant switch cases as well as a redundant if/else
within one of the cases was consolidated to a single call.

case WIFI_REASSOCREQ,WIFI_PROBEREQ,WIFI_BEACON,WIFI_ACTION all have the 
same effect - notably also both the if and else in the WIFI_PROBEREQ case. 

These redundant cases could all be dropped and consolidated into the 
default but for documentation they are combined into a fall through case.

Signed-off-by: Nicholas Mc Guire hof...@osadl.org
---

v2: verified that check_fwstate() has no side-effects requested by
Rasmus Villemoes li...@rasmusvillemoes.dk, and fixed up the log
message based on feedback from Dan Carpenter 
dan.carpen...@oracle.com (hope this is better now)

Patch was only compile-tested for x86_64_defconfig + CONFIG_STAGING=y
CONFIG_R8188EU=m

Patch is against 3.19.0-rc7 (localversion-next is -next-20150204)

 drivers/staging/rtl8188eu/core/rtw_mlme_ext.c |9 -
 1 file changed, 9 deletions(-)

diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c 
b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
index 28918201..cd12dd7 100644
--- a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
@@ -484,17 +484,8 @@ void mgt_dispatcher(struct adapter *padapter, struct 
recv_frame *precv_frame)
/* fall through */
case WIFI_ASSOCREQ:
case WIFI_REASSOCREQ:
-   _mgt_dispatcher(padapter, ptable, precv_frame);
-   break;
case WIFI_PROBEREQ:
-   if (check_fwstate(pmlmepriv, WIFI_AP_STATE))
-   _mgt_dispatcher(padapter, ptable, precv_frame);
-   else
-   _mgt_dispatcher(padapter, ptable, precv_frame);
-   break;
case WIFI_BEACON:
-   _mgt_dispatcher(padapter, ptable, precv_frame);
-   break;
case WIFI_ACTION:
_mgt_dispatcher(padapter, ptable, precv_frame);
break;
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RFC] staging: comedi: dt282x: condition with no effect - if identical to else

[Nicholas Mc Guire]

On Wed, 04 Feb 2015, Hartley Sweeten wrote:

 On Tuesday, February 03, 2015 8:13 AM, Ian Abbott wrote:
  On 03/02/15 12:38, Nicholas Mc Guire wrote:
  The if and the else branch code are identical - so the condition has no
  effect on the effective code - this patch removes the condition and the
  duplicated code.
 
  Signed-off-by: Nicholas Mc Guire hof...@osadl.org
  ---
 
  The if and else branch are identical code thus the condition has no effect
 
   if (cmd-scan_begin_src == TRIG_FOLLOW) {
   /* internal trigger */
   err |= cfc_check_trigger_arg_is(cmd-scan_begin_arg, 0);
   } else {
   /* external trigger */
   /* should be level/edge, hi/lo specification here */
   err |= cfc_check_trigger_arg_is(cmd-scan_begin_arg, 0);
   }
 
  I think what that comment means is that it should allow scan_begin_arg 
  to have various combinations of the CR_EDGE and CR_INVERT bits set. 
  I.e. it ought to allow whatever combination of CR_EDGE and CR_INVERT 
  better describes the nature of the external trigger signal, in addition 
  to allowing the lazy default value 0.
 
  I don't know what the nature of the external trigger signal is, as I 
  haven't seen the manual.  I think Hartley might have seen one.
 
 According to the manual, the external trigger is not programmable. It's
 a Schmitt trigger input, enables on TTL logic low, with a 22K pullup.
 
 Since the 'scan_begin_arg' is not actually used for the analog input async
 command, I think removing the comments completely is fine. Just change
 the check to:
 
   err |= cfc_check_trigger_arg_is(cmd-scan_begin_arg, 0);


thanks for that clarification - will fix it up and resend.

thx!
hofrat
 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2] staging: comedi: dt282x: condition with no effect - if identical to else

[Nicholas Mc Guire]

The if and the else branch code are identical - so the condition has no
effect on the effective code - this patch removes the condition and the
duplicated code.

Signed-off-by: Nicholas Mc Guire hof...@osadl.org
---

v2: Review notes from Ian Abbott abbo...@mev.co.uk and Hartley Sweeten 
hartl...@visionengravers.com confirm that the condition is not 
needed and, as suggested, the misleading comment is completely removed.

Patch was only compile tested for x86_64_defconfig + CONFIG_STAGING=y
CONFIG_COMEDI=m, COMEDI_ISA_DRIVERS=y, CONFIG_COMEDI_DT282X=m

Patch is against 3.19.0-rc7 (localversion = -next-20150204)

 drivers/staging/comedi/drivers/dt282x.c |9 +
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/staging/comedi/drivers/dt282x.c 
b/drivers/staging/comedi/drivers/dt282x.c
index 051dfb2..db21d21 100644
--- a/drivers/staging/comedi/drivers/dt282x.c
+++ b/drivers/staging/comedi/drivers/dt282x.c
@@ -685,14 +685,7 @@ static int dt282x_ai_cmdtest(struct comedi_device *dev,
 
err |= cfc_check_trigger_arg_is(cmd-start_arg, 0);
 
-   if (cmd-scan_begin_src == TRIG_FOLLOW) {
-   /* internal trigger */
-   err |= cfc_check_trigger_arg_is(cmd-scan_begin_arg, 0);
-   } else {
-   /* external trigger */
-   /* should be level/edge, hi/lo specification here */
-   err |= cfc_check_trigger_arg_is(cmd-scan_begin_arg, 0);
-   }
+   err |= cfc_check_trigger_arg_is(cmd-scan_begin_arg, 0);
 
err |= cfc_check_trigger_arg_min(cmd-convert_arg, 4000);
 
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] storvsc: assign wait_for_completion_timeout to appropriately typed var

[Nicholas Mc Guire]

wait_for_completion_timeout() returns unsigned long not int. This assigns
the return value to an appropriately typed variable.

Signed-off-by: Nicholas Mc Guire hof...@osadl.org
---

As a suitable typed and named variable timeout is available and there is
no conflict in this case no new variable is needed rather the declaration
is simply updated.

Patch was only compile tested for x86_64_defconfig + CONFIG_X86_VSMP=y
CONFIG_HYPERV=m, SCSI_LOWLEVEL=y, CONFIG_HYPERV_STORAGE=m

Patch is against 3.19.0-rc7 (localversion-next is -next-20150204)

 drivers/scsi/storvsc_drv.c |6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index efc6e44..a993d12 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -796,7 +796,8 @@ static void  handle_multichannel_storage(struct hv_device 
*device, int max_chns)
int num_sc;
struct storvsc_cmd_request *request;
struct vstor_packet *vstor_packet;
-   int ret, t;
+   int ret;
+   unsigned long t;
 
num_sc = ((max_chns  num_cpus) ? num_cpus : max_chns);
stor_device = get_out_stor_device(device);
@@ -861,7 +862,8 @@ static int storvsc_channel_init(struct hv_device *device)
struct storvsc_device *stor_device;
struct storvsc_cmd_request *request;
struct vstor_packet *vstor_packet;
-   int ret, t;
+   int ret;
+   unsigned long t;
int max_chns;
bool process_sub_channels = false;
 
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/2] staging: rtl8188eu: odm: condition with no effect

[Nicholas Mc Guire]

The if and the else branch code are identical - so the condition has no
effect on the effective code - this patch removes the condition and the
duplicated code.

Signed-off-by: Nicholas Mc Guire hof...@osadl.org
---

The if/else performing the same settings in both branches so the
condition has no effect.

This needs a check by someone who knows the details of the driver 
as it could be that the two branches actually should be different.

Patch was only compile-tested for x86_64_defconfig + CONFIG_STAGING=y
CONFIG_R8188EU=m

Patch is against 3.19.0-rc7 (localversion-next is -next-20150204)

 drivers/staging/rtl8188eu/hal/odm.c |9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/rtl8188eu/hal/odm.c 
b/drivers/staging/rtl8188eu/hal/odm.c
index 9873998..878c460 100644
--- a/drivers/staging/rtl8188eu/hal/odm.c
+++ b/drivers/staging/rtl8188eu/hal/odm.c
@@ -534,13 +534,8 @@ void odm_DIGInit(struct odm_dm_struct *pDM_Odm)
pDM_DigTable-RssiHighThresh= DM_DIG_THRESH_HIGH;
pDM_DigTable-FALowThresh   = DM_false_ALARM_THRESH_LOW;
pDM_DigTable-FAHighThresh  = DM_false_ALARM_THRESH_HIGH;
-   if (pDM_Odm-BoardType == ODM_BOARD_HIGHPWR) {
-   pDM_DigTable-rx_gain_range_max = DM_DIG_MAX_NIC;
-   pDM_DigTable-rx_gain_range_min = DM_DIG_MIN_NIC;
-   } else {
-   pDM_DigTable-rx_gain_range_max = DM_DIG_MAX_NIC;
-   pDM_DigTable-rx_gain_range_min = DM_DIG_MIN_NIC;
-   }
+   pDM_DigTable-rx_gain_range_max = DM_DIG_MAX_NIC;
+   pDM_DigTable-rx_gain_range_min = DM_DIG_MIN_NIC;
pDM_DigTable-BackoffVal = DM_DIG_BACKOFF_DEFAULT;
pDM_DigTable-BackoffVal_range_max = DM_DIG_BACKOFF_MAX;
pDM_DigTable-BackoffVal_range_min = DM_DIG_BACKOFF_MIN;
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/2] staging: rtl8188eu: odm: conditional setting with no effect

[Nicholas Mc Guire]

The if and the else branch code are identical - so the condition has no
effect on the effective code - this patch removes the condition and the
duplicated code. Due to this being a fall-through-if here - the first
if condition has no effect either - so it also can be removed. 
struct mlme_priv is thus also no longer needed here.

Signed-off-by: Nicholas Mc Guire hof...@osadl.org
---

The if/else executes the same code in both branches so the
condition has no effect.

This needs a check by someone who knows the details of the driver
as it could be that the two branches actually should be different.
From the comments it seems that an effect is actually intended so this
may be a bug.

The comment was updated to reflect the changes

Patch was only compile-tested for x86_64_defconfig + CONFIG_STAGING=y
CONFIG_R8188EU=m

Patch is against 3.19.0-rc7 (localversion-next is -next-20150204)

 drivers/staging/rtl8188eu/hal/odm.c |   13 +++--
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/rtl8188eu/hal/odm.c 
b/drivers/staging/rtl8188eu/hal/odm.c
index 878c460..06477e8 100644
--- a/drivers/staging/rtl8188eu/hal/odm.c
+++ b/drivers/staging/rtl8188eu/hal/odm.c
@@ -1133,16 +1133,9 @@ static void FindMinimumRSSI(struct adapter *pAdapter)
 {
struct hal_data_8188e   *pHalData = GET_HAL_DATA(pAdapter);
struct dm_priv  *pdmpriv = pHalData-dmpriv;
-   struct mlme_priv*pmlmepriv = pAdapter-mlmepriv;
-
-   /* 1 1.Determine the minimum RSSI */
-   if ((check_fwstate(pmlmepriv, _FW_LINKED) == false) 
-   (pdmpriv-EntryMinUndecoratedSmoothedPWDB == 0))
-   pdmpriv-MinUndecoratedPWDBForDM = 0;
-   if (check_fwstate(pmlmepriv, _FW_LINKED) == true)   /*  Default 
port */
-   pdmpriv-MinUndecoratedPWDBForDM = 
pdmpriv-EntryMinUndecoratedSmoothedPWDB;
-   else /*  associated entry pwdb */
-   pdmpriv-MinUndecoratedPWDBForDM = 
pdmpriv-EntryMinUndecoratedSmoothedPWDB;
+
+   /* 1 1.Unconditionally set RSSI */
+   pdmpriv-MinUndecoratedPWDBForDM = 
pdmpriv-EntryMinUndecoratedSmoothedPWDB;
 }
 
 void odm_RSSIMonitorCheckCE(struct odm_dm_struct *pDM_Odm)
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8188eu: core: switch with redundant cases

[Nicholas Mc Guire]

On Wed, 04 Feb 2015, Dan Carpenter wrote:

 Btw, what tool are you using to find these?

working on a set of coccinell scripts - they are not yet
really clean - but this one is simply:


virtual context
virtual patch
virtual org
virtual report

@assign@
position p;
statement S1;
@@

+...
* if@p (...) S1 else S1
...+

@script:python@
p  assign.p;
@@

print %s:%s WARNING: condition with no effect % (p[0].file,p[0].line)



I guess this is not wildly impressive - but it seems to be effective
in finding a lot of broken code. The rate of false postivs has been
supprisingly low (very few case of intentional if==else like in
./fs/kernfs/file.c:661 - which is nicely documented)

thx!
hofrat
 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: rtl8188eu: core: switch with redundant cases

[Nicholas Mc Guire]

A few redundant switch cases as well as a redundant if/else
within one of the cases was consolidated to a single call.
The cases are intentionally retained for documentation purposes.

Signed-off-by: Nicholas Mc Guire hof...@osadl.org
---
case WIFI_REASSOCREQ,WIFI_PROBEREQ,WIFI_BEACON,WIFI_ACTION all
have the same effect - notably the also for WIFI_PROBEREQ where
the if/else is executing the same function. 

These redundant cases could all be dropped and consolidated into
the default but probably it is better for documentation/readability
to leave them in the switch/case explicitly.

Patch was only compile-tested for x86_64_defconfig + CONFIG_STAGING=y
CONFIG_R8188EU=m

Patch is against 3.19.0-rc7 (localversion-next is -next-20150204)

 drivers/staging/rtl8188eu/core/rtw_mlme_ext.c |9 -
 1 file changed, 9 deletions(-)

diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c 
b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
index 28918201..cd12dd7 100644
--- a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
@@ -484,17 +484,8 @@ void mgt_dispatcher(struct adapter *padapter, struct 
recv_frame *precv_frame)
/* fall through */
case WIFI_ASSOCREQ:
case WIFI_REASSOCREQ:
-   _mgt_dispatcher(padapter, ptable, precv_frame);
-   break;
case WIFI_PROBEREQ:
-   if (check_fwstate(pmlmepriv, WIFI_AP_STATE))
-   _mgt_dispatcher(padapter, ptable, precv_frame);
-   else
-   _mgt_dispatcher(padapter, ptable, precv_frame);
-   break;
case WIFI_BEACON:
-   _mgt_dispatcher(padapter, ptable, precv_frame);
-   break;
case WIFI_ACTION:
_mgt_dispatcher(padapter, ptable, precv_frame);
break;
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8723au: multiple condition with no effect - if identical to else

[Nicholas Mc Guire]

On Tue, 03 Feb 2015, Jes Sorensen wrote:

 Nicholas Mc Guire hof...@osadl.org writes:
  A number if/else if/else branches are identical - so the condition has no
  effect on the effective code and can be significantly simplified - this 
  patch removes the condition and the duplicated code.
 
  Signed-off-by: Nicholas Mc Guire hof...@osadl.org
  ---
 
  This looks like the output of some broken code-generator - unlikely someone
  wrote this by hand. In any case it needs a review by someone that knows the
  details of the driver. 
 
  Anyway the number of useless code repetition is potentially record breaking 
  !
 
  Patch was compile tested for x86_64_defconfig + CONFIG_STAGING=y
  CONFIG_R8723AU=m, CONFIG_8723AU_BT_COEXIST=y
 
  Patch is against 3.0.19-rc7 (localversoin = -next-20150203)
 
   .../staging/rtl8723au/hal/rtl8723a_bt-coexist.c|   60 
  +++-
   1 file changed, 8 insertions(+), 52 deletions(-)
 
 Why make it simple if you can make it complicated :)
 
 I presume it's against 3.19-rc7 since a 3.0.19 would be rather obsolete.


yes - thats a typo - sorry for that.
 
 Signed-off-by: Jes Sorensen jes.soren...@redhat.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH RFC] staging: comedi: dt282x: condition with no effect - if identical to else

[Nicholas Mc Guire]

The if and the else branch code are identical - so the condition has no
effect on the effective code - this patch removes the condition and the
duplicated code.

Signed-off-by: Nicholas Mc Guire hof...@osadl.org
---

The if and else branch are identical code thus the condition has no effect

if (cmd-scan_begin_src == TRIG_FOLLOW) {
/* internal trigger */
err |= cfc_check_trigger_arg_is(cmd-scan_begin_arg, 0);
} else {
/* external trigger */
/* should be level/edge, hi/lo specification here */
err |= cfc_check_trigger_arg_is(cmd-scan_begin_arg, 0);
}

As the comments indicate that they are serving different purposes this
looks like a bug. In any case - if intentional - it would need some
comments on why.

This needs a review by someone that knows the details of this driver.
Also not sure about the retained comment string if that is still valid now.

Patch was only compile tested for x86_64_defconfig + CONFIG_STAGING=y
CONFIG_COMEDI=m, COMEDI_ISA_DRIVERS=y, CONFIG_COMEDI_DT282X=m

Patch is against 3.0.19-rc7 (localversion = -next-20150203)

 drivers/staging/comedi/drivers/dt282x.c |   10 ++
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/comedi/drivers/dt282x.c 
b/drivers/staging/comedi/drivers/dt282x.c
index 051dfb2..22c59e5 100644
--- a/drivers/staging/comedi/drivers/dt282x.c
+++ b/drivers/staging/comedi/drivers/dt282x.c
@@ -685,14 +685,8 @@ static int dt282x_ai_cmdtest(struct comedi_device *dev,
 
err |= cfc_check_trigger_arg_is(cmd-start_arg, 0);
 
-   if (cmd-scan_begin_src == TRIG_FOLLOW) {
-   /* internal trigger */
-   err |= cfc_check_trigger_arg_is(cmd-scan_begin_arg, 0);
-   } else {
-   /* external trigger */
-   /* should be level/edge, hi/lo specification here */
-   err |= cfc_check_trigger_arg_is(cmd-scan_begin_arg, 0);
-   }
+   /* internal trigger */
+   err |= cfc_check_trigger_arg_is(cmd-scan_begin_arg, 0);
 
err |= cfc_check_trigger_arg_min(cmd-convert_arg, 4000);
 
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: rtl8723au: multiple condition with no effect - if identical to else

[Nicholas Mc Guire]

A number if/else if/else branches are identical - so the condition has no
effect on the effective code and can be significantly simplified - this 
patch removes the condition and the duplicated code.

Signed-off-by: Nicholas Mc Guire hof...@osadl.org
---

This looks like the output of some broken code-generator - unlikely someone
wrote this by hand. In any case it needs a review by someone that knows the
details of the driver. 

Anyway the number of useless code repetition is potentially record breaking !

Patch was compile tested for x86_64_defconfig + CONFIG_STAGING=y
CONFIG_R8723AU=m, CONFIG_8723AU_BT_COEXIST=y

Patch is against 3.0.19-rc7 (localversoin = -next-20150203)

 .../staging/rtl8723au/hal/rtl8723a_bt-coexist.c|   60 +++-
 1 file changed, 8 insertions(+), 52 deletions(-)

diff --git a/drivers/staging/rtl8723au/hal/rtl8723a_bt-coexist.c 
b/drivers/staging/rtl8723au/hal/rtl8723a_bt-coexist.c
index 412d8cf..73cfddd 100644
--- a/drivers/staging/rtl8723au/hal/rtl8723a_bt-coexist.c
+++ b/drivers/staging/rtl8723au/hal/rtl8723a_bt-coexist.c
@@ -7255,63 +7255,19 @@ btdm_2AntTdmaDurationAdjust(struct rtw_adapter 
*padapter, u8 bScoHid,
RTPRINT(FBT, BT_TRACE, ([BTCoex], first run 
TdmaDurationAdjust()!!\n));
if (bScoHid) {
if (bTxPause) {
-   if (maxInterval == 1) {
-   btdm_2AntPsTdma(padapter, true, 15);
-   pBtdm8723-psTdmaDuAdjType = 15;
-   } else if (maxInterval == 2) {
-   btdm_2AntPsTdma(padapter, true, 15);
-   pBtdm8723-psTdmaDuAdjType = 15;
-   } else if (maxInterval == 3) {
-   btdm_2AntPsTdma(padapter, true, 15);
-   pBtdm8723-psTdmaDuAdjType = 15;
-   } else {
-   btdm_2AntPsTdma(padapter, true, 15);
-   pBtdm8723-psTdmaDuAdjType = 15;
-   }
+   btdm_2AntPsTdma(padapter, true, 15);
+   pBtdm8723-psTdmaDuAdjType = 15;
} else {
-   if (maxInterval == 1) {
-   btdm_2AntPsTdma(padapter, true, 11);
-   pBtdm8723-psTdmaDuAdjType = 11;
-   } else if (maxInterval == 2) {
-   btdm_2AntPsTdma(padapter, true, 11);
-   pBtdm8723-psTdmaDuAdjType = 11;
-   } else if (maxInterval == 3) {
-   btdm_2AntPsTdma(padapter, true, 11);
-   pBtdm8723-psTdmaDuAdjType = 11;
-   } else {
-   btdm_2AntPsTdma(padapter, true, 11);
-   pBtdm8723-psTdmaDuAdjType = 11;
-   }
+   btdm_2AntPsTdma(padapter, true, 11);
+   pBtdm8723-psTdmaDuAdjType = 11;
}
} else {
if (bTxPause) {
-   if (maxInterval == 1) {
-   btdm_2AntPsTdma(padapter, true, 7);
-   pBtdm8723-psTdmaDuAdjType = 7;
-   } else if (maxInterval == 2) {
-   btdm_2AntPsTdma(padapter, true, 7);
-   pBtdm8723-psTdmaDuAdjType = 7;
-   } else if (maxInterval == 3) {
-   btdm_2AntPsTdma(padapter, true, 7);
-   pBtdm8723-psTdmaDuAdjType = 7;
-   } else {
-   btdm_2AntPsTdma(padapter, true, 7);
-   pBtdm8723-psTdmaDuAdjType = 7;
-   }
+   btdm_2AntPsTdma(padapter, true, 7);
+   pBtdm8723-psTdmaDuAdjType = 7;
} else {
-   if (maxInterval == 1) {
-   btdm_2AntPsTdma(padapter, true, 3);
-   pBtdm8723-psTdmaDuAdjType = 3;
-   } else if (maxInterval == 2) {
-   btdm_2AntPsTdma(padapter, true, 3);
-   pBtdm8723-psTdmaDuAdjType = 3;
-   } else if (maxInterval == 3) {
-   btdm_2AntPsTdma(padapter, true, 3

Re: [PATCH RFC] staging: comedi: dt282x: condition with no effect - if identical to else

[Nicholas Mc Guire]

On Tue, 03 Feb 2015, Ian Abbott wrote:

 On 03/02/15 12:38, Nicholas Mc Guire wrote:
 The if and the else branch code are identical - so the condition has no
 effect on the effective code - this patch removes the condition and the
 duplicated code.

 Signed-off-by: Nicholas Mc Guire hof...@osadl.org
 ---

 The if and else branch are identical code thus the condition has no effect

  if (cmd-scan_begin_src == TRIG_FOLLOW) {
  /* internal trigger */
  err |= cfc_check_trigger_arg_is(cmd-scan_begin_arg, 0);
  } else {
  /* external trigger */
  /* should be level/edge, hi/lo specification here */
  err |= cfc_check_trigger_arg_is(cmd-scan_begin_arg, 0);
  }

 As the comments indicate that they are serving different purposes this
 looks like a bug. In any case - if intentional - it would need some
 comments on why.

 This needs a review by someone that knows the details of this driver.
 Also not sure about the retained comment string if that is still valid now.

 Patch was only compile tested for x86_64_defconfig + CONFIG_STAGING=y
 CONFIG_COMEDI=m, COMEDI_ISA_DRIVERS=y, CONFIG_COMEDI_DT282X=m

 Patch is against 3.0.19-rc7 (localversion = -next-20150203)

   drivers/staging/comedi/drivers/dt282x.c |   10 ++
   1 file changed, 2 insertions(+), 8 deletions(-)

 diff --git a/drivers/staging/comedi/drivers/dt282x.c 
 b/drivers/staging/comedi/drivers/dt282x.c
 index 051dfb2..22c59e5 100644
 --- a/drivers/staging/comedi/drivers/dt282x.c
 +++ b/drivers/staging/comedi/drivers/dt282x.c
 @@ -685,14 +685,8 @@ static int dt282x_ai_cmdtest(struct comedi_device *dev,

  err |= cfc_check_trigger_arg_is(cmd-start_arg, 0);

 -if (cmd-scan_begin_src == TRIG_FOLLOW) {
 -/* internal trigger */
 -err |= cfc_check_trigger_arg_is(cmd-scan_begin_arg, 0);
 -} else {
 -/* external trigger */
 -/* should be level/edge, hi/lo specification here */

 I think what that comment means is that it should allow scan_begin_arg  
 to have various combinations of the CR_EDGE and CR_INVERT bits set. I.e. 
 it ought to allow whatever combination of CR_EDGE and CR_INVERT better 
 describes the nature of the external trigger signal, in addition to 
 allowing the lazy default value 0.

 I don't know what the nature of the external trigger signal is, as I  
 haven't seen the manual.  I think Hartley might have seen one.

 -err |= cfc_check_trigger_arg_is(cmd-scan_begin_arg, 0);
 -}
 +/* internal trigger */

 That comment would be misleading as it could be an internal or external  
 trigger.


yup - was unsure on what to put there (if any).


 If you want to apply this patch, remove that comment first.  But I'd
 rather leave the existing code there as a reminder.


if that is the preferred solution maybe someone with suitable
knowledge could add a comment in the else case explaining why
it is the same or at least making it clear that it is an
intentional placeholder.

 +err |= cfc_check_trigger_arg_is(cmd-scan_begin_arg, 0);

  err |= cfc_check_trigger_arg_min(cmd-convert_arg, 4000);



thanks for your review comments!

thx!
hofrat

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] hv: channel: match var type to return type of wait_for_completion

[Nicholas Mc Guire]

From: Nicholas Mc Guire der.h...@hofr.at

return type of wait_for_completion_timeout is unsigned long not int, this
patch changes the type of t from int to unsigned long.

Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
---

The return type of wait_for_completion_timeout is unsigned long not
int. This patch resolves the type missmatch by changing the type
to unsigned long.

This patch was only compile tested with x86_64_defconfig +
CONFIG_X86_VSMP=y, CONFIG_HYPERV=m

Patch is against 3.19.0-rc6 -next-20150130

 drivers/hv/channel.c |3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 2978f5e..f749573 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -71,7 +71,8 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 
send_ringbuffer_size,
struct vmbus_channel_msginfo *open_info = NULL;
void *in, *out;
unsigned long flags;
-   int ret, t, err = 0;
+   int ret, err = 0;
+   unsigned long t;
 
spin_lock_irqsave(newchannel-lock, flags);
if (newchannel-state == CHANNEL_OPEN_STATE) {
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] hv: hv_balloon: match var type to return type of wait_for_completion

[Nicholas Mc Guire]

From: Nicholas Mc Guire der.h...@hofr.at

return type of wait_for_completion_timeout is unsigned long not int, this
patch changes the type of t from int to unsigned long.

Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
---

The return type of wait_for_completion_timeout is unsigned long not
int. This patch resolves the type missmatch by changing the type
to unsigned long.

This patch was only compile tested with x86_64_defconfig +
CONFIG_X86_VSMP=y, CONFIG_HYPERV=m, CONFIG_HYPERV_BALLOON=m

Patch is against 3.19.0-rc6 -next-20150130

 drivers/hv/hv_balloon.c |3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
index ff16938..965c37a 100644
--- a/drivers/hv/hv_balloon.c
+++ b/drivers/hv/hv_balloon.c
@@ -1414,7 +1414,8 @@ static void balloon_onchannelcallback(void *context)
 static int balloon_probe(struct hv_device *dev,
const struct hv_vmbus_device_id *dev_id)
 {
-   int ret, t;
+   int ret;
+   unsigned long t;
struct dm_version_request version_req;
struct dm_capabilities cap_msg;
 
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] hv: channel_mgmt: match var type to return type of wait_for_completion

[Nicholas Mc Guire]

From: Nicholas Mc Guire der.h...@hofr.at

return type of wait_for_completion_timeout is unsigned long not int, this
patch changes the type of t from int to unsigned long.

Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
---

The return type of wait_for_completion_timeout is unsigned long not
int. This patch resolves the type missmatch by changing the type
to unsigned long.

This patch was only compile tested with x86_64_defconfig +
CONFIG_X86_VSMP=y, CONFIG_HYPERV=m

Patch is against 3.19.0-rc6 -next-20150130

 drivers/hv/channel_mgmt.c |3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 3736f71..07e055b 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -787,7 +787,8 @@ int vmbus_request_offers(void)
 {
struct vmbus_channel_message_header *msg;
struct vmbus_channel_msginfo *msginfo;
-   int ret, t;
+   int ret;
+   unsigned long t;
 
msginfo = kmalloc(sizeof(*msginfo) +
  sizeof(struct vmbus_channel_message_header),
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] hv: vmbus_drv: match var type to return type of wait_for_completion

[Nicholas Mc Guire]

From: Nicholas Mc Guire der.h...@hofr.at

return type of wait_for_completion_timeout is unsigned long not int, this
patch changes the type of t from int to unsigned long.

Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
---

The return type of wait_for_completion_timeout is unsigned long not
int. This patch resolves the type missmatch by changing the type
to unsigned long.

This patch was only compile tested with x86_64_defconfig +
CONFIG_X86_VSMP=y, CONFIG_HYPERV=m

Patch is against 3.19.0-rc6 -next-20150130

 drivers/hv/vmbus_drv.c |3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index f518b8d7..a6fa142 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -952,7 +952,8 @@ static struct acpi_driver vmbus_acpi_driver = {
 
 static int __init hv_acpi_init(void)
 {
-   int ret, t;
+   int ret;
+   unsigned long t;
 
if (x86_hyper != x86_hyper_ms_hyperv)
return -ENODEV;
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: unisys: use msecs_to_jiffies for conversions

[Nicholas Mc Guire]

This is only an API consolidation to make things more readable.
Instances of  var * HZ / 1000  are replaced by  msecs_to_jiffies(var).

Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
---

Converting milliseconds to jiffies by val * HZ / 1000 is technically
ok but msecs_to_jiffies(val) is the cleaner solution and handles all
corner cases correctly. This is a minor API cleanup only.

This patch was only compile tested with x86_64_defconfig + CONFIG_STAGING=y
CONFIG_UNISYSSPAR=y, CONFIG_UNISYS_VISORUTIL=m,
CONFIG_UNISYS_VISORCHANNEL=m, CONFIG_UNISYS_VISORCHIPSET=m 

Patch is against 3.19.0-rc6 -next-20150129

 drivers/staging/unisys/visorchipset/visorchipset_main.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/unisys/visorchipset/visorchipset_main.c 
b/drivers/staging/unisys/visorchipset/visorchipset_main.c
index 82e259d..f606ee9 100644
--- a/drivers/staging/unisys/visorchipset/visorchipset_main.c
+++ b/drivers/staging/unisys/visorchipset/visorchipset_main.c
@@ -1594,7 +1594,7 @@ parahotplug_next_id(void)
 static unsigned long
 parahotplug_next_expiration(void)
 {
-   return jiffies + PARAHOTPLUG_TIMEOUT_MS * HZ / 1000;
+   return jiffies + msecs_to_jiffies(PARAHOTPLUG_TIMEOUT_MS);
 }
 
 /*
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: rtl8188eu: use msecs_to_jiffies for conversions

[Nicholas Mc Guire]

This is only an API consolidation to make things more readable.
Instances of  var * HZ / 1000  are replaced by  msecs_to_jiffies(var).

Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
---

Converting milliseconds to jiffies by val * HZ / 1000 is technically
ok but msecs_to_jiffies(val) is the cleaner solution and handles all
corner cases correctly. This is a minor API cleanup only.

This patch was only compile tested with x86_64_defconfig + CONFIG_STAGING=y
CONFIG_R8188EU=m

Patch is against 3.19.0-rc6 -next-20150129

 drivers/staging/rtl8188eu/include/osdep_service.h |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8188eu/include/osdep_service.h 
b/drivers/staging/rtl8188eu/include/osdep_service.h
index 82f58f8..3a27477 100644
--- a/drivers/staging/rtl8188eu/include/osdep_service.h
+++ b/drivers/staging/rtl8188eu/include/osdep_service.h
@@ -87,7 +87,7 @@ static inline void _init_timer(struct timer_list *ptimer,
 
 static inline void _set_timer(struct timer_list *ptimer, u32 delay_time)
 {
-   mod_timer(ptimer , (jiffies+(delay_time*HZ/1000)));
+   mod_timer(ptimer , (jiffies+msecs_to_jiffies(delay_time)));
 }
 
 #define RTW_TIMER_HDL_ARGS void *FunctionContext
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/2] staging: rtl8712: cleanup of timeout conversions

[Nicholas Mc Guire]

This is only an API consolidation to make things more readable.
Instances of  var * HZ / 1000  are replaced by  msecs_to_jiffies(var).
As msecs_to_jiffies will return  0 if it is passed a value  0 the
== 0 check is not needed. 

Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
---

Converting milliseconds to jiffies by val * HZ / 1000 is technically
ok but msecs_to_jiffies(val) is the cleaner solution and handles all
corner cases correctly. This is a minor API cleanup only.

As msecs_to_jiffies will not return at least 1 (unless 0 is passed as 
argument) the check for delta == 0 is not needed here.

This patch was only compile tested for x86_64_defconfig +
CONFIG_STAGING=y, CONFIG_R8712U=m

This patch is against 3.0.19-rc6 -next-20150129

 drivers/staging/rtl8712/osdep_service.h |6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/rtl8712/osdep_service.h 
b/drivers/staging/rtl8712/osdep_service.h
index 5153ad9..f933713 100644
--- a/drivers/staging/rtl8712/osdep_service.h
+++ b/drivers/staging/rtl8712/osdep_service.h
@@ -71,7 +71,7 @@ static inline void _init_timer(struct timer_list *ptimer,
 
 static inline void _set_timer(struct timer_list *ptimer, u32 delay_time)
 {
-   mod_timer(ptimer, (jiffies+(delay_time*HZ/1000)));
+   mod_timer(ptimer, (jiffies+msecs_to_jiffies(delay_time)));
 }
 
 static inline void _cancel_timer(struct timer_list *ptimer, u8 *bcancelled)
@@ -101,9 +101,7 @@ static inline void sleep_schedulable(int ms)
 {
u32 delta;
 
-   delta = (ms * HZ) / 1000;/*(ms)*/
-   if (delta == 0)
-   delta = 1;/* 1 ms */
+   delta = msecs_to_jiffies(ms);/*(ms)*/
set_current_state(TASK_INTERRUPTIBLE);
if (schedule_timeout(delta) != 0)
return;
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/2] staging: rtl8712: condition with no effect removed

[Nicholas Mc Guire]

The check for return of schedule_timeout() has no effect on the 
effective control flow of sleep_schedulable() so it can be dropped.

Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
---

The current codes return check for schedule_timeout() has no effect
and can probably be dropped unless it is intended as a placeholder
for some future change in which case a comment should probably be
added.

This patch was only compile tested for x86_64_defconfig +
CONFIG_STAGING=y, CONFIG_R8712U=m


Patch is against 3.0.19-rc6 -next-20150129 but depends on:
[PATCH 1/2] staging: rtl8712: cleanup of timeout conversions

 drivers/staging/rtl8712/osdep_service.h |3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8712/osdep_service.h 
b/drivers/staging/rtl8712/osdep_service.h
index f933713..36348d9 100644
--- a/drivers/staging/rtl8712/osdep_service.h
+++ b/drivers/staging/rtl8712/osdep_service.h
@@ -103,8 +103,7 @@ static inline void sleep_schedulable(int ms)
 
delta = msecs_to_jiffies(ms);/*(ms)*/
set_current_state(TASK_INTERRUPTIBLE);
-   if (schedule_timeout(delta) != 0)
-   return;
+   schedule_timeout(delta);
 }
 
 static inline unsigned char _cancel_timer_ex(struct timer_list *ptimer)
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/3 v2] hyperv: hyperv_fb.c: match wait_for_completion_timeout return type

[Nicholas Mc Guire]

On Mon, 26 Jan 2015, Tomi Valkeinen wrote:

 Hi,
 
 On 25/01/15 16:47, Nicholas Mc Guire wrote:
  Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
  ---
  
  v2: fixed subject line
  
  The return type of wait_for_completion_timeout is unsigned long not
  int. This patch fixes up the declarations only.
  
  Patch was compile tested only for x86_64_defconfig + CONFIG_X86_VSMP=y
  CONFIG_HYPERV=m, CONFIG_FB_HYPERV=m
 
 Why didn't you set the text above as the patch description (which is
 empty at the moment)?
 
basically because the one-line is sufficient to understand the patch
and the rest of the information is not relevant for the git log but only
for the review

if you think it is necessary to understand the patch I'll move it and
resubmit.

thanks for your review !

hofrat
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/3 v2] hyperv: hyperv_fb.c: match wait_for_completion_timeout return type

[Nicholas Mc Guire]

On Thu, 29 Jan 2015, Tomi Valkeinen wrote:

 On 29/01/15 11:38, Nicholas Mc Guire wrote:
  On Mon, 26 Jan 2015, Tomi Valkeinen wrote:
  
  Hi,
 
  On 25/01/15 16:47, Nicholas Mc Guire wrote:
  Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
  ---
 
  v2: fixed subject line
 
  The return type of wait_for_completion_timeout is unsigned long not
  int. This patch fixes up the declarations only.
 
  Patch was compile tested only for x86_64_defconfig + CONFIG_X86_VSMP=y
  CONFIG_HYPERV=m, CONFIG_FB_HYPERV=m
 
  Why didn't you set the text above as the patch description (which is
  empty at the moment)?
 
  basically because the one-line is sufficient to understand the patch
 
 You didn't have one line, you had no description. Patch subject is not
 patch description. In the minimal case, the description should have the
 same text as the subject, but usually it's better to have a bit more
 text in the description.

ok - was not clear about this - got it.

 
  and the rest of the information is not relevant for the git log but only
  for the review
  
  if you think it is necessary to understand the patch I'll move it and
  resubmit.
 
 Well, a good description is not only about understanding the code in the
 patch. It may contain information like which platform/setup this issue
 happened on, are the any possible side effects, or whatever might be
 relevant for someone looking at the patch years later.
 
yup - but it seemed to me that the information on the build
config and kernel version details would not really be relevant for
this cleanup patch - so if I got your right the description line should
have gone up and the config/kernel info stays below ---.

Just resent it - hope this is correct now.

thx!
hofrat

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/3 v2] hyperv: hyperv_fb.c: match wait_for_completion_timeout return type

[Nicholas Mc Guire]

On Thu, 29 Jan 2015, Dan Carpenter wrote:

 On Thu, Jan 29, 2015 at 10:38:39AM +0100, Nicholas Mc Guire wrote:
  On Mon, 26 Jan 2015, Tomi Valkeinen wrote:
  
   Hi,
   
   On 25/01/15 16:47, Nicholas Mc Guire wrote:
Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
---

v2: fixed subject line

The return type of wait_for_completion_timeout is unsigned long not
int. This patch fixes up the declarations only.
   
 
 This line is relevant for the patch description.
  
Patch was compile tested only for x86_64_defconfig + CONFIG_X86_VSMP=y
CONFIG_HYPERV=m, CONFIG_FB_HYPERV=m
 
 This line is not relevant.

thanks - will resend it then - assumed that the one-line would do for this
patch.

thx!
hofrat 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/3 v3] hyperv: hyperv_fb.c: match wait_for_completion_timeout return type

[Nicholas Mc Guire]

The return type of wait_for_completion_timeout is unsigned long not
int. This patch fixes up the declarations only.

Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
---

v2: fixed subject line
v3: fixed patch description as recommended by Dan Carpenter
dan.carpen...@oracle.com

Patch was compile tested only for x86_64_defconfig + CONFIG_X86_VSMP=y
CONFIG_HYPERV=m, CONFIG_FB_HYPERV=m

Patch is against 3.19.0-rc5 -next-20150123

 drivers/video/fbdev/hyperv_fb.c |6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index 4254336..807ee22 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -415,7 +415,8 @@ static int synthvid_negotiate_ver(struct hv_device *hdev, 
u32 ver)
struct fb_info *info = hv_get_drvdata(hdev);
struct hvfb_par *par = info-par;
struct synthvid_msg *msg = (struct synthvid_msg *)par-init_buf;
-   int t, ret = 0;
+   int ret = 0;
+   unsigned long t;
 
memset(msg, 0, sizeof(struct synthvid_msg));
msg-vid_hdr.type = SYNTHVID_VERSION_REQUEST;
@@ -488,7 +489,8 @@ static int synthvid_send_config(struct hv_device *hdev)
struct fb_info *info = hv_get_drvdata(hdev);
struct hvfb_par *par = info-par;
struct synthvid_msg *msg = (struct synthvid_msg *)par-init_buf;
-   int t, ret = 0;
+   int ret = 0;
+   unsigned long t;
 
/* Send VRAM location */
memset(msg, 0, sizeof(struct synthvid_msg));
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 3/3] hyperv: hid-hyperv.c: fixup-of-wait_for_completion_timeout-return-type

[Nicholas Mc Guire]

Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
---

The return type of wait_for_completion_timeout is unsigned long not
int. This patch fixes up the declarations only.

Patch was compile tested only for x86_64_defconfig + CONFIG_X86_VSMP=y
CONFIG_HYPERV=m, CONFIG_HID_HYPERV_MOUSE=m

Patch is against 3.19.0-rc5 -next-20150123

 drivers/hid/hid-hyperv.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c
index 31fad64..6039f07 100644
--- a/drivers/hid/hid-hyperv.c
+++ b/drivers/hid/hid-hyperv.c
@@ -381,7 +381,7 @@ static void mousevsc_on_channel_callback(void *context)
 static int mousevsc_connect_to_vsp(struct hv_device *device)
 {
int ret = 0;
-   int t;
+   unsigned long t;
struct mousevsc_dev *input_dev = hv_get_drvdata(device);
struct mousevsc_prt_msg *request;
struct mousevsc_prt_msg *response;
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/3] hyperv: hyperv_fb.c: fixup-of-wait_for_completion_timeout-return-type

[Nicholas Mc Guire]

Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
---

The return type of wait_for_completion_timeout is unsigned long not
int. This patch fixes up the declarations only.

Patch was compile tested only for x86_64_defconfig + CONFIG_X86_VSMP=y
CONFIG_HYPERV=m, CONFIG_FB_HYPERV=m

Patch is against 3.19.0-rc5 -next-20150123

 drivers/video/fbdev/hyperv_fb.c |6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index 4254336..807ee22 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -415,7 +415,8 @@ static int synthvid_negotiate_ver(struct hv_device *hdev, 
u32 ver)
struct fb_info *info = hv_get_drvdata(hdev);
struct hvfb_par *par = info-par;
struct synthvid_msg *msg = (struct synthvid_msg *)par-init_buf;
-   int t, ret = 0;
+   int ret = 0;
+   unsigned long t;
 
memset(msg, 0, sizeof(struct synthvid_msg));
msg-vid_hdr.type = SYNTHVID_VERSION_REQUEST;
@@ -488,7 +489,8 @@ static int synthvid_send_config(struct hv_device *hdev)
struct fb_info *info = hv_get_drvdata(hdev);
struct hvfb_par *par = info-par;
struct synthvid_msg *msg = (struct synthvid_msg *)par-init_buf;
-   int t, ret = 0;
+   int ret = 0;
+   unsigned long t;
 
/* Send VRAM location */
memset(msg, 0, sizeof(struct synthvid_msg));
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] hyperv: else branch not necessary

[Nicholas Mc Guire]

As the if completes with a unconditional goto the else branch
is not needed here.

Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
---

All paths of execution that did not exit through the if branch will 
go through the else branch so no need for an explicit else here

Patch was compile tested only for x86_64_defconfig + CONFIG_X86_VSMP=y
CONFIG_HYPERV=m, CONFIG_HYPERV_NET=m

Patch is against 3.19.0-rc5 -next-20150123

 drivers/net/hyperv/rndis_filter.c |8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/hyperv/rndis_filter.c 
b/drivers/net/hyperv/rndis_filter.c
index 7bd8387..efb84a9 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -833,10 +833,10 @@ int rndis_filter_set_packet_filter(struct rndis_device 
*dev, u32 new_filter)
 * send completion for it.
 */
goto exit;
-   } else {
-   set_complete = request-response_msg.msg.set_complete;
-   status = set_complete-status;
-   }
+   } 
+
+   set_complete = request-response_msg.msg.set_complete;
+   status = set_complete-status;
 
 cleanup:
if (request)
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] hyperv: fixup of wait_for_completion_timeout return type

[Nicholas Mc Guire]

return type of wait_for_completion_timeout is unsigned long not int, this
patch just fixes up the declarations.

Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
---

The return type of wait_for_completion_timeout is unsigned long not
int. This patch fixes up the declarations only.

Patch was compile tested only for x86_64_defconfig + CONFIG_X86_VSMP=y
CONFIG_HYPERV=m, CONFIG_HYPERV_NET=m

Patch is against 3.19.0-rc5 -next-20150123

 drivers/net/hyperv/rndis_filter.c |   19 ---
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/net/hyperv/rndis_filter.c 
b/drivers/net/hyperv/rndis_filter.c
index ec0c40a..7bd8387 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -470,7 +470,7 @@ static int rndis_filter_query_device(struct rndis_device 
*dev, u32 oid,
struct rndis_query_request *query;
struct rndis_query_complete *query_complete;
int ret = 0;
-   int t;
+   unsigned long t;
 
if (!result)
return -EINVAL;
@@ -560,7 +560,8 @@ int rndis_filter_set_device_mac(struct hv_device *hdev, 
char *mac)
char macstr[2*ETH_ALEN+1];
u32 extlen = sizeof(struct rndis_config_parameter_info) +
2*NWADR_STRLEN + 4*ETH_ALEN;
-   int ret, t;
+   int ret;
+   unsigned long t;
 
request = get_rndis_request(rdev, RNDIS_MSG_SET,
RNDIS_MESSAGE_SIZE(struct rndis_set_request) + extlen);
@@ -634,7 +635,8 @@ int rndis_filter_set_offload_params(struct hv_device *hdev,
struct ndis_offload_params *offload_params;
struct rndis_set_complete *set_complete;
u32 extlen = sizeof(struct ndis_offload_params);
-   int ret, t;
+   int ret;
+   unsigned long t;
u32 vsp_version = nvdev-nvsp_version;
 
if (vsp_version = NVSP_PROTOCOL_VERSION_4) {
@@ -708,7 +710,8 @@ int rndis_filter_set_rss_param(struct rndis_device *rdev, 
int num_queue)
struct ndis_recv_scale_param *rssp;
u32 *itab;
u8 *keyp;
-   int i, t, ret;
+   int i, ret;
+   unsigned long t;
 
request = get_rndis_request(
rdev, RNDIS_MSG_SET,
@@ -792,7 +795,8 @@ int rndis_filter_set_packet_filter(struct rndis_device 
*dev, u32 new_filter)
struct rndis_set_request *set;
struct rndis_set_complete *set_complete;
u32 status;
-   int ret, t;
+   int ret;
+   unsigned long t;
struct net_device *ndev;
 
ndev = dev-net_dev-ndev;
@@ -848,7 +852,8 @@ static int rndis_filter_init_device(struct rndis_device 
*dev)
struct rndis_initialize_request *init;
struct rndis_initialize_complete *init_complete;
u32 status;
-   int ret, t;
+   int ret;
+   unsigned long t;
 
request = get_rndis_request(dev, RNDIS_MSG_INIT,
RNDIS_MESSAGE_SIZE(struct rndis_initialize_request));
@@ -998,7 +1003,7 @@ int rndis_filter_device_add(struct hv_device *dev,
struct netvsc_device_info *device_info = additional_info;
struct ndis_offload_params offloads;
struct nvsp_message *init_packet;
-   int t;
+   unsigned long t;
struct ndis_recv_scale_cap rsscap;
u32 rsscap_size = sizeof(struct ndis_recv_scale_cap);
u32 mtu, size;
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/3] hyperv: netvsc.c: fixup-of-wait_for_completion_timeout-return-type

[Nicholas Mc Guire]

Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
---

The return type of wait_for_completion_timeout is unsigned long not
int. This patch fixes up the declarations only.

Patch was compile tested only for x86_64_defconfig + CONFIG_X86_VSMP=y
CONFIG_HYPERV=m, CONFIG_HYPERV_NET=m

Patch is against 3.19.0-rc5 -next-20150123

 drivers/net/hyperv/netvsc.c |5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 9f49c01..d2af032 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -217,7 +217,7 @@ static int netvsc_destroy_buf(struct netvsc_device 
*net_device)
 static int netvsc_init_buf(struct hv_device *device)
 {
int ret = 0;
-   int t;
+   unsigned long t;
struct netvsc_device *net_device;
struct nvsp_message *init_packet;
struct net_device *ndev;
@@ -409,7 +409,8 @@ static int negotiate_nvsp_ver(struct hv_device *device,
  struct nvsp_message *init_packet,
  u32 nvsp_ver)
 {
-   int ret, t;
+   int ret;
+   unsigned long t;
 
memset(init_packet, 0, sizeof(struct nvsp_message));
init_packet-hdr.msg_type = NVSP_MSG_TYPE_INIT;
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/3 v2] hyperv: hyperv_fb.c: match wait_for_completion_timeout return type

[Nicholas Mc Guire]

Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
---

v2: fixed subject line

The return type of wait_for_completion_timeout is unsigned long not
int. This patch fixes up the declarations only.

Patch was compile tested only for x86_64_defconfig + CONFIG_X86_VSMP=y
CONFIG_HYPERV=m, CONFIG_FB_HYPERV=m

Patch is against 3.19.0-rc5 -next-20150123

 drivers/video/fbdev/hyperv_fb.c |6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index 4254336..807ee22 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -415,7 +415,8 @@ static int synthvid_negotiate_ver(struct hv_device *hdev, 
u32 ver)
struct fb_info *info = hv_get_drvdata(hdev);
struct hvfb_par *par = info-par;
struct synthvid_msg *msg = (struct synthvid_msg *)par-init_buf;
-   int t, ret = 0;
+   int ret = 0;
+   unsigned long t;
 
memset(msg, 0, sizeof(struct synthvid_msg));
msg-vid_hdr.type = SYNTHVID_VERSION_REQUEST;
@@ -488,7 +489,8 @@ static int synthvid_send_config(struct hv_device *hdev)
struct fb_info *info = hv_get_drvdata(hdev);
struct hvfb_par *par = info-par;
struct synthvid_msg *msg = (struct synthvid_msg *)par-init_buf;
-   int t, ret = 0;
+   int ret = 0;
+   unsigned long t;
 
/* Send VRAM location */
memset(msg, 0, sizeof(struct synthvid_msg));
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 3/3 v2] hyperv: hid-hyperv.c: match wait_for_completion_timeout return type

[Nicholas Mc Guire]

Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
---

v2: fixed subject line

The return type of wait_for_completion_timeout is unsigned long not
int. This patch fixes up the declarations only.

Patch was compile tested only for x86_64_defconfig + CONFIG_X86_VSMP=y
CONFIG_HYPERV=m, CONFIG_HID_HYPERV_MOUSE=m

Patch is against 3.19.0-rc5 -next-20150123

 drivers/hid/hid-hyperv.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c
index 31fad64..6039f07 100644
--- a/drivers/hid/hid-hyperv.c
+++ b/drivers/hid/hid-hyperv.c
@@ -381,7 +381,7 @@ static void mousevsc_on_channel_callback(void *context)
 static int mousevsc_connect_to_vsp(struct hv_device *device)
 {
int ret = 0;
-   int t;
+   unsigned long t;
struct mousevsc_dev *input_dev = hv_get_drvdata(device);
struct mousevsc_prt_msg *request;
struct mousevsc_prt_msg *response;
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2] net: hyperv: else branch not necessary

[Nicholas Mc Guire]

As the if completes with a unconditional goto the else branch
is not needed here.

Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
---

v2: added missing subsystem string in subject line - patch unchanged

All paths of execution that did not exit through the if branch will 
go through the else branch so no need for an explicit else here

Patch was compile tested only for x86_64_defconfig + CONFIG_X86_VSMP=y
CONFIG_HYPERV=m, CONFIG_HYPERV_NET=m

Patch is against 3.19.0-rc5 -next-20150123

 drivers/net/hyperv/rndis_filter.c |8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/hyperv/rndis_filter.c 
b/drivers/net/hyperv/rndis_filter.c
index 7bd8387..efb84a9 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -833,10 +833,10 @@ int rndis_filter_set_packet_filter(struct rndis_device 
*dev, u32 new_filter)
 * send completion for it.
 */
goto exit;
-   } else {
-   set_complete = request-response_msg.msg.set_complete;
-   status = set_complete-status;
-   }
+   } 
+
+   set_complete = request-response_msg.msg.set_complete;
+   status = set_complete-status;
 
 cleanup:
if (request)
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH RFC] staging: media: davinci_vpfe: drop condition with no effect

[Nicholas Mc Guire]

As the if and else branch body are identical the condition has no effect and 
can be dropped.

Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
---

As the if and the else branch of the inner conditional paths are the same
the condition is without effect. Given the comments indicate that
the else branch *should* be handling a specific case this may indicate
a bug, in which case the below patch is *wrong*. This needs a review by
someone that knows the specifics of this driver.

If the inner if/else is a placeholder for planed updates then it should
be commented so this is clear.

Patch was only compile tested with davinci_all_defconfig + CONFIG_STAGING=y
CONFIG_STAGING_MEDIA=y, CONFIG_MEDIA_SUPPORT=m,
CONFIG_MEDIA_ANALOG_TV_SUPPORT=y, CONFIG_MEDIA_CAMERA_SUPPORT=y
CONFIG_MEDIA_CONTROLLER=y, CONFIG_VIDEO_V4L2_SUBDEV_API=y
CONFIG_VIDEO_DM365_VPFE=m

Patch is against 3.0.19-rc5 -next-20150123

 drivers/staging/media/davinci_vpfe/dm365_resizer.c |9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/media/davinci_vpfe/dm365_resizer.c 
b/drivers/staging/media/davinci_vpfe/dm365_resizer.c
index 75e70e1..bf2cb7a 100644
--- a/drivers/staging/media/davinci_vpfe/dm365_resizer.c
+++ b/drivers/staging/media/davinci_vpfe/dm365_resizer.c
@@ -63,16 +63,11 @@ resizer_calculate_line_length(u32 pix, int width, int 
height,
if (pix == MEDIA_BUS_FMT_UYVY8_2X8 ||
pix == MEDIA_BUS_FMT_SGRBG12_1X12) {
*line_len = width  1;
-   } else if (pix == MEDIA_BUS_FMT_Y8_1X8 ||
-  pix == MEDIA_BUS_FMT_UV8_1X8) {
-   *line_len = width;
-   *line_len_c = width;
-   } else {
-   /* YUV 420 */
-   /* round width to upper 32 byte boundary */
+   } else { 
*line_len = width;
*line_len_c = width;
}
+
/* adjust the line len to be a multiple of 32 */
*line_len += 31;
*line_len = ~0x1f;
-- 
1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel