From mboxrd@z Thu Jan 1 00:00:00 1970 From: Benjamin Herrenschmidt Date: Sun, 18 Jun 2017 09:54:02 +0000 Subject: Re: [PATCH 42/44] powerpc/cell: use the dma_supported method for ops switching Message-Id: <1497779642.31581.6.camel@kernel.crashing.org> List-Id: References: <20170616181059.19206-1-hch@lst.de> <20170616181059.19206-43-hch@lst.de> <1497732627.2897.128.camel@kernel.crashing.org> <20170618071344.GB18526@infradead.org> In-Reply-To: <20170618071344.GB18526@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Christoph Hellwig Cc: linux-mips@linux-mips.org, linux-samsung-soc@vger.kernel.org, linux-ia64@vger.kernel.org, linux-c6x-dev@linux-c6x.org, linux-s390@vger.kernel.org, linux-sh@vger.kernel.org, linux-hexagon@vger.kernel.org, linux-xtensa@linux-xtensa.org, x86@kernel.org, linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, dmaengine@vger.kernel.org, iommu@lists.linux-foundation.org, openrisc@lists.librecores.org, netdev@vger.kernel.org, sparclinux@vger.kernel.org, xen-devel@lists.xenproject.org, linuxppc-dev@lists.ozlabs.org, Christoph Hellwig , linux-arm-kernel@lists.infradead.org On Sun, 2017-06-18 at 00:13 -0700, Christoph Hellwig wrote: > On Sun, Jun 18, 2017 at 06:50:27AM +1000, Benjamin Herrenschmidt wrote: > > What is your rationale here ? (I have missed patch 0 it seems). > > Less code duplication, more modular dma_map_ops insteance. > > > dma_supported() was supposed to be pretty much a "const" function > > simply informing whether a given setup is possible. Having it perform > > an actual switch of ops seems to be pushing it... > > dma_supported() is already gone from the public DMA API as it doesn't > make sense to be called separately from set_dma_mask. It will be > entirely gone in the next series after this one. Ah ok, in that case it makes much more sense, we can rename it then. > > What if a driver wants to test various dma masks and then pick one ? > > > > Where does the API documents that if a driver calls dma_supported() it > > then *must* set the corresponding mask and use that ? > > Where is the API document for _any_ of the dma routines? (A: work in > progress by me, but I need to clean up the mess of arch hooks before > it can make any sense) Heh fair enough. > > I don't like a function that is a "boolean query" like this one to have > > such a major side effect. > > > > > From an API standpoint, dma_set_mask() is when the mask is established, > > > > and thus when the ops switch should happen. > > And that's exactly what happens at the driver API level. It just turns > out the dma_capable method is they way better place to actually > implement it, as the ->set_dma_mask method requires lots of code > duplication while not offering any actual benefit over ->dma_capable. > And because of that it's gone after this series. > > In theory we could rename ->dma_capable now, but it would require a > _lot_ of churn. Give me another merge window or two and we should > be down to be about 2 handful of dma_map_ops instance, at which point > we could do all this gratious renaming a lot more easily :) Sure, I get it now, as long as it's not publicly exposed to drivers we are fine. Cheers, Ben. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Benjamin Herrenschmidt Subject: Re: [PATCH 42/44] powerpc/cell: use the dma_supported method for ops switching Date: Sun, 18 Jun 2017 19:54:02 +1000 Message-ID: <1497779642.31581.6.camel@kernel.crashing.org> References: <20170616181059.19206-1-hch@lst.de> <20170616181059.19206-43-hch@lst.de> <1497732627.2897.128.camel@kernel.crashing.org> <20170618071344.GB18526@infradead.org> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20170618071344.GB18526@infradead.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Christoph Hellwig Cc: linux-mips@linux-mips.org, linux-samsung-soc@vger.kernel.org, linux-ia64@vger.kernel.org, linux-c6x-dev@linux-c6x.org, linux-s390@vger.kernel.org, linux-sh@vger.kernel.org, linux-hexagon@vger.kernel.org, linux-xtensa@linux-xtensa.org, x86@kernel.org, linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, dmaengine@vger.kernel.org, iommu@lists.linux-foundation.org, openrisc@lists.librecores.org, netdev@vger.kernel.org, sparclinux@vger.kernel.org, xen-devel@lists.xenproject.org, linuxppc-dev@lists.ozlabs.org, Christoph Hellwig , linux-arm-kernel@lists.infradead.org List-Id: linux-tegra@vger.kernel.org T24gU3VuLCAyMDE3LTA2LTE4IGF0IDAwOjEzIC0wNzAwLCBDaHJpc3RvcGggSGVsbHdpZyB3cm90 ZToKPiBPbiBTdW4sIEp1biAxOCwgMjAxNyBhdCAwNjo1MDoyN0FNICsxMDAwLCBCZW5qYW1pbiBI ZXJyZW5zY2htaWR0IHdyb3RlOgo+ID4gV2hhdCBpcyB5b3VyIHJhdGlvbmFsZSBoZXJlID8gKEkg aGF2ZSBtaXNzZWQgcGF0Y2ggMCBpdCBzZWVtcykuCj4gCj4gTGVzcyBjb2RlIGR1cGxpY2F0aW9u LCBtb3JlIG1vZHVsYXIgZG1hX21hcF9vcHMgaW5zdGVhbmNlLgo+IAo+ID4gZG1hX3N1cHBvcnRl ZCgpIHdhcyBzdXBwb3NlZCB0byBiZSBwcmV0dHkgbXVjaCBhICJjb25zdCIgZnVuY3Rpb24KPiA+ IHNpbXBseSBpbmZvcm1pbmcgd2hldGhlciBhIGdpdmVuIHNldHVwIGlzIHBvc3NpYmxlLiBIYXZp bmcgaXQgcGVyZm9ybQo+ID4gYW4gYWN0dWFsIHN3aXRjaCBvZiBvcHMgc2VlbXMgdG8gYmUgcHVz aGluZyBpdC4uLgo+IAo+IGRtYV9zdXBwb3J0ZWQoKSBpcyBhbHJlYWR5IGdvbmUgZnJvbSB0aGUg cHVibGljIERNQSBBUEkgYXMgaXQgZG9lc24ndAo+IG1ha2Ugc2Vuc2UgdG8gYmUgY2FsbGVkIHNl cGFyYXRlbHkgZnJvbSBzZXRfZG1hX21hc2suICBJdCB3aWxsIGJlCj4gZW50aXJlbHkgZ29uZSBp biB0aGUgbmV4dCBzZXJpZXMgYWZ0ZXIgdGhpcyBvbmUuCgpBaCBvaywgaW4gdGhhdCBjYXNlIGl0 IG1ha2VzIG11Y2ggbW9yZSBzZW5zZSwgd2UgY2FuIHJlbmFtZSBpdCB0aGVuLgoKPiA+IFdoYXQg aWYgYSBkcml2ZXIgd2FudHMgdG8gdGVzdCB2YXJpb3VzIGRtYSBtYXNrcyBhbmQgdGhlbiBwaWNr IG9uZSA/Cj4gPiAKPiA+IFdoZXJlIGRvZXMgdGhlIEFQSSBkb2N1bWVudHMgdGhhdCBpZiBhIGRy aXZlciBjYWxscyBkbWFfc3VwcG9ydGVkKCkgaXQKPiA+IHRoZW4gKm11c3QqIHNldCB0aGUgY29y cmVzcG9uZGluZyBtYXNrIGFuZCB1c2UgdGhhdCA/Cj4gCj4gV2hlcmUgaXMgdGhlIEFQSSBkb2N1 bWVudCBmb3IgX2FueV8gb2YgdGhlIGRtYSByb3V0aW5lcz8gKEE6IHdvcmsgaW4KPiBwcm9ncmVz cyBieSBtZSwgYnV0IEkgbmVlZCB0byBjbGVhbiB1cCB0aGUgbWVzcyBvZiBhcmNoIGhvb2tzIGJl Zm9yZQo+IGl0IGNhbiBtYWtlIGFueSBzZW5zZSkKCkhlaCBmYWlyIGVub3VnaC4KCj4gPiBJIGRv bid0IGxpa2UgYSBmdW5jdGlvbiB0aGF0IGlzIGEgImJvb2xlYW4gcXVlcnkiIGxpa2UgdGhpcyBv bmUgdG8gaGF2ZQo+ID4gc3VjaCBhIG1ham9yIHNpZGUgZWZmZWN0Lgo+ID4gCj4gPiA+IEZyb20g YW4gQVBJIHN0YW5kcG9pbnQsIGRtYV9zZXRfbWFzaygpIGlzIHdoZW4gdGhlIG1hc2sgaXMgZXN0 YWJsaXNoZWQsCj4gPiAKPiA+IGFuZCB0aHVzIHdoZW4gdGhlIG9wcyBzd2l0Y2ggc2hvdWxkIGhh cHBlbi4KPiAKPiBBbmQgdGhhdCdzIGV4YWN0bHkgd2hhdCBoYXBwZW5zIGF0IHRoZSBkcml2ZXIg QVBJIGxldmVsLiAgSXQganVzdCB0dXJucwo+IG91dCB0aGUgZG1hX2NhcGFibGUgbWV0aG9kIGlz IHRoZXkgd2F5IGJldHRlciBwbGFjZSB0byBhY3R1YWxseQo+IGltcGxlbWVudCBpdCwgYXMgdGhl IC0+c2V0X2RtYV9tYXNrIG1ldGhvZCByZXF1aXJlcyBsb3RzIG9mIGNvZGUKPiBkdXBsaWNhdGlv biB3aGlsZSBub3Qgb2ZmZXJpbmcgYW55IGFjdHVhbCBiZW5lZml0IG92ZXIgLT5kbWFfY2FwYWJs ZS4KPiBBbmQgYmVjYXVzZSBvZiB0aGF0IGl0J3MgZ29uZSBhZnRlciB0aGlzIHNlcmllcy4KPiAK PiBJbiB0aGVvcnkgd2UgY291bGQgcmVuYW1lIC0+ZG1hX2NhcGFibGUgbm93LCBidXQgaXQgd291 bGQgcmVxdWlyZSBhCj4gX2xvdF8gb2YgY2h1cm4uICBHaXZlIG1lIGFub3RoZXIgbWVyZ2Ugd2lu ZG93IG9yIHR3byBhbmQgd2Ugc2hvdWxkCj4gYmUgZG93biB0byBiZSBhYm91dCAyIGhhbmRmdWwg b2YgZG1hX21hcF9vcHMgaW5zdGFuY2UsIGF0IHdoaWNoIHBvaW50Cj4gd2UgY291bGQgZG8gYWxs IHRoaXMgZ3JhdGlvdXMgcmVuYW1pbmcgYSBsb3QgbW9yZSBlYXNpbHkgOikKClN1cmUsIEkgZ2V0 IGl0IG5vdywgYXMgbG9uZyBhcyBpdCdzIG5vdCBwdWJsaWNseSBleHBvc2VkIHRvIGRyaXZlcnMK d2UgYXJlIGZpbmUuCgpDaGVlcnMsCkJlbi4KCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3Rz LmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xp c3RpbmZvL2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752797AbdFRJz5 (ORCPT ); Sun, 18 Jun 2017 05:55:57 -0400 Received: from gate.crashing.org ([63.228.1.57]:57180 "EHLO gate.crashing.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751009AbdFRJzx (ORCPT ); Sun, 18 Jun 2017 05:55:53 -0400 Message-ID: <1497779642.31581.6.camel@kernel.crashing.org> Subject: Re: [PATCH 42/44] powerpc/cell: use the dma_supported method for ops switching From: Benjamin Herrenschmidt To: Christoph Hellwig Cc: Christoph Hellwig , x86@kernel.org, linux-arm-kernel@lists.infradead.org, xen-devel@lists.xenproject.org, linux-c6x-dev@linux-c6x.org, linux-hexagon@vger.kernel.org, linux-ia64@vger.kernel.org, linux-mips@linux-mips.org, openrisc@lists.librecores.org, linuxppc-dev@lists.ozlabs.org, linux-s390@vger.kernel.org, linux-sh@vger.kernel.org, sparclinux@vger.kernel.org, linux-xtensa@linux-xtensa.org, dmaengine@vger.kernel.org, linux-tegra@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-samsung-soc@vger.kernel.org, iommu@lists.linux-foundation.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Date: Sun, 18 Jun 2017 19:54:02 +1000 In-Reply-To: <20170618071344.GB18526@infradead.org> References: <20170616181059.19206-1-hch@lst.de> <20170616181059.19206-43-hch@lst.de> <1497732627.2897.128.camel@kernel.crashing.org> <20170618071344.GB18526@infradead.org> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.6 (3.22.6-2.fc25) Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, 2017-06-18 at 00:13 -0700, Christoph Hellwig wrote: > On Sun, Jun 18, 2017 at 06:50:27AM +1000, Benjamin Herrenschmidt wrote: > > What is your rationale here ? (I have missed patch 0 it seems). > > Less code duplication, more modular dma_map_ops insteance. > > > dma_supported() was supposed to be pretty much a "const" function > > simply informing whether a given setup is possible. Having it perform > > an actual switch of ops seems to be pushing it... > > dma_supported() is already gone from the public DMA API as it doesn't > make sense to be called separately from set_dma_mask. It will be > entirely gone in the next series after this one. Ah ok, in that case it makes much more sense, we can rename it then. > > What if a driver wants to test various dma masks and then pick one ? > > > > Where does the API documents that if a driver calls dma_supported() it > > then *must* set the corresponding mask and use that ? > > Where is the API document for _any_ of the dma routines? (A: work in > progress by me, but I need to clean up the mess of arch hooks before > it can make any sense) Heh fair enough. > > I don't like a function that is a "boolean query" like this one to have > > such a major side effect. > > > > > From an API standpoint, dma_set_mask() is when the mask is established, > > > > and thus when the ops switch should happen. > > And that's exactly what happens at the driver API level. It just turns > out the dma_capable method is they way better place to actually > implement it, as the ->set_dma_mask method requires lots of code > duplication while not offering any actual benefit over ->dma_capable. > And because of that it's gone after this series. > > In theory we could rename ->dma_capable now, but it would require a > _lot_ of churn. Give me another merge window or two and we should > be down to be about 2 handful of dma_map_ops instance, at which point > we could do all this gratious renaming a lot more easily :) Sure, I get it now, as long as it's not publicly exposed to drivers we are fine. Cheers, Ben. From mboxrd@z Thu Jan 1 00:00:00 1970 From: benh@kernel.crashing.org (Benjamin Herrenschmidt) Date: Sun, 18 Jun 2017 19:54:02 +1000 Subject: [PATCH 42/44] powerpc/cell: use the dma_supported method for ops switching In-Reply-To: <20170618071344.GB18526@infradead.org> References: <20170616181059.19206-1-hch@lst.de> <20170616181059.19206-43-hch@lst.de> <1497732627.2897.128.camel@kernel.crashing.org> <20170618071344.GB18526@infradead.org> Message-ID: <1497779642.31581.6.camel@kernel.crashing.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Sun, 2017-06-18 at 00:13 -0700, Christoph Hellwig wrote: > On Sun, Jun 18, 2017 at 06:50:27AM +1000, Benjamin Herrenschmidt wrote: > > What is your rationale here ? (I have missed patch 0 it seems). > > Less code duplication, more modular dma_map_ops insteance. > > > dma_supported() was supposed to be pretty much a "const" function > > simply informing whether a given setup is possible. Having it perform > > an actual switch of ops seems to be pushing it... > > dma_supported() is already gone from the public DMA API as it doesn't > make sense to be called separately from set_dma_mask. It will be > entirely gone in the next series after this one. Ah ok, in that case it makes much more sense, we can rename it then. > > What if a driver wants to test various dma masks and then pick one ? > > > > Where does the API documents that if a driver calls dma_supported() it > > then *must* set the corresponding mask and use that ? > > Where is the API document for _any_ of the dma routines? (A: work in > progress by me, but I need to clean up the mess of arch hooks before > it can make any sense) Heh fair enough. > > I don't like a function that is a "boolean query" like this one to have > > such a major side effect. > > > > > From an API standpoint, dma_set_mask() is when the mask is established, > > > > and thus when the ops switch should happen. > > And that's exactly what happens at the driver API level. It just turns > out the dma_capable method is they way better place to actually > implement it, as the ->set_dma_mask method requires lots of code > duplication while not offering any actual benefit over ->dma_capable. > And because of that it's gone after this series. > > In theory we could rename ->dma_capable now, but it would require a > _lot_ of churn. Give me another merge window or two and we should > be down to be about 2 handful of dma_map_ops instance, at which point > we could do all this gratious renaming a lot more easily :) Sure, I get it now, as long as it's not publicly exposed to drivers we are fine. Cheers, Ben.