| Message ID | 20190628141550.22938-1-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | driver core: platform: Allow using a dedicated dma_mask for platform_device | expand |
On Fri, Jun 28, 2019 at 08:59:46AM -0700, Christoph Hellwig wrote: > I'd much rather bite the bullet and make dev->dma_mask a scalar > instead of a pointer. The pointer causes way to much boiler plate code, > and the semantics are way to subtile. Below is a POV patch that > compiles and boots with my usual x86 test config, and at least compiles > with the arm and pmac32 defconfigs. It probably breaks just about > everything else, but should give us an idea what is involve in the > switch: > > --- > >From ea73ba2d29f56ff6413066b10f018a671f2b26ac Mon Sep 17 00:00:00 2001 > From: Christoph Hellwig <hch@lst.de> > Date: Fri, 28 Jun 2019 16:24:01 +0200 > Subject: device.h: make dma_mask a scalar instead of a pointer > > Kill the dma_mask indirection to clean up the mess we acquired around > it. I have no objection to this at all. I would love to see the indirection go away. thanks, greg k-h
Hi Christoph, On Fri, 28 Jun 2019 08:59:46 -0700 Christoph Hellwig <hch@infradead.org> wrote: >I'd much rather bite the bullet and make dev->dma_mask a scalar >instead of a pointer. The pointer causes way to much boiler plate code, >and the semantics are way to subtile. I agree that this the real solution, it just seemed a bit overwhelming to me. I'll be happy to help with this though, now that you took a big first step. > Below is a POV patch that >compiles and boots with my usual x86 test config, and at least compiles >with the arm and pmac32 defconfigs. It probably breaks just about >everything else, but should give us an idea what is involve in the >switch: I'll test that on my boards too. Thanks, Maxime
On Mon, Jul 01, 2019 at 01:24:39PM +0200, Maxime Chevallier wrote: > I agree that this the real solution, it just seemed a bit overwhelming > to me. I'll be happy to help with this though, now that you took a big > first step. I think the first step is to resurrect my original patch to default to a 32-bit DMA mask for platform devices, as that will cut a lot of crap from the platform device declarations. IIRC the problem back then was that USB uses the fact that a DMA mask exist to decide if it uses a DMA vs PIO path in the HCD core. So I'll need some help from Greg or other USB folks to clean that up, after that we can try to apply my patch again (preferably early in the next merge window), and once that sticks clean up all the 32-bit dma mask initialization for platform devices, and then turn the dma_mask into a scalar.
diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 4d1729853d1a..35e7bdb8576c 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -256,9 +256,26 @@ EXPORT_SYMBOL_GPL(platform_add_devices); struct platform_object { struct platform_device pdev; + u64 dma_mask; char name[]; }; +/** + * platform_device_setup_dma_mask - Sets the dma_mask pointer + * @pdev: platform device to configure the device's mask + * + * Sets the dma_mask of the underlying device to point to a dedicated region, + * that belongs to the platform_device. + */ +void platform_device_setup_dma_mask(struct platform_device *pdev) +{ + struct platform_object *pa = container_of(pdev, struct platform_object, + pdev); + + pa->pdev.dev.dma_mask = &pa->dma_mask; +} +EXPORT_SYMBOL_GPL(platform_device_setup_dma_mask); + /** * platform_device_put - destroy a platform device * @pdev: platform device to free diff --git a/drivers/of/platform.c b/drivers/of/platform.c index 04ad312fd85b..4a6980e3356c 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -186,8 +186,11 @@ static struct platform_device *of_platform_device_create_pdata( goto err_clear_flag; dev->dev.coherent_dma_mask = DMA_BIT_MASK(32); - if (!dev->dev.dma_mask) - dev->dev.dma_mask = &dev->dev.coherent_dma_mask; + if (!dev->dev.dma_mask) { + platform_device_setup_dma_mask(dev); + *dev->dev.dma_mask = dev->dev.coherent_dma_mask; + } + dev->dev.bus = &platform_bus_type; dev->dev.platform_data = platform_data; of_msi_configure(&dev->dev, dev->dev.of_node); diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h index cc464850b71e..a95c2d224de9 100644 --- a/include/linux/platform_device.h +++ b/include/linux/platform_device.h @@ -181,6 +181,7 @@ extern int platform_device_add_properties(struct platform_device *pdev, extern int platform_device_add(struct platform_device *pdev); extern void platform_device_del(struct platform_device *pdev); extern void platform_device_put(struct platform_device *pdev); +extern void platform_device_setup_dma_mask(struct platform_device *pdev); struct platform_driver { int (*probe)(struct platform_device *);
This patch attempts to solve a long standing situation where dev->dma_mask is a pointer to dev->dma_coherent_mask, meaning that any change to the coherent mask will affect the streaming mask. The API allows to use different values for both masks, but for now platform_device built from DT will simply make the dma_mask point to the coherent mask. This is a problem on a least one driver, the PPv2 network driver, which needs different streaming and coherent masks to overcome a HW limitation. In this case, the issue is a performance degradation since the streaming mask isn't as big as it ought to be, causing a lot of buffer bounces. There were previous attempts to fix this issue. One of them by Brian Brooks, where the dma_mask is reallocated in the driver itself, which wasn't considered to be the best approach : https://lore.kernel.org/netdev/20180820024730.9147-1-brian.brooks@linaro.org/ This lead to a discussion pointing to another attempt to solve the issue, by Christoph Hellwig : https://lore.kernel.org/lkml/20180829062401.8701-2-hch@lst.de/ This more generic approach ended-up causing regressions on some mfd drivers (the sm501 was one of the reports). The current patch tries to be a bit less generic, and allows setting-up the dma_mask for platform devices using a dedicated helper. In this case, the dma_mask is allocated in struct platform_object, as suggested by Russell King. This helper is then used in platform_device creation code from the DT. Suggested-by: Russell King <rmk+kernel@armlinux.org.uk> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> --- Hi everyone, This patch, if suitable, would require a lot of testing to detect drivers that rely on the streaming mask being the same as the coherent mask. Thanks, Maxime drivers/base/platform.c | 17 +++++++++++++++++ drivers/of/platform.c | 7 +++++-- include/linux/platform_device.h | 1 + 3 files changed, 23 insertions(+), 2 deletions(-)