From: Xi Wang <xi.wang@gmail.com> To: Andrew Morton <akpm@linux-foundation.org> Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] mm: fix NULL checking in dma_pool_create() Date: Tue, 06 Nov 2012 15:48:26 -0500 [thread overview] Message-ID: <5099779A.8050009@gmail.com> (raw) In-Reply-To: <20121105132651.f52549b6.akpm@linux-foundation.org> On 11/5/12 4:26 PM, Andrew Morton wrote: > > 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. It's probably ok to call dma_alloc_coherent()/dma_free_coherent() with a NULL dev. Quite a few drivers do that. > dmam_pool_destroy() will clearly cause an oops: > > devres_destroy() > ->devres_remove() > ->spin_lock_irqsave(&dev->devres_lock, flags); Not sure if I missed anything, but I haven't found any use of dmam_pool_destroy() in the tree.. > 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. After more search I've still only found 4 files that invoke dma_pool_create() with a NULL dev. arch/arm/mach-s3c64xx/dma.c drivers/usb/gadget/amd5536udc.c drivers/net/wan/ixp4xx_hss.c drivers/net/ethernet/xscale/ixp4xx_eth.c So, yeah, we could fix those drivers instead, such as adding "WARN_ON_ONCE(dev == NULL)" as you suggested. - xi
WARNING: multiple messages have this Message-ID (diff)
From: Xi Wang <xi.wang@gmail.com> To: Andrew Morton <akpm@linux-foundation.org> Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] mm: fix NULL checking in dma_pool_create() Date: Tue, 06 Nov 2012 15:48:26 -0500 [thread overview] Message-ID: <5099779A.8050009@gmail.com> (raw) In-Reply-To: <20121105132651.f52549b6.akpm@linux-foundation.org> On 11/5/12 4:26 PM, Andrew Morton wrote: > > 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. It's probably ok to call dma_alloc_coherent()/dma_free_coherent() with a NULL dev. Quite a few drivers do that. > dmam_pool_destroy() will clearly cause an oops: > > devres_destroy() > ->devres_remove() > ->spin_lock_irqsave(&dev->devres_lock, flags); Not sure if I missed anything, but I haven't found any use of dmam_pool_destroy() in the tree.. > 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. After more search I've still only found 4 files that invoke dma_pool_create() with a NULL dev. arch/arm/mach-s3c64xx/dma.c drivers/usb/gadget/amd5536udc.c drivers/net/wan/ixp4xx_hss.c drivers/net/ethernet/xscale/ixp4xx_eth.c So, yeah, we could fix those drivers instead, such as adding "WARN_ON_ONCE(dev == NULL)" as you suggested. - xi -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2012-11-06 20:48 UTC|newest] Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top 2012-11-05 6:46 [PATCH] mm: fix NULL checking in dma_pool_create() Xi Wang 2012-11-05 6:46 ` Xi Wang 2012-11-05 20:37 ` Andrew Morton 2012-11-05 20:37 ` Andrew Morton 2012-11-05 20:50 ` Xi Wang 2012-11-05 20:50 ` Xi Wang 2012-11-05 21:26 ` Andrew Morton 2012-11-05 21:26 ` Andrew Morton 2012-11-06 20:48 ` Xi Wang [this message] 2012-11-06 20:48 ` Xi Wang 2012-11-13 21:39 ` [PATCH v2] mm: fix null dev " Xi Wang 2012-11-13 21:39 ` Xi Wang 2012-11-13 23:01 ` David Rientjes 2012-11-13 23:01 ` David Rientjes 2012-11-14 0:47 ` Andrew Morton 2012-11-14 0:47 ` Andrew Morton 2012-11-14 0:58 ` Andrew Morton 2012-11-14 0:58 ` Andrew Morton 2012-11-14 5:50 ` Xi Wang 2012-11-14 5:50 ` Xi Wang 2012-11-14 14:17 ` Felipe Balbi 2012-11-14 14:17 ` Felipe Balbi
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=5099779A.8050009@gmail.com \ --to=xi.wang@gmail.com \ --cc=akpm@linux-foundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.