Messages in this thread | | | Date | Mon, 5 Nov 2012 13:26:51 -0800 | From | Andrew Morton <> | Subject | Re: [PATCH] mm: fix NULL checking in dma_pool_create() |
| |
On Mon, 05 Nov 2012 15:50:32 -0500 Xi Wang <xi.wang@gmail.com> wrote:
> On 11/5/12 3:37 PM, Andrew Morton wrote: > > > > Well, the dma_pool_create() kerneldoc does not describe dev==NULL to be > > acceptable usage and given the lack of oops reports, we can assume that > > no code is calling this function with dev==NULL. > > > > So I think we can just remove the code which handles dev==NULL? > > Actually, a quick grep gives the following... > > arch/arm/mach-s3c64xx/dma.c:731: dma_pool = dma_pool_create("DMA-LLI", NULL, sizeof(struct pl080s_lli), 16, 0); > drivers/usb/gadget/amd5536udc.c:3136: dev->data_requests = dma_pool_create("data_requests", NULL, > drivers/usb/gadget/amd5536udc.c:3148: dev->stp_requests = dma_pool_create("setup requests", NULL, > drivers/net/wan/ixp4xx_hss.c:973: if (!(dma_pool = dma_pool_create(DRV_NAME, NULL, > drivers/net/ethernet/xscale/ixp4xx_eth.c:1106: if (!(dma_pool = dma_pool_create(DRV_NAME, NULL, >
OK, so it seems that those drivers have never been tested on a CONFIG_NUMA kernel. whee.
So we have a large amount of code here which ostensibly supports dev==NULL but which has not been well tested. Take a look at dma_alloc_coherent(), dma_free_coherent() - are they safe? Unobvious.
dmam_pool_destroy() will clearly cause an oops:
devres_destroy() ->devres_remove() ->spin_lock_irqsave(&dev->devres_lock, flags);
So what to do?
I'm thinking we should disallow dev==NULL. We have a lot of code in mm/dmapool.c which _attempts_ to support this case, but is largely untested and obviously isn't working. I don't think it's a good idea to try to fix up and then support this case on behalf of a handful of scruffy drivers. It would be better to fix the drivers, then simplify the core code. drivers/usb/gadget/amd5536udc.c can probably use dev->gadget.dev and drivers/net/wan/ixp4xx_hss.c can probably use port->netdev->dev, etc.
So how about we add a WARN_ON_ONCE(dev == NULL), notify the driver maintainers and later we can remove all that mm/dmapool.c code which is trying to handle dev==NULL?
| |