From: David Miller <davem@xxxxxxxxxxxxx> Sent: 2018年11月11日 8:34
> [ As I am trying to remove direct SKB list pointer accesses I am
> committing this to net-next. If this causes a lot of grief I
> can and will revert, just let me know. ]
>
> Instead of direct SKB list pointer accesses.
>
> The loops in this function had to be rewritten to accommodate this more
> easily.
>
> The first loop iterates now over the target list in the outer loop, and triggers
> an mmc data operation when the per-operation limits are hit.
>
> Then after the loops, if we have any residue, we trigger the last and final
> operation.
>
> For the page aligned workaround, where we have to copy the read data back
> into the original list of SKBs, we use a two-tiered loop. The outer loop stays
> the same and iterates over pktlist, and then we have an inner loop which uses
> skb_peek_next(). The break logic has been simplified because we know that
> the aggregate length of the SKBs in the source and destination lists are the
> same.
>
> This change also ends up fixing a bug, having to do with the maintainance of
> the seg_sz variable and how it drove the outermost loop. It begins as:
>
> seg_sz = target_list->qlen;
>
> ie. the number of packets in the target_list queue. The loop structure was
> then:
>
> while (seq_sz) {
> ...
> while (not at end of target_list) {
> ...
> sg_cnt++
> ...
> }
> ...
> seg_sz -= sg_cnt;
>
> The assumption built into that last statement is that sg_cnt counts how many
> packets from target_list have been fully processed by the inner loop. But
> this not true.
>
> If we hit one of the limits, such as the max segment size or the max request
> size, we will break and copy a partial packet then contine back up to the top
> of the outermost loop.
>
> With the new loops we don't have this problem as we don't guard the loop
> exit with a packet count, but instead use the progression of the pkt_next SKB
> through the list to the end. The general structure is:
>
> sg_cnt = 0;
> skb_queue_walk(target_list, pkt_next) {
> pkt_offset = 0;
> ...
> sg_cnt++;
> ...
> while (pkt_offset < pkt_next->len) {
> pkt_offset += sg_data_size;
> if (queued up max per request)
> mmc_submit_one();
> }
> }
> if (sg_cnt)
> mmc_submit_one();
>
> The variables that maintain where we are in the MMC command state such
> as req_sz, sg_cnt, and sgl are reset when we emit one of these full sized
> requests.
>
> Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>
> ---
> .../broadcom/brcm80211/brcmfmac/bcmsdh.c | 137
> ++++++++++--------
> 1 file changed, 74 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> index 3e37c8cf82c6..b2ad2122c8c4 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> @@ -342,6 +342,37 @@ static int brcmf_sdiod_skbuff_write(struct
> brcmf_sdio_dev *sdiodev,
> return err;
> }
>
> +static int mmc_submit_one(struct mmc_data *md, struct mmc_request *mr,
> + struct mmc_command *mc, int sg_cnt, int req_sz,
> + int func_blk_sz, u32 *addr,
> + struct brcmf_sdio_dev *sdiodev,
> + struct sdio_func *func, int write) {
> + int ret;
> +
> + md->sg_len = sg_cnt;
> + md->blocks = req_sz / func_blk_sz;
> + mc->arg |= (*addr & 0x1FFFF) << 9; /* address */
> + mc->arg |= md->blocks & 0x1FF; /* block count */
> + /* incrementing addr for function 1 */
> + if (func->num == 1)
> + *addr += req_sz;
> +
> + mmc_set_data_timeout(md, func->card);
> + mmc_wait_for_req(func->card->host, mr);
> +
> + ret = mc->error ? mc->error : md->error;
> + if (ret == -ENOMEDIUM) {
> + brcmf_sdiod_change_state(sdiodev, BRCMF_SDIOD_NOMEDIUM);
> + } else if (ret != 0) {
> + brcmf_err("CMD53 sg block %s failed %d\n",
> + write ? "write" : "read", ret);
> + ret = -EIO;
> + }
> +
> + return ret;
> +}
> +
> /**
> * brcmf_sdiod_sglist_rw - SDIO interface function for block data access
> * @sdiodev: brcmfmac sdio device
> @@ -360,11 +391,11 @@ static int brcmf_sdiod_sglist_rw(struct
> brcmf_sdio_dev *sdiodev,
> struct sk_buff_head *pktlist)
> {
> unsigned int req_sz, func_blk_sz, sg_cnt, sg_data_sz, pkt_offset;
> - unsigned int max_req_sz, orig_offset, dst_offset;
> - unsigned short max_seg_cnt, seg_sz;
> + unsigned int max_req_sz, src_offset, dst_offset;
> unsigned char *pkt_data, *orig_data, *dst_data;
> - struct sk_buff *pkt_next = NULL, *local_pkt_next;
> struct sk_buff_head local_list, *target_list;
> + struct sk_buff *pkt_next = NULL, *src;
> + unsigned short max_seg_cnt;
> struct mmc_request mmc_req;
> struct mmc_command mmc_cmd;
> struct mmc_data mmc_dat;
> @@ -404,9 +435,6 @@ static int brcmf_sdiod_sglist_rw(struct
> brcmf_sdio_dev *sdiodev,
> max_req_sz = sdiodev->max_request_size;
> max_seg_cnt = min_t(unsigned short, sdiodev->max_segment_count,
> target_list->qlen);
> - seg_sz = target_list->qlen;
> - pkt_offset = 0;
> - pkt_next = target_list->next;
>
> memset(&mmc_req, 0, sizeof(struct mmc_request));
> memset(&mmc_cmd, 0, sizeof(struct mmc_command)); @@ -425,12
> +453,12 @@ static int brcmf_sdiod_sglist_rw(struct brcmf_sdio_dev
> *sdiodev,
> mmc_req.cmd = &mmc_cmd;
> mmc_req.data = &mmc_dat;
>
> - while (seg_sz) {
> - req_sz = 0;
> - sg_cnt = 0;
> - sgl = sdiodev->sgtable.sgl;
> - /* prep sg table */
> - while (pkt_next != (struct sk_buff *)target_list) {
> + req_sz = 0;
> + sg_cnt = 0;
> + sgl = sdiodev->sgtable.sgl;
> + skb_queue_walk(target_list, pkt_next) {
> + pkt_offset = 0;
> + while (pkt_offset < pkt_next->len) {
> pkt_data = pkt_next->data + pkt_offset;
> sg_data_sz = pkt_next->len - pkt_offset;
> if (sg_data_sz > sdiodev->max_segment_size) @@ -439,72
> +467,55 @@ static int brcmf_sdiod_sglist_rw(struct brcmf_sdio_dev
> *sdiodev,
> sg_data_sz = max_req_sz - req_sz;
>
> sg_set_buf(sgl, pkt_data, sg_data_sz);
> -
> sg_cnt++;
> +
> sgl = sg_next(sgl);
> req_sz += sg_data_sz;
> pkt_offset += sg_data_sz;
> - if (pkt_offset == pkt_next->len) {
> - pkt_offset = 0;
> - pkt_next = pkt_next->next;
> + if (req_sz >= max_req_sz || sg_cnt >= max_seg_cnt) {
> + ret = mmc_submit_one(&mmc_dat, &mmc_req,
> &mmc_cmd,
> + sg_cnt, req_sz, func_blk_sz,
> + &addr, sdiodev, func, write);
> + if (ret)
> + goto exit_queue_walk;
> + req_sz = 0;
> + sg_cnt = 0;
> + sgl = sdiodev->sgtable.sgl;
> }
> -
> - if (req_sz >= max_req_sz || sg_cnt >= max_seg_cnt)
> - break;
> - }
> - seg_sz -= sg_cnt;
> -
> - if (req_sz % func_blk_sz != 0) {
> - brcmf_err("sg request length %u is not %u aligned\n",
> - req_sz, func_blk_sz);
> - ret = -ENOTBLK;
> - goto exit;
> - }
> -
> - mmc_dat.sg_len = sg_cnt;
> - mmc_dat.blocks = req_sz / func_blk_sz;
> - mmc_cmd.arg |= (addr & 0x1FFFF) << 9; /* address */
> - mmc_cmd.arg |= mmc_dat.blocks & 0x1FF; /* block count */
> - /* incrementing addr for function 1 */
> - if (func->num == 1)
> - addr += req_sz;
> -
> - mmc_set_data_timeout(&mmc_dat, func->card);
> - mmc_wait_for_req(func->card->host, &mmc_req);
> -
> - ret = mmc_cmd.error ? mmc_cmd.error : mmc_dat.error;
> - if (ret == -ENOMEDIUM) {
> - brcmf_sdiod_change_state(sdiodev,
> BRCMF_SDIOD_NOMEDIUM);
> - break;
> - } else if (ret != 0) {
> - brcmf_err("CMD53 sg block %s failed %d\n",
> - write ? "write" : "read", ret);
> - ret = -EIO;
> - break;
> }
> }
> -
> + if (sg_cnt)
> + ret = mmc_submit_one(&mmc_dat, &mmc_req, &mmc_cmd,
> + sg_cnt, req_sz, func_blk_sz,
> + &addr, sdiodev, func, write);
> +exit_queue_walk:
> if (!write && sdiodev->settings->bus.sdio.broken_sg_support) {
> - local_pkt_next = local_list.next;
> - orig_offset = 0;
> + src = __skb_peek(&local_list);
> + src_offset = 0;
> skb_queue_walk(pktlist, pkt_next) {
> dst_offset = 0;
> - do {
> - req_sz = local_pkt_next->len - orig_offset;
> - req_sz = min_t(uint, pkt_next->len - dst_offset,
> - req_sz);
> - orig_data = local_pkt_next->data + orig_offset;
> +
> + /* This is safe because we must have enough SKB data
> + * in the local list to cover everything in pktlist.
> + */
> + while (1) {
> + req_sz = pkt_next->len - dst_offset;
> + if (req_sz > src->len - src_offset)
> + req_sz = src->len - src_offset;
> +
> + orig_data = src->data + src_offset;
> dst_data = pkt_next->data + dst_offset;
> memcpy(dst_data, orig_data, req_sz);
> - orig_offset += req_sz;
> - dst_offset += req_sz;
> - if (orig_offset == local_pkt_next->len) {
> - orig_offset = 0;
> - local_pkt_next = local_pkt_next->next;
> +
> + src_offset += req_sz;
> + if (src_offset == src->len) {
> + src_offset = 0;
> + src = skb_peek_next(src, &local_list);
> }
> + dst_offset += req_sz;
> if (dst_offset == pkt_next->len)
> break;
> - } while (!skb_queue_empty(&local_list));
> + }
> }
> }
>
> --
> 2.19.1
I just have bcm4339 in hands, test the patch on i.MX7D sdb board with bcm4339, it works fine with iperf testing.
Tested-by: Fugang Duan <fugang.duan@xxxxxxx>
