| Message ID | 20190516114721.27694-2-laurentiu.tudor@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | prerequisites for device reserved local mem rework | expand |
On Thu, May 16, 2019 at 02:47:19PM +0300, laurentiu.tudor@nxp.com wrote: > From: Laurentiu Tudor <laurentiu.tudor@nxp.com> > > For HCs that have local memory, replace the current DMA API usage > with a genalloc generic allocator to manage the mappings for these > devices. > This is in preparation for dropping the existing "coherent" dma > mem declaration APIs. Current implementation was relying on a short > circuit in the DMA API that in the end, was acting as an allocator > for these type of devices. > > For context, see thread here: https://lkml.org/lkml/2019/4/22/357 > > Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com> > --- > drivers/usb/core/buffer.c | 15 +++++++++++---- > drivers/usb/host/ohci-hcd.c | 23 ++++++++++++++++++----- > include/linux/usb/hcd.h | 3 +++ > 3 files changed, 32 insertions(+), 9 deletions(-) > > diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c > index f641342cdec0..22a8f3f5679b 100644 > --- a/drivers/usb/core/buffer.c > +++ b/drivers/usb/core/buffer.c > @@ -16,6 +16,7 @@ > #include <linux/io.h> > #include <linux/dma-mapping.h> > #include <linux/dmapool.h> > +#include <linux/genalloc.h> > #include <linux/usb.h> > #include <linux/usb/hcd.h> > > @@ -124,10 +125,12 @@ void *hcd_buffer_alloc( > if (size == 0) > return NULL; > > + if (hcd->driver->flags & HCD_LOCAL_MEM) > + return gen_pool_dma_alloc(hcd->localmem_pool, size, dma); Does this patch now break things? hcd->localmem_pool at this point in time is NULL, so this call will fail. There's no chance for any host controller driver to actually set up this pool in this patch, so is bisection broken? I think you fix this up in later patches, right? And if so, why do we even need HCD_LOCAL_MEM anymore? Can't we just test for the presence of hcd->localmem_pool in order to determine which allocation method to use? thanks, greg k-h
On Tue, May 21, 2019 at 10:16:57AM +0200, Greg KH wrote: > > + if (hcd->driver->flags & HCD_LOCAL_MEM) > > + return gen_pool_dma_alloc(hcd->localmem_pool, size, dma); > > Does this patch now break things? hcd->localmem_pool at this point in > time is NULL, so this call will fail. There's no chance for any host > controller driver to actually set up this pool in this patch, so is > bisection broken? > > I think you fix this up in later patches, right? > > And if so, why do we even need HCD_LOCAL_MEM anymore? Can't we just > test for the presence of hcd->localmem_pool in order to determine which > allocation method to use? True. And that also sound like a good bisectability strategy: - first add hcd->localmem_pool and test for it - convert drivers over to it - remove HCD_LOCAL_MEM
On 21.05.2019 11:16, Greg KH wrote: > On Thu, May 16, 2019 at 02:47:19PM +0300, laurentiu.tudor@nxp.com wrote: >> From: Laurentiu Tudor <laurentiu.tudor@nxp.com> >> >> For HCs that have local memory, replace the current DMA API usage >> with a genalloc generic allocator to manage the mappings for these >> devices. >> This is in preparation for dropping the existing "coherent" dma >> mem declaration APIs. Current implementation was relying on a short >> circuit in the DMA API that in the end, was acting as an allocator >> for these type of devices. >> >> For context, see thread here: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F4%2F22%2F357&data=02%7C01%7Claurentiu.tudor%40nxp.com%7Cf5242fb28d154ff9653208d6ddc4b41c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636940234237524499&sdata=KEEUP1KH%2BaraWcVKogeYBzrauh%2FFTzGjSxjk%2BuNozjA%3D&reserved=0 >> >> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com> >> --- >> drivers/usb/core/buffer.c | 15 +++++++++++---- >> drivers/usb/host/ohci-hcd.c | 23 ++++++++++++++++++----- >> include/linux/usb/hcd.h | 3 +++ >> 3 files changed, 32 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c >> index f641342cdec0..22a8f3f5679b 100644 >> --- a/drivers/usb/core/buffer.c >> +++ b/drivers/usb/core/buffer.c >> @@ -16,6 +16,7 @@ >> #include <linux/io.h> >> #include <linux/dma-mapping.h> >> #include <linux/dmapool.h> >> +#include <linux/genalloc.h> >> #include <linux/usb.h> >> #include <linux/usb/hcd.h> >> >> @@ -124,10 +125,12 @@ void *hcd_buffer_alloc( >> if (size == 0) >> return NULL; >> >> + if (hcd->driver->flags & HCD_LOCAL_MEM) >> + return gen_pool_dma_alloc(hcd->localmem_pool, size, dma); > > Does this patch now break things? hcd->localmem_pool at this point in > time is NULL, so this call will fail. There's no chance for any host > controller driver to actually set up this pool in this patch, so is > bisection broken? Unfortunately, yes. I could lump the patches together but I think Christoph suggestion is much better. > I think you fix this up in later patches, right? Correct. The last 2 patches update the driver. > And if so, why do we even need HCD_LOCAL_MEM anymore? Can't we just > test for the presence of hcd->localmem_pool in order to determine which > allocation method to use? Sure. There are a few more places that need updates but no big deal. --- Best Regards, Laurentiu
On Tue, May 21, 2019 at 11:04:12AM +0000, Laurentiu Tudor wrote: > > > On 21.05.2019 11:16, Greg KH wrote: > > On Thu, May 16, 2019 at 02:47:19PM +0300, laurentiu.tudor@nxp.com wrote: > >> From: Laurentiu Tudor <laurentiu.tudor@nxp.com> > >> > >> For HCs that have local memory, replace the current DMA API usage > >> with a genalloc generic allocator to manage the mappings for these > >> devices. > >> This is in preparation for dropping the existing "coherent" dma > >> mem declaration APIs. Current implementation was relying on a short > >> circuit in the DMA API that in the end, was acting as an allocator > >> for these type of devices. > >> > >> For context, see thread here: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F4%2F22%2F357&data=02%7C01%7Claurentiu.tudor%40nxp.com%7Cf5242fb28d154ff9653208d6ddc4b41c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636940234237524499&sdata=KEEUP1KH%2BaraWcVKogeYBzrauh%2FFTzGjSxjk%2BuNozjA%3D&reserved=0 > >> > >> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com> > >> --- > >> drivers/usb/core/buffer.c | 15 +++++++++++---- > >> drivers/usb/host/ohci-hcd.c | 23 ++++++++++++++++++----- > >> include/linux/usb/hcd.h | 3 +++ > >> 3 files changed, 32 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c > >> index f641342cdec0..22a8f3f5679b 100644 > >> --- a/drivers/usb/core/buffer.c > >> +++ b/drivers/usb/core/buffer.c > >> @@ -16,6 +16,7 @@ > >> #include <linux/io.h> > >> #include <linux/dma-mapping.h> > >> #include <linux/dmapool.h> > >> +#include <linux/genalloc.h> > >> #include <linux/usb.h> > >> #include <linux/usb/hcd.h> > >> > >> @@ -124,10 +125,12 @@ void *hcd_buffer_alloc( > >> if (size == 0) > >> return NULL; > >> > >> + if (hcd->driver->flags & HCD_LOCAL_MEM) > >> + return gen_pool_dma_alloc(hcd->localmem_pool, size, dma); > > > > Does this patch now break things? hcd->localmem_pool at this point in > > time is NULL, so this call will fail. There's no chance for any host > > controller driver to actually set up this pool in this patch, so is > > bisection broken? > > Unfortunately, yes. I could lump the patches together but I think > Christoph suggestion is much better. I do too, can you redo these patches to work in that manner please? thanks, greg k-h
diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c index f641342cdec0..22a8f3f5679b 100644 --- a/drivers/usb/core/buffer.c +++ b/drivers/usb/core/buffer.c @@ -16,6 +16,7 @@ #include <linux/io.h> #include <linux/dma-mapping.h> #include <linux/dmapool.h> +#include <linux/genalloc.h> #include <linux/usb.h> #include <linux/usb/hcd.h> @@ -124,10 +125,12 @@ void *hcd_buffer_alloc( if (size == 0) return NULL; + if (hcd->driver->flags & HCD_LOCAL_MEM) + return gen_pool_dma_alloc(hcd->localmem_pool, size, dma); + /* some USB hosts just use PIO */ if (!IS_ENABLED(CONFIG_HAS_DMA) || - (!is_device_dma_capable(bus->sysdev) && - !(hcd->driver->flags & HCD_LOCAL_MEM))) { + !is_device_dma_capable(bus->sysdev)) { *dma = ~(dma_addr_t) 0; return kmalloc(size, mem_flags); } @@ -152,9 +155,13 @@ void hcd_buffer_free( if (!addr) return; + if (hcd->driver->flags & HCD_LOCAL_MEM) { + gen_pool_free(hcd->localmem_pool, (unsigned long)addr, size); + return; + } + if (!IS_ENABLED(CONFIG_HAS_DMA) || - (!is_device_dma_capable(bus->sysdev) && - !(hcd->driver->flags & HCD_LOCAL_MEM))) { + !is_device_dma_capable(bus->sysdev)) { kfree(addr); return; } diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c index 210181fd98d2..f9462c372943 100644 --- a/drivers/usb/host/ohci-hcd.c +++ b/drivers/usb/host/ohci-hcd.c @@ -40,6 +40,7 @@ #include <linux/dmapool.h> #include <linux/workqueue.h> #include <linux/debugfs.h> +#include <linux/genalloc.h> #include <asm/io.h> #include <asm/irq.h> @@ -505,8 +506,15 @@ static int ohci_init (struct ohci_hcd *ohci) timer_setup(&ohci->io_watchdog, io_watchdog_func, 0); ohci->prev_frame_no = IO_WATCHDOG_OFF; - ohci->hcca = dma_alloc_coherent (hcd->self.controller, - sizeof(*ohci->hcca), &ohci->hcca_dma, GFP_KERNEL); + if (hcd->driver->flags & HCD_LOCAL_MEM) + ohci->hcca = gen_pool_dma_alloc(hcd->localmem_pool, + sizeof(*ohci->hcca), + &ohci->hcca_dma); + else + ohci->hcca = dma_alloc_coherent(hcd->self.controller, + sizeof(*ohci->hcca), + &ohci->hcca_dma, + GFP_KERNEL); if (!ohci->hcca) return -ENOMEM; @@ -990,9 +998,14 @@ static void ohci_stop (struct usb_hcd *hcd) remove_debug_files (ohci); ohci_mem_cleanup (ohci); if (ohci->hcca) { - dma_free_coherent (hcd->self.controller, - sizeof *ohci->hcca, - ohci->hcca, ohci->hcca_dma); + if (hcd->driver->flags & HCD_LOCAL_MEM) + gen_pool_free(hcd->localmem_pool, + (unsigned long)ohci->hcca, + sizeof(*ohci->hcca)); + else + dma_free_coherent(hcd->self.controller, + sizeof(*ohci->hcca), + ohci->hcca, ohci->hcca_dma); ohci->hcca = NULL; ohci->hcca_dma = 0; } diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h index bb57b5af4700..1b0fc3cebc08 100644 --- a/include/linux/usb/hcd.h +++ b/include/linux/usb/hcd.h @@ -216,6 +216,9 @@ struct usb_hcd { #define HC_IS_RUNNING(state) ((state) & __ACTIVE) #define HC_IS_SUSPENDED(state) ((state) & __SUSPEND) + /* allocator for HCs having local memory */ + struct gen_pool *localmem_pool; + /* more shared queuing code would be good; it should support * smarter scheduling, handle transaction translators, etc; * input size of periodic table to an interrupt scheduler.