[PATCH] rsi: consolidate kmalloc/memset 0 calls to kzalloc

[Nicholas Mc Guire]

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

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

Found by coccinelle script (relaxed version of
scripts/coccinelle/api/alloc/kzalloc-simple.cocci)

Patch was compile tested with: x86_64_defconfig +
CONFIG_RSI_91X=m

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

 drivers/net/wireless/rsi/rsi_91x_mgmt.c |3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_mgmt.c 
b/drivers/net/wireless/rsi/rsi_91x_mgmt.c
index 8d110fd..697caab 100644
--- a/drivers/net/wireless/rsi/rsi_91x_mgmt.c
+++ b/drivers/net/wireless/rsi/rsi_91x_mgmt.c
@@ -1023,7 +1023,7 @@ static int rsi_send_auto_rate_request(struct rsi_common 
*common)
return -ENOMEM;
}
 
-   selected_rates = kmalloc(2 * RSI_TBL_SZ, GFP_KERNEL);
+   selected_rates = kzalloc(2 * RSI_TBL_SZ, GFP_KERNEL);
if (!selected_rates) {
rsi_dbg(ERR_ZONE, "%s: Failed in allocation of mem\n",
__func__);
@@ -1032,7 +1032,6 @@ static int rsi_send_auto_rate_request(struct rsi_common 
*common)
}
 
memset(skb->data, 0, sizeof(struct rsi_auto_rate));
-   memset(selected_rates, 0, 2 * RSI_TBL_SZ);
 
auto_rate = (struct rsi_auto_rate *)skb->data;
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] rsi: bool tests do not need comparison

[Nicholas Mc Guire]

This is an API consolidation only. Bool initializations should 
use true and false thus bool tests don't need an explicit comparison.

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

Found by coccinelle: scripts/coccinelle/misc/boolinit.cocci

Patch was compile tested with: x86_64_defconfig +
CONFIG_RSI_91X=m

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

 drivers/net/wireless/rsi/rsi_91x_mgmt.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_mgmt.c 
b/drivers/net/wireless/rsi/rsi_91x_mgmt.c
index 8d110fd..90161aa 100644
--- a/drivers/net/wireless/rsi/rsi_91x_mgmt.c
+++ b/drivers/net/wireless/rsi/rsi_91x_mgmt.c
@@ -1227,7 +1227,7 @@ int rsi_send_block_unblock_frame(struct rsi_common 
*common, bool block_event)
mgmt_frame->desc_word[0] = cpu_to_le16(RSI_WIFI_MGMT_Q << 12);
mgmt_frame->desc_word[1] = cpu_to_le16(BLOCK_HW_QUEUE);
 
-   if (block_event == true) {
+   if (block_event) {
rsi_dbg(INFO_ZONE, "blocking the data qs\n");
mgmt_frame->desc_word[4] = cpu_to_le16(0xf);
} else {
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] wlcore: consolidate kmalloc + memset 0 into kzalloc

[Nicholas Mc Guire]

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

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

Found by coccinelle script (relaxed version of
scripts/coccinelle/api/alloc/kzalloc-simple.cocci)

Patch was compile tested with: x86_64_defconfig +
CONFIG_WL12XX=m (implies CONFIG_WLCORE=m)

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

 drivers/net/wireless/ti/wlcore/main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/main.c 
b/drivers/net/wireless/ti/wlcore/main.c
index ec7f6af..dfc49bf 100644
--- a/drivers/net/wireless/ti/wlcore/main.c
+++ b/drivers/net/wireless/ti/wlcore/main.c
@@ -838,7 +838,7 @@ static void wl12xx_read_fwlog_panic(struct wl1271 *wl)
 
wl1271_info("Reading FW panic log");
 
-   block = kmalloc(wl->fw_mem_block_size, GFP_KERNEL);
+   block = kzalloc(wl->fw_mem_block_size, GFP_KERNEL);
if (!block)
return;
 
@@ -885,7 +885,6 @@ static void wl12xx_read_fwlog_panic(struct wl1271 *wl)
goto out;
}
 
-   memset(block, 0, wl->fw_mem_block_size);
ret = wlcore_read_hwaddr(wl, addr, block,
wl->fw_mem_block_size, false);
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] wlcore: consolidate kmalloc + memset 0 into kzalloc

[Nicholas Mc Guire]

On Tue, Dec 22, 2015 at 09:56:10AM +1100, Julian Calaby wrote:
> Hi,
> 
> On Tue, Dec 22, 2015 at 3:47 AM, Nicholas Mc Guire  wrote:
> > This is an API consolidation only. The use of kmalloc + memset to 0
> > is equivalent to kzalloc.
> >
> > Signed-off-by: Nicholas Mc Guire 
> > ---
> >
> > Found by coccinelle script (relaxed version of
> > scripts/coccinelle/api/alloc/kzalloc-simple.cocci)
> >
> > Patch was compile tested with: x86_64_defconfig +
> > CONFIG_WL12XX=m (implies CONFIG_WLCORE=m)
> >
> > Patch is against linux-next (localversion-next is -next-20151221)
> >
> >  drivers/net/wireless/ti/wlcore/main.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/wireless/ti/wlcore/main.c 
> > b/drivers/net/wireless/ti/wlcore/main.c
> > index ec7f6af..dfc49bf 100644
> > --- a/drivers/net/wireless/ti/wlcore/main.c
> > +++ b/drivers/net/wireless/ti/wlcore/main.c
> > @@ -838,7 +838,7 @@ static void wl12xx_read_fwlog_panic(struct wl1271 *wl)
> >
> > wl1271_info("Reading FW panic log");
> >
> > -   block = kmalloc(wl->fw_mem_block_size, GFP_KERNEL);
> > +   block = kzalloc(wl->fw_mem_block_size, GFP_KERNEL);
> > if (!block)
> > return;
> >
> > @@ -885,7 +885,6 @@ static void wl12xx_read_fwlog_panic(struct wl1271 *wl)
> > goto out;
> > }
> > -   memset(block, 0, wl->fw_mem_block_size);
> 
> I don't think you can't remove this line. It appears that the loop
> this is part of resets block to be all zero, reads a chunk of data in,
> then operates on it. I'm guessing that the code after the following
> line expects that there isn't any data left over from previous runs
> through the loop.
>
the rational for this being ok is thta the copy operation into block is: 
ret = wlcore_read_hwaddr(wl, addr, block,
wl->fw_mem_block_size, false);

this will end up in the .read methods where block should be completely 
overwritten (length == full block size), so within the loop if successful
this should be correct - if not successful it would "goto out" witout 
using the content of block.

Am I overlooking something here ?

thx!
hofrat
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC] wlcore: match wait_for_completion_timeout return type

[Nicholas Mc Guire]

return type of wait_for_completion_timeout is unsigned long not int, this
patch adds an appropriate return type and assignment.

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

The return type of wait_for_completion_timeout is unsigned long not
int. This patch adds a separate variable of proper type for handling
of the wait_for_completion_timeout.

The alternative would be to fold it into the if condition and not use
a separate variable like so:

if (!pending) {
if (!wait_for_completion_timeout( &compl,
msecs_to_jiffies(WL1271_WAKEUP_TIMEOUT)) {
wl1271_error("ELP wakeup timeout!");
wl12xx_queue_recovery_work(wl);
ret = -ETIMEDOUT;
goto err;
}
}

not sure if this or the below solution is preferable.

Patch was compile tested only for x86_64_defconfig + CONFIG_WL_TI=y, 
CONFIG_WLCORE=m

Patch is against 3.19.0-rc5 -next-20150123

 drivers/net/wireless/ti/wlcore/ps.c |5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/ps.c 
b/drivers/net/wireless/ti/wlcore/ps.c
index 4cd316e..d5f918a4 100644
--- a/drivers/net/wireless/ti/wlcore/ps.c
+++ b/drivers/net/wireless/ti/wlcore/ps.c
@@ -109,6 +109,7 @@ int wl1271_ps_elp_wakeup(struct wl1271 *wl)
DECLARE_COMPLETION_ONSTACK(compl);
unsigned long flags;
int ret;
+   unsigned long timeout;
unsigned long start_time = jiffies;
bool pending = false;
 
@@ -145,9 +146,9 @@ int wl1271_ps_elp_wakeup(struct wl1271 *wl)
}
 
if (!pending) {
-   ret = wait_for_completion_timeout(
+   timeout = wait_for_completion_timeout(
&compl, msecs_to_jiffies(WL1271_WAKEUP_TIMEOUT));
-   if (ret == 0) {
+   if (timeout == 0) {
wl1271_error("ELP wakeup timeout!");
wl12xx_queue_recovery_work(wl);
ret = -ETIMEDOUT;
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

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  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 
> > ---
> >
> > 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 
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] cw1200: use msecs_to_jiffies for conversion

[Nicholas Mc Guire]

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

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

Converting milliseconds to jiffies by "val * HZ / 1000" or passing
HZ / 10 is technically OK but appropriate msecs_to_jiffies(val), respectively
msecs_to_jiffies(100) is the cleaner solution and handles all corner cases 
correctly. This is a minor API cleanup only.

Patch was only compile tested for x86_64_defconfig + CONFIG_CW1200=m

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

 drivers/net/wireless/cw1200/scan.c |8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/cw1200/scan.c 
b/drivers/net/wireless/cw1200/scan.c
index f2e276f..bff81b8 100644
--- a/drivers/net/wireless/cw1200/scan.c
+++ b/drivers/net/wireless/cw1200/scan.c
@@ -39,9 +39,9 @@ static int cw1200_scan_start(struct cw1200_common *priv, 
struct wsm_scan *scan)
cancel_delayed_work_sync(&priv->clear_recent_scan_work);
atomic_set(&priv->scan.in_progress, 1);
atomic_set(&priv->recent_scan, 1);
-   cw1200_pm_stay_awake(&priv->pm_state, tmo * HZ / 1000);
+   cw1200_pm_stay_awake(&priv->pm_state, msecs_to_jiffies(tmo));
queue_delayed_work(priv->workqueue, &priv->scan.timeout,
-  tmo * HZ / 1000);
+  msecs_to_jiffies(tmo));
ret = wsm_scan(priv, scan);
if (ret) {
atomic_set(&priv->scan.in_progress, 0);
@@ -386,8 +386,8 @@ void cw1200_probe_work(struct work_struct *work)
if (down_trylock(&priv->scan.lock)) {
/* Scan is already in progress. Requeue self. */
schedule();
-   queue_delayed_work(priv->workqueue,
-  &priv->scan.probe_work, HZ / 10);
+   queue_delayed_work(priv->workqueue, &priv->scan.probe_work,
+  msecs_to_jiffies(100));
mutex_unlock(&priv->conf_mutex);
return;
}
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] wireless: orinoco: orinoco_tmd use msecs_to_jiffies for conversion

[Nicholas Mc Guire]

This is only an API consolidation and should make things more readable
it replaces var * HZ / 1000 by msecs_to_jiffies(var).

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

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.

Patch was compile-tested only for x86_64_defconfig + CONFIG_HERMES=m
CONFIG_TMD_HERMES=m

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

 drivers/net/wireless/orinoco/orinoco_tmd.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/orinoco/orinoco_tmd.c 
b/drivers/net/wireless/orinoco/orinoco_tmd.c
index 79d0e33..20ce569 100644
--- a/drivers/net/wireless/orinoco/orinoco_tmd.c
+++ b/drivers/net/wireless/orinoco/orinoco_tmd.c
@@ -71,7 +71,7 @@ static int orinoco_tmd_cor_reset(struct orinoco_private *priv)
mdelay(1);
 
/* Just in case, wait more until the card is no longer busy */
-   timeout = jiffies + (TMD_RESET_TIME * HZ / 1000);
+   timeout = jiffies + msecs_to_jiffies(TMD_RESET_TIME);
reg = hermes_read_regn(hw, CMD);
while (time_before(jiffies, timeout) && (reg & HERMES_CMD_BUSY)) {
mdelay(1);
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] wireless: orinoco: orinoco_pci use msecs_to_jiffies for conversion

[Nicholas Mc Guire]

This is only an API consolidation and should make things more readable
it replaces var * HZ / 1000 by msecs_to_jiffies(var).

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

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.

Patch was compile-tested only for x86_64_defconfig + CONFIG_HERMES=m
CONFIG_HERMES_PRISM=y, CONFIG_PCI_HERMES=m

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

 drivers/net/wireless/orinoco/orinoco_pci.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/orinoco/orinoco_pci.c 
b/drivers/net/wireless/orinoco/orinoco_pci.c
index b6bdad6..74219d5 100644
--- a/drivers/net/wireless/orinoco/orinoco_pci.c
+++ b/drivers/net/wireless/orinoco/orinoco_pci.c
@@ -94,7 +94,7 @@ static int orinoco_pci_cor_reset(struct orinoco_private *priv)
mdelay(HERMES_PCI_COR_OFFT);
 
/* The card is ready when it's no longer busy */
-   timeout = jiffies + (HERMES_PCI_COR_BUSYT * HZ / 1000);
+   timeout = jiffies + msecs_to_jiffies(HERMES_PCI_COR_BUSYT);
reg = hermes_read_regn(hw, CMD);
while (time_before(jiffies, timeout) && (reg & HERMES_CMD_BUSY)) {
mdelay(1);
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] wireless: orinoco: orinoco_plx use msecs_to_jiffies for conversion

[Nicholas Mc Guire]

This is only an API consolidation and should make things more readable
it replaces var * HZ / 1000 by msecs_to_jiffies(var).

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

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.

Patch was compile-tested only for x86_64_defconfig + CONFIG_HERMES=m
CONFIG_PLX_HERMES=m

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

 drivers/net/wireless/orinoco/orinoco_plx.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/orinoco/orinoco_plx.c 
b/drivers/net/wireless/orinoco/orinoco_plx.c
index b8f6e5c..8b04523 100644
--- a/drivers/net/wireless/orinoco/orinoco_plx.c
+++ b/drivers/net/wireless/orinoco/orinoco_plx.c
@@ -121,7 +121,7 @@ static int orinoco_plx_cor_reset(struct orinoco_private 
*priv)
mdelay(1);
 
/* Just in case, wait more until the card is no longer busy */
-   timeout = jiffies + (PLX_RESET_TIME * HZ / 1000);
+   timeout = jiffies + msecs_to_jiffies(PLX_RESET_TIME);
reg = hermes_read_regn(hw, CMD);
while (time_before(jiffies, timeout) && (reg & HERMES_CMD_BUSY)) {
mdelay(1);
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] brcmfmac: use msecs_to_jiffies for time conversion

[Nicholas Mc Guire]

This is only an API consolidation and should make things more readable
it replaces var * HZ / 1000 by msecs_to_jiffies(var).

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

Patch was only compile tested with x86_64_defconfig + CONFIG_BRCMFMAC=m,
CONFIG_MMC=m, CONFIG_BRCMFMAC_SDIO=y

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

 drivers/net/wireless/brcm80211/brcmfmac/sdio.c |6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/sdio.c 
b/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
index 5e9d208..4f9469b 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
@@ -3972,7 +3972,7 @@ brcmf_sdio_watchdog(unsigned long data)
/* Reschedule the watchdog */
if (bus->wd_timer_valid)
mod_timer(&bus->timer,
- jiffies + BRCMF_WD_POLL_MS * HZ / 1000);
+ jiffies + msecs_to_jiffies(BRCMF_WD_POLL_MS));
}
 }
 
@@ -4291,13 +4291,13 @@ void brcmf_sdio_wd_timer(struct brcmf_sdio *bus, uint 
wdtick)
   dynamically changed or in the first instance
 */
bus->timer.expires =
-   jiffies + BRCMF_WD_POLL_MS * HZ / 1000;
+   jiffies + msecs_to_jiffies(BRCMF_WD_POLL_MS);
add_timer(&bus->timer);
 
} else {
/* Re arm the timer, at last watchdog period */
mod_timer(&bus->timer,
-   jiffies + BRCMF_WD_POLL_MS * HZ / 1000);
+   jiffies + msecs_to_jiffies(BRCMF_WD_POLL_MS));
}
 
bus->wd_timer_valid = true;
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] brcm80211: drop unreachable else case

[Nicholas Mc Guire]

the if/elseif/else is exhaustive - there is no 4th case given the 
  rssi_ctrl_mask = RADIO_2055_NBRSSI_SEL | RADIO_2055_WBRSSI_G1_SEL | 
  RADIO_2055_WBRSSI_G2_SEL;
so this unreachable else case (dead code) can be dropped.

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

It seems that there are only three cases:
RADIO_2055_NBRSSI_SEL | RADIO_2055_WBRSSI_G1_SEL | RADIO_2055_WBRSSI_G2_SEL
The conditional call to wlc_phy_rssisel_nphy(... NPHY_RSSI_SEL_*) thus do 
not seem to have a 4th case which would be covered by the final else.

If the final else is there to accommodate extensions then this patch is
of course wrong - in that case it would be good though to comment the
final else as it appears to be dead-code at the moment.

Patch was only compile tested with x86_64_defconfig + CONFIG_BRCMSMAC=m

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

 drivers/net/wireless/brcm80211/brcmsmac/phy/phy_n.c |   11 ++-
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmsmac/phy/phy_n.c 
b/drivers/net/wireless/brcm80211/brcmsmac/phy/phy_n.c
index 084f18f..99dac9b 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/phy/phy_n.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/phy/phy_n.c
@@ -23041,10 +23041,7 @@ static void wlc_phy_rssi_cal_nphy_rev2(struct 
brcms_phy *pi, u8 rssi_type)
else if (rssi_ctrl_state[0] == RADIO_2055_WBRSSI_G1_SEL)
wlc_phy_rssisel_nphy(pi, RADIO_MIMO_CORESEL_CORE1,
 NPHY_RSSI_SEL_W1);
-   else if (rssi_ctrl_state[0] == RADIO_2055_WBRSSI_G2_SEL)
-   wlc_phy_rssisel_nphy(pi, RADIO_MIMO_CORESEL_CORE1,
-NPHY_RSSI_SEL_W2);
-   else
+   else /* RADIO_2055_WBRSSI_G2_SEL */
wlc_phy_rssisel_nphy(pi, RADIO_MIMO_CORESEL_CORE1,
 NPHY_RSSI_SEL_W2);
if (rssi_ctrl_state[1] == RADIO_2055_NBRSSI_SEL)
@@ -23053,13 +23050,9 @@ static void wlc_phy_rssi_cal_nphy_rev2(struct 
brcms_phy *pi, u8 rssi_type)
else if (rssi_ctrl_state[1] == RADIO_2055_WBRSSI_G1_SEL)
wlc_phy_rssisel_nphy(pi, RADIO_MIMO_CORESEL_CORE2,
 NPHY_RSSI_SEL_W1);
-   else if (rssi_ctrl_state[1] == RADIO_2055_WBRSSI_G2_SEL)
+   else /* RADIO_2055_WBRSSI_G1_SEL */
wlc_phy_rssisel_nphy(pi, RADIO_MIMO_CORESEL_CORE2,
 NPHY_RSSI_SEL_W2);
-   else
-   wlc_phy_rssisel_nphy(pi, RADIO_MIMO_CORESEL_CORE2,
-NPHY_RSSI_SEL_W2);
-
wlc_phy_rssisel_nphy(pi, RADIO_MIMO_CORESEL_OFF, rssi_type);
 
write_phy_reg(pi, 0x91, rfctrlintc_state[0]);
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] mesh_plink: fixup type of timeout to match usage

[Nicholas Mc Guire]

timeout was being passed as int but assigned from u32/u16 values and used 
as unsigned type. This is really only for better readability.

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

After review of the use locations of mesh_plink_timer_set() and declarations 
in sta_info.h this should not be breaking any integer promotions.

This patch applies on top of 
"[PATCH 1/2] mesh_plink: use msecs_to_jiffies for proper time conversion"

Patch was only compile tested with x86_64_defconfig + CONFIG_MAC80211_MESH=y

Patch is against 4.0-rc1 (localversion-next is -next-20150302)

 net/mac80211/mesh_plink.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
index 4eefd5d..8465c05 100644
--- a/net/mac80211/mesh_plink.c
+++ b/net/mac80211/mesh_plink.c
@@ -621,7 +621,7 @@ static void mesh_plink_timer(unsigned long data)
sta->llid, sta->plid, reason);
 }
 
-static inline void mesh_plink_timer_set(struct sta_info *sta, int timeout)
+static inline void mesh_plink_timer_set(struct sta_info *sta, u32 timeout)
 {
sta->plink_timer.expires = jiffies + msecs_to_jiffies(timeout);
sta->plink_timer.data = (unsigned long) sta;
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] mesh_plink: use msecs_to_jiffies for proper time conversion

[Nicholas Mc Guire]

This is primarily an API consolidation and should make things more readable
it replaces var * HZ / 1000 by msecs_to_jiffies(var) which also handles
corner cases correctly.

There is a change of behavior as e.g. for HZ 100, t * HZ / 1000 will
return 0 for t < 10 but msecs_to_jiffies will return at least 1 always.

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

Patch was only compile tested with x86_64_defconfig + CONFIG_MAC80211_MESH=y

Patch is against 4.0-rc1 (localversion-next is -next-20150302)

 net/mac80211/mesh_plink.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
index b488e18..4eefd5d 100644
--- a/net/mac80211/mesh_plink.c
+++ b/net/mac80211/mesh_plink.c
@@ -17,7 +17,7 @@
 #define PLINK_GET_PLID(p) (p + 4)
 
 #define mod_plink_timer(s, t) (mod_timer(&s->plink_timer, \
-   jiffies + HZ * t / 1000))
+   jiffies + msecs_to_jiffies(t)))
 
 enum plink_event {
PLINK_UNDEFINED,
@@ -623,7 +623,7 @@ static void mesh_plink_timer(unsigned long data)
 
 static inline void mesh_plink_timer_set(struct sta_info *sta, int timeout)
 {
-   sta->plink_timer.expires = jiffies + (HZ * timeout / 1000);
+   sta->plink_timer.expires = jiffies + msecs_to_jiffies(timeout);
sta->plink_timer.data = (unsigned long) sta;
sta->plink_timer.function = mesh_plink_timer;
sta->plink_timeout = timeout;
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ath10k: match wait_for_completion_timeout return type

[Nicholas Mc Guire]

Return type of wait_for_completion_timeout is unsigned long not int.
An appropriately named unsigned long is added and the assignments fixed up.

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

While at it the missing spaces 3*HZ -> 3 * HZ were added as well (should
this be in a separate patch ?)

Patch was only compile tested with x86_64_defconfig + CONFIG_ATH_CARDS=m,
CONFIG_ATH10K=m

Patch is against 4.0-rc3 (localversion-next is -next-20150311)

 drivers/net/wireless/ath/ath10k/mac.c |   35 +++--
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c 
b/drivers/net/wireless/ath/ath10k/mac.c
index e8cc19f..7db8b81 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -98,6 +98,7 @@ static int ath10k_install_key(struct ath10k_vif *arvif,
  const u8 *macaddr, bool def_idx)
 {
struct ath10k *ar = arvif->ar;
+   unsigned long time_left;
int ret;
 
lockdep_assert_held(&ar->conf_mutex);
@@ -108,8 +109,8 @@ static int ath10k_install_key(struct ath10k_vif *arvif,
if (ret)
return ret;
 
-   ret = wait_for_completion_timeout(&ar->install_key_done, 3*HZ);
-   if (ret == 0)
+   time_left = wait_for_completion_timeout(&ar->install_key_done, 3 * HZ);
+   if (!time_left)
return -ETIMEDOUT;
 
return 0;
@@ -561,16 +562,16 @@ static void ath10k_mac_vif_beacon_cleanup(struct 
ath10k_vif *arvif)
 
 static inline int ath10k_vdev_setup_sync(struct ath10k *ar)
 {
-   int ret;
+   unsigned long time_left;
 
lockdep_assert_held(&ar->conf_mutex);
 
if (test_bit(ATH10K_FLAG_CRASH_FLUSH, &ar->dev_flags))
return -ESHUTDOWN;
 
-   ret = wait_for_completion_timeout(&ar->vdev_setup_done,
+   time_left = wait_for_completion_timeout(&ar->vdev_setup_done,
ATH10K_VDEV_SETUP_TIMEOUT_HZ);
-   if (ret == 0)
+   if (!time_left)
return -ETIMEDOUT;
 
return 0;
@@ -2432,6 +2433,7 @@ void ath10k_offchan_tx_work(struct work_struct *work)
const u8 *peer_addr;
int vdev_id;
int ret;
+   unsigned long time_left;
 
/* FW requirement: We must create a peer before FW will send out
 * an offchannel frame. Otherwise the frame will be stuck and
@@ -2477,9 +2479,9 @@ void ath10k_offchan_tx_work(struct work_struct *work)
 
ath10k_tx_htt(ar, skb);
 
-   ret = wait_for_completion_timeout(&ar->offchan_tx_completed,
- 3 * HZ);
-   if (ret == 0)
+   time_left = wait_for_completion_timeout(
+   &ar->offchan_tx_completed, 3 * HZ);
+   if (!time_left)
ath10k_warn(ar, "timed out waiting for offchannel skb 
%p\n",
skb);
 
@@ -2573,6 +2575,7 @@ static int ath10k_scan_stop(struct ath10k *ar)
.u.scan_id = ATH10K_SCAN_ID,
};
int ret;
+   unsigned long time_left;
 
lockdep_assert_held(&ar->conf_mutex);
 
@@ -2582,11 +2585,11 @@ static int ath10k_scan_stop(struct ath10k *ar)
goto out;
}
 
-   ret = wait_for_completion_timeout(&ar->scan.completed, 3*HZ);
-   if (ret == 0) {
+   time_left = wait_for_completion_timeout(&ar->scan.completed, 3 * HZ);
+   if (!time_left) {
ath10k_warn(ar, "failed to receive scan abortion completion: 
timed out\n");
ret = -ETIMEDOUT;
-   } else if (ret > 0) {
+   } else if (time_left > 0) {
ret = 0;
}
 
@@ -2655,6 +2658,7 @@ static int ath10k_start_scan(struct ath10k *ar,
 const struct wmi_start_scan_arg *arg)
 {
int ret;
+   unsigned long time_left;
 
lockdep_assert_held(&ar->conf_mutex);
 
@@ -2662,8 +2666,8 @@ static int ath10k_start_scan(struct ath10k *ar,
if (ret)
return ret;
 
-   ret = wait_for_completion_timeout(&ar->scan.started, 1*HZ);
-   if (ret == 0) {
+   time_left = wait_for_completion_timeout(&ar->scan.started, 1 * HZ);
+   if (!time_left) {
ret = ath10k_scan_stop(ar);
if (ret)
ath10k_warn(ar, "failed to stop scan: %d\n", ret);
@@ -4355,6 +4359,7 @@ static int ath10k_remain_on_channel(struct ieee80211_hw 
*hw,
struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
struct wmi_start_scan_arg arg;
int ret = 0;
+   unsigned long time_left;
 
mutex_lock(&ar->conf_mutex);
 
@@ -4404,8 +4409,8 @@ static int ath10k_remain_on_channel(struct ieee80211_hw 
*h

[PATCH 1/2 RFC] ath10k: move code out of the parameter list

[Nicholas Mc Guire]

Putting code into the parameter list of wait_event_timeout() might be 
legal C-code but not really readable - the "inline" code is simply
moved into a function and that passed to wait_event_timeout() as the
condition.

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

Thanks to Bjorn Mork  for clarifying my initial confusion !

Patch was only compile tested with x86_64_defconfig + CONFIG_ATH_CARDS=m,
CONFIG_ATH10K=m

Patch is against 4.0-rc3 (localversion-next is -next-20150311)

 drivers/net/wireless/ath/ath10k/mac.c |   32 ++--
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c 
b/drivers/net/wireless/ath/ath10k/mac.c
index e8cc19f..7b27d99 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -4463,11 +4463,25 @@ static int ath10k_set_rts_threshold(struct ieee80211_hw 
*hw, u32 value)
return ret;
 }
 
+static bool check_htt_state(struct ath10k *ar, bool skip)
+{
+   bool empty;
+
+   spin_lock_bh(&ar->htt.tx_lock);
+   empty = (ar->htt.num_pending_tx == 0);
+   spin_unlock_bh(&ar->htt.tx_lock);
+
+   skip = (ar->state == ATH10K_STATE_WEDGED) ||
+   test_bit(ATH10K_FLAG_CRASH_FLUSH,
+&ar->dev_flags);
+   return (empty || skip);
+}
+
 static void ath10k_flush(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 u32 queues, bool drop)
 {
struct ath10k *ar = hw->priv;
-   bool skip;
+   bool skip = false;
int ret;
 
/* mac80211 doesn't care if we really xmit queued frames or not
@@ -4480,19 +4494,9 @@ static void ath10k_flush(struct ieee80211_hw *hw, struct 
ieee80211_vif *vif,
if (ar->state == ATH10K_STATE_WEDGED)
goto skip;
 
-   ret = wait_event_timeout(ar->htt.empty_tx_wq, ({
-   bool empty;
-
-   spin_lock_bh(&ar->htt.tx_lock);
-   empty = (ar->htt.num_pending_tx == 0);
-   spin_unlock_bh(&ar->htt.tx_lock);
-
-   skip = (ar->state == ATH10K_STATE_WEDGED) ||
-  test_bit(ATH10K_FLAG_CRASH_FLUSH,
-   &ar->dev_flags);
-
-   (empty || skip);
-   }), ATH10K_FLUSH_TIMEOUT_HZ);
+   ret = wait_event_timeout(ar->htt.empty_tx_wq,
+check_htt_state(ar, skip),
+ATH10K_FLUSH_TIMEOUT_HZ);
 
if (ret <= 0 || skip)
ath10k_warn(ar, "failed to flush transmit queue (skip %i 
ar-state %i): %i\n",
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] ath10k: drop unnecessary check for negative return value

[Nicholas Mc Guire]

wait_event_timeout() returns 0 on timeout and >= 1 on success but never
< 0 - so checking for timeout should be for equiavalence to 0 no <= 0.

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

Patch was only compile tested with x86_64_defconfig + CONFIG_ATH_CARDS=m,
CONFIG_ATH10K=m

Patch is against 4.0-rc3 (localversion-next is -next-20150311)

 drivers/net/wireless/ath/ath10k/mac.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c 
b/drivers/net/wireless/ath/ath10k/mac.c
index 7b27d99..4286f5c 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -4498,7 +4498,7 @@ static void ath10k_flush(struct ieee80211_hw *hw, struct 
ieee80211_vif *vif,
 check_htt_state(ar, skip),
 ATH10K_FLUSH_TIMEOUT_HZ);
 
-   if (ret <= 0 || skip)
+   if (ret == 0 || skip)
ath10k_warn(ar, "failed to flush transmit queue (skip %i 
ar-state %i): %i\n",
skip, ar->state, ret);
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2 RFC] ath10k: move code out of the parameter list

[Nicholas Mc Guire]

On Wed, 11 Mar 2015, Pat Erley wrote:

> On 03/11/2015 12:19 PM, Nicholas Mc Guire wrote:
>> Putting code into the parameter list of wait_event_timeout() might be
>> legal C-code but not really readable - the "inline" code is simply
>> moved into a function and that passed to wait_event_timeout() as the
>> condition.
>>
>> Signed-off-by: Nicholas Mc Guire 
>> ---
>>
>> Thanks to Bjorn Mork  for clarifying my initial confusion !
>>
>> Patch was only compile tested with x86_64_defconfig + CONFIG_ATH_CARDS=m,
>> CONFIG_ATH10K=m
>>
>> Patch is against 4.0-rc3 (localversion-next is -next-20150311)
>>
>>   drivers/net/wireless/ath/ath10k/mac.c |   32 
>> ++--
>>   1 file changed, 18 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c 
>> b/drivers/net/wireless/ath/ath10k/mac.c
>> index e8cc19f..7b27d99 100644
>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>> @@ -4463,11 +4463,25 @@ static int ath10k_set_rts_threshold(struct 
>> ieee80211_hw *hw, u32 value)
>>  return ret;
>>   }
>>
>> +static bool check_htt_state(struct ath10k *ar, bool skip)
>> +{
>> +bool empty;
>> +
>> +spin_lock_bh(&ar->htt.tx_lock);
>> +empty = (ar->htt.num_pending_tx == 0);
>> +spin_unlock_bh(&ar->htt.tx_lock);
>> +
>> +skip = (ar->state == ATH10K_STATE_WEDGED) ||
>> +test_bit(ATH10K_FLAG_CRASH_FLUSH,
>> + &ar->dev_flags);
>> +return (empty || skip);
>> +}
>> +
>>   static void ath10k_flush(struct ieee80211_hw *hw, struct ieee80211_vif 
>> *vif,
>>   u32 queues, bool drop)
>>   {
>>  struct ath10k *ar = hw->priv;
>> -bool skip;
>> +bool skip = false;
>>  int ret;
>>
>>  /* mac80211 doesn't care if we really xmit queued frames or not
>> @@ -4480,19 +4494,9 @@ static void ath10k_flush(struct ieee80211_hw *hw, 
>> struct ieee80211_vif *vif,
>>  if (ar->state == ATH10K_STATE_WEDGED)
>>  goto skip;
>>
>> -ret = wait_event_timeout(ar->htt.empty_tx_wq, ({
>> -bool empty;
>> -
>> -spin_lock_bh(&ar->htt.tx_lock);
>> -empty = (ar->htt.num_pending_tx == 0);
>> -spin_unlock_bh(&ar->htt.tx_lock);
>> -
>> -skip = (ar->state == ATH10K_STATE_WEDGED) ||
>> -   test_bit(ATH10K_FLAG_CRASH_FLUSH,
>> -&ar->dev_flags);
>> -
>> -(empty || skip);
>> -}), ATH10K_FLUSH_TIMEOUT_HZ);
>> +ret = wait_event_timeout(ar->htt.empty_tx_wq,
>> + check_htt_state(ar, skip),
>> + ATH10K_FLUSH_TIMEOUT_HZ);
>>
>>  if (ret <= 0 || skip)
>>  ath10k_warn(ar, "failed to flush transmit queue (skip %i 
>> ar-state %i): %i\n",
>>
>
> Doesn't this change not assign to 'skip' in the calling function?  So  
> you'd want to make it:
>
> static bool check_htt_state(struct ath10k *ar, bool *skip)
> {
>   bool empty;
>
>   spin_lock_bh(&ar->htt.tx_lock);
>   empty = (ar->htt.num_pending_tx == 0);
>   spin_unlock_bh(&ar->htt.tx_lock);
>
>   *skip = (ar->state == ATH10K_STATE_WEDGED) ||
>   test_bit(ATH10K_FLAG_CRASH_FLUSH,
>&ar->dev_flags);
>   return (empty || *skip);
> }
>
> ret = wait_event_timeout(ar->htt.empty_tx_wq,
>check_htt_state(ar, &skip),
>ATH10K_FLUSH_TIMEOUT_HZ);
>
> To preserve the previous behavior?

yup - thats a bit braindead on my side - as skip is used later to evaluate 
the status call by value is buggy here. Will fix that up once I know if
the principle approach to this cleanup is ok.

thx!
hofrat
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2 RFC] ath10k: move code out of the parameter list

[Nicholas Mc Guire]

On Wed, 11 Mar 2015, Bj??rn Mork wrote:

> Nicholas Mc Guire  writes:
> 
> > Putting code into the parameter list of wait_event_timeout() might be 
> > legal C-code but not really readable - the "inline" code is simply
> > moved into a function and that passed to wait_event_timeout() as the
> > condition.
> >
> > Signed-off-by: Nicholas Mc Guire 
> > ---
> >
> > Thanks to Bjorn Mork  for clarifying my initial confusion !
> >
> > Patch was only compile tested with x86_64_defconfig + CONFIG_ATH_CARDS=m,
> > CONFIG_ATH10K=m
> >
> > Patch is against 4.0-rc3 (localversion-next is -next-20150311)
> >
> >  drivers/net/wireless/ath/ath10k/mac.c |   32 
> > ++--
> >  1 file changed, 18 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/net/wireless/ath/ath10k/mac.c 
> > b/drivers/net/wireless/ath/ath10k/mac.c
> > index e8cc19f..7b27d99 100644
> > --- a/drivers/net/wireless/ath/ath10k/mac.c
> > +++ b/drivers/net/wireless/ath/ath10k/mac.c
> > @@ -4463,11 +4463,25 @@ static int ath10k_set_rts_threshold(struct 
> > ieee80211_hw *hw, u32 value)
> > return ret;
> >  }
> >  
> > +static bool check_htt_state(struct ath10k *ar, bool skip)
> > +{
> > +   bool empty;
> > +
> > +   spin_lock_bh(&ar->htt.tx_lock);
> > +   empty = (ar->htt.num_pending_tx == 0);
> > +   spin_unlock_bh(&ar->htt.tx_lock);
> > +
> > +   skip = (ar->state == ATH10K_STATE_WEDGED) ||
> > +   test_bit(ATH10K_FLAG_CRASH_FLUSH,
> > +&ar->dev_flags);
> > +   return (empty || skip);
> > +}
> 
> 
> There is no value in the "skip" input argument here.  It could just as
> well be a local variable.
> 
> 
> >  static void ath10k_flush(struct ieee80211_hw *hw, struct ieee80211_vif 
> > *vif,
> >  u32 queues, bool drop)
> >  {
> > struct ath10k *ar = hw->priv;
> > -   bool skip;
> > +   bool skip = false;
> > int ret;
> >  
> > /* mac80211 doesn't care if we really xmit queued frames or not
> > @@ -4480,19 +4494,9 @@ static void ath10k_flush(struct ieee80211_hw *hw, 
> > struct ieee80211_vif *vif,
> > if (ar->state == ATH10K_STATE_WEDGED)
> > goto skip;
> >  
> > -   ret = wait_event_timeout(ar->htt.empty_tx_wq, ({
> > -   bool empty;
> > -
> > -   spin_lock_bh(&ar->htt.tx_lock);
> > -   empty = (ar->htt.num_pending_tx == 0);
> > -   spin_unlock_bh(&ar->htt.tx_lock);
> > -
> > -   skip = (ar->state == ATH10K_STATE_WEDGED) ||
> > -  test_bit(ATH10K_FLAG_CRASH_FLUSH,
> > -   &ar->dev_flags);
> > -
> > -   (empty || skip);
> > -   }), ATH10K_FLUSH_TIMEOUT_HZ);
> > +   ret = wait_event_timeout(ar->htt.empty_tx_wq,
> > +check_htt_state(ar, skip),
> > +ATH10K_FLUSH_TIMEOUT_HZ);
> >  
> > if (ret <= 0 || skip)
> > ath10k_warn(ar, "failed to flush transmit queue (skip %i 
> > ar-state %i): %i\n",
> 
> 
> Which is why this won't work.  The check_htt_state() function won't
> update the "skip" variable, so it is always false here. The test now
> fails to detect any of the two "skip conditions".  Not really a big
> problem of course, as it only masks a warning.  But still: Your attempt
> to clean up has changed the behaviour in an unintentional way.
> 
> I'd suggest to leave this alone as it is.  The existing code is really
> fine. And testing these odd corner cases is probably difficult, even for
> someone with the actual hardware.  I have no idea what will trigger the
> ATH10K_FLAG_CRASH_FLUSH flag for example..
>

Well testing of this sort of stuff is of course goig to be limited
I do think that cleanups of this form are worth doing as the code
readability is not very good in its current form - of course the
passing skip by value was a bit braindead - fixing it up is though simple
and I guess one then can assess that the behavior is the same as before.
 
> It's better to look into some real bug, where you are able to verify a
> fix.
>
to some extent the two are connected - if the code is hard to read and
also hard to formally describe then using tools like static code checkers
to verify correct API usage (atleast partially) is limited.

This was actually found while writing up a cocci scripts checking for
incorrect handling of the return value of wait_event_timeout (in this
case checking for <= 0 when the return is always >= 0.

So I think it would make sense to fix this type of code in general - even
if my ffirst attempt was broken.

thx!
hofrat
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC v2] ath10k: move code from parameter list into a function

[Nicholas Mc Guire]

On Wed, 11 Mar 2015, Johannes Berg wrote:

> On Wed, 2015-03-11 at 15:01 -0400, Nicholas Mc Guire wrote:
> > Putting code into the parameter list of wait_event_timeout() might be
> > legal C-code but not really readable - the "inline" code is simply
> > moved into a function and that passed to wait_event_timeout() as the
> > condition.
> 
> Arguably, that's even more unreadable since if you don't know this macro
> well you might assume the function is called only once, which is clearly
> not true...
> 
> Don't get me wrong, I'm not opposed to this change, but if you ask me
> it's not completely clear that this makes it more readable.
>
I'm not into this long enough to say what is better and if the consensus
is that this patch is no more readable than the original code
and no more maintainable either, then it is not worth the effort.

so thanks for your comments!

thx!
hofrat 
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC v2] ath10k: move code from parameter list into a function

[Nicholas Mc Guire]

On Wed, 11 Mar 2015, Sergei Shtylyov wrote:

> Hello.
>
> On 03/11/2015 10:01 PM, Nicholas Mc Guire wrote:
>
>> Putting code into the parameter list of wait_event_timeout() might be
>> legal C-code but not really readable - the "inline" code is simply
>> moved into a function and that passed to wait_event_timeout() as the
>> condition. As wait_event_timeout will always return >= 0 the following
>> timeout check is fixed up to  ret == 0 .
>
>> Signed-off-by: Nicholas Mc Guire 
> [...]
>
>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c 
>> b/drivers/net/wireless/ath/ath10k/mac.c
>> index e8cc19f..a7a12cc 100644
>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>> @@ -4463,11 +4463,25 @@ static int ath10k_set_rts_threshold(struct 
>> ieee80211_hw *hw, u32 value)
>>  return ret;
>>   }
>>
>> +static bool check_htt_state(struct ath10k *ar, bool *skip)
>> +{
>> +bool empty;
>> +
>> +spin_lock_bh(&ar->htt.tx_lock);
>> +empty = (ar->htt.num_pending_tx == 0);
>> +spin_unlock_bh(&ar->htt.tx_lock);
>> +
>> +*skip = (ar->state == ATH10K_STATE_WEDGED) ||
>> +test_bit(ATH10K_FLAG_CRASH_FLUSH,
>> + &ar->dev_flags);
>> +return (empty || *skip);
>
>Parens not needed here.
>
Thanks - will see what else I forgot and then repost a cleaned up
version.

thx!
hofrat
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC v2] ath10k: move code from parameter list into a function

[Nicholas Mc Guire]

Putting code into the parameter list of wait_event_timeout() might be
legal C-code but not really readable - the "inline" code is simply
moved into a function and that passed to wait_event_timeout() as the
condition. As wait_event_timeout will always return >= 0 the following
timeout check is fixed up to  ret == 0 .

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

Thanks to Bjorn Mork  for clarifying my initial confusion !

v2: Thanks to Pat Erley  for catching the botched skip
parameter - should have been call by reference not value - fixed here.

As Bjorn Mork  pointed out, a patch like this is hard to
verify by testing - but I do think that the code readability is much
better (if the patch is correct with respect to code behavior) and helps
simplify/automate API usage checking so its worth the effort.

Comments on if this is functionally equivalent to the original code as well
as if the helper func name is suitable would be appreciated.

Patch was only compile tested with x86_64_defconfig + CONFIG_ATH_CARDS=m,
CONFIG_ATH10K=m and the generated mac.i file code reviewed.

Patch is against 4.0-rc3 (localversion-next is -next-20150311)

 drivers/net/wireless/ath/ath10k/mac.c |   34 ++---
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c 
b/drivers/net/wireless/ath/ath10k/mac.c
index e8cc19f..a7a12cc 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -4463,11 +4463,25 @@ static int ath10k_set_rts_threshold(struct ieee80211_hw 
*hw, u32 value)
return ret;
 }
 
+static bool check_htt_state(struct ath10k *ar, bool *skip)
+{
+   bool empty;
+
+   spin_lock_bh(&ar->htt.tx_lock);
+   empty = (ar->htt.num_pending_tx == 0);
+   spin_unlock_bh(&ar->htt.tx_lock);
+
+   *skip = (ar->state == ATH10K_STATE_WEDGED) ||
+   test_bit(ATH10K_FLAG_CRASH_FLUSH,
+&ar->dev_flags);
+   return (empty || *skip);
+}
+
 static void ath10k_flush(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 u32 queues, bool drop)
 {
struct ath10k *ar = hw->priv;
-   bool skip;
+   bool skip = false;
int ret;
 
/* mac80211 doesn't care if we really xmit queued frames or not
@@ -4480,21 +4494,11 @@ static void ath10k_flush(struct ieee80211_hw *hw, 
struct ieee80211_vif *vif,
if (ar->state == ATH10K_STATE_WEDGED)
goto skip;
 
-   ret = wait_event_timeout(ar->htt.empty_tx_wq, ({
-   bool empty;
-
-   spin_lock_bh(&ar->htt.tx_lock);
-   empty = (ar->htt.num_pending_tx == 0);
-   spin_unlock_bh(&ar->htt.tx_lock);
-
-   skip = (ar->state == ATH10K_STATE_WEDGED) ||
-  test_bit(ATH10K_FLAG_CRASH_FLUSH,
-   &ar->dev_flags);
-
-   (empty || skip);
-   }), ATH10K_FLUSH_TIMEOUT_HZ);
+   ret = wait_event_timeout(ar->htt.empty_tx_wq,
+check_htt_state(ar, &skip),
+ATH10K_FLUSH_TIMEOUT_HZ);
 
-   if (ret <= 0 || skip)
+   if (ret == 0 || skip)
ath10k_warn(ar, "failed to flush transmit queue (skip %i 
ar-state %i): %i\n",
skip, ar->state, ret);
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] ath10k: htc: match wait_for_completion_timeout return type

[Nicholas Mc Guire]

Return type of wait_for_completion_timeout is unsigned long not int.
An appropriately named unsigned long is added and the assignments fixed up.

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

Note: this patch generates a checkpatch CHECK
CHECK: Alignment should match open parenthesis
#53: FILE: drivers/net/wireless/ath/ath10k/htc.c:574:
but the alternative is to go over 80 char so this should be ok ?

Patch was only compile tested with x86_64_defconfig + CONFIG_ATH_CARDS=m,
CONFIG_ATH10K=m

Patch is against 4.0-rc3 (localversion-next is -next-20150312)

 drivers/net/wireless/ath/ath10k/htc.c |   22 --
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htc.c 
b/drivers/net/wireless/ath/ath10k/htc.c
index 2fd9e18..c15edc1 100644
--- a/drivers/net/wireless/ath/ath10k/htc.c
+++ b/drivers/net/wireless/ath/ath10k/htc.c
@@ -548,6 +548,7 @@ int ath10k_htc_wait_target(struct ath10k_htc *htc)
 {
struct ath10k *ar = htc->ar;
int i, status = 0;
+   unsigned long time_left;
struct ath10k_htc_svc_conn_req conn_req;
struct ath10k_htc_svc_conn_resp conn_resp;
struct ath10k_htc_msg *msg;
@@ -555,9 +556,9 @@ int ath10k_htc_wait_target(struct ath10k_htc *htc)
u16 credit_count;
u16 credit_size;
 
-   status = wait_for_completion_timeout(&htc->ctl_resp,
-ATH10K_HTC_WAIT_TIMEOUT_HZ);
-   if (status == 0) {
+   time_left = wait_for_completion_timeout(&htc->ctl_resp,
+   ATH10K_HTC_WAIT_TIMEOUT_HZ);
+   if (!time_left) {
/* Workaround: In some cases the PCI HIF doesn't
 * receive interrupt for the control response message
 * even if the buffer was completed. It is suspected
@@ -569,10 +570,10 @@ int ath10k_htc_wait_target(struct ath10k_htc *htc)
for (i = 0; i < CE_COUNT; i++)
ath10k_hif_send_complete_check(htc->ar, i, 1);
 
-   status = wait_for_completion_timeout(&htc->ctl_resp,
-
ATH10K_HTC_WAIT_TIMEOUT_HZ);
+   time_left = wait_for_completion_timeout(&htc->ctl_resp,
+   ATH10K_HTC_WAIT_TIMEOUT_HZ);
 
-   if (status == 0)
+   if (!time_left)
status = -ETIMEDOUT;
}
 
@@ -646,6 +647,7 @@ int ath10k_htc_connect_service(struct ath10k_htc *htc,
struct sk_buff *skb;
unsigned int max_msg_size = 0;
int length, status;
+   unsigned long time_left;
bool disable_credit_flow_ctrl = false;
u16 message_id, service_id, flags = 0;
u8 tx_alloc = 0;
@@ -701,10 +703,10 @@ int ath10k_htc_connect_service(struct ath10k_htc *htc,
}
 
/* wait for response */
-   status = wait_for_completion_timeout(&htc->ctl_resp,
-ATH10K_HTC_CONN_SVC_TIMEOUT_HZ);
-   if (status == 0) {
-   ath10k_err(ar, "Service connect timeout: %d\n", status);
+   time_left = wait_for_completion_timeout(&htc->ctl_resp,
+   ATH10K_HTC_CONN_SVC_TIMEOUT_HZ);
+   if (!time_left) {
+   ath10k_err(ar, "Service connect timeout\n");
return -ETIMEDOUT;
}
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3 RFC] ath10k: wmi: match wait_for_completion_timeout return type

[Nicholas Mc Guire]

Return type of wait_for_completion_timeout is unsigned long not int.
An appropriately named unsigned long is added and the assignments fixed up.
Rather than returning 0 (timeout) or a more or less random remaining time
(completion success) this return 0 or 1 which also resolves the type of the
functions being int.

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

Checking the call-sites of ath10k_wmi_wait_for_unified_ready and 
ath10k_wmi_wait_for_service_ready the positive return value (remaining
time in jiffies) is never passed up the call-chain nor used so it is 
cleaner to treat this like a boolean success/fail only (actually the two
functions should probably be of type bool - but that does not seem to be
common practice in the ath10k code base)

Patch was only compile tested with x86_64_defconfig + CONFIG_ATH_CARDS=m,
CONFIG_ATH10K=m

Patch is against 4.0-rc3 (localversion-next is -next-20150312)
:
 drivers/net/wireless/ath/ath10k/wmi.c |   20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/wmi.c 
b/drivers/net/wireless/ath/ath10k/wmi.c
index c7ea77e..a1cdcba 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -884,20 +884,24 @@ void ath10k_wmi_put_wmi_channel(struct wmi_channel *ch,
 
 int ath10k_wmi_wait_for_service_ready(struct ath10k *ar)
 {
-   int ret;
+   unsigned long time_left;
 
-   ret = wait_for_completion_timeout(&ar->wmi.service_ready,
- WMI_SERVICE_READY_TIMEOUT_HZ);
-   return ret;
+   time_left = wait_for_completion_timeout(&ar->wmi.service_ready,
+   WMI_SERVICE_READY_TIMEOUT_HZ);
+   if(!time_left)
+   return 0;
+   return 1;
 }
 
 int ath10k_wmi_wait_for_unified_ready(struct ath10k *ar)
 {
-   int ret;
+   unsigned long time_left;
 
-   ret = wait_for_completion_timeout(&ar->wmi.unified_ready,
- WMI_UNIFIED_READY_TIMEOUT_HZ);
-   return ret;
+   time_left = wait_for_completion_timeout(&ar->wmi.unified_ready,
+   WMI_UNIFIED_READY_TIMEOUT_HZ);
+   if(!time_left)
+   return 0;
+   return 1;
 }
 
 struct sk_buff *ath10k_wmi_alloc_skb(struct ath10k *ar, u32 len)
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] ath10k: core: drop check for impossible negative return case

[Nicholas Mc Guire]

fixup ath10k_wmi_wait_for_service_ready and
ath10k_wmi_wait_for_unified_ready return >=0 but never
negative as they are just passing on the return value from
wait_for_completion_timeout - so the error check should be
against equality to 0 not <= 0.

Note that the code behavior is correct even with the current <= 0 check
as on success >= 1 is returned always - so this really is just a cleanup.

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

Patch was only compile tested with x86_64_defconfig + CONFIG_ATH_CARDS=m,
CONFIG_ATH10K=m

Patch is against 4.0-rc3 (localversion-next is -next-20150312)

 drivers/net/wireless/ath/ath10k/core.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c 
b/drivers/net/wireless/ath/ath10k/core.c
index c0e454b..b764336 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -1080,7 +1080,7 @@ int ath10k_core_start(struct ath10k *ar, enum 
ath10k_firmware_mode mode)
 
if (mode == ATH10K_FIRMWARE_MODE_NORMAL) {
status = ath10k_wmi_wait_for_service_ready(ar);
-   if (status <= 0) {
+   if (status == 0) {
ath10k_warn(ar, "wmi service ready event not received");
status = -ETIMEDOUT;
goto err_hif_stop;
@@ -1098,7 +1098,7 @@ int ath10k_core_start(struct ath10k *ar, enum 
ath10k_firmware_mode mode)
}
 
status = ath10k_wmi_wait_for_unified_ready(ar);
-   if (status <= 0) {
+   if (status == 0) {
ath10k_err(ar, "wmi unified ready event not received\n");
status = -ETIMEDOUT;
goto err_hif_stop;
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3 RFC] ath10k: wmi: match wait_for_completion_timeout return type

[Nicholas Mc Guire]

On Fri, 13 Mar 2015, Kalle Valo wrote:

> Nicholas Mc Guire  writes:
> 
> > Return type of wait_for_completion_timeout is unsigned long not int.
> > An appropriately named unsigned long is added and the assignments fixed up.
> > Rather than returning 0 (timeout) or a more or less random remaining time
> > (completion success) this return 0 or 1 which also resolves the type of the
> > functions being int.
> >
> > Signed-off-by: Nicholas Mc Guire 
> 
> Why does patch 2 in this patchset have RFC in the title but patches 1
> and 3 not? That just makes me confused, I can't tell what you want me to
> do with the patches. Normally I just drop all patches (or patchsets)
> which have RFC, and that's what I'm going to do now.
> 
> To save everyone's time, when submitting something please state clearly
> what's your intention.
>
ok - was simply unsure about the proposed change 
and 1 was a trivial cleanup (which should have been 
sent out as a seperate patch and not part of a series - my mistake)

Will fix this up and repost it.

sorry for the screwup - no intent to wast anybodies time.

thx!
hofrat
 
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ath10k: match wait_for_completion_timeout return type

[Nicholas Mc Guire]

On Fri, 13 Mar 2015, Kalle Valo wrote:

> Nicholas Mc Guire  writes:
> 
> > Return type of wait_for_completion_timeout is unsigned long not int.
> > An appropriately named unsigned long is added and the assignments fixed up.
> >
> > Signed-off-by: Nicholas Mc Guire 
> 
> Doesn't apply:
> 
> Applying: ath10k: match wait_for_completion_timeout return type
> fatal: sha1 information is lacking or useless 
> (drivers/net/wireless/ath/ath10k/mac.c).
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.
> Patch failed at 0001 ath10k: match wait_for_completion_timeout return type
>
sorry - no idea how I managed that - checking it.

Will fix it up and repost.

thx!
hofrat
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ath10k: mac: match wait_for_completion_timeout return type

[Nicholas Mc Guire]

Return type of wait_for_completion_timeout is unsigned long not int.
An appropriately named unsigned long is added, respectively 'ret'
renamed, and the assignments fixed up.

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

Patch was only compile tested with x86_64_defconfig + CONFIG_ATH_CARDS=m,
CONFIG_ATH10K=m

Patch is against 4.0-rc3 (localversion-next is -next-20150313)

 drivers/net/wireless/ath/ath10k/mac.c |   20 +++-
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c 
b/drivers/net/wireless/ath/ath10k/mac.c
index e8cc19f..3fa11d8 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -99,6 +99,7 @@ static int ath10k_install_key(struct ath10k_vif *arvif,
 {
struct ath10k *ar = arvif->ar;
int ret;
+   unsigned long time_left;
 
lockdep_assert_held(&ar->conf_mutex);
 
@@ -108,8 +109,8 @@ static int ath10k_install_key(struct ath10k_vif *arvif,
if (ret)
return ret;
 
-   ret = wait_for_completion_timeout(&ar->install_key_done, 3*HZ);
-   if (ret == 0)
+   time_left = wait_for_completion_timeout(&ar->install_key_done, 3 * HZ);
+   if (time_left == 0)
return -ETIMEDOUT;
 
return 0;
@@ -561,16 +562,16 @@ static void ath10k_mac_vif_beacon_cleanup(struct 
ath10k_vif *arvif)
 
 static inline int ath10k_vdev_setup_sync(struct ath10k *ar)
 {
-   int ret;
+   unsigned long time_left;
 
lockdep_assert_held(&ar->conf_mutex);
 
if (test_bit(ATH10K_FLAG_CRASH_FLUSH, &ar->dev_flags))
return -ESHUTDOWN;
 
-   ret = wait_for_completion_timeout(&ar->vdev_setup_done,
- ATH10K_VDEV_SETUP_TIMEOUT_HZ);
-   if (ret == 0)
+   time_left = wait_for_completion_timeout(&ar->vdev_setup_done,
+   ATH10K_VDEV_SETUP_TIMEOUT_HZ);
+   if (time_left == 0)
return -ETIMEDOUT;
 
return 0;
@@ -2432,6 +2433,7 @@ void ath10k_offchan_tx_work(struct work_struct *work)
const u8 *peer_addr;
int vdev_id;
int ret;
+   unsigned long time_left;
 
/* FW requirement: We must create a peer before FW will send out
 * an offchannel frame. Otherwise the frame will be stuck and
@@ -2477,9 +2479,9 @@ void ath10k_offchan_tx_work(struct work_struct *work)
 
ath10k_tx_htt(ar, skb);
 
-   ret = wait_for_completion_timeout(&ar->offchan_tx_completed,
- 3 * HZ);
-   if (ret == 0)
+   time_left =
+   wait_for_completion_timeout(&ar->offchan_tx_completed, 3 * HZ);
+   if (time_left == 0)
ath10k_warn(ar, "timed out waiting for offchannel skb 
%p\n",
skb);
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ath10k: htc: match wait_for_completion_timeout return type

[Nicholas Mc Guire]

Return type of wait_for_completion_timeout is unsigned long not int.
An appropriately named unsigned long is added and the assignments fixed up.

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

Patch was only compile tested with x86_64_defconfig + CONFIG_ATH_CARDS=m,
CONFIG_ATH10K=m

Patch is against 4.0-rc3 (localversion-next is -next-20150313)

 drivers/net/wireless/ath/ath10k/htc.c |   23 +--
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htc.c 
b/drivers/net/wireless/ath/ath10k/htc.c
index 2fd9e18..690a2f5 100644
--- a/drivers/net/wireless/ath/ath10k/htc.c
+++ b/drivers/net/wireless/ath/ath10k/htc.c
@@ -548,6 +548,7 @@ int ath10k_htc_wait_target(struct ath10k_htc *htc)
 {
struct ath10k *ar = htc->ar;
int i, status = 0;
+   unsigned long time_left;
struct ath10k_htc_svc_conn_req conn_req;
struct ath10k_htc_svc_conn_resp conn_resp;
struct ath10k_htc_msg *msg;
@@ -555,9 +556,9 @@ int ath10k_htc_wait_target(struct ath10k_htc *htc)
u16 credit_count;
u16 credit_size;
 
-   status = wait_for_completion_timeout(&htc->ctl_resp,
-ATH10K_HTC_WAIT_TIMEOUT_HZ);
-   if (status == 0) {
+   time_left = wait_for_completion_timeout(&htc->ctl_resp,
+   ATH10K_HTC_WAIT_TIMEOUT_HZ);
+   if (!time_left) {
/* Workaround: In some cases the PCI HIF doesn't
 * receive interrupt for the control response message
 * even if the buffer was completed. It is suspected
@@ -569,10 +570,11 @@ int ath10k_htc_wait_target(struct ath10k_htc *htc)
for (i = 0; i < CE_COUNT; i++)
ath10k_hif_send_complete_check(htc->ar, i, 1);
 
-   status = wait_for_completion_timeout(&htc->ctl_resp,
-
ATH10K_HTC_WAIT_TIMEOUT_HZ);
+   time_left =
+   wait_for_completion_timeout(&htc->ctl_resp,
+   ATH10K_HTC_WAIT_TIMEOUT_HZ);
 
-   if (status == 0)
+   if (!time_left)
status = -ETIMEDOUT;
}
 
@@ -646,6 +648,7 @@ int ath10k_htc_connect_service(struct ath10k_htc *htc,
struct sk_buff *skb;
unsigned int max_msg_size = 0;
int length, status;
+   unsigned long time_left;
bool disable_credit_flow_ctrl = false;
u16 message_id, service_id, flags = 0;
u8 tx_alloc = 0;
@@ -701,10 +704,10 @@ int ath10k_htc_connect_service(struct ath10k_htc *htc,
}
 
/* wait for response */
-   status = wait_for_completion_timeout(&htc->ctl_resp,
-ATH10K_HTC_CONN_SVC_TIMEOUT_HZ);
-   if (status == 0) {
-   ath10k_err(ar, "Service connect timeout: %d\n", status);
+   time_left = wait_for_completion_timeout(&htc->ctl_resp,
+   ATH10K_HTC_CONN_SVC_TIMEOUT_HZ);
+   if (!time_left) {
+   ath10k_err(ar, "Service connect timeout\n");
return -ETIMEDOUT;
}
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] ath10k: wmi: match wait_for_completion_timeout return type

[Nicholas Mc Guire]

return type of wait_for_completion_timeout is unsigned long not int. This
patch adjusts the type and name of the return variable and adjusts the 
success/fail values to match those used at the call site in ath10k_core_start

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

Adjustment of return values was proposed by Michal Kazior
 - thanks!

Patch was only compile tested with x86_64_defconfig + CONFIG_ATH_CARDS=m,
CONFIG_ATH10K=m

Patch is against 4.0-rc3 (localversion-next is -next-20150313)

 drivers/net/wireless/ath/ath10k/wmi.c |   20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/wmi.c 
b/drivers/net/wireless/ath/ath10k/wmi.c
index c7ea77e..65e495d 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -884,20 +884,24 @@ void ath10k_wmi_put_wmi_channel(struct wmi_channel *ch,
 
 int ath10k_wmi_wait_for_service_ready(struct ath10k *ar)
 {
-   int ret;
+   unsigned long time_left;
 
-   ret = wait_for_completion_timeout(&ar->wmi.service_ready,
- WMI_SERVICE_READY_TIMEOUT_HZ);
-   return ret;
+   time_left = wait_for_completion_timeout(&ar->wmi.service_ready,
+   WMI_SERVICE_READY_TIMEOUT_HZ);
+   if (!time_left)
+   return -ETIMEDOUT;
+   return 0;
 }
 
 int ath10k_wmi_wait_for_unified_ready(struct ath10k *ar)
 {
-   int ret;
+   unsigned long time_left;
 
-   ret = wait_for_completion_timeout(&ar->wmi.unified_ready,
- WMI_UNIFIED_READY_TIMEOUT_HZ);
-   return ret;
+   time_left = wait_for_completion_timeout(&ar->wmi.unified_ready,
+   WMI_UNIFIED_READY_TIMEOUT_HZ);
+   if (!time_left)
+   return -ETIMEDOUT;
+   return 0;
 }
 
 struct sk_buff *ath10k_wmi_alloc_skb(struct ath10k *ar, u32 len)
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] ath10k: core: harmonize error case handling in ath10k_core_start

[Nicholas Mc Guire]

All of the bringup/init functions called in ath10k_core_start return 0 on
success and != 0 on failure - ath10k_wmi_wait_for_service_ready and
ath10k_wmi_wait_for_unified_ready were adjusted to fit this model and the
call sites here fixed up accordingly. 

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

Patch was only compile tested with x86_64_defconfig + CONFIG_ATH_CARDS=m,
CONFIG_ATH10K=m

Patch is against 4.0-rc3 (localversion-next is -next-20150313)

 drivers/net/wireless/ath/ath10k/core.c |6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c 
b/drivers/net/wireless/ath/ath10k/core.c
index c0e454b..e97069f 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -1080,9 +1080,8 @@ int ath10k_core_start(struct ath10k *ar, enum 
ath10k_firmware_mode mode)
 
if (mode == ATH10K_FIRMWARE_MODE_NORMAL) {
status = ath10k_wmi_wait_for_service_ready(ar);
-   if (status <= 0) {
+   if (status) {
ath10k_warn(ar, "wmi service ready event not received");
-   status = -ETIMEDOUT;
goto err_hif_stop;
}
}
@@ -1098,9 +1097,8 @@ int ath10k_core_start(struct ath10k *ar, enum 
ath10k_firmware_mode mode)
}
 
status = ath10k_wmi_wait_for_unified_ready(ar);
-   if (status <= 0) {
+   if (status) {
ath10k_err(ar, "wmi unified ready event not received\n");
-   status = -ETIMEDOUT;
goto err_hif_stop;
}
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3 RFC] ath10k: wmi: match wait_for_completion_timeout return type

[Nicholas Mc Guire]

On Fri, 13 Mar 2015, Michal Kazior wrote:

> On 12 March 2015 at 16:49, Nicholas Mc Guire  wrote:
> > Return type of wait_for_completion_timeout is unsigned long not int.
> > An appropriately named unsigned long is added and the assignments fixed up.
> > Rather than returning 0 (timeout) or a more or less random remaining time
> > (completion success) this return 0 or 1 which also resolves the type of the
> > functions being int.
> >
> > Signed-off-by: Nicholas Mc Guire 
> > ---
> >
> > Checking the call-sites of ath10k_wmi_wait_for_unified_ready and
> > ath10k_wmi_wait_for_service_ready the positive return value (remaining
> > time in jiffies) is never passed up the call-chain nor used so it is
> > cleaner to treat this like a boolean success/fail only (actually the two
> > functions should probably be of type bool - but that does not seem to be
> > common practice in the ath10k code base)
> 
> It'd make sense to have these functions return 0 or -ETIMEDOUT. In
> that case both call sites would need to be adjusted to treat "< 0" or
> "!x" as an error (instead of the current "<= 0") condition and not set
> -ETIMEDOUT themselves.
>
looking at the call sites in ath10k_core_start more or less 
all other initialization calls will treate 0 as success and
!=0 as failure so this is the cleaner solution. as its all
now

status = call()
if(status)
error

patch just posted.

thx!
hofrat

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ath10k: debug: match wait_for_completion_timeout return type

[Nicholas Mc Guire]

Return type of wait_for_completion_timeout is unsigned long not int.
An appropriately named unsigned long is added and the assignments fixed up.
Missing spaces 1*HZ -> 1 * HZ were also added along the way.

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

Patch was only compile tested with x86_64_defconfig + CONFIG_ATH_CARDS=m,
CONFIG_ATH10K=m

Patch is against 4.0-rc3 (localversion-next is -next-20150313)

 drivers/net/wireless/ath/ath10k/debug.c |   11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/debug.c 
b/drivers/net/wireless/ath/ath10k/debug.c
index 301081d..a252774 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -380,12 +380,12 @@ unlock:
 
 static int ath10k_debug_fw_stats_request(struct ath10k *ar)
 {
-   unsigned long timeout;
+   unsigned long timeout, time_left;
int ret;
 
lockdep_assert_held(&ar->conf_mutex);
 
-   timeout = jiffies + msecs_to_jiffies(1*HZ);
+   timeout = jiffies + msecs_to_jiffies(1 * HZ);
 
ath10k_debug_fw_stats_reset(ar);
 
@@ -404,9 +404,10 @@ static int ath10k_debug_fw_stats_request(struct ath10k *ar)
return ret;
}
 
-   ret = wait_for_completion_timeout(&ar->debug.fw_stats_complete,
- 1*HZ);
-   if (ret == 0)
+   time_left =
+   wait_for_completion_timeout(&ar->debug.fw_stats_complete,
+   1 * HZ);
+   if (!time_left)
return -ETIMEDOUT;
 
spin_lock_bh(&ar->data_lock);
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ath10k: core: match wait_for_completion_timeout return type

[Nicholas Mc Guire]

Return type of wait_for_completion_timeout is unsigned long not int.
An appropriately named unsigned long is added and the assignments fixed up.

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

Patch was only compile tested with x86_64_defconfig + CONFIG_ATH_CARDS=m,
CONFIG_ATH10K=m

Patch is against 4.0-rc3 (localversion-next is -next-20150313)

 drivers/net/wireless/ath/ath10k/core.c |5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c 
b/drivers/net/wireless/ath/ath10k/core.c
index c0e454b..5fd833e 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -1151,6 +1151,7 @@ EXPORT_SYMBOL(ath10k_core_start);
 int ath10k_wait_for_suspend(struct ath10k *ar, u32 suspend_opt)
 {
int ret;
+   unsigned long time_left;
 
reinit_completion(&ar->target_suspend);
 
@@ -1160,9 +1161,9 @@ int ath10k_wait_for_suspend(struct ath10k *ar, u32 
suspend_opt)
return ret;
}
 
-   ret = wait_for_completion_timeout(&ar->target_suspend, 1 * HZ);
+   time_left = wait_for_completion_timeout(&ar->target_suspend, 1 * HZ);
 
-   if (ret == 0) {
+   if (!time_left) {
ath10k_warn(ar, "suspend timed out - target pause event never 
came\n");
return -ETIMEDOUT;
}
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ath10k: thermal: match wait_for_completion_timeout return type

[Nicholas Mc Guire]

Return type of wait_for_completion_timeout is unsigned long not int.
An appropriately named unsigned long is added and the assignments fixed up.

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

Patch was only compile tested with x86_64_defconfig + CONFIG_ATH_CARDS=m,
CONFIG_ATH10K=m

Patch is against 4.0-rc3 (localversion-next is -next-20150313)

 drivers/net/wireless/ath/ath10k/thermal.c |7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/thermal.c 
b/drivers/net/wireless/ath/ath10k/thermal.c
index aede750..dfa7c93 100644
--- a/drivers/net/wireless/ath/ath10k/thermal.c
+++ b/drivers/net/wireless/ath/ath10k/thermal.c
@@ -127,6 +127,7 @@ static ssize_t ath10k_thermal_show_temp(struct device *dev,
 {
struct ath10k *ar = dev_get_drvdata(dev);
int ret, temperature;
+   unsigned long time_left;
 
mutex_lock(&ar->conf_mutex);
 
@@ -148,9 +149,9 @@ static ssize_t ath10k_thermal_show_temp(struct device *dev,
goto out;
}
 
-   ret = wait_for_completion_timeout(&ar->thermal.wmi_sync,
- ATH10K_THERMAL_SYNC_TIMEOUT_HZ);
-   if (ret == 0) {
+   time_left = wait_for_completion_timeout(&ar->thermal.wmi_sync,
+   ATH10K_THERMAL_SYNC_TIMEOUT_HZ);
+   if (!time_left) {
ath10k_warn(ar, "failed to synchronize thermal read\n");
ret = -ETIMEDOUT;
goto out;
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ath10k: core: harmonize error case handling in ath10k_core_start

[Nicholas Mc Guire]

On Sat, 14 Mar 2015, Sergei Shtylyov wrote:

> Hello.
>
> On 03/14/2015 11:55 AM, Nicholas Mc Guire wrote:
>
>> All of the bringup/init functions called in ath10k_core_start return 0 on
>> success and != 0 on failure - ath10k_wmi_wait_for_service_ready and
>> ath10k_wmi_wait_for_unified_ready were adjusted to fit this model and the
>> call sites here fixed up accordingly.
>
>If you've changed the sense of these function's results, you need to  
> adjust the call sites in the same patch, because otherwise one wouldn't 
> be able to bisect this...
>

yup both patches would mess up if individually applied or reverted.
sorry my ignorance - did not think of that problem - will fix it 
up as a single patch and repost. 

thx!
hofrat
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ath10k: harmonize error case handling in ath10k_core_start

[Nicholas Mc Guire]

All of the bringup/init functions called in ath10k_core_start return 0 
on success and != 0 on failure. ath10k_wmi_wait_for_service_ready(),
ath10k_wmi_wait_for_unified_ready() and their call sites were adjusted
to fit this model.
The return type of wait_for_completion_timeout is unsigned long not int so
ath10k_wmi_wait_for_service_ready() and ath10k_wmi_wait_for_unified_ready()
were fixed up accordingly.

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

This was initially in two separate patches which were joined as suggested
by Sergei Shtylyov  to ensure that it
will not break bisection - thanks!

Patch was only compile test with x86_64_defconfig + CONFIG_ATH_CARDS=m,
CONFIG_ATH10K=m

Patch is against 4.0-rc3 (localversion-next is -next-20150316)

 drivers/net/wireless/ath/ath10k/core.c |6 ++
 drivers/net/wireless/ath/ath10k/wmi.c  |   20 
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c 
b/drivers/net/wireless/ath/ath10k/core.c
index c0e454b..e97069f 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -1080,9 +1080,8 @@ int ath10k_core_start(struct ath10k *ar, enum 
ath10k_firmware_mode mode)
 
if (mode == ATH10K_FIRMWARE_MODE_NORMAL) {
status = ath10k_wmi_wait_for_service_ready(ar);
-   if (status <= 0) {
+   if (status) {
ath10k_warn(ar, "wmi service ready event not received");
-   status = -ETIMEDOUT;
goto err_hif_stop;
}
}
@@ -1098,9 +1097,8 @@ int ath10k_core_start(struct ath10k *ar, enum 
ath10k_firmware_mode mode)
}
 
status = ath10k_wmi_wait_for_unified_ready(ar);
-   if (status <= 0) {
+   if (status) {
ath10k_err(ar, "wmi unified ready event not received\n");
-   status = -ETIMEDOUT;
goto err_hif_stop;
}
 
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c 
b/drivers/net/wireless/ath/ath10k/wmi.c
index c7ea77e..19ed27a 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -884,20 +884,24 @@ void ath10k_wmi_put_wmi_channel(struct wmi_channel *ch,
 
 int ath10k_wmi_wait_for_service_ready(struct ath10k *ar)
 {
-   int ret;
+   unsigned long time_left;
 
-   ret = wait_for_completion_timeout(&ar->wmi.service_ready,
- WMI_SERVICE_READY_TIMEOUT_HZ);
-   return ret;
+   time_left = wait_for_completion_timeout(&ar->wmi.service_ready,
+   WMI_SERVICE_READY_TIMEOUT_HZ);
+   if (!time_left)
+   return -ETIMEDOUT;
+   return 0;
 }
 
 int ath10k_wmi_wait_for_unified_ready(struct ath10k *ar)
 {
-   int ret;
+   unsigned long time_left;
 
-   ret = wait_for_completion_timeout(&ar->wmi.unified_ready,
- WMI_UNIFIED_READY_TIMEOUT_HZ);
-   return ret;
+   time_left = wait_for_completion_timeout(&ar->wmi.unified_ready,
+   WMI_UNIFIED_READY_TIMEOUT_HZ);
+   if (!time_left)
+   return -ETIMEDOUT;
+   return 0;
 }
 
 struct sk_buff *ath10k_wmi_alloc_skb(struct ath10k *ar, u32 len)
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ath10k: match wait_for_completion_timeout return type

[Nicholas Mc Guire]

On Mon, 16 Mar 2015, Kalle Valo wrote:

> Nicholas Mc Guire  writes:
> 
> > On Fri, 13 Mar 2015, Kalle Valo wrote:
> >
> >> Nicholas Mc Guire  writes:
> >> 
> >> > Return type of wait_for_completion_timeout is unsigned long not int.
> >> > An appropriately named unsigned long is added and the assignments fixed 
> >> > up.
> >> >
> >> > Signed-off-by: Nicholas Mc Guire 
> >> 
> >> Doesn't apply:
> >> 
> >> Applying: ath10k: match wait_for_completion_timeout return type
> >> fatal: sha1 information is lacking or useless 
> >> (drivers/net/wireless/ath/ath10k/mac.c).
> >> Repository lacks necessary blobs to fall back on 3-way merge.
> >> Cannot fall back to three-way merge.
> >> Patch failed at 0001 ath10k: match wait_for_completion_timeout return type
> >>
> > sorry - no idea how I managed that - checking it.
> >
> > Will fix it up and repost.
> 
> Did you use master branch from ath.git repository? ath10k is in quite
> active development so if you use something else there's a substantial
> risk that the patch will not apply.
> 
> https://wireless.wiki.kernel.org/en/users/drivers/ath10k/sources
>
no I was using linux-next - will switch to that branch for the
ath10k related cleanups (although the one I posted today was the
last from the completion fixup in ath10k - a few more in ath9k
left).

The failure of this patch was though due to a bad fix of a 
checkpatch warning - I alligned the original code rather than 
the replacement - updated my procedure so that re-applying the
patch is the final check before sending out.

thx!
hofrat
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] brcmfmac: cfg80211: use msecs_to_jiffies for time conversion

[Nicholas Mc Guire]

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 consolidation only and
should make things more readable.

Patch was compile tested with x86_64_defconfig + CONFIG_BRCMFMAC=m

Patch is agianst 4.0-rc4 (localversion-next is -next-20150317)

Signed-off-by: Nicholas Mc Guire 
---
 drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c 
b/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
index 9b805c9..1996dc2 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
@@ -1110,7 +1110,7 @@ brcmf_cfg80211_escan(struct wiphy *wiphy, struct 
brcmf_cfg80211_vif *vif,
 
/* Arm scan timeout timer */
mod_timer(&cfg->escan_timeout, jiffies +
-   WL_ESCAN_TIMER_INTERVAL_MS * HZ / 1000);
+ msecs_to_jiffies(WL_ESCAN_TIMER_INTERVAL_MS));
 
return 0;
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] brcmfmac: cfg80211: use msecs_to_jiffies for time conversion

[Nicholas Mc Guire]

On Tue, 17 Mar 2015, Joe Perches wrote:

> On Tue, 2015-03-17 at 08:06 -0400, Nicholas Mc Guire wrote:
> > 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 consolidation only and
> > should make things more readable.
> 
> Hi Nicholas
> 
> These API consolidation changes now always have a function
> call when the compiler may have previously been able to
> optimize out the "constant * HZ / 1000" calculation.
>
> Perhaps the [um]secs_to_jiffies calls should be indirected
> with yet another static inline with a __builtin_constant_p()
> test so that the function calls can again be avoided when
> possible.

will give it a try 

> 
> (and a trivial style note)
> 
> > diff --git a/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c 
> > b/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
> []
> > @@ -1110,7 +1110,7 @@ brcmf_cfg80211_escan(struct wiphy *wiphy, struct 
> > brcmf_cfg80211_vif *vif,
> >  
> > /* Arm scan timeout timer */
> > mod_timer(&cfg->escan_timeout, jiffies +
> > -   WL_ESCAN_TIMER_INTERVAL_MS * HZ / 1000);
> > + msecs_to_jiffies(WL_ESCAN_TIMER_INTERVAL_MS));
> 
> It may be nicer to keep the arithmetic on one line

sorry that was plain carelessness afer it went over 80 char.

> 
>   mod_timer(&cfg->escan_timeout,
> jiffies + msecs_to_jiffies(WL_ESCAN_TIMER_INTERVAL_MS));
>

thx!
hofrat 
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RFC if==else in halbtc8723b1ant.c

[Nicholas Mc Guire]

Hi !

 in your commit f5b586909581 ("rtlwifi: btcoexist: Modify driver to support 
 BT coexistence in rtl8723be") you introduced a if/else where both branches 
 are the same but the comment in the else branch suggests that this might be
 unintended.

 from code review only I can´t say what the intent is.

/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8723b1ant.c:halbtc8723b1ant_action_wifi_connected_bt_acl_busy()

1838 if ((bt_rssi_state == BTC_RSSI_STATE_HIGH) ||
1839 (bt_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) {
1840 halbtc8723b1ant_ps_tdma(btcoexist, NORMAL_EXEC,
1841 true, 14);
1842 coex_dm->auto_tdma_adjust = false;
1843 } else { /*for low BT RSSI*/
1844 halbtc8723b1ant_ps_tdma(btcoexist, NORMAL_EXEC,
1845 true, 14);
1846 coex_dm->auto_tdma_adjust = false;
1847 }

  basically the same construct is also in 
 halbtc8723b1ant_run_coexist_mechanism()

2213 if ((wifi_rssi_state == BTC_RSSI_STATE_HIGH) ||
2214 (wifi_rssi_state == BTC_RSSI_STATE_STAY_HIGH)) 
{
2215 halbtc8723b1ant_limited_tx(btcoexist,
2216NORMAL_EXEC,
22171, 1, 1, 1);
2218 } else {
2219 halbtc8723b1ant_limited_tx(btcoexist,
2220NORMAL_EXEC,
22211, 1, 1, 1);
 }

 where the if condition is the same so the else may also only apply to the
 low BT RSSI - and the if and else are again the same - if this is intended
 or not is not clear. If this is intended it should have appropriate comments.

thx!
hofrat

[PATCH RFC] rtlwifi: btcoexist: fix port assignment

[Nicholas Mc Guire]

The port assignment in the if case should be to AUX not MAIN.

Fixes: commit baa170229095 ("rtlwifi: btcoexist: Implement antenna selection")
Signed-off-by: Nicholas Mc Guire 
---
problem located by coccinelle

in:
drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.c:exhalbtc_set_ant_num()
973   /* The antenna position:
974* Main (default) or Aux for pgAntNum=2 && btdmAntNum =1.
975* The antenna position should be determined by
976* auto-detect mechanism.
977* The following is assumed to main,
978* and those must be modified
979* if y auto-detect mechanism is ready
980*/
981   if ((gl_bt_coexist.board_info.pg_ant_num == 2) &&
982   (gl_bt_coexist.board_info.btdm_ant_num == 1))
983   gl_bt_coexist.board_info.btdm_ant_pos =
984  BTC_ANTENNA_AT_MAIN_PORT;
985   else
986   gl_bt_coexist.board_info.btdm_ant_pos =
987  BTC_ANTENNA_AT_MAIN_PORT;
(line number from 4.9.0-rc2 linux-next 20161028)

the if and else branch here are the same but the comment seems to indicate
that the first case should be the AUX port and not the MAIN port here (the
second sentence in the comment though is not really clear to me). If the 
intent is to set it to MAIN unconditionally and then let autodetect fix it
then the if/else construct is useless.

Looks like a cut&past bug, but this needs a check by someone who knows the
details of the device.

Patch was compile tested with: x86_64_defconfig + RTL8723AE=m 
(implies CONFIG_RTLBTCOEXIST)

Patch is against 4.9.0-rc2 (localversion-next is next-20161028)

 drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.c 
b/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.c
index 91cc139..588c8ed 100644
--- a/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.c
+++ b/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.c
@@ -981,7 +981,7 @@ void exhalbtc_set_ant_num(struct rtl_priv *rtlpriv, u8 
type, u8 ant_num)
if ((gl_bt_coexist.board_info.pg_ant_num == 2) &&
(gl_bt_coexist.board_info.btdm_ant_num == 1))
gl_bt_coexist.board_info.btdm_ant_pos =
-  BTC_ANTENNA_AT_MAIN_PORT;
+  BTC_ANTENNA_AT_AUX_PORT;
else
gl_bt_coexist.board_info.btdm_ant_pos =
   BTC_ANTENNA_AT_MAIN_PORT;
-- 
2.1.4

[PATCH 1/4] ath10k: fixup wait_for_completion_timeout return handling

[Nicholas Mc Guire]

wait_for_completion_timeout does not return negative values so
"ret" handling here should check for timeout only.

patch was only compile tested for x86_64_defconfig + CONFIG_ATH_DEBUG=y

patch is against linux-next 3.19.0-rc1 -next-20141226

Signed-off-by: Nicholas Mc Guire 
---
 drivers/net/wireless/ath/ath10k/debug.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/debug.c 
b/drivers/net/wireless/ath/ath10k/debug.c
index a716758..7e1fe93 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -373,7 +373,7 @@ static int ath10k_debug_fw_stats_request(struct ath10k *ar)
 
ret = wait_for_completion_timeout(&ar->debug.fw_stats_complete,
  1*HZ);
-   if (ret <= 0)
+   if (ret == 0)
return -ETIMEDOUT;
 
spin_lock_bh(&ar->data_lock);
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] ath10k: fixup wait_for_completion_timeout return handling

[Nicholas Mc Guire]

wait_for_completion_timeout does not return negative values so
"ret" handling here should check for timeout only.

patch was only compile tested for x86_64_defconfig + CONFIG_ATH10K=m

patch is against linux-next 3.19.0-rc1 -next-20141226

Signed-off-by: Nicholas Mc Guire 
---
 drivers/net/wireless/ath/ath10k/htc.c |6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htc.c 
b/drivers/net/wireless/ath/ath10k/htc.c
index f1946a6..2fd9e18 100644
--- a/drivers/net/wireless/ath/ath10k/htc.c
+++ b/drivers/net/wireless/ath/ath10k/htc.c
@@ -703,11 +703,9 @@ int ath10k_htc_connect_service(struct ath10k_htc *htc,
/* wait for response */
status = wait_for_completion_timeout(&htc->ctl_resp,
 ATH10K_HTC_CONN_SVC_TIMEOUT_HZ);
-   if (status <= 0) {
-   if (status == 0)
-   status = -ETIMEDOUT;
+   if (status == 0) {
ath10k_err(ar, "Service connect timeout: %d\n", status);
-   return status;
+   return -ETIMEDOUT;
}
 
/* we controlled the buffer creation, it's aligned */
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] ath10k: fixup wait_for_completion_timeout return handling

[Nicholas Mc Guire]

wait_for_completion_timeout does not return negative values so
"ret" handling here should check for timeout only.

patch was only compile tested for x86_64_defconfig + CONFIG_ATH10K=m

patch is against linux-next 3.19.0-rc1 -next-20141226

Signed-off-by: Nicholas Mc Guire 
---
 drivers/net/wireless/ath/ath10k/mac.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c 
b/drivers/net/wireless/ath/ath10k/mac.c
index c400567..f9d7dbb 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2151,7 +2151,7 @@ void ath10k_offchan_tx_work(struct work_struct *work)
 
ret = wait_for_completion_timeout(&ar->offchan_tx_completed,
  3 * HZ);
-   if (ret <= 0)
+   if (ret == 0)
ath10k_warn(ar, "timed out waiting for offchannel skb 
%p\n",
skb);
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/4] ath10k: a few incorrect return handling fix-up

[Nicholas Mc Guire]

wait_for_completion_timeout does not return negative values so the tests
for <= 0 are not needed and the case differentiation in the error handling
path unnecessary.

patch was only compile tested x86_64_defconfig + CONFIG_ATH_CARDS=m
CONFIG_ATH10K=m

patch is against linux-next 3.19.0-rc1 -next-20141226

None of the proposed cleanups are critical.
All changes should only be removing unreachable cases.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] ath10k: fixup wait_for_completion_timeout return handling

[Nicholas Mc Guire]

wait_for_completion_timeout does not return negative values so
"ret" handling here should check for timeout only.

patch was only compile tested for x86_64_defconfig + CONFIG_ATH10K=m

patch is against linux-next 3.19.0-rc1 -next-20141226

Signed-off-by: Nicholas Mc Guire 
---
 drivers/net/wireless/ath/ath10k/htt.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt.c 
b/drivers/net/wireless/ath/ath10k/htt.c
index 56cb4ac..58b6fc1 100644
--- a/drivers/net/wireless/ath/ath10k/htt.c
+++ b/drivers/net/wireless/ath/ath10k/htt.c
@@ -102,7 +102,7 @@ int ath10k_htt_setup(struct ath10k_htt *htt)
 
status = wait_for_completion_timeout(&htt->target_version_received,
 HTT_TARGET_VERSION_TIMEOUT_HZ);
-   if (status <= 0) {
+   if (status == 0) {
ath10k_warn(ar, "htt version request timed out\n");
return -ETIMEDOUT;
}
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] ath10k: a few incorrect return handling fix-up

[Nicholas Mc Guire]

On Tue, 30 Dec 2014, Sergei Shtylyov wrote:

> Hello.
>
> On 12/30/2014 03:20 PM, Nicholas Mc Guire wrote:
>
>> wait_for_completion_timeout does not return negative values so the tests
>> for <= 0 are not needed and the case differentiation in the error handling
>> path unnecessary.
>
>I decided to verify your statement and I saw that it seems wrong.  
> do_wait_for_common() can return -ERESTARTSYS and the return value gets  
> returned by its callers unchanged.

the -ERESTARTSYS only can be returned if state matches but
wait_for_completion_timemout passes TASK_UNINTERRUPTIBLE
so signal_pending_state will return 0 and never negativ

my understanding of the callchain is:
wait_for_completion_timemout with TASK_UNINTERRUPTIBLE
  -> wait_for_common(...TASK_UNINTERRUPTIBLE)
-> __wait_for_common(...TASK_UNINTERRUPTIBLE)
  -> do_wait_for_common(...TASK_UNINTERRUPTIBLE)
-> signal_pending_state(TASK_UNINTERRUPTIBLE...)

static inline int signal_pending_state(long state, struct task_struct *p)
{
if (!(state & (TASK_INTERRUPTIBLE | TASK_WAKEKILL)))
return 0;

so wait_for_completion_timemout should return 0 or 1 only

>
>> patch was only compile tested x86_64_defconfig + CONFIG_ATH_CARDS=m
>> CONFIG_ATH10K=m
>
>> patch is against linux-next 3.19.0-rc1 -next-20141226
>
>Rather patches. It would have been better to send one patch instead of 
> 4 patches with the same name.
>
sorry for that - I had split it into separate patches as it was
in different files - giving them the same name of course was a bit
brain-dead.

please do give it one more look - if the above argument is invalid
I apologize for the noise.


thx!
hofrat
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] ath10k: a few incorrect return handling fix-up

[Nicholas Mc Guire]

On Tue, 30 Dec 2014, Sergei Shtylyov wrote:

> On 12/30/2014 09:28 PM, Nicholas Mc Guire wrote:
>
>>>> wait_for_completion_timeout does not return negative values so the tests
>>>> for <= 0 are not needed and the case differentiation in the error handling
>>>> path unnecessary.
>
>>> I decided to verify your statement and I saw that it seems wrong.
>>> do_wait_for_common() can return -ERESTARTSYS and the return value gets
>>> returned by its callers unchanged.
>
>> the -ERESTARTSYS only can be returned if state matches but
>> wait_for_completion_timemout passes TASK_UNINTERRUPTIBLE
>> so signal_pending_state will return 0 and never negativ
>
>> my understanding of the callchain is:
>> wait_for_completion_timemout with TASK_UNINTERRUPTIBLE
>>-> wait_for_common(...TASK_UNINTERRUPTIBLE)
>>  -> __wait_for_common(...TASK_UNINTERRUPTIBLE)
>>-> do_wait_for_common(...TASK_UNINTERRUPTIBLE)
>>  -> signal_pending_state(TASK_UNINTERRUPTIBLE...)
>
>> static inline int signal_pending_state(long state, struct task_struct *p)
>> {
>>  if (!(state & (TASK_INTERRUPTIBLE | TASK_WAKEKILL)))
>>  return 0;
>
>Right. I didn't look into TASK_UNINTERRUPTIBLE thing before sending my 
> mail.
>
>> so wait_for_completion_timemout should return 0 or 1 only
>
>0 or the remaining time, to be precise.
>

yup - thanks for the confirmation!

>>>> patch was only compile tested x86_64_defconfig + CONFIG_ATH_CARDS=m
>>>> CONFIG_ATH10K=m
>
>>>> patch is against linux-next 3.19.0-rc1 -next-20141226
>
>>> Rather patches. It would have been better to send one patch instead of
>>> 4 patches with the same name.
>
>> sorry for that - I had split it into separate patches as it was
>> in different files - giving them the same name of course was a bit
>> brain-dead.
>
>You should have mentioned the modified files in the subject. But IMHO 
> it would be better to have just one patch.
>
resent as a single patch as v2

thx!
hofrat
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] ath10k: fixup wait_for_completion_timeout return handling

[Nicholas Mc Guire]

wait_for_completion_timeout does not return negative values so the tests
for <= 0 are not needed and the case differentiation in the error handling
path unnecessary.

v2: all wait_for_completion_timeout changes in a single patch

patch was only compile tested x86_64_defconfig + CONFIG_ATH_CARDS=m
CONFIG_ATH10K=m

patch is against linux-next 3.19.0-rc1 -next-20141226

None of the proposed cleanups are critical.
All changes should only be removing unreachable cases.

Signed-off-by: Nicholas Mc Guire 
---
 drivers/net/wireless/ath/ath10k/debug.c |2 +-
 drivers/net/wireless/ath/ath10k/htc.c   |6 ++
 drivers/net/wireless/ath/ath10k/htt.c   |2 +-
 drivers/net/wireless/ath/ath10k/mac.c   |2 +-
 4 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/debug.c 
b/drivers/net/wireless/ath/ath10k/debug.c
index a716758..7e1fe93 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -373,7 +373,7 @@ static int ath10k_debug_fw_stats_request(struct ath10k *ar)
 
ret = wait_for_completion_timeout(&ar->debug.fw_stats_complete,
  1*HZ);
-   if (ret <= 0)
+   if (ret == 0)
return -ETIMEDOUT;
 
spin_lock_bh(&ar->data_lock);
diff --git a/drivers/net/wireless/ath/ath10k/htc.c 
b/drivers/net/wireless/ath/ath10k/htc.c
index f1946a6..2fd9e18 100644
--- a/drivers/net/wireless/ath/ath10k/htc.c
+++ b/drivers/net/wireless/ath/ath10k/htc.c
@@ -703,11 +703,9 @@ int ath10k_htc_connect_service(struct ath10k_htc *htc,
/* wait for response */
status = wait_for_completion_timeout(&htc->ctl_resp,
 ATH10K_HTC_CONN_SVC_TIMEOUT_HZ);
-   if (status <= 0) {
-   if (status == 0)
-   status = -ETIMEDOUT;
+   if (status == 0) {
ath10k_err(ar, "Service connect timeout: %d\n", status);
-   return status;
+   return -ETIMEDOUT;
}
 
/* we controlled the buffer creation, it's aligned */
diff --git a/drivers/net/wireless/ath/ath10k/htt.c 
b/drivers/net/wireless/ath/ath10k/htt.c
index 56cb4ac..58b6fc1 100644
--- a/drivers/net/wireless/ath/ath10k/htt.c
+++ b/drivers/net/wireless/ath/ath10k/htt.c
@@ -102,7 +102,7 @@ int ath10k_htt_setup(struct ath10k_htt *htt)
 
status = wait_for_completion_timeout(&htt->target_version_received,
 HTT_TARGET_VERSION_TIMEOUT_HZ);
-   if (status <= 0) {
+   if (status == 0) {
ath10k_warn(ar, "htt version request timed out\n");
return -ETIMEDOUT;
}
diff --git a/drivers/net/wireless/ath/ath10k/mac.c 
b/drivers/net/wireless/ath/ath10k/mac.c
index c400567..f9d7dbb 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2151,7 +2151,7 @@ void ath10k_offchan_tx_work(struct work_struct *work)
 
ret = wait_for_completion_timeout(&ar->offchan_tx_completed,
  3 * HZ);
-   if (ret <= 0)
+   if (ret == 0)
ath10k_warn(ar, "timed out waiting for offchannel skb 
%p\n",
skb);
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3] ath10k: fixup wait_for_completion_timeout return handling

[Nicholas Mc Guire]

wait_for_completion_timeout does not return negative values so the tests
for <= 0 are not needed and the case differentiation in the error handling
path unnecessary.

v2: all wait_for_completion_timeout changes in a single patch
v3: patch formatting cleanups - no change of actual patch

Signed-off-by: Nicholas Mc Guire 
---
patch was only compile tested x86_64_defconfig + CONFIG_ATH_CARDS=m
CONFIG_ATH10K=m

patch is against linux-next 3.19.0-rc1 -next-20141226

None of the proposed cleanups are critical.
All changes should only be removing unreachable cases.

 drivers/net/wireless/ath/ath10k/debug.c |2 +-
 drivers/net/wireless/ath/ath10k/htc.c   |6 ++
 drivers/net/wireless/ath/ath10k/htt.c   |2 +-
 drivers/net/wireless/ath/ath10k/mac.c   |2 +-
 4 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/debug.c 
b/drivers/net/wireless/ath/ath10k/debug.c
index a716758..7e1fe93 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -373,7 +373,7 @@ static int ath10k_debug_fw_stats_request(struct ath10k *ar)
 
ret = wait_for_completion_timeout(&ar->debug.fw_stats_complete,
  1*HZ);
-   if (ret <= 0)
+   if (ret == 0)
return -ETIMEDOUT;
 
spin_lock_bh(&ar->data_lock);
diff --git a/drivers/net/wireless/ath/ath10k/htc.c 
b/drivers/net/wireless/ath/ath10k/htc.c
index f1946a6..2fd9e18 100644
--- a/drivers/net/wireless/ath/ath10k/htc.c
+++ b/drivers/net/wireless/ath/ath10k/htc.c
@@ -703,11 +703,9 @@ int ath10k_htc_connect_service(struct ath10k_htc *htc,
/* wait for response */
status = wait_for_completion_timeout(&htc->ctl_resp,
 ATH10K_HTC_CONN_SVC_TIMEOUT_HZ);
-   if (status <= 0) {
-   if (status == 0)
-   status = -ETIMEDOUT;
+   if (status == 0) {
ath10k_err(ar, "Service connect timeout: %d\n", status);
-   return status;
+   return -ETIMEDOUT;
}
 
/* we controlled the buffer creation, it's aligned */
diff --git a/drivers/net/wireless/ath/ath10k/htt.c 
b/drivers/net/wireless/ath/ath10k/htt.c
index 56cb4ac..58b6fc1 100644
--- a/drivers/net/wireless/ath/ath10k/htt.c
+++ b/drivers/net/wireless/ath/ath10k/htt.c
@@ -102,7 +102,7 @@ int ath10k_htt_setup(struct ath10k_htt *htt)
 
status = wait_for_completion_timeout(&htt->target_version_received,
 HTT_TARGET_VERSION_TIMEOUT_HZ);
-   if (status <= 0) {
+   if (status == 0) {
ath10k_warn(ar, "htt version request timed out\n");
return -ETIMEDOUT;
}
diff --git a/drivers/net/wireless/ath/ath10k/mac.c 
b/drivers/net/wireless/ath/ath10k/mac.c
index c400567..f9d7dbb 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2151,7 +2151,7 @@ void ath10k_offchan_tx_work(struct work_struct *work)
 
ret = wait_for_completion_timeout(&ar->offchan_tx_completed,
  3 * HZ);
-   if (ret <= 0)
+   if (ret == 0)
ath10k_warn(ar, "timed out waiting for offchannel skb 
%p\n",
skb);
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] wireless: p54: add handling of the signal case

[Nicholas Mc Guire]

if(!wait_for_completion_interruptible_timeout(...))
only handles the timeout case - this patch adds handling the
signal case the same as timeout.

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

Only the timeout case was being handled, the signal case
(-ERESTARTSYS) was treated just like the case of successful
completion, which is most likely not reasonable.

p54_download_eeprom() is called in p54_read_eeprom() and will 
terminate if p54_download_eeprom() returned != 0 so the logic
should be correct - but this needs a check from someone who
knows the driver. Translating -ETIMEOUT to -EBUSY might be ok
not sure if -ERESTARTSYS also should be returned as -EBUSY ?

Patch was only compild tested with x86_64_defcofnig +
CONFIG_P54_COMMON=m, CONFIG_P54_PCI=m

Patch is against 3.19.0-rc5 -next-20150119

 drivers/net/wireless/p54/fwio.c |9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/p54/fwio.c b/drivers/net/wireless/p54/fwio.c
index bc065e8..5367d51 100644
--- a/drivers/net/wireless/p54/fwio.c
+++ b/drivers/net/wireless/p54/fwio.c
@@ -220,6 +220,7 @@ int p54_download_eeprom(struct p54_common *priv, void *buf,
struct sk_buff *skb;
size_t eeprom_hdr_size;
int ret = 0;
+   long timeout;
 
if (priv->fw_var >= 0x509)
eeprom_hdr_size = sizeof(*eeprom_hdr);
@@ -249,9 +250,11 @@ int p54_download_eeprom(struct p54_common *priv, void *buf,
 
p54_tx(priv, skb);
 
-   if (!wait_for_completion_interruptible_timeout(
-&priv->eeprom_comp, HZ)) {
-   wiphy_err(priv->hw->wiphy, "device does not respond!\n");
+   timeout = wait_for_completion_interruptible_timeout(
+   &priv->eeprom_comp, HZ);
+   if (timeout <= 0) {
+   wiphy_err(priv->hw->wiphy,
+   "device does not respond or signal received!\n");
ret = -EBUSY;
}
priv->eeprom = NULL;
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] wireless: p54pci: add handling of signal case

[Nicholas Mc Guire]

if(!wait_for_completion_interruptible_timeout(...))
only handles the timeout case - this patch adds handling the
signal case the same as timeout.

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

Only the timeout case was being handled, the signal case 
(-ERESTARTSYS) was treated just like the case of successful 
completion, which is most likely not reasonable.

The callsite for p54p_open() in p54p_firmware_step2() expect !=0 to 
indicate error so both -ERESTARTSYS and -ETIMEDOUT should be fine 
for the current handling.

Patch was only compild tested with x86_64_defcofnig +
CONFIG_P54_COMMON=m, CONFIG_P54_PCI=m

Patch is against 3.19.0-rc5 -next-20150119

 drivers/net/wireless/p54/p54pci.c |7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/p54/p54pci.c 
b/drivers/net/wireless/p54/p54pci.c
index d4aee64..27a4906 100644
--- a/drivers/net/wireless/p54/p54pci.c
+++ b/drivers/net/wireless/p54/p54pci.c
@@ -431,6 +431,7 @@ static int p54p_open(struct ieee80211_hw *dev)
 {
struct p54p_priv *priv = dev->priv;
int err;
+   long timeout;
 
init_completion(&priv->boot_comp);
err = request_irq(priv->pdev->irq, p54p_interrupt,
@@ -468,10 +469,12 @@ static int p54p_open(struct ieee80211_hw *dev)
P54P_WRITE(dev_int, cpu_to_le32(ISL38XX_DEV_INT_RESET));
P54P_READ(dev_int);
 
-   if (!wait_for_completion_interruptible_timeout(&priv->boot_comp, HZ)) {
+   timeout = wait_for_completion_interruptible_timeout(
+   &priv->boot_comp, HZ);
+   if (timeout <= 0) {
wiphy_err(dev->wiphy, "Cannot boot firmware!\n");
p54p_stop(dev);
-   return -ETIMEDOUT;
+   return timeout ? -ERESTARTSYS : -ETIMEDOUT;
}
 
P54P_WRITE(int_enable, cpu_to_le32(ISL38XX_INT_IDENT_UPDATE));
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] wireless: p54: add handling of the signal case

[Nicholas Mc Guire]

On Thu, 22 Jan 2015, Christian Lamparter wrote:

> On Tuesday, January 20, 2015 06:25:43 AM Nicholas Mc Guire wrote:
> > if(!wait_for_completion_interruptible_timeout(...))
> > only handles the timeout case - this patch adds handling the
> > signal case the same as timeout.
> > 
> > Signed-off-by: Nicholas Mc Guire 
> Acked-by: Christian Lamparter 
> 
> > diff --git a/drivers/net/wireless/p54/fwio.c 
> > b/drivers/net/wireless/p54/fwio.c
> > index bc065e8..5367d51 100644
> > --- a/drivers/net/wireless/p54/fwio.c
> > +++ b/drivers/net/wireless/p54/fwio.c
> > @@ -249,9 +250,11 @@ int p54_download_eeprom(struct p54_common *priv, void 
> > *buf,
> >  
> > p54_tx(priv, skb);
> >  
> > -   if (!wait_for_completion_interruptible_timeout(
> > -&priv->eeprom_comp, HZ)) {
> > -   wiphy_err(priv->hw->wiphy, "device does not respond!\n");
> > +   timeout = wait_for_completion_interruptible_timeout(
> > +   &priv->eeprom_comp, HZ);
> > +   if (timeout <= 0) {
> > +   wiphy_err(priv->hw->wiphy,
> > +   "device does not respond or signal received!\n");
> > ret = -EBUSY;
> > }
> > priv->eeprom = NULL;
> > 
> 
> CHECK: Alignment should match open parenthesis
> #98: FILE: drivers/net/wireless/p54/fwio.c:257:
> +   wiphy_err(priv->hw->wiphy,
> +   "device does not respond or signal received!\n");
> 
> total: 0 errors, 0 warnings, 1 checks, 21 lines checked
> 

Tried fixing this up - but simply no clue what coding style
rule that might have violated - and my attempts to fix this
did not succeed - allignment seems righta - the complete siequence is:

timeout = wait_for_completion_interruptible_timeout(
&priv->eeprom_comp, HZ);
if (timeout <= 0) {
wiphy_err(priv->hw->wiphy,
"device does not respond or signal received!\n");
ret = -EBUSY;
}

what am I overlooking ?

thx!
hofrat
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ath9k_htc: wmi: match wait_for_completion_timeout return type

[Nicholas Mc Guire]

On Thu, 14 May 2015, Kalle Valo wrote:

> Nicholas Mc Guire  writes:
> 
> > Patch is against 4.1-rc3 (localversion-next is -next-20150514)
> 
> BTW, this info should be under "---" line so that it doesn't get stored
> to the actual commit log.
>
that is where I had been putting it until ask by Josh Triplett and 
Steven Rostedt to put it above the "---" line explicitly
see: http://lkml.org/lkml/2015/5/11/552 

So now I'm a bit lost where to actually put this buildtest info ?

thx!
hofrat
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ath10k: mac: remove unreachable negative return check

[Nicholas Mc Guire]

wait_event_timeout(), introduced in 'commit 5e3dd157d7e7 ("ath10k: mac80211
driver for Qualcomm Atheros 802.11ac CQA98xx devices")' never returns < 0
so the only failure condition to be checked is ==0 (timeout). Further the
return type is long not int - an appropriately named variable is added 
and the assignments fixed up.

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

Patch was compile tested with x86_64_defconfig + CONFIG_ATH_CARD=y
CONFIG_ATH10K=m

Patch is against 4.1-rc7 (localversion-next is -next-20150611)

 drivers/net/wireless/ath/ath10k/mac.c |   10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c 
b/drivers/net/wireless/ath/ath10k/mac.c
index 0ed422a..cc67de9 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -5643,7 +5643,7 @@ static void ath10k_flush(struct ieee80211_hw *hw, struct 
ieee80211_vif *vif,
 {
struct ath10k *ar = hw->priv;
bool skip;
-   int ret;
+   long time_left;
 
/* mac80211 doesn't care if we really xmit queued frames or not
 * we'll collect those frames either way if we stop/delete vdevs */
@@ -5655,7 +5655,7 @@ static void ath10k_flush(struct ieee80211_hw *hw, struct 
ieee80211_vif *vif,
if (ar->state == ATH10K_STATE_WEDGED)
goto skip;
 
-   ret = wait_event_timeout(ar->htt.empty_tx_wq, ({
+   time_left = wait_event_timeout(ar->htt.empty_tx_wq, ({
bool empty;
 
spin_lock_bh(&ar->htt.tx_lock);
@@ -5669,9 +5669,9 @@ static void ath10k_flush(struct ieee80211_hw *hw, struct 
ieee80211_vif *vif,
(empty || skip);
}), ATH10K_FLUSH_TIMEOUT_HZ);
 
-   if (ret <= 0 || skip)
-   ath10k_warn(ar, "failed to flush transmit queue (skip %i 
ar-state %i): %i\n",
-   skip, ar->state, ret);
+   if (time_left == 0 || skip)
+   ath10k_warn(ar, "failed to flush transmit queue (skip %i 
ar-state %i): %ld\n",
+   skip, ar->state, time_left);
 
 skip:
mutex_unlock(&ar->conf_mutex);
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ath10k: txrx: remove unreachable negative return check and fixup type

[Nicholas Mc Guire]

wait_event_timeout(), introduced in 'commit 5e3dd157d7e7 ("ath10k: mac80211
driver for Qualcomm Atheros 802.11ac CQA98xx devices")' never returns < 0
so the only failure condition to be checked is == 0 (timeout). Further the
return type is long not int - an appropriately named variable is added
and the assignments fixed up.

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

Patch was compile tested with x86_64_defconfig + CONFIG_ATH_CARD=y
CONFIG_ATH10K=m

Patch is against 4.1-rc7 (localversion-next is -next-20150611)
 
 drivers/net/wireless/ath/ath10k/txrx.c |6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/txrx.c 
b/drivers/net/wireless/ath/ath10k/txrx.c
index 826500b..6cf2891 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.c
+++ b/drivers/net/wireless/ath/ath10k/txrx.c
@@ -147,9 +147,9 @@ struct ath10k_peer *ath10k_peer_find_by_id(struct ath10k 
*ar, int peer_id)
 static int ath10k_wait_for_peer_common(struct ath10k *ar, int vdev_id,
   const u8 *addr, bool expect_mapped)
 {
-   int ret;
+   long time_left;
 
-   ret = wait_event_timeout(ar->peer_mapping_wq, ({
+   time_left = wait_event_timeout(ar->peer_mapping_wq, ({
bool mapped;
 
spin_lock_bh(&ar->data_lock);
@@ -160,7 +160,7 @@ static int ath10k_wait_for_peer_common(struct ath10k *ar, 
int vdev_id,
 test_bit(ATH10K_FLAG_CRASH_FLUSH, &ar->dev_flags));
}), 3*HZ);
 
-   if (ret <= 0)
+   if (time_left == 0)
return -ETIMEDOUT;
 
return 0;
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[BUG ?] delay always evaluates to 0

[Nicholas Mc Guire]

Hi !

commit 2c86c275015c ("Add ipw2100 wireless driver.") introduced

drivers/net/wireless/ipw2100.c - line-numbers are from next-20150511
1410 static int ipw2100_hw_phy_off(struct ipw2100_priv *priv)
1411 {
1412 
1413 #define HW_PHY_OFF_LOOP_DELAY (HZ / 5000)  
1414
...
1437 
1438 schedule_timeout_uninterruptible(HW_PHY_OFF_LOOP_DELAY);
1439 }

but (HZ / 5000) will evaluate to 0 for all configurable HZ values - typo ?
and this schedule_timeout_uninterruptible() is probably not doing what
is intended.

thx!
hofrat
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[BUG ?] rtlwifi: rtl8723be: condition with no effect

[Nicholas Mc Guire]

From: Nicholas Mc Guire   

scanning for trivial bug-patters with coccinelle spatches returned:
./drivers/net/wireless/rtlwifi/rtl8723be/dm.c:886
WARNING: condition with no effect (if branch == else)

Added in 'commit a619d1abe20c ("rtlwifi: rtl8723be: Add new driver")'

drivers/net/wireless/rtlwifi/rtl8723be/dm.c - line numbers from 4.1-rc7
886 if (thermalvalue > rtlefuse->eeprom_thermalmeter)  
887 rtl8723be_dm_tx_power_track_set_power(hw, BBSWING, 0,
888 index_for_channel);
889 else
890 rtl8723be_dm_tx_power_track_set_power(hw, BBSWING, 0,
891 index_for_channel);

Can't figure out what the intent of this condition is but it currently has
no effect as if == else and this most likely is not the intent.

thx!
hofrat
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Nicholas Mc Guire]

On Sat, 13 Jun 2015, Jes Sorensen wrote:

> Nicholas Mc Guire  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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG ?] rtlwifi: rtl8723be: condition with no effect

[Nicholas Mc Guire]

On Sun, 14 Jun 2015, Larry Finger wrote:

> On 06/13/2015 05:43 AM, Nicholas Mc Guire wrote:
>> From: Nicholas Mc Guire 
>>
>> scanning for trivial bug-patters with coccinelle spatches returned:
>> ./drivers/net/wireless/rtlwifi/rtl8723be/dm.c:886
>>  WARNING: condition with no effect (if branch == else)
>>
>> Added in 'commit a619d1abe20c ("rtlwifi: rtl8723be: Add new driver")'
>>
>> drivers/net/wireless/rtlwifi/rtl8723be/dm.c - line numbers from 4.1-rc7
>> 886 if (thermalvalue > rtlefuse->eeprom_thermalmeter)
>> 887 rtl8723be_dm_tx_power_track_set_power(hw, BBSWING, 0,
>> 888  index_for_channel);
>> 889 else
>> 890 rtl8723be_dm_tx_power_track_set_power(hw, BBSWING, 0,
>> 891  index_for_channel);
>>
>> Can't figure out what the intent of this condition is but it currently has
>> no effect as if == else and this most likely is not the intent.
>
> I do not know what the correct code should be here, but I will contact 
> the Realtek engineers and get their interpretation. In the meantime, 
> please do not change this code.
>
as noted - I have no clue - so no intent to attempt a change
my mail is only intended as probable bug report.

thx!
hofrat
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] wireless: ipw2100: fix timeout bug - always evaluated to 0

[Nicholas Mc Guire]

commit: commit 2c86c275015c ("Add ipw2100 wireless driver.") introduced 
HW_PHY_OFF_LOOP_DELAY (HZ / 5000) which always evaluated to 0. Clarified
by Stanislav Yakovlev  that it should be 50
milliseconds thus fixed up to msecs_to_jiffies(50).

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

Patch was compile tested with x86_64_defconfig + CONFIG_IPW2100=m
(with a few buildwarnings in ipw2100.c though not related to this patch)

Patch is against 4.1-rc7 (localversion-next is -next-20150615)

 drivers/net/wireless/ipw2x00/ipw2100.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ipw2x00/ipw2100.c 
b/drivers/net/wireless/ipw2x00/ipw2100.c
index 08eb229..36818c7 100644
--- a/drivers/net/wireless/ipw2x00/ipw2100.c
+++ b/drivers/net/wireless/ipw2x00/ipw2100.c
@@ -1410,7 +1410,7 @@ static int ipw2100_power_cycle_adapter(struct 
ipw2100_priv *priv)
 static int ipw2100_hw_phy_off(struct ipw2100_priv *priv)
 {
 
-#define HW_PHY_OFF_LOOP_DELAY (HZ / 5000)
+#define HW_PHY_OFF_LOOP_DELAY (msecs_to_jiffies(50))
 
struct host_command cmd = {
.host_command = CARD_DISABLE_PHY_OFF,
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG ?] delay always evaluates to 0

[Nicholas Mc Guire]

On Mon, 15 Jun 2015, Stanislav Yakovlev wrote:

> Hi Nicholas,
> 
> On 12 June 2015 at 20:58, Nicholas Mc Guire  wrote:
> > Hi !
> >
> > commit 2c86c275015c ("Add ipw2100 wireless driver.") introduced
> >
> > drivers/net/wireless/ipw2100.c - line-numbers are from next-20150511
> > 1410 static int ipw2100_hw_phy_off(struct ipw2100_priv *priv)
> > 1411 {
> > 1412
> > 1413 #define HW_PHY_OFF_LOOP_DELAY (HZ / 5000)
> > 1414
> > ...
> > 1437
> > 1438 
> > schedule_timeout_uninterruptible(HW_PHY_OFF_LOOP_DELAY);
> > 1439 }
> >
> > but (HZ / 5000) will evaluate to 0 for all configurable HZ values - typo ?
> > and this schedule_timeout_uninterruptible() is probably not doing what
> > is intended.
> 
> Yes, you are right. This is a bug. I think it should be:
> 
> -#define HW_PHY_OFF_LOOP_DELAY (HZ / 5000)
> +#define HW_PHY_OFF_LOOP_DELAY (msecs_to_jiffies(50))
> 
> Will you send us a patch?
>
just sent it out - thanks!

hofrat 
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[BUG ?] brcmsmac: condition with no effect

[Nicholas Mc Guire]

From: Nicholas Mc Guire   

scanning for trivial bug-patters with coccinelle spatches returned:
drivers/net/wireless/brcm80211/brcmsmac/phy/phy_lcn.c:3391
WARNING: condition with no effect (if branch == else)

added in 'commit 5b435de0d786 ("net: wireless: add brcm80211 drivers")'

drivers/net/wireless/brcm80211/brcmsmac/phy/phy_lcn.c:wlc_lcnphy_deaf_mode()
(line numbers are from linux-next v4.2-rc2)
3391 if (LCNREV_LT(pi->pubpi.phy_rev, 2)) {
3392 mod_phy_reg(pi, 0x4b0, (0x1 << 5), (mode) << 5);   

3393 mod_phy_reg(pi, 0x4b1, (0x1 << 9), 0 << 9);
3394 } else {
3395 mod_phy_reg(pi, 0x4b0, (0x1 << 5), (mode) << 5);
3396 mod_phy_reg(pi, 0x4b1, (0x1 << 9), 0 << 9);
3397 }

Can't figure out what the intent of this condition is but it currently has
no effect as if == else and this most likely is not the intent.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] mwifiex: drop condition with no effect

[Nicholas Mc Guire]

scanning for trivial bug-patters with coccinelle spatches returned:
./drivers/net/wireless/mwifiex/sta_cmdresp.c:895
WARNING: condition with no effect (if branch == else)

originally added in 'commit d8d2f19feb16 ("mwifiex: silence TDLS link
delete failure for nonexistent link")' with dev_dbg/dev_err (though
with the same message) to differentiate severity and then in 'commit
acebe8c10a6e ("mwifiex: change dbg print func to mwifiex_dbg")' all
dev_dbg,dev_warn and dev_err got converted to mwifiex_dbg which should
thus probably drop this if/else as well.

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

If dropping the if/else is not the right thing to do then commit 
acebe8c10a6e "mwifiex: change dbg print func to mwifiex_dbg" probably
needs a review as well.

Patch was compile tested with x86_64_defconfig + CONFIG_MWIFIEX=m

Patch is against 4.2-rc1 (localversion-next is -next-20150708)

 drivers/net/wireless/mwifiex/sta_cmdresp.c |   11 +++
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/mwifiex/sta_cmdresp.c 
b/drivers/net/wireless/mwifiex/sta_cmdresp.c
index b645884..e58f900 100644
--- a/drivers/net/wireless/mwifiex/sta_cmdresp.c
+++ b/drivers/net/wireless/mwifiex/sta_cmdresp.c
@@ -892,14 +892,9 @@ static int mwifiex_ret_tdls_oper(struct mwifiex_private 
*priv,
switch (action) {
case ACT_TDLS_DELETE:
if (reason) {
-   if (!node || reason == TDLS_ERR_LINK_NONEXISTENT)
-   mwifiex_dbg(priv->adapter, ERROR,
-   "TDLS link delete for %pM failed: 
reason %d\n",
-   cmd_tdls_oper->peer_mac, reason);
-   else
-   mwifiex_dbg(priv->adapter, ERROR,
-   "TDLS link delete for %pM failed: 
reason %d\n",
-   cmd_tdls_oper->peer_mac, reason);
+   mwifiex_dbg(priv->adapter, ERROR,
+   "TDLS link delete for %pM failed: reason 
%d\n",
+   cmd_tdls_oper->peer_mac, reason);
} else {
mwifiex_dbg(priv->adapter, MSG,
"TDLS link delete for %pM successful\n",
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mwifiex: drop condition with no effect

[Nicholas Mc Guire]

On Wed, 08 Jul 2015, Avinash Patil wrote:

> Hi Nicholas,
> 
> On Wed, Jul 8, 2015 at 7:15 AM, Nicholas Mc Guire  wrote:
> > scanning for trivial bug-patters with coccinelle spatches returned:
> > ./drivers/net/wireless/mwifiex/sta_cmdresp.c:895
> > WARNING: condition with no effect (if branch == else)
> >
> > originally added in 'commit d8d2f19feb16 ("mwifiex: silence TDLS link
> > delete failure for nonexistent link")' with dev_dbg/dev_err (though
> > with the same message) to differentiate severity and then in 'commit
> > acebe8c10a6e ("mwifiex: change dbg print func to mwifiex_dbg")' all
> > dev_dbg,dev_warn and dev_err got converted to mwifiex_dbg which should
> > thus probably drop this if/else as well.
> >
> > Signed-off-by: Nicholas Mc Guire 
> > ---
> >
> > If dropping the if/else is not the right thing to do then commit
> > acebe8c10a6e "mwifiex: change dbg print func to mwifiex_dbg" probably
> > needs a review as well.
> >
> > Patch was compile tested with x86_64_defconfig + CONFIG_MWIFIEX=m
> >
> > Patch is against 4.2-rc1 (localversion-next is -next-20150708)
> >
> >  drivers/net/wireless/mwifiex/sta_cmdresp.c |   11 +++
> >  1 file changed, 3 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/wireless/mwifiex/sta_cmdresp.c 
> > b/drivers/net/wireless/mwifiex/sta_cmdresp.c
> > index b645884..e58f900 100644
> > --- a/drivers/net/wireless/mwifiex/sta_cmdresp.c
> > +++ b/drivers/net/wireless/mwifiex/sta_cmdresp.c
> > @@ -892,14 +892,9 @@ static int mwifiex_ret_tdls_oper(struct 
> > mwifiex_private *priv,
> > switch (action) {
> > case ACT_TDLS_DELETE:
> > if (reason) {
> > -   if (!node || reason == TDLS_ERR_LINK_NONEXISTENT)
> > -   mwifiex_dbg(priv->adapter, ERROR,
> > -   "TDLS link delete for %pM 
> > failed: reason %d\n",
> > -   cmd_tdls_oper->peer_mac, 
> > reason);
> > -   else
> > -   mwifiex_dbg(priv->adapter, ERROR,
> > -   "TDLS link delete for %pM 
> > failed: reason %d\n",
> > -   cmd_tdls_oper->peer_mac, 
> > reason);
> > +   mwifiex_dbg(priv->adapter, ERROR,
> > +   "TDLS link delete for %pM failed: 
> > reason %d\n",
> > +   cmd_tdls_oper->peer_mac, reason);
> 
> I think reason why we had these 2 different prints is for first
> occurrence in "if" which may be not so serious we used to print with
> dev_dbg and second occurrence in "else" is more serious issue which
> was under dev_err.
> It would be better to use mwifiex_dbg with DBG/MSG in"if" condition
> instead of removing whole if/else.
>

ok - it seemed to me that since the reason was being printed it would be
clear from the error message which case was triggering this message but
you are right that the initial intent to differentiate severity levels
(from 'commit d8d2f19feb16 ("mwifiex: silence TDLS link delete failure
for nonexistent link")' would be better served with your suggestion - I 
just was not clear if this initial reason still holds.

will fix it up and resend.

thx!
hofrat
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] nfc: nxp-nci: use msleep for long delays

[Nicholas Mc Guire]

ulseep_range() uses hrtimers and provides no advantage over msleep()
for larger delays. For this large delay msleep() is preferable.

Fixes: commit 6be88670fc59 ("NFC: nxp-nci_i2c: Add I2C support to NXP NCI 
driver")
Link: http://lkml.org/lkml/2017/1/11/377
Signed-off-by: Nicholas Mc Guire 
---
Problem was found by cocinelle script.

nxp_nci_i2c_write takes the negative return code as indicator that the
NFC device was probably in stand-by mode, the first transaction attempt
woke it up and that after 110ms latest it would be ready to receive.
Overrunning this time by a few milliseconds will not hurt though so
msleep() should be fine here.

Patch was compile tested with: x86_64_defconfig + CONFIG_NFC=m,
CONFIG_NFC_NCI=m, CONFIG_NFC_NXP_NCI=m, CONFIG_NFC_NXP_NCI_I2C=m

Patch is against 4.10-rc4 (localversion-next is next-20170120)

 drivers/nfc/nxp-nci/i2c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nfc/nxp-nci/i2c.c b/drivers/nfc/nxp-nci/i2c.c
index 36099e5..ceb815c 100644
--- a/drivers/nfc/nxp-nci/i2c.c
+++ b/drivers/nfc/nxp-nci/i2c.c
@@ -86,7 +86,7 @@ static int nxp_nci_i2c_write(void *phy_id, struct sk_buff 
*skb)
r = i2c_master_send(client, skb->data, skb->len);
if (r < 0) {
/* Retry, chip was in standby */
-   usleep_range(11, 12);
+   msleep(110);
r = i2c_master_send(client, skb->data, skb->len);
}
 
-- 
2.1.4