* [PATCH v4 2/9] dmapool: remove checks for dev == NULL
@ 2018-11-12 15:42 Tony Battersby
[not found] ` <df529b6e-6744-b1af-01ce-a1b691fbcf0d-vFAe+i1/wJI5UWNf+nJyDw@public.gmane.org>
0 siblings, 1 reply; 4+ messages in thread
From: Tony Battersby @ 2018-11-12 15:42 UTC (permalink / raw)
To: Matthew Wilcox, Christoph Hellwig, Marek Szyprowski,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg
Cc: linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
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-vFAe+i1/wJI5UWNf+nJyDw@public.gmane.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;
}
}
^ permalink raw reply [flat|nested] 4+ messages in thread[parent not found: <df529b6e-6744-b1af-01ce-a1b691fbcf0d-vFAe+i1/wJI5UWNf+nJyDw@public.gmane.org>]
* Re: [PATCH v4 2/9] dmapool: remove checks for dev == NULL [not found] ` <df529b6e-6744-b1af-01ce-a1b691fbcf0d-vFAe+i1/wJI5UWNf+nJyDw@public.gmane.org> @ 2018-11-12 16:32 ` John Garry [not found] ` <5a32095b-4626-9967-784b-9becac303994-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> 2018-11-13 6:14 ` Matthew Wilcox 1 sibling, 1 reply; 4+ messages in thread From: John Garry @ 2018-11-12 16:32 UTC (permalink / raw) To: Tony Battersby, Matthew Wilcox, Christoph Hellwig, Marek Szyprowski, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg Cc: linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 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-vFAe+i1/wJI5UWNf+nJyDw@public.gmane.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; > } > } > > > . > ^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <5a32095b-4626-9967-784b-9becac303994-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH v4 2/9] dmapool: remove checks for dev == NULL [not found] ` <5a32095b-4626-9967-784b-9becac303994-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> @ 2018-11-12 17:06 ` Tony Battersby 0 siblings, 0 replies; 4+ messages in thread From: Tony Battersby @ 2018-11-12 17:06 UTC (permalink / raw) To: John Garry, Matthew Wilcox, Christoph Hellwig, Marek Szyprowski, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg Cc: linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 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 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v4 2/9] dmapool: remove checks for dev == NULL [not found] ` <df529b6e-6744-b1af-01ce-a1b691fbcf0d-vFAe+i1/wJI5UWNf+nJyDw@public.gmane.org> 2018-11-12 16:32 ` John Garry @ 2018-11-13 6:14 ` Matthew Wilcox 1 sibling, 0 replies; 4+ messages in thread From: Matthew Wilcox @ 2018-11-13 6:14 UTC (permalink / raw) To: Tony Battersby Cc: linux-mm-Bw31MaZKKs3YtjvyW6yDsg, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Christoph Hellwig, linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 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-vFAe+i1/wJI5UWNf+nJyDw@public.gmane.org> Acked-by: Matthew Wilcox <willy-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-11-13 6:14 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-11-12 15:42 [PATCH v4 2/9] dmapool: remove checks for dev == NULL Tony Battersby
[not found] ` <df529b6e-6744-b1af-01ce-a1b691fbcf0d-vFAe+i1/wJI5UWNf+nJyDw@public.gmane.org>
2018-11-12 16:32 ` John Garry
[not found] ` <5a32095b-4626-9967-784b-9becac303994-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
2018-11-12 17:06 ` Tony Battersby
2018-11-13 6:14 ` Matthew Wilcox
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).