This series adds a few more fixes for SDIO and high latency devices.
I have had these on my private tree for quite some time now so they
should be considered fairly well tested.
4 out of 6 patches are from Alagu Sankar at Silex.
I have made some adjustments to some of them in order to make them
smaller and easier to review.
Alagu Sankar (4):
ath10k: use clean packet headers
ath10k: high latency fixes for beacon buffer
ath10k: sdio: read RX packets in bundles
ath10k: sdio: add MSDU ID allocation in HTT TX path
Erik Stromdahl (2):
ath10k: sdio: add missing error check
ath10k: sdio: replace skb_trim with explicit set of skb->len
drivers/net/wireless/ath/ath10k/htc.c | 1 +
drivers/net/wireless/ath/ath10k/htt_rx.c | 4 +-
drivers/net/wireless/ath/ath10k/htt_tx.c | 16 ++++-
drivers/net/wireless/ath/ath10k/hw.c | 2 +
drivers/net/wireless/ath/ath10k/mac.c | 31 +++++++---
drivers/net/wireless/ath/ath10k/sdio.c | 78 +++++++++++++++++++-----
drivers/net/wireless/ath/ath10k/sdio.h | 2 +
7 files changed, 109 insertions(+), 25 deletions(-)
--
2.19.1
From: Alagu Sankar <[email protected]>
HTC header carries junk values that may be interpreted by the firmware
differently. Enable credit update only if flow control is enabled for
the corresponding endpoint.
PLL clock setting sequence does not mask the PLL_CONTROL
register value. Side effect of not masking the values is not known as
the entire pll clock setting sequence is undocumented.
Signed-off-by: Alagu Sankar <[email protected]>
---
drivers/net/wireless/ath/ath10k/htc.c | 1 +
drivers/net/wireless/ath/ath10k/hw.c | 2 ++
2 files changed, 3 insertions(+)
diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c
index 805a7f8a04f2..1d4d1a1992fe 100644
--- a/drivers/net/wireless/ath/ath10k/htc.c
+++ b/drivers/net/wireless/ath/ath10k/htc.c
@@ -73,6 +73,7 @@ static void ath10k_htc_prepare_tx_skb(struct ath10k_htc_ep *ep,
struct ath10k_htc_hdr *hdr;
hdr = (struct ath10k_htc_hdr *)skb->data;
+ memset(hdr, 0, sizeof(struct ath10k_htc_hdr));
hdr->eid = ep->eid;
hdr->len = __cpu_to_le16(skb->len - sizeof(*hdr));
diff --git a/drivers/net/wireless/ath/ath10k/hw.c b/drivers/net/wireless/ath/ath10k/hw.c
index ad082b7d7643..cfc232f1fdbc 100644
--- a/drivers/net/wireless/ath/ath10k/hw.c
+++ b/drivers/net/wireless/ath/ath10k/hw.c
@@ -814,6 +814,8 @@ static int ath10k_hw_qca6174_enable_pll_clock(struct ath10k *ar)
if (ret)
return -EINVAL;
+ reg_val &= ~(WLAN_PLL_CONTROL_REFDIV_MASK | WLAN_PLL_CONTROL_DIV_MASK |
+ WLAN_PLL_CONTROL_NOPWD_MASK);
reg_val |= (SM(hw_clk->refdiv, WLAN_PLL_CONTROL_REFDIV) |
SM(hw_clk->div, WLAN_PLL_CONTROL_DIV) |
SM(1, WLAN_PLL_CONTROL_NOPWD));
--
2.19.1
From: Alagu Sankar <[email protected]>
Beacon buffer for high latency devices does not use DMA. other similar
buffer allocation methods in the driver have already been modified for
high latency path. Fix the beacon buffer allocation left out in the
earlier high latency changes.
Signed-off-by: Alagu Sankar <[email protected]>
---
drivers/net/wireless/ath/ath10k/mac.c | 31 ++++++++++++++++++++-------
1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index c9e700b844f8..2dd99ce44453 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -953,8 +953,12 @@ static void ath10k_mac_vif_beacon_cleanup(struct ath10k_vif *arvif)
ath10k_mac_vif_beacon_free(arvif);
if (arvif->beacon_buf) {
- dma_free_coherent(ar->dev, IEEE80211_MAX_FRAME_LEN,
- arvif->beacon_buf, arvif->beacon_paddr);
+ if (ar->dev_type == ATH10K_DEV_TYPE_HL)
+ kfree(arvif->beacon_buf);
+ else
+ dma_free_coherent(ar->dev, IEEE80211_MAX_FRAME_LEN,
+ arvif->beacon_buf,
+ arvif->beacon_paddr);
arvif->beacon_buf = NULL;
}
}
@@ -5210,10 +5214,17 @@ static int ath10k_add_interface(struct ieee80211_hw *hw,
if (vif->type == NL80211_IFTYPE_ADHOC ||
vif->type == NL80211_IFTYPE_MESH_POINT ||
vif->type == NL80211_IFTYPE_AP) {
- arvif->beacon_buf = dma_alloc_coherent(ar->dev,
- IEEE80211_MAX_FRAME_LEN,
- &arvif->beacon_paddr,
- GFP_ATOMIC);
+ if (ar->dev_type == ATH10K_DEV_TYPE_HL) {
+ arvif->beacon_buf = kmalloc(IEEE80211_MAX_FRAME_LEN,
+ GFP_KERNEL);
+ arvif->beacon_paddr = (dma_addr_t)arvif->beacon_buf;
+ } else {
+ arvif->beacon_buf =
+ dma_alloc_coherent(ar->dev,
+ IEEE80211_MAX_FRAME_LEN,
+ &arvif->beacon_paddr,
+ GFP_ATOMIC);
+ }
if (!arvif->beacon_buf) {
ret = -ENOMEM;
ath10k_warn(ar, "failed to allocate beacon buffer: %d\n",
@@ -5424,8 +5435,12 @@ static int ath10k_add_interface(struct ieee80211_hw *hw,
err:
if (arvif->beacon_buf) {
- dma_free_coherent(ar->dev, IEEE80211_MAX_FRAME_LEN,
- arvif->beacon_buf, arvif->beacon_paddr);
+ if (ar->dev_type == ATH10K_DEV_TYPE_HL)
+ kfree(arvif->beacon_buf);
+ else
+ dma_free_coherent(ar->dev, IEEE80211_MAX_FRAME_LEN,
+ arvif->beacon_buf,
+ arvif->beacon_paddr);
arvif->beacon_buf = NULL;
}
--
2.19.1
From: Alagu Sankar <[email protected]>
This makes the SDIO HTT TX path more similar to PCIe.
Transmit completion for SDIO is similar to PCIe, via the T2H message
HTT_T2H_MSG_TYPE_TX_COMPL_IND. This means that we will create a unique
MSDU ID for each transmitted frame just as we do in the PCIe case.
As a result of this, the TX skb will be freed when we receive the
HTT_T2H_MSG_TYPE_TX_COMPL_IND message. Thus, we must not free the skb in
the HTT ep_tx_complete handler in the SDIO case.
Co-developed-by: Erik Stromdahl <[email protected]>
Signed-off-by: Alagu Sankar <[email protected]>
Signed-off-by: Erik Stromdahl <[email protected]>
---
drivers/net/wireless/ath/ath10k/htt_rx.c | 4 +++-
drivers/net/wireless/ath/ath10k/htt_tx.c | 16 ++++++++++++++--
2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index a20ea270d519..6e3331b96c0f 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -2277,7 +2277,9 @@ static void ath10k_htt_rx_tx_compl_ind(struct ath10k *ar,
* Note that with only one concurrent reader and one concurrent
* writer, you don't need extra locking to use these macro.
*/
- if (!kfifo_put(&htt->txdone_fifo, tx_done)) {
+ if (ar->hif.bus == ATH10K_BUS_SDIO) {
+ ath10k_txrx_tx_unref(htt, &tx_done);
+ } else if (!kfifo_put(&htt->txdone_fifo, tx_done)) {
ath10k_warn(ar, "txdone fifo overrun, msdu_id %d status %d\n",
tx_done.msdu_id, tx_done.status);
ath10k_txrx_tx_unref(htt, &tx_done);
diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c b/drivers/net/wireless/ath/ath10k/htt_tx.c
index 619c2b87b8bb..e5e6e206a52f 100644
--- a/drivers/net/wireless/ath/ath10k/htt_tx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
@@ -543,7 +543,8 @@ void ath10k_htt_tx_free(struct ath10k_htt *htt)
void ath10k_htt_htc_tx_complete(struct ath10k *ar, struct sk_buff *skb)
{
- dev_kfree_skb_any(skb);
+ if (!(ar->hif.bus == ATH10K_BUS_SDIO))
+ dev_kfree_skb_any(skb);
}
void ath10k_htt_hif_tx_complete(struct ath10k *ar, struct sk_buff *skb)
@@ -1244,6 +1245,7 @@ static int ath10k_htt_tx_hl(struct ath10k_htt *htt, enum ath10k_hw_txrx_mode txm
u8 tid = ath10k_htt_tx_get_tid(msdu, is_eth);
u8 flags0 = 0;
u16 flags1 = 0;
+ u16 msdu_id = 0;
data_len = msdu->len;
@@ -1291,6 +1293,16 @@ static int ath10k_htt_tx_hl(struct ath10k_htt *htt, enum ath10k_hw_txrx_mode txm
}
}
+ if (ar->hif.bus == ATH10K_BUS_SDIO) {
+ flags1 |= HTT_DATA_TX_DESC_FLAGS1_POSTPONED;
+ res = ath10k_htt_tx_alloc_msdu_id(htt, msdu);
+ if (res < 0) {
+ ath10k_err(ar, "msdu_id allocation failed %d\n", res);
+ goto out;
+ }
+ msdu_id = res;
+ }
+
skb_push(msdu, sizeof(*cmd_hdr));
skb_push(msdu, sizeof(*tx_desc));
cmd_hdr = (struct htt_cmd_hdr *)msdu->data;
@@ -1300,7 +1312,7 @@ static int ath10k_htt_tx_hl(struct ath10k_htt *htt, enum ath10k_hw_txrx_mode txm
tx_desc->flags0 = flags0;
tx_desc->flags1 = __cpu_to_le16(flags1);
tx_desc->len = __cpu_to_le16(data_len);
- tx_desc->id = 0;
+ tx_desc->id = __cpu_to_le16(msdu_id);
tx_desc->frags_paddr = 0; /* always zero */
/* Initialize peer_id to INVALID_PEER because this is NOT
* Reinjection path
--
2.19.1
Although not likely, the bundle allocation might fail.
Add proper error check and warning print.
Signed-off-by: Erik Stromdahl <[email protected]>
---
drivers/net/wireless/ath/ath10k/sdio.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
index 295e1e7ec3b0..3eb241cb8a25 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -586,6 +586,11 @@ static int ath10k_sdio_mbox_rx_alloc(struct ath10k *ar,
act_len,
&bndl_cnt);
+ if (ret) {
+ ath10k_warn(ar, "alloc_bundle error %d\n", ret);
+ goto err;
+ }
+
n_lookaheads += bndl_cnt;
i += bndl_cnt;
/*Next buffer will be the last in the bundle */
--
2.19.1
From: Alagu Sankar <[email protected]>
The existing implementation of initiating multiple sdio transfers for
receive bundling is slowing down the receive speed.
Instead of having one sdio transfer for each packet in the bundle, we
read all packets in one sdio transfer.
This results in significant performance improvement on some targets.
Co-developed-by: Erik Stromdahl <[email protected]>
Signed-off-by: Alagu Sankar <[email protected]>
Signed-off-by: Erik Stromdahl <[email protected]>
---
drivers/net/wireless/ath/ath10k/sdio.c | 66 +++++++++++++++++++++-----
drivers/net/wireless/ath/ath10k/sdio.h | 2 +
2 files changed, 55 insertions(+), 13 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
index fae56c67766f..295e1e7ec3b0 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -24,6 +24,8 @@
#include "trace.h"
#include "sdio.h"
+#define ATH10K_SDIO_READ_BUF_SIZE (32 * 1024)
+
/* inlined helper functions */
static inline int ath10k_sdio_calc_txrx_padded_len(struct ath10k_sdio *ar_sdio,
@@ -618,41 +620,68 @@ static int ath10k_sdio_mbox_rx_alloc(struct ath10k *ar,
return ret;
}
-static int ath10k_sdio_mbox_rx_packet(struct ath10k *ar,
- struct ath10k_sdio_rx_data *pkt)
+static int ath10k_sdio_mbox_rx_fetch(struct ath10k *ar)
{
struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
+ struct ath10k_sdio_rx_data *pkt = &ar_sdio->rx_pkts[0];
struct sk_buff *skb = pkt->skb;
int ret;
- ret = ath10k_sdio_readsb(ar, ar_sdio->mbox_info.htc_addr,
- skb->data, pkt->alloc_len);
- pkt->status = ret;
- if (!ret)
+ ret = ath10k_sdio_read(ar, ar_sdio->mbox_info.htc_addr,
+ skb->data, pkt->alloc_len);
+ if (ret) {
+ ar_sdio->n_rx_pkts = 0;
+ ath10k_sdio_mbox_free_rx_pkt(pkt);
+ } else {
+ pkt->status = ret;
skb_put(skb, pkt->act_len);
+ }
return ret;
}
-static int ath10k_sdio_mbox_rx_fetch(struct ath10k *ar)
+static int ath10k_sdio_mbox_rx_fetch_bundle(struct ath10k *ar)
{
struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
+ struct ath10k_sdio_rx_data *pkt;
int ret, i;
+ u32 pkt_offset = 0, pkt_bundle_len = 0;
+
+ for (i = 0; i < ar_sdio->n_rx_pkts; i++)
+ pkt_bundle_len += ar_sdio->rx_pkts[i].alloc_len;
+
+ if (pkt_bundle_len > ATH10K_SDIO_READ_BUF_SIZE) {
+ ret = -ENOMEM;
+ ath10k_err(ar, "bundle size (%d) exceeding limit %d\n",
+ pkt_bundle_len, ATH10K_SDIO_READ_BUF_SIZE);
+ goto err;
+ }
+
+ ret = ath10k_sdio_readsb(ar, ar_sdio->mbox_info.htc_addr,
+ ar_sdio->sdio_read_buf, pkt_bundle_len);
+ if (ret)
+ goto err;
for (i = 0; i < ar_sdio->n_rx_pkts; i++) {
- ret = ath10k_sdio_mbox_rx_packet(ar,
- &ar_sdio->rx_pkts[i]);
- if (ret)
- goto err;
+ struct sk_buff *skb = ar_sdio->rx_pkts[i].skb;
+
+ pkt = &ar_sdio->rx_pkts[i];
+ memcpy(skb->data, ar_sdio->sdio_read_buf + pkt_offset,
+ pkt->alloc_len);
+ pkt->status = 0;
+ skb_put(skb, pkt->act_len);
+ pkt_offset += pkt->alloc_len;
}
return 0;
err:
/* Free all packets that was not successfully fetched. */
- for (; i < ar_sdio->n_rx_pkts; i++)
+ for (i = 0; i < ar_sdio->n_rx_pkts; i++)
ath10k_sdio_mbox_free_rx_pkt(&ar_sdio->rx_pkts[i]);
+ ar_sdio->n_rx_pkts = 0;
+
return ret;
}
@@ -695,7 +724,10 @@ static int ath10k_sdio_mbox_rxmsg_pending_handler(struct ath10k *ar,
*/
*done = false;
- ret = ath10k_sdio_mbox_rx_fetch(ar);
+ if (ar_sdio->n_rx_pkts > 1)
+ ret = ath10k_sdio_mbox_rx_fetch_bundle(ar);
+ else
+ ret = ath10k_sdio_mbox_rx_fetch(ar);
/* Process fetched packets. This will potentially update
* n_lookaheads depending on if the packets contain lookahead
@@ -2001,6 +2033,14 @@ static int ath10k_sdio_probe(struct sdio_func *func,
goto err_core_destroy;
}
+ ar_sdio->sdio_read_buf = devm_kzalloc(ar->dev,
+ ATH10K_SDIO_READ_BUF_SIZE,
+ GFP_KERNEL);
+ if (!ar_sdio->sdio_read_buf) {
+ ret = -ENOMEM;
+ goto err_core_destroy;
+ }
+
ar_sdio->func = func;
sdio_set_drvdata(func, ar_sdio);
diff --git a/drivers/net/wireless/ath/ath10k/sdio.h b/drivers/net/wireless/ath/ath10k/sdio.h
index b8c7ac0330bd..07e2cc6a3bd8 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.h
+++ b/drivers/net/wireless/ath/ath10k/sdio.h
@@ -196,6 +196,8 @@ struct ath10k_sdio {
struct ath10k *ar;
struct ath10k_sdio_irq_data irq_data;
+ u8 *sdio_read_buf;
+
/* temporary buffer for BMI requests */
u8 *bmi_buf;
--
2.19.1
This patch fixes a bug with padding of the skb data buffer.
Since skb_trim can only be used to reduce the skb len, it is useless when
we pad (increase the length of) the skb. Instead we must set skb->len
directly.
Signed-off-by: Erik Stromdahl <[email protected]>
---
drivers/net/wireless/ath/ath10k/sdio.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
index 3eb241cb8a25..989e3f563f3d 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -1496,7 +1496,12 @@ static int ath10k_sdio_hif_tx_sg(struct ath10k *ar, u8 pipe_id,
skb = items[i].transfer_context;
padded_len = ath10k_sdio_calc_txrx_padded_len(ar_sdio,
skb->len);
- skb_trim(skb, padded_len);
+ /* FIXME: unsure if just extending the skb len is the right
+ * thing to do since we might read outside the skb->data
+ * buffer. But we really don't want to realloc the skb just to
+ * pad the length.
+ */
+ skb->len = padded_len;
/* Write TX data to the end of the mbox address space */
address = ar_sdio->mbox_addr[eid] + ar_sdio->mbox_size[eid] -
--
2.19.1
Erik Stromdahl <[email protected]> writes:
> This series adds a few more fixes for SDIO and high latency devices.
>
> I have had these on my private tree for quite some time now so they
> should be considered fairly well tested.
>
> 4 out of 6 patches are from Alagu Sankar at Silex.
> I have made some adjustments to some of them in order to make them
> smaller and easier to review.
>
> Alagu Sankar (4):
> ath10k: use clean packet headers
> ath10k: high latency fixes for beacon buffer
> ath10k: sdio: read RX packets in bundles
> ath10k: sdio: add MSDU ID allocation in HTT TX path
>
> Erik Stromdahl (2):
> ath10k: sdio: add missing error check
> ath10k: sdio: replace skb_trim with explicit set of skb->len
Bad timing as also me and Wen have been cleaning up these patches and
finalising the SDIO support so our work overlapped. I'll send our
version of patches soon and we can then compare.
--
Kalle Valo
Erik Stromdahl <[email protected]> writes:
> From: Alagu Sankar <[email protected]>
>
> HTC header carries junk values that may be interpreted by the firmware
> differently. Enable credit update only if flow control is enabled for
> the corresponding endpoint.
>
> PLL clock setting sequence does not mask the PLL_CONTROL
> register value. Side effect of not masking the values is not known as
> the entire pll clock setting sequence is undocumented.
One logical change per patch, please. So this should be split to two.
> Signed-off-by: Alagu Sankar <[email protected]>
Erik's s-o-b missing.
> --- a/drivers/net/wireless/ath/ath10k/hw.c
> +++ b/drivers/net/wireless/ath/ath10k/hw.c
> @@ -814,6 +814,8 @@ static int ath10k_hw_qca6174_enable_pll_clock(struct ath10k *ar)
> if (ret)
> return -EINVAL;
>
> + reg_val &= ~(WLAN_PLL_CONTROL_REFDIV_MASK | WLAN_PLL_CONTROL_DIV_MASK |
> + WLAN_PLL_CONTROL_NOPWD_MASK);
> reg_val |= (SM(hw_clk->refdiv, WLAN_PLL_CONTROL_REFDIV) |
> SM(hw_clk->div, WLAN_PLL_CONTROL_DIV) |
> SM(1, WLAN_PLL_CONTROL_NOPWD));
The commit log mentions that there are no visible changes after this
patch. So why add it? :)
And do note that this also changes functionality for QCA6174 and QCA9377
PCI devices, so we have to be careful here.
--
Kalle Valo
Erik Stromdahl <[email protected]> writes:
> From: Alagu Sankar <[email protected]>
>
> The existing implementation of initiating multiple sdio transfers for
> receive bundling is slowing down the receive speed.
>
> Instead of having one sdio transfer for each packet in the bundle, we
> read all packets in one sdio transfer.
>
> This results in significant performance improvement on some targets.
Do you have any numbers? Before and after would be nice to know.
> +static int ath10k_sdio_mbox_rx_fetch(struct ath10k *ar)
> {
> struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
> + struct ath10k_sdio_rx_data *pkt = &ar_sdio->rx_pkts[0];
> struct sk_buff *skb = pkt->skb;
> int ret;
>
> - ret = ath10k_sdio_readsb(ar, ar_sdio->mbox_info.htc_addr,
> - skb->data, pkt->alloc_len);
> - pkt->status = ret;
> - if (!ret)
> + ret = ath10k_sdio_read(ar, ar_sdio->mbox_info.htc_addr,
> + skb->data, pkt->alloc_len);
> + if (ret) {
> + ar_sdio->n_rx_pkts = 0;
> + ath10k_sdio_mbox_free_rx_pkt(pkt);
> + } else {
> + pkt->status = ret;
> skb_put(skb, pkt->act_len);
> + }
With this you can avoid the else branch:
if (ret) {
ar_sdio->n_rx_pkts = 0;
ath10k_sdio_mbox_free_rx_pkt(pkt);
return ret;
}
> -static int ath10k_sdio_mbox_rx_fetch(struct ath10k *ar)
> +static int ath10k_sdio_mbox_rx_fetch_bundle(struct ath10k *ar)
> {
> struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
> + struct ath10k_sdio_rx_data *pkt;
> int ret, i;
> + u32 pkt_offset = 0, pkt_bundle_len = 0;
> +
> + for (i = 0; i < ar_sdio->n_rx_pkts; i++)
> + pkt_bundle_len += ar_sdio->rx_pkts[i].alloc_len;
> +
> + if (pkt_bundle_len > ATH10K_SDIO_READ_BUF_SIZE) {
> + ret = -ENOMEM;
> + ath10k_err(ar, "bundle size (%d) exceeding limit %d\n",
> + pkt_bundle_len, ATH10K_SDIO_READ_BUF_SIZE);
As this is a recoverable case please use ath10k_warn(). And would
-ENOSPC be more descriptive error?
> + goto err;
> + }
> +
> + ret = ath10k_sdio_readsb(ar, ar_sdio->mbox_info.htc_addr,
> + ar_sdio->sdio_read_buf, pkt_bundle_len);
> + if (ret)
> + goto err;
>
> for (i = 0; i < ar_sdio->n_rx_pkts; i++) {
> - ret = ath10k_sdio_mbox_rx_packet(ar,
> - &ar_sdio->rx_pkts[i]);
> - if (ret)
> - goto err;
> + struct sk_buff *skb = ar_sdio->rx_pkts[i].skb;
> +
> + pkt = &ar_sdio->rx_pkts[i];
> + memcpy(skb->data, ar_sdio->sdio_read_buf + pkt_offset,
> + pkt->alloc_len);
> + pkt->status = 0;
> + skb_put(skb, pkt->act_len);
Shouldn't you call first skb_put() and then memcpy()?
--
Kalle Valo
Erik Stromdahl <[email protected]> writes:
> This patch fixes a bug with padding of the skb data buffer.
> Since skb_trim can only be used to reduce the skb len, it is useless when
> we pad (increase the length of) the skb. Instead we must set skb->len
> directly.
>
> Signed-off-by: Erik Stromdahl <[email protected]>
> ---
> drivers/net/wireless/ath/ath10k/sdio.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
> index 3eb241cb8a25..989e3f563f3d 100644
> --- a/drivers/net/wireless/ath/ath10k/sdio.c
> +++ b/drivers/net/wireless/ath/ath10k/sdio.c
> @@ -1496,7 +1496,12 @@ static int ath10k_sdio_hif_tx_sg(struct ath10k *ar, u8 pipe_id,
> skb = items[i].transfer_context;
> padded_len = ath10k_sdio_calc_txrx_padded_len(ar_sdio,
> skb->len);
> - skb_trim(skb, padded_len);
> + /* FIXME: unsure if just extending the skb len is the right
> + * thing to do since we might read outside the skb->data
> + * buffer. But we really don't want to realloc the skb just to
> + * pad the length.
> + */
> + skb->len = padded_len;
Good catch! But I don't think you can modify skb->len directly like
that. There is skb_pad() but that doesn't change skb->len, so that most
likely needs more changes. So maybe skb_put() is the safest here?
--
Kalle Valo
On 4/12/19 2:36 PM, Kalle Valo wrote:
> Erik Stromdahl <[email protected]> writes:
>
>> This series adds a few more fixes for SDIO and high latency devices.
>>
>> I have had these on my private tree for quite some time now so they
>> should be considered fairly well tested.
>>
>> 4 out of 6 patches are from Alagu Sankar at Silex.
>> I have made some adjustments to some of them in order to make them
>> smaller and easier to review.
>>
>> Alagu Sankar (4):
>> ath10k: use clean packet headers
>> ath10k: high latency fixes for beacon buffer
>> ath10k: sdio: read RX packets in bundles
>> ath10k: sdio: add MSDU ID allocation in HTT TX path
>>
>> Erik Stromdahl (2):
>> ath10k: sdio: add missing error check
>> ath10k: sdio: replace skb_trim with explicit set of skb->len
>
> Bad timing as also me and Wen have been cleaning up these patches and
> finalising the SDIO support so our work overlapped. I'll send our
> version of patches soon and we can then compare.
>
Ok, I will rework these patches and apply them on top of yours and Wen's.
I'll send a v2 set later.
On 4/12/19 3:17 PM, Kalle Valo wrote:
> Erik Stromdahl <[email protected]> writes:
>
>> This patch fixes a bug with padding of the skb data buffer.
>> Since skb_trim can only be used to reduce the skb len, it is useless when
>> we pad (increase the length of) the skb. Instead we must set skb->len
>> directly.
>>
>> Signed-off-by: Erik Stromdahl <[email protected]>
>> ---
>> drivers/net/wireless/ath/ath10k/sdio.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
>> index 3eb241cb8a25..989e3f563f3d 100644
>> --- a/drivers/net/wireless/ath/ath10k/sdio.c
>> +++ b/drivers/net/wireless/ath/ath10k/sdio.c
>> @@ -1496,7 +1496,12 @@ static int ath10k_sdio_hif_tx_sg(struct ath10k *ar, u8 pipe_id,
>> skb = items[i].transfer_context;
>> padded_len = ath10k_sdio_calc_txrx_padded_len(ar_sdio,
>> skb->len);
>> - skb_trim(skb, padded_len);
>> + /* FIXME: unsure if just extending the skb len is the right
>> + * thing to do since we might read outside the skb->data
>> + * buffer. But we really don't want to realloc the skb just to
>> + * pad the length.
>> + */
>> + skb->len = padded_len;
>
> Good catch! But I don't think you can modify skb->len directly like
> that. There is skb_pad() but that doesn't change skb->len, so that most
> likely needs more changes. So maybe skb_put() is the safest here?
>
I have tried a few different solutions for this, but none seems to be
bullet proof.
skb_pad() raises a BUG() if there is not enough space in skb->data.
The best candidate so far has been skb_put_padto(). It pads and reallocates
the skb if needed.
The problem is that it also cause a panic if there is more than one reference
to the skb (skb_shared() returns true).
Some of the management frames via nl80211 have a refcount of 2.
In this case it is not possible to free and allocate the skb since there are
other users/references.
I think I will have to make some kind of solution where I copy the content of
the skb to an internal buffer instead.
+ johannes
Erik Stromdahl <[email protected]> writes:
> On 4/12/19 3:17 PM, Kalle Valo wrote:
>> Erik Stromdahl <[email protected]> writes:
>>
>>> This patch fixes a bug with padding of the skb data buffer.
>>> Since skb_trim can only be used to reduce the skb len, it is useless when
>>> we pad (increase the length of) the skb. Instead we must set skb->len
>>> directly.
>>>
>>> Signed-off-by: Erik Stromdahl <[email protected]>
>>> ---
>>> drivers/net/wireless/ath/ath10k/sdio.c | 7 ++++++-
>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
>>> index 3eb241cb8a25..989e3f563f3d 100644
>>> --- a/drivers/net/wireless/ath/ath10k/sdio.c
>>> +++ b/drivers/net/wireless/ath/ath10k/sdio.c
>>> @@ -1496,7 +1496,12 @@ static int ath10k_sdio_hif_tx_sg(struct ath10k *ar, u8 pipe_id,
>>> skb = items[i].transfer_context;
>>> padded_len = ath10k_sdio_calc_txrx_padded_len(ar_sdio,
>>> skb->len);
>>> - skb_trim(skb, padded_len);
>>> + /* FIXME: unsure if just extending the skb len is the right
>>> + * thing to do since we might read outside the skb->data
>>> + * buffer. But we really don't want to realloc the skb just to
>>> + * pad the length.
>>> + */
>>> + skb->len = padded_len;
>>
>> Good catch! But I don't think you can modify skb->len directly like
>> that. There is skb_pad() but that doesn't change skb->len, so that most
>> likely needs more changes. So maybe skb_put() is the safest here?
>>
> I have tried a few different solutions for this, but none seems to be
> bullet proof.
>
> skb_pad() raises a BUG() if there is not enough space in skb->data.
>
> The best candidate so far has been skb_put_padto(). It pads and reallocates
> the skb if needed.
>
> The problem is that it also cause a panic if there is more than one reference
> to the skb (skb_shared() returns true).
>
> Some of the management frames via nl80211 have a refcount of 2.
> In this case it is not possible to free and allocate the skb since there are
> other users/references.
>
> I think I will have to make some kind of solution where I copy the content of
> the skb to an internal buffer instead.
Sorry for the late reply, I see that you also tried a different (and
more complex) approach here:
https://patchwork.kernel.org/patch/10906011/
In my opinion the cleanest approach would be to add extra_tx_tailroom to
struct ieee80211_hw, similarly like we have extra_tx_headroom, and that
way ath10k could easily add the padding with skb_pad(). Or what do you
think?
Of course I don't know what Johannes thinks as it would need several
changes in mac80211, but at least struct net_device has needed_tailroom
as well. And I would imagine that there is some other hardware which
needs to do padding like this, or are ath10k SDIO devices really the
first mac80211 drivers needing tailroom?
--
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On Tue, 2019-10-01 at 15:21 +0300, Kalle Valo wrote:
> > > > padded_len = ath10k_sdio_calc_txrx_padded_len(ar_sdio,
> > > > skb->len);
> > > > - skb_trim(skb, padded_len);
> > > > + /* FIXME: unsure if just extending the skb len is the right
> > > > + * thing to do since we might read outside the skb->data
> > > > + * buffer. But we really don't want to realloc the skb just to
> > > > + * pad the length.
> > > > + */
> > > > + skb->len = padded_len;
> > >
> > > Good catch! But I don't think you can modify skb->len directly like
> > > that. There is skb_pad() but that doesn't change skb->len, so that most
> > > likely needs more changes. So maybe skb_put() is the safest here?
This seems unsafe to me - if you don't have any tailroom, then you'll
end up sending data to the device that's not really for the device, or
depending on how all this is allocated you might even fault later
because of sdio_memcpy_toio(..., ..., skb->data, skb->len)...
> > I have tried a few different solutions for this, but none seems to be
> > bullet proof.
> >
> > skb_pad() raises a BUG() if there is not enough space in skb->data.
As it should.
> > The best candidate so far has been skb_put_padto(). It pads and reallocates
> > the skb if needed.
> >
> > The problem is that it also cause a panic if there is more than one reference
> > to the skb (skb_shared() returns true).
As it also should :-)
> In my opinion the cleanest approach would be to add extra_tx_tailroom to
> struct ieee80211_hw, similarly like we have extra_tx_headroom, and that
> way ath10k could easily add the padding with skb_pad(). Or what do you
> think?
I disagree, adding tailroom to the SKB just for padding would be
useless...
Probably all you really have to do is this:
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -1485,11 +1485,10 @@ static int ath10k_sdio_hif_tx_sg(struct ath10k *ar, u8 pipe_id,
skb = items[i].transfer_context;
padded_len = ath10k_sdio_calc_txrx_padded_len(ar_sdio,
skb->len);
- skb_trim(skb, padded_len);
/* Write TX data to the end of the mbox address space */
address = ar_sdio->mbox_addr[eid] + ar_sdio->mbox_size[eid] -
- skb->len;
+ padded_len;
ret = ath10k_sdio_prep_async_req(ar, address, skb,
NULL, true, eid);
if (ret)
since the device evidently doesn't care what's in the pad bytes, so it
can just stay as is inside its own memory?
johannes