From mboxrd@z Thu Jan 1 00:00:00 1970 From: Arend van Spriel Subject: Re: [PATCH] brcmfmac: Use standard SKB list accessors in brcmf_sdiod_sglist_rw. Date: Tue, 13 Nov 2018 22:07:09 +0100 Message-ID: <60f8f1c2-2b20-be60-e248-13dfcc58298d@broadcom.com> References: <20181110.163402.130407398146253939.davem@davemloft.net> <87pnv99qdh.fsf@purkki.adurom.net> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, linux-wireless@vger.kernel.org To: Kalle Valo , David Miller Return-path: Received: from mail-yb1-f182.google.com ([209.85.219.182]:37684 "EHLO mail-yb1-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725748AbeKNHHK (ORCPT ); Wed, 14 Nov 2018 02:07:10 -0500 Received: by mail-yb1-f182.google.com with SMTP id d18-v6so6014466yba.4 for ; Tue, 13 Nov 2018 13:07:13 -0800 (PST) In-Reply-To: <87pnv99qdh.fsf@purkki.adurom.net> Sender: netdev-owner@vger.kernel.org List-ID: On 11/13/2018 12:19 PM, Kalle Valo wrote: > David Miller 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 > > Looks good to me, thanks. Looks good to me too. However, I currently do not have the hardware at hands to give it a run for its money. I would prefer to have a tested-by tag. May take me a couple of days to revive a setup. Regards, Arend