| Message ID | 1351086561-13569-2-git-send-email-gregory.clement@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello, On 10/24/2012 3:49 PM, Gregory CLEMENT wrote: > For Armada 370/XP we have the same problem that for the commit > cb01b63, so we applied the same solution: "The default 256 KiB > coherent pool may be too small for some of the Kirkwood devices, so > increase it to make sure that devices will be able to allocate their > buffers with GFP_ATOMIC flag" > > Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> > > Cc: Marek Szyprowski <m.szyprowski@samsung.com> Acked-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > arch/arm/mach-mvebu/armada-370-xp.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c > index 2af6ce5..cbad821 100644 > --- a/arch/arm/mach-mvebu/armada-370-xp.c > +++ b/arch/arm/mach-mvebu/armada-370-xp.c > @@ -17,6 +17,7 @@ > #include <linux/of_platform.h> > #include <linux/io.h> > #include <linux/time-armada-370-xp.h> > +#include <linux/dma-mapping.h> > #include <asm/mach/arch.h> > #include <asm/mach/map.h> > #include <asm/mach/time.h> > @@ -43,6 +44,16 @@ void __init armada_370_xp_timer_and_clk_init(void) > armada_370_xp_timer_init(); > } > > +void __init armada_370_xp_init_early(void) > +{ > + /* > + * Some Armada 370/XP devices allocate their coherent buffers > + * from atomic context. Increase size of atomic coherent pool > + * to make sure such the allocations won't fail. > + */ > + init_dma_coherent_pool_size(SZ_1M); > +} > + > struct sys_timer armada_370_xp_timer = { > .init = armada_370_xp_timer_and_clk_init, > }; > @@ -61,6 +72,7 @@ static const char * const armada_370_xp_dt_board_dt_compat[] = { > DT_MACHINE_START(ARMADA_XP_DT, "Marvell Aramada 370/XP (Device Tree)") > .init_machine = armada_370_xp_dt_init, > .map_io = armada_370_xp_map_io, > + .init_early = armada_370_xp_init_early, > .init_irq = armada_370_xp_init_irq, > .handle_irq = armada_370_xp_handle_irq, > .timer = &armada_370_xp_timer, > Best regards
On Wednesday 24 October 2012, Gregory CLEMENT wrote: > For Armada 370/XP we have the same problem that for the commit > cb01b63, so we applied the same solution: "The default 256 KiB > coherent pool may be too small for some of the Kirkwood devices, so > increase it to make sure that devices will be able to allocate their > buffers with GFP_ATOMIC flag" > > Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> Do you know why the ATA driver needs this? I find it surprising that it's necessary, so I'd like to make sure we're not just working around a device driver bug here. Arnd
Arnd, On Thu, 25 Oct 2012 11:27:36 +0000, Arnd Bergmann wrote: > On Wednesday 24 October 2012, Gregory CLEMENT wrote: > > For Armada 370/XP we have the same problem that for the commit > > cb01b63, so we applied the same solution: "The default 256 KiB > > coherent pool may be too small for some of the Kirkwood devices, so > > increase it to make sure that devices will be able to allocate their > > buffers with GFP_ATOMIC flag" > > > > Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> > > Do you know why the ATA driver needs this? I find it surprising that > it's necessary, so I'd like to make sure we're not just working around > a device driver bug here. The sata_mv driver create dma_pool and allocate objects from them, and all the memory allocated for dma_pools is allocated using dma_alloc_coherent(), and I guess the driver is using too much of them. Seems like the driver is too lazy and allocates everything coherent to avoid the hassle of doing dma_map/dma_unmap operations when needed, but I haven't looked in details at the driver yet to see if it would be possible to switch those DMA coherent allocations into non-coherent allocations + appropriate calls to the DMA operations. That said, that's for sure a larger task than just enabling SATA on Armada 370/XP, so I would advocate to handle this problem separately. Best regards, Thomas
On Thursday 25 October 2012, Thomas Petazzoni wrote: > On Thu, 25 Oct 2012 11:27:36 +0000, Arnd Bergmann wrote: > > On Wednesday 24 October 2012, Gregory CLEMENT wrote: > > > For Armada 370/XP we have the same problem that for the commit > > > cb01b63, so we applied the same solution: "The default 256 KiB > > > coherent pool may be too small for some of the Kirkwood devices, so > > > increase it to make sure that devices will be able to allocate their > > > buffers with GFP_ATOMIC flag" > > > > > > Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> > > > > Do you know why the ATA driver needs this? I find it surprising that > > it's necessary, so I'd like to make sure we're not just working around > > a device driver bug here. > > The sata_mv driver create dma_pool and allocate objects from them, and > all the memory allocated for dma_pools is allocated using > dma_alloc_coherent(), and I guess the driver is using too much of them. > > Seems like the driver is too lazy and allocates everything coherent to > avoid the hassle of doing dma_map/dma_unmap operations when needed, but > I haven't looked in details at the driver yet to see if it would be > possible to switch those DMA coherent allocations into non-coherent > allocations + appropriate calls to the DMA operations. Using coherent allocations is fine, I was wondering whether they need to be atomic or not. > That said, that's for sure a larger task than just enabling SATA on > Armada 370/XP, so I would advocate to handle this problem separately. Agreed. Arnd
On Thu, 25 Oct 2012 13:46:41 +0000, Arnd Bergmann wrote: > > Seems like the driver is too lazy and allocates everything coherent > > to avoid the hassle of doing dma_map/dma_unmap operations when > > needed, but I haven't looked in details at the driver yet to see if > > it would be possible to switch those DMA coherent allocations into > > non-coherent allocations + appropriate calls to the DMA operations. > > Using coherent allocations is fine, I was wondering whether they need > to be atomic or not. You're raising a good point here: all the dma_pool_alloc() allocations done by sata_mv are GFP_KERNEL. So why are we having problems with the /atomic/ coherent pool size? Is it the libata core that's doing GFP_ATOMIC DMA coherent allocations. It doesn't seem so. Something's odd. Thomas
Hello, On 10/25/2012 3:46 PM, Arnd Bergmann wrote: > On Thursday 25 October 2012, Thomas Petazzoni wrote: >> On Thu, 25 Oct 2012 11:27:36 +0000, Arnd Bergmann wrote: >>> On Wednesday 24 October 2012, Gregory CLEMENT wrote: >>>> For Armada 370/XP we have the same problem that for the commit >>>> cb01b63, so we applied the same solution: "The default 256 KiB >>>> coherent pool may be too small for some of the Kirkwood devices, so >>>> increase it to make sure that devices will be able to allocate their >>>> buffers with GFP_ATOMIC flag" >>>> >>>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> >>> >>> Do you know why the ATA driver needs this? I find it surprising that >>> it's necessary, so I'd like to make sure we're not just working around >>> a device driver bug here. >> >> The sata_mv driver create dma_pool and allocate objects from them, and >> all the memory allocated for dma_pools is allocated using >> dma_alloc_coherent(), and I guess the driver is using too much of them. >> >> Seems like the driver is too lazy and allocates everything coherent to >> avoid the hassle of doing dma_map/dma_unmap operations when needed, but >> I haven't looked in details at the driver yet to see if it would be >> possible to switch those DMA coherent allocations into non-coherent >> allocations + appropriate calls to the DMA operations. > > Using coherent allocations is fine, I was wondering whether they need > to be atomic or not. I've checked the code and it turned out that ALL allocations from dma_pool are made with GFP_ATOMIC flag. GFP flags passed from caller are simply ignored, see mm/dmapool.c dma_pool_alloc() function. This looks like an ugly hack or workaround from ancient times, because dma_pool_alloc() tries to implement __GFP_WAIT style approach on it's own. I've tried to find the reason for such approach, but it looks that this code comes from pre-git times. IMHO this strange code inside dma_pool_alloc() should be replaced with simple call to pool_alloc_page(pool, mem_flags) and let dma-mapping and core memory subsystems to handle gfp flags correctly. Arnd: do you have any opinion on this? I'm aware that this change might have some side effects on drivers which don't pass correct gfp flags, but using the emergency memory pools all the time for all dma_pool allocations really doesn't make much sense. Best regards
diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c index 2af6ce5..cbad821 100644 --- a/arch/arm/mach-mvebu/armada-370-xp.c +++ b/arch/arm/mach-mvebu/armada-370-xp.c @@ -17,6 +17,7 @@ #include <linux/of_platform.h> #include <linux/io.h> #include <linux/time-armada-370-xp.h> +#include <linux/dma-mapping.h> #include <asm/mach/arch.h> #include <asm/mach/map.h> #include <asm/mach/time.h> @@ -43,6 +44,16 @@ void __init armada_370_xp_timer_and_clk_init(void) armada_370_xp_timer_init(); } +void __init armada_370_xp_init_early(void) +{ + /* + * Some Armada 370/XP devices allocate their coherent buffers + * from atomic context. Increase size of atomic coherent pool + * to make sure such the allocations won't fail. + */ + init_dma_coherent_pool_size(SZ_1M); +} + struct sys_timer armada_370_xp_timer = { .init = armada_370_xp_timer_and_clk_init, }; @@ -61,6 +72,7 @@ static const char * const armada_370_xp_dt_board_dt_compat[] = { DT_MACHINE_START(ARMADA_XP_DT, "Marvell Aramada 370/XP (Device Tree)") .init_machine = armada_370_xp_dt_init, .map_io = armada_370_xp_map_io, + .init_early = armada_370_xp_init_early, .init_irq = armada_370_xp_init_irq, .handle_irq = armada_370_xp_handle_irq, .timer = &armada_370_xp_timer,
For Armada 370/XP we have the same problem that for the commit cb01b63, so we applied the same solution: "The default 256 KiB coherent pool may be too small for some of the Kirkwood devices, so increase it to make sure that devices will be able to allocate their buffers with GFP_ATOMIC flag" Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> Cc: Marek Szyprowski <m.szyprowski@samsung.com> --- arch/arm/mach-mvebu/armada-370-xp.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)