DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3324B22507
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 99C2160B7A
From:   Kalle Valo <kvalo@codeaurora.org>
To:     David Miller <davem@davemloft.net>
Cc:     netdev@vger.kernel.org, linux-wireless@vger.kernel.org
Subject: Re: [PATCH] brcmfmac: Use standard SKB list accessors in brcmf_sdiod_sglist_rw.
References: <20181110.163402.130407398146253939.davem@davemloft.net>
Date:   Tue, 13 Nov 2018 13:19:22 +0200
In-Reply-To: <20181110.163402.130407398146253939.davem@davemloft.net> (David
        Miller's message of "Sat, 10 Nov 2018 16:34:02 -0800 (PST)")
Message-ID: <87pnv99qdh.fsf@purkki.adurom.net>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.4 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

David Miller <davem@davemloft.net> writes:

> [ 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@davemloft.net>

Looks good to me, thanks.

Acked-by: Kalle Valo <kvalo@codeaurora.org>

-- 
Kalle Valo