DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org EDC0D223DD
Subject: Re: [PATCH] brcmfmac: Use standard SKB list accessors in
 brcmf_sdiod_sglist_rw.
To:     Kalle Valo <kvalo@codeaurora.org>,
        David Miller <davem@davemloft.net>
References: <20181110.163402.130407398146253939.davem@davemloft.net>
 <87pnv99qdh.fsf@purkki.adurom.net>
Cc:     netdev@vger.kernel.org, linux-wireless@vger.kernel.org
From:   Arend van Spriel <arend.vanspriel@broadcom.com>
Message-ID: <60f8f1c2-2b20-be60-e248-13dfcc58298d@broadcom.com>
Date:   Tue, 13 Nov 2018 22:07:09 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <87pnv99qdh.fsf@purkki.adurom.net>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On 11/13/2018 12:19 PM, Kalle Valo wrote:
> 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.

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