This smallish set contains:
* rework headroom handling in .start_xmit() callback.
* add Cypress maintainers for brcm80211.
The series apply to the master branch of the wireless-drivers-next
repository.
Arend van Spriel (4):
brcmfmac: cleanup kerneldoc for struct brcmf_bus
brcmfmac: use atomic_t for statistic counter in struct brcmf_bus
brcmfmac: rework headroom check in .start_xmit()
MAINTAINERS: add brcm80211 maintainer info from Cypress
MAINTAINERS | 3 +++
.../net/wireless/broadcom/brcm80211/brcmfmac/bus.h | 18 +++++++++++++----
.../wireless/broadcom/brcm80211/brcmfmac/core.c | 23 +++++++++++++++-------
.../wireless/broadcom/brcm80211/brcmfmac/sdio.c | 13 +++++++-----
4 files changed, 41 insertions(+), 16 deletions(-)
--
1.9.1
Arend van Spriel <[email protected]> writes:
> Since commit 9cc4b7cb86cb ("brcmfmac: Make skb header writable
> before use") the headroom usage has been fixed. However, the
> driver was keeping statistics that got lost. So reworking the
> code so we get those driver statistics back for debugging.
>
> Cc: James Hughes <[email protected]>
> Reviewed-by: Hante Meuleman <[email protected]>
> Reviewed-by: Pieter-Paul Giesberts <[email protected]>
> Reviewed-by: Franky Lin <[email protected]>
> Signed-off-by: Arend van Spriel <[email protected]>
> ---
> .../net/wireless/broadcom/brcm80211/brcmfmac/bus.h | 15 ++++++++++++--
> .../wireless/broadcom/brcm80211/brcmfmac/core.c | 23 +++++++++++++++-------
> .../wireless/broadcom/brcm80211/brcmfmac/sdio.c | 13 +++++++-----
> 3 files changed, 37 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
> index e1a4d9e..163ddc4 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
> @@ -113,6 +113,17 @@ struct brcmf_bus_msgbuf {
>
>
> /**
> + * struct brcmf_bus_stats - bus statistic counters.
> + *
> + * @pktcowed: packets cowed for extra headroom/unorphan.
> + * @pktcow_failed: packets dropped due to failed cow-ing.
> + */
> +struct brcmf_bus_stats {
> + atomic_t pktcowed;
> + atomic_t pktcow_failed;
> +};
Same question as in the previous patch. I only see updates for these
variables, but nobody reading them?
--
Kalle Valo
Since Cypress took over IoT part of Broadcom some chips supported
by brcmfmac moved over as well. Adding maintainer info of our peers
at Cypress to make their support official.
Cc: Chi-Hsien Lin <[email protected]>
Cc: Wright Feng <[email protected]>
Reviewed-by: Hante Meuleman <[email protected]>
Reviewed-by: Pieter-Paul Giesberts <[email protected]>
Reviewed-by: Franky Lin <[email protected]>
Signed-off-by: Arend van Spriel <[email protected]>
---
MAINTAINERS | 3 +++
1 file changed, 3 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index f4e682c..fa9744f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2785,8 +2785,11 @@ BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER
M: Arend van Spriel <[email protected]>
M: Franky Lin <[email protected]>
M: Hante Meuleman <[email protected]>
+M: Chi-Hsien Lin <[email protected]>
+M: Wright Feng <[email protected]>
L: [email protected]
L: [email protected]
+L: [email protected]
S: Supported
F: drivers/net/wireless/broadcom/brcm80211/
--
1.9.1
Since commit 9cc4b7cb86cb ("brcmfmac: Make skb header writable
before use") the headroom usage has been fixed. However, the
driver was keeping statistics that got lost. So reworking the
code so we get those driver statistics back for debugging.
Cc: James Hughes <[email protected]>
Reviewed-by: Hante Meuleman <[email protected]>
Reviewed-by: Pieter-Paul Giesberts <[email protected]>
Reviewed-by: Franky Lin <[email protected]>
Signed-off-by: Arend van Spriel <[email protected]>
---
.../net/wireless/broadcom/brcm80211/brcmfmac/bus.h | 15 ++++++++++++--
.../wireless/broadcom/brcm80211/brcmfmac/core.c | 23 +++++++++++++++-------
.../wireless/broadcom/brcm80211/brcmfmac/sdio.c | 13 +++++++-----
3 files changed, 37 insertions(+), 14 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
index e1a4d9e..163ddc4 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
@@ -113,6 +113,17 @@ struct brcmf_bus_msgbuf {
/**
+ * struct brcmf_bus_stats - bus statistic counters.
+ *
+ * @pktcowed: packets cowed for extra headroom/unorphan.
+ * @pktcow_failed: packets dropped due to failed cow-ing.
+ */
+struct brcmf_bus_stats {
+ atomic_t pktcowed;
+ atomic_t pktcow_failed;
+};
+
+/**
* struct brcmf_bus - interface structure between common and bus layer
*
* @bus_priv: pointer to private bus device.
@@ -120,8 +131,8 @@ struct brcmf_bus_msgbuf {
* @dev: device pointer of bus device.
* @drvr: public driver information.
* @state: operational state of the bus interface.
+ * @stats: statistics shared between common and bus layer.
* @maxctl: maximum size for rxctl request message.
- * @tx_realloc: number of tx packets realloced for headroom.
* @chip: device identifier of the dongle chip.
* @always_use_fws_queue: bus wants use queue also when fwsignal is inactive.
* @wowl_supported: is wowl supported by bus driver.
@@ -137,8 +148,8 @@ struct brcmf_bus {
struct device *dev;
struct brcmf_pub *drvr;
enum brcmf_bus_state state;
+ struct brcmf_bus_stats stats;
uint maxctl;
- atomic_t tx_realloc;
u32 chip;
u32 chiprev;
bool always_use_fws_queue;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index a88da5a..790151b 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -199,6 +199,7 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
struct brcmf_if *ifp = netdev_priv(ndev);
struct brcmf_pub *drvr = ifp->drvr;
struct ethhdr *eh;
+ int head_delta;
brcmf_dbg(DATA, "Enter, bsscfgidx=%d\n", ifp->bsscfgidx);
@@ -211,13 +212,21 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
goto done;
}
- /* Make sure there's enough writable headroom*/
- ret = skb_cow_head(skb, drvr->hdrlen);
- if (ret < 0) {
- brcmf_err("%s: skb_cow_head failed\n",
- brcmf_ifname(ifp));
- dev_kfree_skb(skb);
- goto done;
+ /* Make sure there's enough writeable headroom */
+ if (skb_headroom(skb) < drvr->hdrlen || skb_header_cloned(skb)) {
+ head_delta = drvr->hdrlen - skb_headroom(skb);
+
+ brcmf_dbg(INFO, "%s: insufficient headroom (%d)\n",
+ brcmf_ifname(ifp), head_delta);
+ atomic_inc(&drvr->bus_if->stats.pktcowed);
+ ret = pskb_expand_head(skb, ALIGN(head_delta, NET_SKB_PAD), 0,
+ GFP_ATOMIC);
+ if (ret < 0) {
+ brcmf_err("%s: failed to expand headroom\n",
+ brcmf_ifname(ifp));
+ atomic_inc(&drvr->bus_if->stats.pktcow_failed);
+ goto done;
+ }
}
/* validate length for ether packet */
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index a280536..e745bc1 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -2037,6 +2037,7 @@ static uint brcmf_sdio_readframes(struct brcmf_sdio *bus, uint maxframes)
static int brcmf_sdio_txpkt_hdalign(struct brcmf_sdio *bus, struct sk_buff *pkt)
{
+ struct brcmf_bus_stats *stats;
u16 head_pad;
u8 *dat_buf;
@@ -2046,16 +2047,18 @@ static int brcmf_sdio_txpkt_hdalign(struct brcmf_sdio *bus, struct sk_buff *pkt)
head_pad = ((unsigned long)dat_buf % bus->head_align);
if (head_pad) {
if (skb_headroom(pkt) < head_pad) {
- atomic_inc(&bus->sdiodev->bus_if->tx_realloc);
- head_pad = 0;
- if (skb_cow(pkt, head_pad))
+ stats = &bus->sdiodev->bus_if->stats;
+ atomic_inc(&stats->pktcowed);
+ if (skb_cow_head(pkt, head_pad)) {
+ atomic_inc(&stats->pktcow_failed);
return -ENOMEM;
+ }
}
skb_push(pkt, head_pad);
dat_buf = (u8 *)(pkt->data);
- memset(dat_buf, 0, head_pad + bus->tx_hdrlen);
}
- return head_pad;
+ memset(dat_buf, 0, head_pad + bus->tx_hdrlen);
+ return 0;
}
/**
--
1.9.1
On 27-06-17 16:09, Kalle Valo wrote:
> Arend van Spriel <[email protected]> writes:
>
>> Since commit 9cc4b7cb86cb ("brcmfmac: Make skb header writable
>> before use") the headroom usage has been fixed. However, the
>> driver was keeping statistics that got lost. So reworking the
>> code so we get those driver statistics back for debugging.
>>
>> Cc: James Hughes <[email protected]>
>> Reviewed-by: Hante Meuleman <[email protected]>
>> Reviewed-by: Pieter-Paul Giesberts <[email protected]>
>> Reviewed-by: Franky Lin <[email protected]>
>> Signed-off-by: Arend van Spriel <[email protected]>
>> ---
>> .../net/wireless/broadcom/brcm80211/brcmfmac/bus.h | 15 ++++++++++++--
>> .../wireless/broadcom/brcm80211/brcmfmac/core.c | 23 +++++++++++++++-------
>> .../wireless/broadcom/brcm80211/brcmfmac/sdio.c | 13 +++++++-----
>> 3 files changed, 37 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
>> index e1a4d9e..163ddc4 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
>> @@ -113,6 +113,17 @@ struct brcmf_bus_msgbuf {
>>
>>
>> /**
>> + * struct brcmf_bus_stats - bus statistic counters.
>> + *
>> + * @pktcowed: packets cowed for extra headroom/unorphan.
>> + * @pktcow_failed: packets dropped due to failed cow-ing.
>> + */
>> +struct brcmf_bus_stats {
>> + atomic_t pktcowed;
>> + atomic_t pktcow_failed;
>> +};
>
> Same question as in the previous patch. I only see updates for these
> variables, but nobody reading them?
Hi Kalle,
You are right. My intention was to expose these to debugfs, but clearly
did not include it in this patch series. So how do you want to handle
this. Feel free to drop patch 2 and 3. I can resubmit them with
subsequent patches dealing with exposing it to debugfs if that makes
more sense.
Regards,
Arend
The statistic counter is used in common layer and in the bus layer
in different thread contexts so change to use atomic operations.
Reviewed-by: Hante Meuleman <[email protected]>
Reviewed-by: Pieter-Paul Giesberts <[email protected]>
Reviewed-by: Franky Lin <[email protected]>
Signed-off-by: Arend van Spriel <[email protected]>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h | 2 +-
drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
index e2c895b..e1a4d9e 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
@@ -138,7 +138,7 @@ struct brcmf_bus {
struct brcmf_pub *drvr;
enum brcmf_bus_state state;
uint maxctl;
- unsigned long tx_realloc;
+ atomic_t tx_realloc;
u32 chip;
u32 chiprev;
bool always_use_fws_queue;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index 81f9594..a280536 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -2046,7 +2046,7 @@ static int brcmf_sdio_txpkt_hdalign(struct brcmf_sdio *bus, struct sk_buff *pkt)
head_pad = ((unsigned long)dat_buf % bus->head_align);
if (head_pad) {
if (skb_headroom(pkt) < head_pad) {
- bus->sdiodev->bus_if->tx_realloc++;
+ atomic_inc(&bus->sdiodev->bus_if->tx_realloc);
head_pad = 0;
if (skb_cow(pkt, head_pad))
return -ENOMEM;
--
1.9.1
Arend van Spriel <[email protected]> writes:
> The statistic counter is used in common layer and in the bus layer
> in different thread contexts so change to use atomic operations.
>
> Reviewed-by: Hante Meuleman <[email protected]>
> Reviewed-by: Pieter-Paul Giesberts <[email protected]>
> Reviewed-by: Franky Lin <[email protected]>
> Signed-off-by: Arend van Spriel <[email protected]>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h | 2 +-
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
> index e2c895b..e1a4d9e 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
> @@ -138,7 +138,7 @@ struct brcmf_bus {
> struct brcmf_pub *drvr;
> enum brcmf_bus_state state;
> uint maxctl;
> - unsigned long tx_realloc;
> + atomic_t tx_realloc;
> u32 chip;
> u32 chiprev;
> bool always_use_fws_queue;
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> index 81f9594..a280536 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> @@ -2046,7 +2046,7 @@ static int brcmf_sdio_txpkt_hdalign(struct brcmf_sdio *bus, struct sk_buff *pkt)
> head_pad = ((unsigned long)dat_buf % bus->head_align);
> if (head_pad) {
> if (skb_headroom(pkt) < head_pad) {
> - bus->sdiodev->bus_if->tx_realloc++;
> + atomic_inc(&bus->sdiodev->bus_if->tx_realloc);
What's the point of this field? AFAICS it's only incremented but never
read. What am I missing?
--
Kalle Valo
Arend Van Spriel <[email protected]> wrote:
> A couple of old fields were still described and one field was not
> described.
>
> Reviewed-by: Hante Meuleman <[email protected]>
> Reviewed-by: Pieter-Paul Giesberts <[email protected]>
> Reviewed-by: Franky Lin <[email protected]>
> Signed-off-by: Arend van Spriel <[email protected]>
4 patches applied to wireless-drivers-next.git, thanks.
ca2e99b2cae9 brcmfmac: cleanup kerneldoc for struct brcmf_bus
a833f3d4de53 brcmfmac: use atomic_t for statistic counter in struct brcmf_bus
270a6c1f65fe brcmfmac: rework headroom check in .start_xmit()
21394d57feba MAINTAINERS: add brcm80211 maintainer info from Cypress
--
https://patchwork.kernel.org/patch/9803889/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
A couple of old fields were still described and one field was not
described.
Reviewed-by: Hante Meuleman <[email protected]>
Reviewed-by: Pieter-Paul Giesberts <[email protected]>
Reviewed-by: Franky Lin <[email protected]>
Signed-off-by: Arend van Spriel <[email protected]>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
index b55c329..e2c895b 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
@@ -122,9 +122,8 @@ struct brcmf_bus_msgbuf {
* @state: operational state of the bus interface.
* @maxctl: maximum size for rxctl request message.
* @tx_realloc: number of tx packets realloced for headroom.
- * @dstats: dongle-based statistical data.
- * @dcmd_list: bus/device specific dongle initialization commands.
* @chip: device identifier of the dongle chip.
+ * @always_use_fws_queue: bus wants use queue also when fwsignal is inactive.
* @wowl_supported: is wowl supported by bus driver.
* @chiprev: revision of the dongle chip.
*/
--
1.9.1
Arend van Spriel <[email protected]> writes:
> On 27-06-17 16:09, Kalle Valo wrote:
>> Arend van Spriel <[email protected]> writes:
>>
>>> Since commit 9cc4b7cb86cb ("brcmfmac: Make skb header writable
>>> before use") the headroom usage has been fixed. However, the
>>> driver was keeping statistics that got lost. So reworking the
>>> code so we get those driver statistics back for debugging.
>>>
>>> Cc: James Hughes <[email protected]>
>>> Reviewed-by: Hante Meuleman <[email protected]>
>>> Reviewed-by: Pieter-Paul Giesberts <[email protected]>
>>> Reviewed-by: Franky Lin <[email protected]>
>>> Signed-off-by: Arend van Spriel <[email protected]>
>>> ---
>>> .../net/wireless/broadcom/brcm80211/brcmfmac/bus.h | 15 ++++++++++++--
>>> .../wireless/broadcom/brcm80211/brcmfmac/core.c | 23 +++++++++++++++-------
>>> .../wireless/broadcom/brcm80211/brcmfmac/sdio.c | 13 +++++++-----
>>> 3 files changed, 37 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
>>> index e1a4d9e..163ddc4 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
>>> @@ -113,6 +113,17 @@ struct brcmf_bus_msgbuf {
>>>
>>>
>>> /**
>>> + * struct brcmf_bus_stats - bus statistic counters.
>>> + *
>>> + * @pktcowed: packets cowed for extra headroom/unorphan.
>>> + * @pktcow_failed: packets dropped due to failed cow-ing.
>>> + */
>>> +struct brcmf_bus_stats {
>>> + atomic_t pktcowed;
>>> + atomic_t pktcow_failed;
>>> +};
>>
>> Same question as in the previous patch. I only see updates for these
>> variables, but nobody reading them?
>
> Hi Kalle,
>
> You are right. My intention was to expose these to debugfs, but clearly
> did not include it in this patch series. So how do you want to handle
> this. Feel free to drop patch 2 and 3. I can resubmit them with
> subsequent patches dealing with exposing it to debugfs if that makes
> more sense.
No need to resend if you are going to add the debugfs interface soon.
But the general rule is that we don't add any dead code to kernel and
there was no explanation in the commit log, that's why I asked.
--
Kalle Valo