[PATCH] staging: brcm80211: fix cast to pointer from integer

[PATCH] staging: brcm80211: fix cast to pointer from integer

[Clemens Noss]

bcm_pktq_flush and related functions only ever get 0 or a pointer for
arg, so make it a pointer.

This might fix a crash on 64bit.

Signed-off-by: Clemens Noss <cnoss@gmx.de>
---
 drivers/staging/brcm80211/brcmfmac/dhd_sdio.c  |    2 +-
 drivers/staging/brcm80211/brcmsmac/wlc_ampdu.c |    4 ++--
 drivers/staging/brcm80211/brcmsmac/wlc_main.c  |    4 ++--
 drivers/staging/brcm80211/include/bcmutils.h   |    6 +++---
 drivers/staging/brcm80211/util/bcmutils.c      |    4 ++--
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/brcm80211/brcmfmac/dhd_sdio.c b/drivers/staging/brcm80211/brcmfmac/dhd_sdio.c
index 8683eeb..a71c6f8 100644
--- a/drivers/staging/brcm80211/brcmfmac/dhd_sdio.c
+++ b/drivers/staging/brcm80211/brcmfmac/dhd_sdio.c
@@ -2845,7 +2845,7 @@ void dhd_bus_stop(struct dhd_bus *bus, bool enforce_mutex)
 	dhdsdio_clkctl(bus, CLK_SDONLY, false);
 
 	/* Clear the data packet queues */
-	bcm_pktq_flush(&bus->txq, true, NULL, 0);
+	bcm_pktq_flush(&bus->txq, true, NULL, NULL);
 
 	/* Clear any held glomming stuff */
 	if (bus->glomd)
diff --git a/drivers/staging/brcm80211/brcmsmac/wlc_ampdu.c b/drivers/staging/brcm80211/brcmsmac/wlc_ampdu.c
index 5b041a6..0d93bd6 100644
--- a/drivers/staging/brcm80211/brcmsmac/wlc_ampdu.c
+++ b/drivers/staging/brcm80211/brcmsmac/wlc_ampdu.c
@@ -1225,7 +1225,7 @@ void wlc_ampdu_shm_upd(struct ampdu_info *ampdu)
 /*
  * callback function that helps flushing ampdu packets from a priority queue
  */
-static bool cb_del_ampdu_pkt(void *p, int arg_a)
+static bool cb_del_ampdu_pkt(void *p, void *arg_a)
 {
 	struct sk_buff *mpdu = (struct sk_buff *)p;
 	struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(mpdu);
@@ -1269,7 +1269,7 @@ void wlc_ampdu_flush(struct wlc_info *wlc,
 	ampdu_pars.tid = tid;
 	for (prec = 0; prec < pq->num_prec; prec++) {
 		bcm_pktq_pflush(pq, prec, true, cb_del_ampdu_pkt,
-			    (int)&ampdu_pars);
+			    (void *)&ampdu_pars);
 	}
 	wlc_inval_dma_pkts(wlc->hw, sta, dma_cb_fn_ampdu);
 }
diff --git a/drivers/staging/brcm80211/brcmsmac/wlc_main.c b/drivers/staging/brcm80211/brcmsmac/wlc_main.c
index 2fed0eb..926b01a 100644
--- a/drivers/staging/brcm80211/brcmsmac/wlc_main.c
+++ b/drivers/staging/brcm80211/brcmsmac/wlc_main.c
@@ -2520,7 +2520,7 @@ uint wlc_down(struct wlc_info *wlc)
 
 	/* flush tx queues */
 	for (qi = wlc->tx_queues; qi != NULL; qi = qi->next) {
-		bcm_pktq_flush(&qi->q, true, NULL, 0);
+		bcm_pktq_flush(&qi->q, true, NULL, NULL);
 	}
 
 	callbacks += wlc_bmac_down_finish(wlc->hw);
@@ -8135,7 +8135,7 @@ void wlc_wait_for_tx_completion(struct wlc_info *wlc, bool drop)
 {
 	/* flush packet queue when requested */
 	if (drop)
-		bcm_pktq_flush(&wlc->pkt_queue->q, false, NULL, 0);
+		bcm_pktq_flush(&wlc->pkt_queue->q, false, NULL, NULL);
 
 	/* wait for queue and DMA fifos to run dry */
 	while (!pktq_empty(&wlc->pkt_queue->q) ||
diff --git a/drivers/staging/brcm80211/include/bcmutils.h b/drivers/staging/brcm80211/include/bcmutils.h
index aa00e83..1d57934 100644
--- a/drivers/staging/brcm80211/include/bcmutils.h
+++ b/drivers/staging/brcm80211/include/bcmutils.h
@@ -74,7 +74,7 @@
 #define PKTQ_PREC_ITER(pq, prec)        for (prec = (pq)->num_prec - 1; prec >= 0; prec--)
 
 /* fn(pkt, arg).  return true if pkt belongs to if */
-	typedef bool(*ifpkt_cb_t) (void *, int);
+	typedef bool(*ifpkt_cb_t) (void *, void *);
 
 /* operations on a specific precedence in packet queue */
 
@@ -100,7 +100,7 @@ extern void bcm_pkt_buf_free_skb(struct sk_buff *skb);
 
 /* Empty the queue at particular precedence level */
 extern void bcm_pktq_pflush(struct pktq *pq, int prec,
-	bool dir, ifpkt_cb_t fn, int arg);
+	bool dir, ifpkt_cb_t fn, void *arg);
 
 /* operations on a set of precedences in packet queue */
 
@@ -127,7 +127,7 @@ extern void bcm_pktq_init(struct pktq *pq, int num_prec, int max_len);
 /* prec_out may be NULL if caller is not interested in return value */
 extern struct sk_buff *bcm_pktq_peek_tail(struct pktq *pq, int *prec_out);
 extern void bcm_pktq_flush(struct pktq *pq, bool dir,
-	ifpkt_cb_t fn, int arg);
+	ifpkt_cb_t fn, void *arg);
 
 /* externs */
 /* packet */
diff --git a/drivers/staging/brcm80211/util/bcmutils.c b/drivers/staging/brcm80211/util/bcmutils.c
index e185e28..43e5bb3 100644
--- a/drivers/staging/brcm80211/util/bcmutils.c
+++ b/drivers/staging/brcm80211/util/bcmutils.c
@@ -231,7 +231,7 @@ EXPORT_SYMBOL(bcm_pktq_pdeq_tail);
 
 void
 bcm_pktq_pflush(struct pktq *pq, int prec, bool dir,
-	    ifpkt_cb_t fn, int arg)
+	    ifpkt_cb_t fn, void *arg)
 {
 	struct pktq_prec *q;
 	struct sk_buff *p, *prev = NULL;
@@ -263,7 +263,7 @@ bcm_pktq_pflush(struct pktq *pq, int prec, bool dir,
 EXPORT_SYMBOL(bcm_pktq_pflush);
 
 void bcm_pktq_flush(struct pktq *pq, bool dir,
-		ifpkt_cb_t fn, int arg)
+		ifpkt_cb_t fn, void *arg)
 {
 	int prec;
 	for (prec = 0; prec < pq->num_prec; prec++)
-- 
1.7.4.5

Re: [PATCH] staging: brcm80211: fix cast to pointer from integer

[Hauke Mehrtens]

On 05/15/2011 07:04 PM, Clemens Noss wrote:
> bcm_pktq_flush and related functions only ever get 0 or a pointer for
> arg, so make it a pointer.
> 
> This might fix a crash on 64bit.
> 
> Signed-off-by: Clemens Noss <cnoss@gmx.de>
> ---
>  drivers/staging/brcm80211/brcmfmac/dhd_sdio.c  |    2 +-
>  drivers/staging/brcm80211/brcmsmac/wlc_ampdu.c |    4 ++--
>  drivers/staging/brcm80211/brcmsmac/wlc_main.c  |    4 ++--
>  drivers/staging/brcm80211/include/bcmutils.h   |    6 +++---
>  drivers/staging/brcm80211/util/bcmutils.c      |    4 ++--
>  5 files changed, 10 insertions(+), 10 deletions(-)

> ....

> diff --git a/drivers/staging/brcm80211/brcmsmac/wlc_ampdu.c b/drivers/staging/brcm80211/brcmsmac/wlc_ampdu.c
> index 5b041a6..0d93bd6 100644
> --- a/drivers/staging/brcm80211/brcmsmac/wlc_ampdu.c
> +++ b/drivers/staging/brcm80211/brcmsmac/wlc_ampdu.c
> @@ -1225,7 +1225,7 @@ void wlc_ampdu_shm_upd(struct ampdu_info *ampdu)
>  /*
>   * callback function that helps flushing ampdu packets from a priority queue
>   */
> -static bool cb_del_ampdu_pkt(void *p, int arg_a)
> +static bool cb_del_ampdu_pkt(void *p, void *arg_a)
>  {
>  	struct sk_buff *mpdu = (struct sk_buff *)p;
>  	struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(mpdu);
Why don't you use the correct pointer type here instead of casting then
around and the parameter names are meaningless too.

static bool cb_del_ampdu_pkt(struct sk_buff *mpdu, struct
cb_del_ampdu_pars *ampdu_pars)

The signature of dma_cb_fn_ampdu(void *txi, void *arg_a) should be
changed too.

> ...

Hauke

Re: [PATCH] staging: brcm80211: fix cast to pointer from integer

[Arend van Spriel]

On 05/15/2011 08:10 PM, Hauke Mehrtens wrote:
> On 05/15/2011 07:04 PM, Clemens Noss wrote:
>> bcm_pktq_flush and related functions only ever get 0 or a pointer for
>> arg, so make it a pointer.
>>
>> This might fix a crash on 64bit.
>>
>> Signed-off-by: Clemens Noss<cnoss@gmx.de>
>> ---
> Why don't you use the correct pointer type here instead of casting then
> around and the parameter names are meaningless too.
>
> static bool cb_del_ampdu_pkt(struct sk_buff *mpdu, struct
> cb_del_ampdu_pars *ampdu_pars)
>
> The signature of dma_cb_fn_ampdu(void *txi, void *arg_a) should be
> changed too.
Actually this is a generic callback mechanism in which an additional 
parameter can be passed, which can be of *any* type hence a void pointer 
seems justified here and a meaningless name is used. I do agree that the 
txi parameter will always be a struct sk_buff and should be indicated as 
such.

Apart from that: good catch. This indeed may cause issues on 64-bit systems.

Gr. AvS

-- 
Almost nobody dances sober, unless they happen to be insane.
-- H.P. Lovecraft --

Re: [PATCH] staging: brcm80211: fix cast to pointer from integer

[Arend van Spriel]

On 05/15/2011 07:04 PM, Clemens Noss wrote:
> bcm_pktq_flush and related functions only ever get 0 or a pointer for
> arg, so make it a pointer.
>
> This might fix a crash on 64bit.
>
> Signed-off-by: Clemens Noss<cnoss@gmx.de>
> ---
Hi Clemens,

Please note that patches on our driver should be sent to Greg KH 
<gregkh@suse.de> and Cc <devel@linuxdriverproject.org> as we are still 
in staging tree.

Gr. AvS

-- 
Almost nobody dances sober, unless they happen to be insane.
-- H.P. Lovecraft --

[PATCH] staging: brcm80211: use correct pointer types in bcm_pktq_flush

[Clemens Noss]

to clarify the intention:

On Sun, May 15, 2011 at 09:54:26PM +0200, Arend van Spriel wrote:
> On 05/15/2011 08:10 PM, Hauke Mehrtens wrote:
[...]
>> Why don't you use the correct pointer type here instead of casting then
>> around and the parameter names are meaningless too.
>>
>> static bool cb_del_ampdu_pkt(struct sk_buff *mpdu, struct
>> cb_del_ampdu_pars *ampdu_pars)
[...]
> Actually this is a generic callback mechanism in which an additional
> parameter can be passed, which can be of *any* type hence a void pointer
> seems justified here and a meaningless name is used. I do agree that the
> txi parameter will always be a struct sk_buff and should be indicated as
> such.

Reported-by: Hauke Mehrtens <hauke@hauke-m.de>
Signed-off-by: Clemens Noss <cnoss@gmx.de>
---
 drivers/staging/brcm80211/brcmsmac/wlc_ampdu.c |    3 +--
 drivers/staging/brcm80211/include/bcmutils.h   |    2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/brcm80211/brcmsmac/wlc_ampdu.c b/drivers/staging/brcm80211/brcmsmac/wlc_ampdu.c
index 0d93bd6..b1bc1a6 100644
--- a/drivers/staging/brcm80211/brcmsmac/wlc_ampdu.c
+++ b/drivers/staging/brcm80211/brcmsmac/wlc_ampdu.c
@@ -1225,9 +1225,8 @@ void wlc_ampdu_shm_upd(struct ampdu_info *ampdu)
 /*
  * callback function that helps flushing ampdu packets from a priority queue
  */
-static bool cb_del_ampdu_pkt(void *p, void *arg_a)
+static bool cb_del_ampdu_pkt(struct sk_buff *mpdu, void *arg_a)
 {
-	struct sk_buff *mpdu = (struct sk_buff *)p;
 	struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(mpdu);
 	struct cb_del_ampdu_pars *ampdu_pars =
 				 (struct cb_del_ampdu_pars *)arg_a;
diff --git a/drivers/staging/brcm80211/include/bcmutils.h b/drivers/staging/brcm80211/include/bcmutils.h
index 1d57934..4dc190b 100644
--- a/drivers/staging/brcm80211/include/bcmutils.h
+++ b/drivers/staging/brcm80211/include/bcmutils.h
@@ -74,7 +74,7 @@
 #define PKTQ_PREC_ITER(pq, prec)        for (prec = (pq)->num_prec - 1; prec >= 0; prec--)
 
 /* fn(pkt, arg).  return true if pkt belongs to if */
-	typedef bool(*ifpkt_cb_t) (void *, void *);
+typedef bool(*ifpkt_cb_t) (struct sk_buff *, void *);
 
 /* operations on a specific precedence in packet queue */
 
-- 
1.7.4.5

Re: [PATCH] staging: brcm80211: fix cast to pointer from integer

[Roland Vossen]

Greg, please also merge this to the staging-linus branch since it might 
resolve an instability issue.

On 05/15/2011 10:45 PM, Clemens Noss wrote:
> bcm_pktq_flush and related functions only ever get 0 or a pointer for
> arg, so make it a pointer.
>
> This might fix a crash on 64bit.

Acked-by: Roland Vossen <rvossen@broadcom.com>

>
> Signed-off-by: Clemens Noss<cnoss@gmx.de>
> ---
>   drivers/staging/brcm80211/brcmfmac/dhd_sdio.c  |    2 +-
>   drivers/staging/brcm80211/brcmsmac/wlc_ampdu.c |    4 ++--
>   drivers/staging/brcm80211/brcmsmac/wlc_main.c  |    4 ++--
>   drivers/staging/brcm80211/include/bcmutils.h   |    6 +++---
>   drivers/staging/brcm80211/util/bcmutils.c      |    4 ++--
>   5 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/brcm80211/brcmfmac/dhd_sdio.c b/drivers/staging/brcm80211/brcmfmac/dhd_sdio.c
> index 8683eeb..a71c6f8 100644
> --- a/drivers/staging/brcm80211/brcmfmac/dhd_sdio.c
> +++ b/drivers/staging/brcm80211/brcmfmac/dhd_sdio.c
> @@ -2845,7 +2845,7 @@ void dhd_bus_stop(struct dhd_bus *bus, bool enforce_mutex)
>   	dhdsdio_clkctl(bus, CLK_SDONLY, false);
>
>   	/* Clear the data packet queues */
> -	bcm_pktq_flush(&bus->txq, true, NULL, 0);
> +	bcm_pktq_flush(&bus->txq, true, NULL, NULL);
>
>   	/* Clear any held glomming stuff */
>   	if (bus->glomd)
> diff --git a/drivers/staging/brcm80211/brcmsmac/wlc_ampdu.c b/drivers/staging/brcm80211/brcmsmac/wlc_ampdu.c
> index 5b041a6..0d93bd6 100644
> --- a/drivers/staging/brcm80211/brcmsmac/wlc_ampdu.c
> +++ b/drivers/staging/brcm80211/brcmsmac/wlc_ampdu.c
> @@ -1225,7 +1225,7 @@ void wlc_ampdu_shm_upd(struct ampdu_info *ampdu)
>   /*
>    * callback function that helps flushing ampdu packets from a priority queue
>    */
> -static bool cb_del_ampdu_pkt(void *p, int arg_a)
> +static bool cb_del_ampdu_pkt(void *p, void *arg_a)
>   {
>   	struct sk_buff *mpdu = (struct sk_buff *)p;
>   	struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(mpdu);
> @@ -1269,7 +1269,7 @@ void wlc_ampdu_flush(struct wlc_info *wlc,
>   	ampdu_pars.tid = tid;
>   	for (prec = 0; prec<  pq->num_prec; prec++) {
>   		bcm_pktq_pflush(pq, prec, true, cb_del_ampdu_pkt,
> -			    (int)&ampdu_pars);
> +			    (void *)&ampdu_pars);
>   	}
>   	wlc_inval_dma_pkts(wlc->hw, sta, dma_cb_fn_ampdu);
>   }
> diff --git a/drivers/staging/brcm80211/brcmsmac/wlc_main.c b/drivers/staging/brcm80211/brcmsmac/wlc_main.c
> index 2fed0eb..926b01a 100644
> --- a/drivers/staging/brcm80211/brcmsmac/wlc_main.c
> +++ b/drivers/staging/brcm80211/brcmsmac/wlc_main.c
> @@ -2520,7 +2520,7 @@ uint wlc_down(struct wlc_info *wlc)
>
>   	/* flush tx queues */
>   	for (qi = wlc->tx_queues; qi != NULL; qi = qi->next) {
> -		bcm_pktq_flush(&qi->q, true, NULL, 0);
> +		bcm_pktq_flush(&qi->q, true, NULL, NULL);
>   	}
>
>   	callbacks += wlc_bmac_down_finish(wlc->hw);
> @@ -8135,7 +8135,7 @@ void wlc_wait_for_tx_completion(struct wlc_info *wlc, bool drop)
>   {
>   	/* flush packet queue when requested */
>   	if (drop)
> -		bcm_pktq_flush(&wlc->pkt_queue->q, false, NULL, 0);
> +		bcm_pktq_flush(&wlc->pkt_queue->q, false, NULL, NULL);
>
>   	/* wait for queue and DMA fifos to run dry */
>   	while (!pktq_empty(&wlc->pkt_queue->q) ||
> diff --git a/drivers/staging/brcm80211/include/bcmutils.h b/drivers/staging/brcm80211/include/bcmutils.h
> index aa00e83..1d57934 100644
> --- a/drivers/staging/brcm80211/include/bcmutils.h
> +++ b/drivers/staging/brcm80211/include/bcmutils.h
> @@ -74,7 +74,7 @@
>   #define PKTQ_PREC_ITER(pq, prec)        for (prec = (pq)->num_prec - 1; prec>= 0; prec--)
>
>   /* fn(pkt, arg).  return true if pkt belongs to if */
> -	typedef bool(*ifpkt_cb_t) (void *, int);
> +	typedef bool(*ifpkt_cb_t) (void *, void *);
>
>   /* operations on a specific precedence in packet queue */
>
> @@ -100,7 +100,7 @@ extern void bcm_pkt_buf_free_skb(struct sk_buff *skb);
>
>   /* Empty the queue at particular precedence level */
>   extern void bcm_pktq_pflush(struct pktq *pq, int prec,
> -	bool dir, ifpkt_cb_t fn, int arg);
> +	bool dir, ifpkt_cb_t fn, void *arg);
>
>   /* operations on a set of precedences in packet queue */
>
> @@ -127,7 +127,7 @@ extern void bcm_pktq_init(struct pktq *pq, int num_prec, int max_len);
>   /* prec_out may be NULL if caller is not interested in return value */
>   extern struct sk_buff *bcm_pktq_peek_tail(struct pktq *pq, int *prec_out);
>   extern void bcm_pktq_flush(struct pktq *pq, bool dir,
> -	ifpkt_cb_t fn, int arg);
> +	ifpkt_cb_t fn, void *arg);
>
>   /* externs */
>   /* packet */
> diff --git a/drivers/staging/brcm80211/util/bcmutils.c b/drivers/staging/brcm80211/util/bcmutils.c
> index e185e28..43e5bb3 100644
> --- a/drivers/staging/brcm80211/util/bcmutils.c
> +++ b/drivers/staging/brcm80211/util/bcmutils.c
> @@ -231,7 +231,7 @@ EXPORT_SYMBOL(bcm_pktq_pdeq_tail);
>
>   void
>   bcm_pktq_pflush(struct pktq *pq, int prec, bool dir,
> -	    ifpkt_cb_t fn, int arg)
> +	    ifpkt_cb_t fn, void *arg)
>   {
>   	struct pktq_prec *q;
>   	struct sk_buff *p, *prev = NULL;
> @@ -263,7 +263,7 @@ bcm_pktq_pflush(struct pktq *pq, int prec, bool dir,
>   EXPORT_SYMBOL(bcm_pktq_pflush);
>
>   void bcm_pktq_flush(struct pktq *pq, bool dir,
> -		ifpkt_cb_t fn, int arg)
> +		ifpkt_cb_t fn, void *arg)
>   {
>   	int prec;
>   	for (prec = 0; prec<  pq->num_prec; prec++)

Re: [PATCH] staging: brcm80211: use correct pointer types in bcm_pktq_flush

[Roland Vossen]

> Reported-by: Hauke Mehrtens<hauke@hauke-m.de>
> Signed-off-by: Clemens Noss<cnoss@gmx.de>

Acked-by: Roland Vossen <rvossen@broadcom.com>

Re: [PATCH] staging: brcm80211: fix cast to pointer from integer

[Greg KH]

On Mon, May 16, 2011 at 01:09:50PM +0200, Roland Vossen wrote:
> Greg, please also merge this to the staging-linus branch since it
> might resolve an instability issue.

It "might"?

Is this a regression?  Do people see the problem on the .38 kernel as
well?

It's way too late for anything but severe regressions to be sent to
Linus at this point in time...

thanks,

greg k-h

Re: [PATCH] staging: brcm80211: fix cast to pointer from integer

[Roland Vossen]

>> Greg, please also merge this to the staging-linus branch since it
>> might resolve an instability issue.
>
> It "might"?

Correction: I tested it now. Indeed it resolves a problem (segmentation 
fault) upon dissociation on amd64.

> Is this a regression?  Do people see the problem on the .38 kernel as
> well?

People on the .38 kernel see the problem even worse: they experience a 
crash on dissociation on both i386 and amd64.

> It's way too late for anything but severe regressions to be sent to
> Linus at this point in time...

Understood. Since behavior has not regressed, I think this fix does not 
qualify to be sent to Linus. Perhaps we can get it into a 
compat-wireless package.

Thanks, Roland.