[PATCH 1/3] WDEV: ath5k, fix lock imbalance

[PATCH 1/3] WDEV: ath5k, fix lock imbalance

[Jiri Slaby]

Omitted lock causes sparse warning
drivers/net/wireless/ath5k/base.c:1682:1: warning: context imbalance in 'ath5k_tasklet_rx' - different lock contexts for basic block

Add the lock to the guilty fail path.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
Cc: Nick Kossifidis <mickflemm@gmail.com>
Cc: Luis R. Rodriguez <mcgrof@gmail.com>
---
 drivers/net/wireless/ath5k/base.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
index ddc8714..49d38e8 100644
--- a/drivers/net/wireless/ath5k/base.c
+++ b/drivers/net/wireless/ath5k/base.c
@@ -1715,6 +1715,7 @@ ath5k_tasklet_rx(unsigned long data)
 			break;
 		else if (unlikely(ret)) {
 			ATH5K_ERR(sc, "error in processing rx descriptor\n");
+			spin_unlock(&sc->rxbuflock);
 			return;
 		}
 
-- 
1.5.3.8

[PATCH 2/3] WDEV, ath5k, don't return int from bool function

[Jiri Slaby]

sparse sees int -> bool cast as an error:
hw.c:3754:10: warning: cast truncates bits from constant value (ffffffea becomes 0)
Fix it by converting the rettype to int and check appropriately.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
Cc: Nick Kossifidis <mickflemm@gmail.com>
Cc: Luis R. Rodriguez <mcgrof@gmail.com>
---
 drivers/net/wireless/ath5k/ath5k.h |    2 +-
 drivers/net/wireless/ath5k/base.c  |    5 ++++-
 drivers/net/wireless/ath5k/hw.c    |    8 ++++----
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath5k/ath5k.h b/drivers/net/wireless/ath5k/ath5k.h
index c79066b..69dea33 100644
--- a/drivers/net/wireless/ath5k/ath5k.h
+++ b/drivers/net/wireless/ath5k/ath5k.h
@@ -1035,7 +1035,7 @@ struct ath5k_hw {
 		unsigned int, unsigned int, enum ath5k_pkt_type, unsigned int,
 		unsigned int, unsigned int, unsigned int, unsigned int,
 		unsigned int, unsigned int, unsigned int);
-	bool (*ah_setup_xtx_desc)(struct ath5k_hw *, struct ath5k_desc *,
+	int (*ah_setup_xtx_desc)(struct ath5k_hw *, struct ath5k_desc *,
 		unsigned int, unsigned int, unsigned int, unsigned int,
 		unsigned int, unsigned int);
 	int (*ah_proc_tx_desc)(struct ath5k_hw *, struct ath5k_desc *);
diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
index 49d38e8..59e5d56 100644
--- a/drivers/net/wireless/ath5k/base.c
+++ b/drivers/net/wireless/ath5k/base.c
@@ -668,7 +668,10 @@ ath5k_attach(struct pci_dev *pdev, struct ieee80211_hw *hw)
 	 * return false w/o doing anything.  MAC's that do
 	 * support it will return true w/o doing anything.
 	 */
-	if (ah->ah_setup_xtx_desc(ah, NULL, 0, 0, 0, 0, 0, 0))
+	ret = ah->ah_setup_xtx_desc(ah, NULL, 0, 0, 0, 0, 0, 0);
+	if (ret < 0)
+		goto err;
+	if (ret > 0)
 		__set_bit(ATH_STAT_MRRETRY, sc->status);
 
 	/*
diff --git a/drivers/net/wireless/ath5k/hw.c b/drivers/net/wireless/ath5k/hw.c
index 93a75f2..463413a 100644
--- a/drivers/net/wireless/ath5k/hw.c
+++ b/drivers/net/wireless/ath5k/hw.c
@@ -45,7 +45,7 @@ static int ath5k_hw_setup_4word_tx_desc(struct ath5k_hw *, struct ath5k_desc *,
 	unsigned int, unsigned int, enum ath5k_pkt_type, unsigned int,
 	unsigned int, unsigned int, unsigned int, unsigned int, unsigned int,
 	unsigned int, unsigned int);
-static bool ath5k_hw_setup_xr_tx_desc(struct ath5k_hw *, struct ath5k_desc *,
+static int ath5k_hw_setup_xr_tx_desc(struct ath5k_hw *, struct ath5k_desc *,
 	unsigned int, unsigned int, unsigned int, unsigned int, unsigned int,
 	unsigned int);
 static int ath5k_hw_proc_4word_tx_status(struct ath5k_hw *, struct ath5k_desc *);
@@ -3733,7 +3733,7 @@ static int ath5k_hw_setup_4word_tx_desc(struct ath5k_hw *ah,
 /*
  * Initialize a 4-word multirate tx descriptor on 5212
  */
-static bool
+static int
 ath5k_hw_setup_xr_tx_desc(struct ath5k_hw *ah, struct ath5k_desc *desc,
 	unsigned int tx_rate1, u_int tx_tries1, u_int tx_rate2, u_int tx_tries2,
 	unsigned int tx_rate3, u_int tx_tries3)
@@ -3773,10 +3773,10 @@ ath5k_hw_setup_xr_tx_desc(struct ath5k_hw *ah, struct ath5k_desc *desc,
 
 #undef _XTX_TRIES
 
-		return true;
+		return 1;
 	}
 
-	return false;
+	return 0;
 }
 
 /*
-- 
1.5.3.8

[PATCH 3/3] WDEV: ath5k, typecheck on nonDEBUG

[Jiri Slaby]

At least type check the ATH5K_TRACE paramter on !ATH5K_DEBUG configs.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
Cc: Nick Kossifidis <mickflemm@gmail.com>
Cc: Luis R. Rodriguez <mcgrof@gmail.com>
---
 drivers/net/wireless/ath5k/debug.h |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath5k/debug.h b/drivers/net/wireless/ath5k/debug.h
index c4fd8c4..44324c3 100644
--- a/drivers/net/wireless/ath5k/debug.h
+++ b/drivers/net/wireless/ath5k/debug.h
@@ -171,7 +171,9 @@ ath5k_debug_printtxbuf(struct ath5k_softc *sc,
 
 #else /* no debugging */
 
-#define ATH5K_TRACE(_sc) /* empty */
+#include <linux/compiler.h>
+
+#define ATH5K_TRACE(_sc) typecheck(struct ath5k_softc *, (_sc))
 
 static inline void __attribute__ ((format (printf, 3, 4)))
 ATH5K_DBG(struct ath5k_softc *sc, unsigned int m, const char *fmt, ...) {}
-- 
1.5.3.8

Re: [PATCH 1/3] WDEV: ath5k, fix lock imbalance

[Nick Kossifidis]

2008/2/15, Jiri Slaby <jirislaby@gmail.com>:
> Omitted lock causes sparse warning
>  drivers/net/wireless/ath5k/base.c:1682:1: warning: context imbalance in 'ath5k_tasklet_rx' - different lock contexts for basic block
>
>  Add the lock to the guilty fail path.
>
>  Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
>  Cc: Nick Kossifidis <mickflemm@gmail.com>
>  Cc: Luis R. Rodriguez <mcgrof@gmail.com>
>  ---
>   drivers/net/wireless/ath5k/base.c |    1 +
>   1 files changed, 1 insertions(+), 0 deletions(-)
>
>  diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
>  index ddc8714..49d38e8 100644
>  --- a/drivers/net/wireless/ath5k/base.c
>  +++ b/drivers/net/wireless/ath5k/base.c
>  @@ -1715,6 +1715,7 @@ ath5k_tasklet_rx(unsigned long data)
>                         break;
>                 else if (unlikely(ret)) {
>                         ATH5K_ERR(sc, "error in processing rx descriptor\n");
>  +                       spin_unlock(&sc->rxbuflock);
>                         return;
>                 }
>
>


Acked-by: Nick Kossifidis <mickflemm@gmail.com>


-- 
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [PATCH 2/3] WDEV, ath5k, don't return int from bool function

[Nick Kossifidis]

2008/2/15, Jiri Slaby <jirislaby@gmail.com>:
> sparse sees int -> bool cast as an error:
>  hw.c:3754:10: warning: cast truncates bits from constant value (ffffffea becomes 0)
>  Fix it by converting the rettype to int and check appropriately.
>
>  Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
>  Cc: Nick Kossifidis <mickflemm@gmail.com>
>  Cc: Luis R. Rodriguez <mcgrof@gmail.com>
>  ---
>   drivers/net/wireless/ath5k/ath5k.h |    2 +-
>   drivers/net/wireless/ath5k/base.c  |    5 ++++-
>   drivers/net/wireless/ath5k/hw.c    |    8 ++++----
>   3 files changed, 9 insertions(+), 6 deletions(-)
>
>  diff --git a/drivers/net/wireless/ath5k/ath5k.h b/drivers/net/wireless/ath5k/ath5k.h
>  index c79066b..69dea33 100644
>  --- a/drivers/net/wireless/ath5k/ath5k.h
>  +++ b/drivers/net/wireless/ath5k/ath5k.h
>  @@ -1035,7 +1035,7 @@ struct ath5k_hw {
>                 unsigned int, unsigned int, enum ath5k_pkt_type, unsigned int,
>                 unsigned int, unsigned int, unsigned int, unsigned int,
>                 unsigned int, unsigned int, unsigned int);
>  -       bool (*ah_setup_xtx_desc)(struct ath5k_hw *, struct ath5k_desc *,
>  +       int (*ah_setup_xtx_desc)(struct ath5k_hw *, struct ath5k_desc *,
>                 unsigned int, unsigned int, unsigned int, unsigned int,
>                 unsigned int, unsigned int);
>         int (*ah_proc_tx_desc)(struct ath5k_hw *, struct ath5k_desc *);
>  diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
>  index 49d38e8..59e5d56 100644
>  --- a/drivers/net/wireless/ath5k/base.c
>  +++ b/drivers/net/wireless/ath5k/base.c
>  @@ -668,7 +668,10 @@ ath5k_attach(struct pci_dev *pdev, struct ieee80211_hw *hw)
>          * return false w/o doing anything.  MAC's that do
>          * support it will return true w/o doing anything.
>          */
>  -       if (ah->ah_setup_xtx_desc(ah, NULL, 0, 0, 0, 0, 0, 0))
>  +       ret = ah->ah_setup_xtx_desc(ah, NULL, 0, 0, 0, 0, 0, 0);
>  +       if (ret < 0)
>  +               goto err;
>  +       if (ret > 0)
>                 __set_bit(ATH_STAT_MRRETRY, sc->status);
>
>         /*
>  diff --git a/drivers/net/wireless/ath5k/hw.c b/drivers/net/wireless/ath5k/hw.c
>  index 93a75f2..463413a 100644
>  --- a/drivers/net/wireless/ath5k/hw.c
>  +++ b/drivers/net/wireless/ath5k/hw.c
>  @@ -45,7 +45,7 @@ static int ath5k_hw_setup_4word_tx_desc(struct ath5k_hw *, struct ath5k_desc *,
>         unsigned int, unsigned int, enum ath5k_pkt_type, unsigned int,
>         unsigned int, unsigned int, unsigned int, unsigned int, unsigned int,
>         unsigned int, unsigned int);
>  -static bool ath5k_hw_setup_xr_tx_desc(struct ath5k_hw *, struct ath5k_desc *,
>  +static int ath5k_hw_setup_xr_tx_desc(struct ath5k_hw *, struct ath5k_desc *,
>         unsigned int, unsigned int, unsigned int, unsigned int, unsigned int,
>         unsigned int);
>   static int ath5k_hw_proc_4word_tx_status(struct ath5k_hw *, struct ath5k_desc *);
>  @@ -3733,7 +3733,7 @@ static int ath5k_hw_setup_4word_tx_desc(struct ath5k_hw *ah,
>   /*
>   * Initialize a 4-word multirate tx descriptor on 5212
>   */
>  -static bool
>  +static int
>   ath5k_hw_setup_xr_tx_desc(struct ath5k_hw *ah, struct ath5k_desc *desc,
>         unsigned int tx_rate1, u_int tx_tries1, u_int tx_rate2, u_int tx_tries2,
>         unsigned int tx_rate3, u_int tx_tries3)
>  @@ -3773,10 +3773,10 @@ ath5k_hw_setup_xr_tx_desc(struct ath5k_hw *ah, struct ath5k_desc *desc,
>
>   #undef _XTX_TRIES
>
>  -               return true;
>  +               return 1;
>         }
>
>  -       return false;
>  +       return 0;
>   }
>
>   /*
>


Acked-by: Nick Kossifidis <mickflemm@gmail.com>


-- 
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [PATCH 2/3] WDEV, ath5k, don't return int from bool function

[Luis R. Rodriguez]

On Fri, Feb 15, 2008 at 3:58 PM, Jiri Slaby <jirislaby@gmail.com> wrote:

>  -static bool
>  +static int
>   ath5k_hw_setup_xr_tx_desc(struct ath5k_hw *ah, struct ath5k_desc *desc,
>         unsigned int tx_rate1, u_int tx_tries1, u_int tx_rate2, u_int tx_tries2,
>         unsigned int tx_rate3, u_int tx_tries3)
>  @@ -3773,10 +3773,10 @@ ath5k_hw_setup_xr_tx_desc(struct ath5k_hw *ah, struct ath5k_desc *desc,
>
>   #undef _XTX_TRIES
>
>  -               return true;
>  +               return 1;
>         }
>
>  -       return false;
>  +       return 0;
>   }

Shouldn't we then treat 0 as OK?

  Luis

Re: [PATCH 3/3] WDEV: ath5k, typecheck on nonDEBUG

[Luis R. Rodriguez]

On Fri, Feb 15, 2008 at 3:58 PM, Jiri Slaby <jirislaby@gmail.com> wrote:
> At least type check the ATH5K_TRACE paramter on !ATH5K_DEBUG configs.

That's pretty cool.

>  Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
>  Cc: Nick Kossifidis <mickflemm@gmail.com>
>  Cc: Luis R. Rodriguez <mcgrof@gmail.com>

Acked-by: Luis R. Rodriguez <mcgrof@winlab.rutgers.edu>

  Luis

Re: [PATCH 2/3] WDEV, ath5k, don't return int from bool function

[Jiri Slaby]

On 02/15/2008 11:08 PM, Luis R. Rodriguez wrote:
> On Fri, Feb 15, 2008 at 3:58 PM, Jiri Slaby <jirislaby@gmail.com> wrote:
> 
>>  -static bool
>>  +static int
>>   ath5k_hw_setup_xr_tx_desc(struct ath5k_hw *ah, struct ath5k_desc *desc,
>>         unsigned int tx_rate1, u_int tx_tries1, u_int tx_rate2, u_int tx_tries2,
>>         unsigned int tx_rate3, u_int tx_tries3)
>>  @@ -3773,10 +3773,10 @@ ath5k_hw_setup_xr_tx_desc(struct ath5k_hw *ah, struct ath5k_desc *desc,
>>
>>   #undef _XTX_TRIES
>>
>>  -               return true;
>>  +               return 1;
>>         }
>>
>>  -       return false;
>>  +       return 0;
>>   }
> 
> Shouldn't we then treat 0 as OK?

Sorry, I don't understand you. There is return -EINVAL in the function above 
this too and we need to cope with another two states but the error: it is 
supported/it isn't. You mean to consider 0 as supported, -ENODEV/-EOPNOTSUPP as 
unsupported and the rest as error?

Re: [PATCH 2/3] WDEV, ath5k, don't return int from bool function

[Luis R. Rodriguez]

On Fri, Feb 15, 2008 at 5:38 PM, Jiri Slaby <jirislaby@gmail.com> wrote:
>
> You mean to consider 0 as supported, -ENODEV/-EOPNOTSUPP as
>  unsupported and the rest as error?
>

Yeap, what do you think?

  Luis

Re: [PATCH 2/3] WDEV, ath5k, don't return int from bool function

[Jiri Slaby]

On 02/16/2008 12:24 AM, Luis R. Rodriguez wrote:
> On Fri, Feb 15, 2008 at 5:38 PM, Jiri Slaby <jirislaby@gmail.com> wrote:
>> You mean to consider 0 as supported, -ENODEV/-EOPNOTSUPP as
>>  unsupported and the rest as error?
>>
> 
> Yeap, what do you think?

I have no opinion in this case, which one do you (all) like more?