From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755318AbcGFTku (ORCPT ); Wed, 6 Jul 2016 15:40:50 -0400 Received: from comal.ext.ti.com ([198.47.26.152]:55132 "EHLO comal.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754599AbcGFTkr (ORCPT ); Wed, 6 Jul 2016 15:40:47 -0400 Subject: Re: [RFC] [PATCH v2 1/3] scatterlist: Add support to clone scatterlist To: Andy Shevchenko References: <1467039249-7816-1-git-send-email-fcooper@ti.com> <1467039249-7816-2-git-send-email-fcooper@ti.com> <577D3B5C.7080805@ti.com> CC: , Jens Axboe , Andrew Morton , Ming Lin , "linux-kernel@vger.kernel.org" , Mark Brown , linux-spi , Sekhar Nori , Peter Ujfalusi , Robert Jarzmik From: "Franklin S Cooper Jr." Message-ID: <577D5E78.2010307@ti.com> Date: Wed, 6 Jul 2016 14:39:36 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/06/2016 12:46 PM, Andy Shevchenko wrote: > On Wed, Jul 6, 2016 at 8:09 PM, Franklin S Cooper Jr. wrote: > >>>> + last_entry = sg_is_last(sgl); >>>> + *sgl = *orig_sgl; >>>> + >>>> + /* >>>> + * If page_link is pointing to a chained sgl then set it to >>>> + * zero since we already compensated for chained sgl. If >>>> + * page_link is pointing to a page then clear bits 1 and 0. >>>> + */ >>>> + if (sg_is_chain(orig_sgl)) >>>> + sgl->page_link = 0x0; >>>> + else >>>> + sgl->page_link &= ~0x03; >>>> + >>> >>>> + if (last_entry) { >>>> + sg_dma_len(sgl) = len; >>> >>> Hmm... If it's not DMA mapped and CONFIG_NEED_SG_DMA_LENGTH=y, you >>> will set dma_length field and len otherwise. Is it on purpose? >> >> Thanks for pointing this out. I didn't take into consideration the above >> scenario. >> >> After looking at this more it seems like on occasion dma_length != >> length. I see this occurring in various map_sg functions for several >> architectures. >> >> I'm simply trying to reduce the overall sgl length by a certain amount. >> I'm not sure what the proper approach would be since dma_length and >> length can be set to different values. Simply setting sgl->dma_length to >> sgl->length or vice a versa seems like it can be problematic in some >> scenarios. What confuses me even more is if dma_length != length then >> how can sg_nents_for_len only use the sgl length property. >> >> Any thoughts on what would be the best way to handle this? > > First come to my mind is to refuse to clone if SG table is DMA mapped. > > Imagine the scenario where you have for example last entry which your > caller asked to split as > > total_entry_len = 37235 (input) > dma_length = 65536 (64k alignment) Do you know of an example where dma_length will be set to some kind of alignment size rather than based on the number of bytes you want transfered? Or maybe I'm miss understanding this. > > asked_len = 32768 (Split chunks: 32768 + 4467) > resulting_dma_len = ? > > What do you put here? Initially it perhaps means the same 64k. But how > to distinguish? > (I couldn't come with better one, but there are many alike scenarios) > > Only hack we have is to supply struct device of the DMA device to this > SG table where you can get max segment size (but I dunno about > alignment). > > P.S. I'm not familiar of the details of all these. > Unfortunately, for the original purpose of this series the scatterlist I want to clone is indeed DMA mapped. The times I've seen dma_length != length is when the dma_map_sg is trying to combine contiguous sgl (ex ppc_iommu_map_sg and dma_map_cont). So maybe it is better to subtract a value from length and dma_length rather than explicitly setting it to a value. This way it wouldn't matter that dma_length and length aren't equal. This looks like a similar approach used by sg_split_mapped. Although sg_split skips first x number of bytes while I want to skip the last x number of bytes. Maybe Robert can add his thoughts since he created sg_split.