| Message ID | df529b6e-6744-b1af-01ce-a1b691fbcf0d@cybernetics.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | None | expand |
On 12/11/2018 15:42, Tony Battersby wrote: > dmapool originally tried to support pools without a device because > dma_alloc_coherent() supports allocations without a device. But nobody > ended up using dma pools without a device, so the current checks in > dmapool.c for pool->dev == NULL are both insufficient and causing bloat. > Remove them. > As an aside, is it right that dma_pool_create() does not actually reject dev==NULL and would crash from a NULL-pointer dereference? Thanks, John > Signed-off-by: Tony Battersby <tonyb@cybernetics.com> > --- > --- linux/mm/dmapool.c.orig 2018-08-03 16:12:23.000000000 -0400 > +++ linux/mm/dmapool.c 2018-08-03 16:13:44.000000000 -0400 > @@ -277,7 +277,7 @@ void dma_pool_destroy(struct dma_pool *p > mutex_lock(&pools_reg_lock); > mutex_lock(&pools_lock); > list_del(&pool->pools); > - if (pool->dev && list_empty(&pool->dev->dma_pools)) > + if (list_empty(&pool->dev->dma_pools)) > empty = true; > mutex_unlock(&pools_lock); > if (empty) > @@ -289,13 +289,9 @@ void dma_pool_destroy(struct dma_pool *p > page = list_entry(pool->page_list.next, > struct dma_page, page_list); > if (is_page_busy(page)) { > - if (pool->dev) > - dev_err(pool->dev, > - "dma_pool_destroy %s, %p busy\n", > - pool->name, page->vaddr); > - else > - pr_err("dma_pool_destroy %s, %p busy\n", > - pool->name, page->vaddr); > + dev_err(pool->dev, > + "dma_pool_destroy %s, %p busy\n", > + pool->name, page->vaddr); > /* leak the still-in-use consistent memory */ > list_del(&page->page_list); > kfree(page); > @@ -357,13 +353,9 @@ void *dma_pool_alloc(struct dma_pool *po > for (i = sizeof(page->offset); i < pool->size; i++) { > if (data[i] == POOL_POISON_FREED) > continue; > - if (pool->dev) > - dev_err(pool->dev, > - "dma_pool_alloc %s, %p (corrupted)\n", > - pool->name, retval); > - else > - pr_err("dma_pool_alloc %s, %p (corrupted)\n", > - pool->name, retval); > + dev_err(pool->dev, > + "dma_pool_alloc %s, %p (corrupted)\n", > + pool->name, retval); > > /* > * Dump the first 4 bytes even if they are not > @@ -418,13 +410,9 @@ void dma_pool_free(struct dma_pool *pool > page = pool_find_page(pool, dma); > if (!page) { > spin_unlock_irqrestore(&pool->lock, flags); > - if (pool->dev) > - dev_err(pool->dev, > - "dma_pool_free %s, %p/%lx (bad dma)\n", > - pool->name, vaddr, (unsigned long)dma); > - else > - pr_err("dma_pool_free %s, %p/%lx (bad dma)\n", > - pool->name, vaddr, (unsigned long)dma); > + dev_err(pool->dev, > + "dma_pool_free %s, %p/%lx (bad dma)\n", > + pool->name, vaddr, (unsigned long)dma); > return; > } > > @@ -432,13 +420,9 @@ void dma_pool_free(struct dma_pool *pool > #ifdef DMAPOOL_DEBUG > if ((dma - page->dma) != offset) { > spin_unlock_irqrestore(&pool->lock, flags); > - if (pool->dev) > - dev_err(pool->dev, > - "dma_pool_free %s, %p (bad vaddr)/%pad\n", > - pool->name, vaddr, &dma); > - else > - pr_err("dma_pool_free %s, %p (bad vaddr)/%pad\n", > - pool->name, vaddr, &dma); > + dev_err(pool->dev, > + "dma_pool_free %s, %p (bad vaddr)/%pad\n", > + pool->name, vaddr, &dma); > return; > } > { > @@ -449,12 +433,9 @@ void dma_pool_free(struct dma_pool *pool > continue; > } > spin_unlock_irqrestore(&pool->lock, flags); > - if (pool->dev) > - dev_err(pool->dev, "dma_pool_free %s, dma %pad already free\n", > - pool->name, &dma); > - else > - pr_err("dma_pool_free %s, dma %pad already free\n", > - pool->name, &dma); > + dev_err(pool->dev, > + "dma_pool_free %s, dma %pad already free\n", > + pool->name, &dma); > return; > } > } > > > . >
On 11/12/18 11:32 AM, John Garry wrote: > On 12/11/2018 15:42, Tony Battersby wrote: >> dmapool originally tried to support pools without a device because >> dma_alloc_coherent() supports allocations without a device. But nobody >> ended up using dma pools without a device, so the current checks in >> dmapool.c for pool->dev == NULL are both insufficient and causing bloat. >> Remove them. >> > As an aside, is it right that dma_pool_create() does not actually reject > dev==NULL and would crash from a NULL-pointer dereference? > > Thanks, > John > When passed a NULL dev, dma_pool_create() will already crash with a NULL-pointer dereference before this patch series, because it checks for dev == NULL in some places but not others. Specifically, it will crash in one of these two places in dma_pool_create(): retval = kmalloc_node(sizeof(*retval), GFP_KERNEL, dev_to_node(dev)); -or- if (list_empty(&dev->dma_pools)) So removing the checks for dev == NULL will not make previously-working code to start crashing suddenly. And since passing dev == NULL would be an API misuse error and not a runtime error, I would rather not add a new check to reject it. Tony
On Mon, Nov 12, 2018 at 10:42:12AM -0500, Tony Battersby wrote: > dmapool originally tried to support pools without a device because > dma_alloc_coherent() supports allocations without a device. But nobody > ended up using dma pools without a device, so the current checks in > dmapool.c for pool->dev == NULL are both insufficient and causing bloat. > Remove them. > > Signed-off-by: Tony Battersby <tonyb@cybernetics.com> Acked-by: Matthew Wilcox <willy@infradead.org>
--- linux/mm/dmapool.c.orig 2018-08-03 16:12:23.000000000 -0400 +++ linux/mm/dmapool.c 2018-08-03 16:13:44.000000000 -0400 @@ -277,7 +277,7 @@ void dma_pool_destroy(struct dma_pool *p mutex_lock(&pools_reg_lock); mutex_lock(&pools_lock); list_del(&pool->pools); - if (pool->dev && list_empty(&pool->dev->dma_pools)) + if (list_empty(&pool->dev->dma_pools)) empty = true; mutex_unlock(&pools_lock); if (empty) @@ -289,13 +289,9 @@ void dma_pool_destroy(struct dma_pool *p page = list_entry(pool->page_list.next, struct dma_page, page_list); if (is_page_busy(page)) { - if (pool->dev) - dev_err(pool->dev, - "dma_pool_destroy %s, %p busy\n", - pool->name, page->vaddr); - else - pr_err("dma_pool_destroy %s, %p busy\n", - pool->name, page->vaddr); + dev_err(pool->dev, + "dma_pool_destroy %s, %p busy\n", + pool->name, page->vaddr); /* leak the still-in-use consistent memory */ list_del(&page->page_list); kfree(page); @@ -357,13 +353,9 @@ void *dma_pool_alloc(struct dma_pool *po for (i = sizeof(page->offset); i < pool->size; i++) { if (data[i] == POOL_POISON_FREED) continue; - if (pool->dev) - dev_err(pool->dev, - "dma_pool_alloc %s, %p (corrupted)\n", - pool->name, retval); - else - pr_err("dma_pool_alloc %s, %p (corrupted)\n", - pool->name, retval); + dev_err(pool->dev, + "dma_pool_alloc %s, %p (corrupted)\n", + pool->name, retval); /* * Dump the first 4 bytes even if they are not @@ -418,13 +410,9 @@ void dma_pool_free(struct dma_pool *pool page = pool_find_page(pool, dma); if (!page) { spin_unlock_irqrestore(&pool->lock, flags); - if (pool->dev) - dev_err(pool->dev, - "dma_pool_free %s, %p/%lx (bad dma)\n", - pool->name, vaddr, (unsigned long)dma); - else - pr_err("dma_pool_free %s, %p/%lx (bad dma)\n", - pool->name, vaddr, (unsigned long)dma); + dev_err(pool->dev, + "dma_pool_free %s, %p/%lx (bad dma)\n", + pool->name, vaddr, (unsigned long)dma); return; } @@ -432,13 +420,9 @@ void dma_pool_free(struct dma_pool *pool #ifdef DMAPOOL_DEBUG if ((dma - page->dma) != offset) { spin_unlock_irqrestore(&pool->lock, flags); - if (pool->dev) - dev_err(pool->dev, - "dma_pool_free %s, %p (bad vaddr)/%pad\n", - pool->name, vaddr, &dma); - else - pr_err("dma_pool_free %s, %p (bad vaddr)/%pad\n", - pool->name, vaddr, &dma); + dev_err(pool->dev, + "dma_pool_free %s, %p (bad vaddr)/%pad\n", + pool->name, vaddr, &dma); return; } { @@ -449,12 +433,9 @@ void dma_pool_free(struct dma_pool *pool continue; } spin_unlock_irqrestore(&pool->lock, flags); - if (pool->dev) - dev_err(pool->dev, "dma_pool_free %s, dma %pad already free\n", - pool->name, &dma); - else - pr_err("dma_pool_free %s, dma %pad already free\n", - pool->name, &dma); + dev_err(pool->dev, + "dma_pool_free %s, dma %pad already free\n", + pool->name, &dma); return; } }
dmapool originally tried to support pools without a device because dma_alloc_coherent() supports allocations without a device. But nobody ended up using dma pools without a device, so the current checks in dmapool.c for pool->dev == NULL are both insufficient and causing bloat. Remove them. Signed-off-by: Tony Battersby <tonyb@cybernetics.com> ---