[PATCH 3.4 0/2] brcm80211: two fullmac bug fix for 3.4

[PATCH 3.4 0/2] brcm80211: two fullmac bug fix for 3.4

[Franky Lin]

Hi John,

Here are two bug fix patches for brcmfmac aim for 3.4. One corrects a wrong SDIO function register write access behavior. The other one fixes an issue than dpc could miss completion events in heavy loading system.

Thanks,
Franky

Franky Lin (2):
  brcm80211: fmac: fix SDIO function 0 register r/w issue
  brcm80211: fmac: fix missing completion events issue

 .../net/wireless/brcm80211/brcmfmac/bcmsdh_sdmmc.c |    6 ++-
 drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c |   63 ++++++++++++++++----
 2 files changed, 56 insertions(+), 13 deletions(-)

-- 
1.7.5.4

[PATCH 3.4 1/2] brcm80211: fmac: fix SDIO function 0 register r/w issue

[Franky Lin]

SDIO stack doesn't have a structure for function 0. The structure
pointer stored in card->sdio_func[0] is actually for function 1.
With current implementation the register read/write is applied to
function 1. This pathch fixes the issue.

Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Reviewed-by: Arend van Spriel <arend@broadcom.com>
Signed-off-by: Franky Lin <frankyl@broadcom.com>
---
 .../net/wireless/brcm80211/brcmfmac/bcmsdh_sdmmc.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh_sdmmc.c b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh_sdmmc.c
index 4688904..6989916 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh_sdmmc.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh_sdmmc.c
@@ -108,9 +108,13 @@ static inline int brcmf_sdioh_f0_write_byte(struct brcmf_sdio_dev *sdiodev,
 			sdio_release_host(sdfunc);
 		}
 	} else if (regaddr == SDIO_CCCR_ABORT) {
+		sdfunc = kzalloc(sizeof(struct sdio_func), GFP_KERNEL);
+		memcpy(sdfunc, sdiodev->func[0], sizeof(struct sdio_func));
+		sdfunc->num = 0;
 		sdio_claim_host(sdfunc);
 		sdio_writeb(sdfunc, *byte, regaddr, &err_ret);
 		sdio_release_host(sdfunc);
+		kfree(sdfunc);
 	} else if (regaddr < 0xF0) {
 		brcmf_dbg(ERROR, "F0 Wr:0x%02x: write disallowed\n", regaddr);
 		err_ret = -EPERM;
@@ -486,7 +490,7 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func,
 			kfree(bus_if);
 			return -ENOMEM;
 		}
-		sdiodev->func[0] = func->card->sdio_func[0];
+		sdiodev->func[0] = func;
 		sdiodev->func[1] = func;
 		sdiodev->bus_if = bus_if;
 		bus_if->bus_priv.sdio = sdiodev;
-- 
1.7.5.4

Re: [PATCH 3.4 1/2] brcm80211: fmac: fix SDIO function 0 register r/w issue

[Florian Fainelli]

Hi Franky,

Le 04/20/12 23:27, Franky Lin a écrit :
> SDIO stack doesn't have a structure for function 0. The structure
> pointer stored in card->sdio_func[0] is actually for function 1.
> With current implementation the register read/write is applied to
> function 1. This pathch fixes the issue.
>
> Reviewed-by: Pieter-Paul Giesberts<pieterpg@broadcom.com>
> Reviewed-by: Arend van Spriel<arend@broadcom.com>
> Signed-off-by: Franky Lin<frankyl@broadcom.com>
> ---
>   .../net/wireless/brcm80211/brcmfmac/bcmsdh_sdmmc.c |    6 +++++-
>   1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh_sdmmc.c b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh_sdmmc.c
> index 4688904..6989916 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh_sdmmc.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh_sdmmc.c
> @@ -108,9 +108,13 @@ static inline int brcmf_sdioh_f0_write_byte(struct brcmf_sdio_dev *sdiodev,
>   			sdio_release_host(sdfunc);
>   		}
>   	} else if (regaddr == SDIO_CCCR_ABORT) {
> +		sdfunc = kzalloc(sizeof(struct sdio_func), GFP_KERNEL);

You are not catching a kzalloc() possible failure here.

> +		memcpy(sdfunc, sdiodev->func[0], sizeof(struct sdio_func));
> +		sdfunc->num = 0;
>   		sdio_claim_host(sdfunc);
>   		sdio_writeb(sdfunc, *byte, regaddr,&err_ret);
>   		sdio_release_host(sdfunc);
> +		kfree(sdfunc);
>   	} else if (regaddr<  0xF0) {
>   		brcmf_dbg(ERROR, "F0 Wr:0x%02x: write disallowed\n", regaddr);
>   		err_ret = -EPERM;
> @@ -486,7 +490,7 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func,
>   			kfree(bus_if);
>   			return -ENOMEM;
>   		}
> -		sdiodev->func[0] = func->card->sdio_func[0];
> +		sdiodev->func[0] = func;
>   		sdiodev->func[1] = func;
>   		sdiodev->bus_if = bus_if;
>   		bus_if->bus_priv.sdio = sdiodev;

Re: [PATCH 3.4 1/2] brcm80211: fmac: fix SDIO function 0 register r/w issue

[Johannes Berg]

On Mon, 2012-04-23 at 10:45 +0200, Florian Fainelli wrote:

> >   	} else if (regaddr == SDIO_CCCR_ABORT) {
> > +		sdfunc = kzalloc(sizeof(struct sdio_func), GFP_KERNEL);
> 
> You are not catching a kzalloc() possible failure here.
> 
> > +		memcpy(sdfunc, sdiodev->func[0], sizeof(struct sdio_func));

and it should probably be kmemdup anyway?

johannes

Re: [PATCH 3.4 1/2] brcm80211: fmac: fix SDIO function 0 register r/w issue

[Franky Lin]

On 04/23/2012 01:50 AM, Johannes Berg wrote:
> On Mon, 2012-04-23 at 10:45 +0200, Florian Fainelli wrote:
>
>>>    	} else if (regaddr == SDIO_CCCR_ABORT) {
>>> +		sdfunc = kzalloc(sizeof(struct sdio_func), GFP_KERNEL);
>>
>> You are not catching a kzalloc() possible failure here.
>>
>>> +		memcpy(sdfunc, sdiodev->func[0], sizeof(struct sdio_func));
>
> and it should probably be kmemdup anyway?
>
> johannes
>

Thx Florian and Johannes, I will send out a V2.

Franky

[PATCH 3.4 2/2] brcm80211: fmac: fix missing completion events issue

[Franky Lin]

dpc takes care of all data packets transmissions for sdio function
2. It is possible that it misses some completion events when the
traffic is heavy or it's running on a slow cpu. A linked list is
introduced to make sure dpc is invoked whenever needed.

Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Reviewed-by: Arend van Spriel <arend@broadcom.com>
Signed-off-by: Franky Lin <frankyl@broadcom.com>
---
 drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c |   63 ++++++++++++++++----
 1 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c b/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
index 2bf5dda..eb3829b 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
@@ -574,6 +574,8 @@ struct brcmf_sdio {
 
 	struct task_struct *dpc_tsk;
 	struct completion dpc_wait;
+	struct list_head dpc_tsklst;
+	spinlock_t dpc_tl_lock;
 
 	struct semaphore sdsem;
 
@@ -2594,29 +2596,58 @@ clkwait:
 	return resched;
 }
 
+static inline void brcmf_sdbrcm_adddpctsk(struct brcmf_sdio *bus)
+{
+	struct list_head *new_hd;
+	unsigned long flags;
+
+	if (in_interrupt())
+		new_hd = kzalloc(sizeof(struct list_head), GFP_ATOMIC);
+	else
+		new_hd = kzalloc(sizeof(struct list_head), GFP_KERNEL);
+	if (new_hd == NULL)
+		return;
+
+	spin_lock_irqsave(&bus->dpc_tl_lock, flags);
+	list_add_tail(new_hd, &bus->dpc_tsklst);
+	spin_unlock_irqrestore(&bus->dpc_tl_lock, flags);
+}
+
 static int brcmf_sdbrcm_dpc_thread(void *data)
 {
 	struct brcmf_sdio *bus = (struct brcmf_sdio *) data;
+	struct list_head *cur_hd, *tmp_hd;
+	unsigned long flags;
 
 	allow_signal(SIGTERM);
 	/* Run until signal received */
 	while (1) {
 		if (kthread_should_stop())
 			break;
-		if (!wait_for_completion_interruptible(&bus->dpc_wait)) {
-			/* Call bus dpc unless it indicated down
-			(then clean stop) */
-			if (bus->sdiodev->bus_if->state != BRCMF_BUS_DOWN) {
-				if (brcmf_sdbrcm_dpc(bus))
-					complete(&bus->dpc_wait);
-			} else {
+
+		if (list_empty(&bus->dpc_tsklst))
+			if (wait_for_completion_interruptible(&bus->dpc_wait))
+				break;
+
+		spin_lock_irqsave(&bus->dpc_tl_lock, flags);
+		list_for_each_safe(cur_hd, tmp_hd, &bus->dpc_tsklst) {
+			spin_unlock_irqrestore(&bus->dpc_tl_lock, flags);
+
+			if (bus->sdiodev->bus_if->state == BRCMF_BUS_DOWN) {
 				/* after stopping the bus, exit thread */
 				brcmf_sdbrcm_bus_stop(bus->sdiodev->dev);
 				bus->dpc_tsk = NULL;
 				break;
 			}
-		} else
-			break;
+
+			if (brcmf_sdbrcm_dpc(bus))
+				brcmf_sdbrcm_adddpctsk(bus);
+
+			spin_lock_irqsave(&bus->dpc_tl_lock, flags);
+			list_del(cur_hd);
+			kfree(cur_hd);
+		}
+		spin_unlock_irqrestore(&bus->dpc_tl_lock, flags);
 	}
 	return 0;
 }
@@ -2669,8 +2700,10 @@ static int brcmf_sdbrcm_bus_txdata(struct device *dev, struct sk_buff *pkt)
 	/* Schedule DPC if needed to send queued packet(s) */
 	if (!bus->dpc_sched) {
 		bus->dpc_sched = true;
-		if (bus->dpc_tsk)
+		if (bus->dpc_tsk) {
+			brcmf_sdbrcm_adddpctsk(bus);
 			complete(&bus->dpc_wait);
+		}
 	}
 
 	return ret;
@@ -3514,8 +3547,10 @@ void brcmf_sdbrcm_isr(void *arg)
 		brcmf_dbg(ERROR, "isr w/o interrupt configured!\n");
 
 	bus->dpc_sched = true;
-	if (bus->dpc_tsk)
+	if (bus->dpc_tsk) {
+		brcmf_sdbrcm_adddpctsk(bus);
 		complete(&bus->dpc_wait);
+	}
 }
 
 static bool brcmf_sdbrcm_bus_watchdog(struct brcmf_sdio *bus)
@@ -3559,8 +3594,10 @@ static bool brcmf_sdbrcm_bus_watchdog(struct brcmf_sdio *bus)
 				bus->ipend = true;
 
 				bus->dpc_sched = true;
-				if (bus->dpc_tsk)
+				if (bus->dpc_tsk) {
+					brcmf_sdbrcm_adddpctsk(bus);
 					complete(&bus->dpc_wait);
+				}
 			}
 		}
 
@@ -3897,6 +3934,8 @@ void *brcmf_sdbrcm_probe(u32 regsva, struct brcmf_sdio_dev *sdiodev)
 	}
 	/* Initialize DPC thread */
 	init_completion(&bus->dpc_wait);
+	INIT_LIST_HEAD(&bus->dpc_tsklst);
+	spin_lock_init(&bus->dpc_tl_lock);
 	bus->dpc_tsk = kthread_run(brcmf_sdbrcm_dpc_thread,
 				   bus, "brcmf_dpc");
 	if (IS_ERR(bus->dpc_tsk)) {
-- 
1.7.5.4

[PATCH 3.4 V2 0/2] brcm80211: two fullmac bug fix for 3.4

[Franky Lin]

Here are two bug fix patches for brcmfmac aim for 3.4. One corrects a wrong SDIO function register write access behavior. The other one fixes an issue than dpc could miss completion events in heavy loading system.

V2: Rework "brcm80211: fmac: fix SDIO function 0 register r/w issue" based on remark from Florian and Johannes.

Thanks,
Franky

Franky Lin (2):
  brcm80211: fmac: fix SDIO function 0 register r/w issue
  brcm80211: fmac: fix missing completion events issue

 .../net/wireless/brcm80211/brcmfmac/bcmsdh_sdmmc.c |    8 ++-
 drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c |   63 ++++++++++++++++----
 2 files changed, 58 insertions(+), 13 deletions(-)

-- 
1.7.5.4

[PATCH 3.4 V2 1/2] brcm80211: fmac: fix SDIO function 0 register r/w issue

[Franky Lin]

SDIO stack doesn't have a structure for function 0. The structure
pointer stored in card->sdio_func[0] is actually for function 1.
With current implementation the register read/write is applied to
function 1. This pathch fixes the issue.

Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Reviewed-by: Arend van Spriel <arend@broadcom.com>
Reviewed-by: Florian Fainelli <florian@openwrt.org>
Reviewed-by: Johannes Berg <johannes@sipsolutions.net>
Signed-off-by: Franky Lin <frankyl@broadcom.com>
---
 .../net/wireless/brcm80211/brcmfmac/bcmsdh_sdmmc.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh_sdmmc.c b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh_sdmmc.c
index 4688904..758c115 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh_sdmmc.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh_sdmmc.c
@@ -108,9 +108,15 @@ static inline int brcmf_sdioh_f0_write_byte(struct brcmf_sdio_dev *sdiodev,
 			sdio_release_host(sdfunc);
 		}
 	} else if (regaddr == SDIO_CCCR_ABORT) {
+		sdfunc = kmemdup(sdiodev->func[0], sizeof(struct sdio_func),
+				 GFP_KERNEL);
+		if (!sdfunc)
+			return -ENOMEM;
+		sdfunc->num = 0;
 		sdio_claim_host(sdfunc);
 		sdio_writeb(sdfunc, *byte, regaddr, &err_ret);
 		sdio_release_host(sdfunc);
+		kfree(sdfunc);
 	} else if (regaddr < 0xF0) {
 		brcmf_dbg(ERROR, "F0 Wr:0x%02x: write disallowed\n", regaddr);
 		err_ret = -EPERM;
@@ -486,7 +492,7 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func,
 			kfree(bus_if);
 			return -ENOMEM;
 		}
-		sdiodev->func[0] = func->card->sdio_func[0];
+		sdiodev->func[0] = func;
 		sdiodev->func[1] = func;
 		sdiodev->bus_if = bus_if;
 		bus_if->bus_priv.sdio = sdiodev;
-- 
1.7.5.4

[PATCH 3.4 V2 2/2] brcm80211: fmac: fix missing completion events issue

[Franky Lin]

dpc takes care of all data packets transmissions for sdio function
2. It is possible that it misses some completion events when the
traffic is heavy or it's running on a slow cpu. A linked list is
introduced to make sure dpc is invoked whenever needed.

Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Reviewed-by: Arend van Spriel <arend@broadcom.com>
Signed-off-by: Franky Lin <frankyl@broadcom.com>
---
 drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c |   63 ++++++++++++++++----
 1 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c b/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
index 2bf5dda..eb3829b 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
@@ -574,6 +574,8 @@ struct brcmf_sdio {
 
 	struct task_struct *dpc_tsk;
 	struct completion dpc_wait;
+	struct list_head dpc_tsklst;
+	spinlock_t dpc_tl_lock;
 
 	struct semaphore sdsem;
 
@@ -2594,29 +2596,58 @@ clkwait:
 	return resched;
 }
 
+static inline void brcmf_sdbrcm_adddpctsk(struct brcmf_sdio *bus)
+{
+	struct list_head *new_hd;
+	unsigned long flags;
+
+	if (in_interrupt())
+		new_hd = kzalloc(sizeof(struct list_head), GFP_ATOMIC);
+	else
+		new_hd = kzalloc(sizeof(struct list_head), GFP_KERNEL);
+	if (new_hd == NULL)
+		return;
+
+	spin_lock_irqsave(&bus->dpc_tl_lock, flags);
+	list_add_tail(new_hd, &bus->dpc_tsklst);
+	spin_unlock_irqrestore(&bus->dpc_tl_lock, flags);
+}
+
 static int brcmf_sdbrcm_dpc_thread(void *data)
 {
 	struct brcmf_sdio *bus = (struct brcmf_sdio *) data;
+	struct list_head *cur_hd, *tmp_hd;
+	unsigned long flags;
 
 	allow_signal(SIGTERM);
 	/* Run until signal received */
 	while (1) {
 		if (kthread_should_stop())
 			break;
-		if (!wait_for_completion_interruptible(&bus->dpc_wait)) {
-			/* Call bus dpc unless it indicated down
-			(then clean stop) */
-			if (bus->sdiodev->bus_if->state != BRCMF_BUS_DOWN) {
-				if (brcmf_sdbrcm_dpc(bus))
-					complete(&bus->dpc_wait);
-			} else {
+
+		if (list_empty(&bus->dpc_tsklst))
+			if (wait_for_completion_interruptible(&bus->dpc_wait))
+				break;
+
+		spin_lock_irqsave(&bus->dpc_tl_lock, flags);
+		list_for_each_safe(cur_hd, tmp_hd, &bus->dpc_tsklst) {
+			spin_unlock_irqrestore(&bus->dpc_tl_lock, flags);
+
+			if (bus->sdiodev->bus_if->state == BRCMF_BUS_DOWN) {
 				/* after stopping the bus, exit thread */
 				brcmf_sdbrcm_bus_stop(bus->sdiodev->dev);
 				bus->dpc_tsk = NULL;
 				break;
 			}
-		} else
-			break;
+
+			if (brcmf_sdbrcm_dpc(bus))
+				brcmf_sdbrcm_adddpctsk(bus);
+
+			spin_lock_irqsave(&bus->dpc_tl_lock, flags);
+			list_del(cur_hd);
+			kfree(cur_hd);
+		}
+		spin_unlock_irqrestore(&bus->dpc_tl_lock, flags);
 	}
 	return 0;
 }
@@ -2669,8 +2700,10 @@ static int brcmf_sdbrcm_bus_txdata(struct device *dev, struct sk_buff *pkt)
 	/* Schedule DPC if needed to send queued packet(s) */
 	if (!bus->dpc_sched) {
 		bus->dpc_sched = true;
-		if (bus->dpc_tsk)
+		if (bus->dpc_tsk) {
+			brcmf_sdbrcm_adddpctsk(bus);
 			complete(&bus->dpc_wait);
+		}
 	}
 
 	return ret;
@@ -3514,8 +3547,10 @@ void brcmf_sdbrcm_isr(void *arg)
 		brcmf_dbg(ERROR, "isr w/o interrupt configured!\n");
 
 	bus->dpc_sched = true;
-	if (bus->dpc_tsk)
+	if (bus->dpc_tsk) {
+		brcmf_sdbrcm_adddpctsk(bus);
 		complete(&bus->dpc_wait);
+	}
 }
 
 static bool brcmf_sdbrcm_bus_watchdog(struct brcmf_sdio *bus)
@@ -3559,8 +3594,10 @@ static bool brcmf_sdbrcm_bus_watchdog(struct brcmf_sdio *bus)
 				bus->ipend = true;
 
 				bus->dpc_sched = true;
-				if (bus->dpc_tsk)
+				if (bus->dpc_tsk) {
+					brcmf_sdbrcm_adddpctsk(bus);
 					complete(&bus->dpc_wait);
+				}
 			}
 		}
 
@@ -3897,6 +3934,8 @@ void *brcmf_sdbrcm_probe(u32 regsva, struct brcmf_sdio_dev *sdiodev)
 	}
 	/* Initialize DPC thread */
 	init_completion(&bus->dpc_wait);
+	INIT_LIST_HEAD(&bus->dpc_tsklst);
+	spin_lock_init(&bus->dpc_tl_lock);
 	bus->dpc_tsk = kthread_run(brcmf_sdbrcm_dpc_thread,
 				   bus, "brcmf_dpc");
 	if (IS_ERR(bus->dpc_tsk)) {
-- 
1.7.5.4