From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757328AbcFAFgt (ORCPT ); Wed, 1 Jun 2016 01:36:49 -0400 Received: from mailout1.w1.samsung.com ([210.118.77.11]:23914 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753965AbcFAFgr (ORCPT ); Wed, 1 Jun 2016 01:36:47 -0400 X-AuditID: cbfec7f4-f796c6d000001486-63-574e746c2eb3 Subject: Re: [RFC v2] dma-mapping: Use unsigned long for dma_attrs To: Christoph Hellwig References: <1464609246-6948-1-git-send-email-k.kozlowski@samsung.com> <1464609246-6948-2-git-send-email-k.kozlowski@samsung.com> <20160531170420.GB25366@infradead.org> Cc: Russell King , Catalin Marinas , Will Deacon , Joerg Roedel , Andrew Morton , Marek Szyprowski , Michal Hocko , Mel Gorman , Arnd Bergmann , Andy Lutomirski , linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, xen-devel@lists.xenproject.org, dri-devel@lists.freedesktop.org, linux-samsung-soc@vger.kernel.org, iommu@lists.linux-foundation.org, sstabellini@kernel.org, Bartlomiej Zolnierkiewicz From: Krzysztof Kozlowski Message-id: <574E746A.2030806@samsung.com> Date: Wed, 01 Jun 2016 07:36:42 +0200 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: <20160531170420.GB25366@infradead.org> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrDIsWRmVeSWpSXmKPExsVy+t/xq7o5JX7hBgdvsFrMWb+GzeLvpGPs FhtnrGe1eL+sh9Hiytf3bBanJyxisliw39qic/YGdovXLwwtNj2+xmqxsG0Ji8XlXXPYLGac 38dkcWjqXkaL87vWslqsPXKX3WLHUqDY/T4Hi9Xr4i1efjzBYvF9y2QmB1GPJwfnMXmsmbeG 0ePytYvMHr9/TWL02LxCy2PTqk42jxMzfrN43O8+zuSxeUm9x+Qbyxk9Dn+4wuLRt2UVo8f6 LVdZPLb+svP4vEkugD+KyyYlNSezLLVI3y6BK+POq/tMBZuFK97uW8LawLiFv4uRk0NCwETi yKFHzBC2mMSFe+vZuhi5OIQEljJKXLvVwgLhPGOUeNw9lxGkSljASWL7yU2sILaIgKbEreXt zBBFaxgljv9+DdbOLLCfVaLvwGqwKjYBY4nNy5cAJTg4eAW0JO4ddAAJswioSvSd+gO2WlQg QmLW9h9MIDavgKDEj8n3WEBsTqDWjo9zGEFamQX0JO5f1AIJMwvIS2xe85Z5AqPALCQdsxCq ZiGpWsDIvIpRNLU0uaA4KT3XUK84Mbe4NC9dLzk/dxMjJI6/7GBcfMzqEKMAB6MSD6/CRd9w IdbEsuLK3EOMEhzMSiK8H/L9woV4UxIrq1KL8uOLSnNSiw8xSnOwKInzzt31PkRIID2xJDU7 NbUgtQgmy8TBKdXA6CzyIu59a92THdOPuzPK3999dMZJa0exP9NSXwcW7Jb4sEBA8kLXtMl/ q0x39axqt7wpcvqipPbd7xwuJW7z1j096CXTGXygcFGxmeXL/6yOPvKhU7fEbQxTazyod/tr j0bm5JM7Y8Tef3sScdr2Q+8VXb2Ibt3VVtL59x49c7RNv3/NtSn8lRJLcUaioRZzUXEiABWW 8+nfAgAA Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/31/2016 07:04 PM, Christoph Hellwig wrote: > On Mon, May 30, 2016 at 01:54:06PM +0200, Krzysztof Kozlowski wrote: >> The dma-mapping core and the implementations do not change the >> DMA attributes passed by pointer. Thus the pointer can point to const >> data. However the attributes do not have to be a bitfield. Instead >> unsigned long will do fine: >> >> 1. This is just simpler. Both in terms of reading the code and setting >> attributes. Instead of initializing local attributes on the stack and >> passing pointer to it to dma_set_attr(), just set the bits. >> >> 2. It brings safeness and checking for const correctness because the >> attributes are passed by value. >> >> Please have in mind that this is RFC, not finished yet. Only ARM and >> ARM64 are fixed (and not everywhere). >> However other API users also have to be converted which is quite >> intrusive. I would rather avoid it until the overall approach is >> accepted. > > This looks great! Please continue doing the full conversion. > >> +/** >> + * List of possible attributes associated with a DMA mapping. The semantics >> + * of each attribute should be defined in Documentation/DMA-attributes.txt. >> + */ >> +#define DMA_ATTR_WRITE_BARRIER BIT(1) >> +#define DMA_ATTR_WEAK_ORDERING BIT(2) >> +#define DMA_ATTR_WRITE_COMBINE BIT(3) >> +#define DMA_ATTR_NON_CONSISTENT BIT(4) >> +#define DMA_ATTR_NO_KERNEL_MAPPING BIT(5) >> +#define DMA_ATTR_SKIP_CPU_SYNC BIT(6) >> +#define DMA_ATTR_FORCE_CONTIGUOUS BIT(7) >> +#define DMA_ATTR_ALLOC_SINGLE_PAGES BIT(8) > > No really for this patch, but I would much prefer to document them next > to the code in the long run. Also I really think these BIT() macros > are a distraction compared to the (1 << N) notation. Not much difference to me but maybe plain number: ... 0x01u ... 0x02u ? > >> +/** >> + * dma_get_attr - check for a specific attribute >> + * @attr: attribute to look for >> + * @attrs: attributes to check within >> + */ >> +static inline bool dma_get_attr(unsigned long attr, unsigned long attrs) >> +{ >> + return !!(attr & attrs); >> +} > > I'd just kill this helper, much easier to simply open code it in the > caller. Keeping it for now helps reducing the number of changes in the patch. The patch will be quite big as it has to replace all the uses atomically. I can get rid of the helper in consecutive patch. Best regards, Krzysztof