[lkml]   [2018]   [Sep]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [RFC PATCH 0/2] dma-mapping: introduce helper for setting dma_parms
On 11/09/18 17:30, Wolfram Sang wrote:
> Hi all,
> commit 78c47830a5cb ("dma-debug: check scatterlist segments") triggers
> for Renesas hardware I look after, so thanks for pointing out we should
> have proper dma_parms for our DMA providers.
> When trying to fix it, I became a bit puzzled about the life cycle of
> the pointer to dma_parms. AFAIU most drivers leave the pointer dangling
> on driver unbind. Check drivers/dma/bcm2835-dma.c, for example:
> od = devm_kzalloc(&pdev->dev, sizeof(*od), GFP_KERNEL);
> if (!od)
> return -ENOMEM;
> pdev->dev.dma_parms = &od->dma_parms;
> dma_set_max_seg_size(&pdev->dev, 0x3FFFFFFF);
> And that's all about handling dma_parms. So, on unbind, the memory for
> 'od' gets freed and dma_params is a dangling pointer.

That's the typical case - the dma_parms structure is simply embedded in
some other private data which tends to have the appropriate lifetime
anyway. I can't see that the dangling pointer is an issue when it's set
unconditionally on probe and valid until remove, because anyone
dereferencing dev->dma_parms when dev has no driver bound is doing
something very very wrong anyway. I suppose we could zero it in
__device_release_driver() for good measure though - shame we've found
something dma_deconfigure() could have been useful for just after we
killed it ;)

> drivers/gpu/drm/exynos/exynos_drm_iommu.c seems to do it correctly:

...but could be even simpler if it was just included in
exynos_drm_private. Realistically, I struggle to imagine a driver which
would need to set dma_parms that didn't already have some other private
state into which it could fit.

Note that ultimately we'd like to have a single structure to hold all
the masks and other gubbins (per the very original intent of dma_parms),
so there's a fair chance of this getting fundamentally rejigged at some
point anyway.


> static inline int configure_dma_max_seg_size(struct device *dev)
> {
> if (!dev->dma_parms)
> dev->dma_parms = kzalloc(sizeof(*dev->dma_parms), GFP_KERNEL);
> if (!dev->dma_parms)
> return -ENOMEM;
> dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
> return 0;
> }
> static inline void clear_dma_max_seg_size(struct device *dev)
> {
> kfree(dev->dma_parms);
> dev->dma_parms = NULL;
> }
> But this seems error prone and quite some code to add for every DMA
> provider. So, I wondered if we couldn't have a helper for that. After
> some brainstorming, I favour a dmam_-type of function. It will ensure
> the memory gets freed and the pointer cleared on unbind. And it should
> be easy to use.
> I attached an RFC which I tested on a Renesas R-Car H3 SoC with the internal
> DMAC of the SD controller. A branch can be found here (still waiting for
> buildbot results):
> git:// renesas/sdhi/set_max_seg
> I added the companion function dmam_free_dma_parms() for completeness
> although there is no user yet. I'd be totally open to drop it until
> someone needs it.
> Please let me know what you think. If this is the right track, I'll be
> willing to fix the dangling pointers with it, too.
> Thanks and happy hacking,
> Wolfram
> Wolfram Sang (2):
> dma-mapping: introduce helper for setting dma_parms
> mmc: sdhi: internal_dmac: set dma_parms
> drivers/mmc/host/renesas_sdhi_internal_dmac.c | 2 +
> include/linux/dma-mapping.h | 5 ++
> kernel/dma/mapping.c | 50 +++++++++++++++++++
> 3 files changed, 57 insertions(+)

 \ /
  Last update: 2018-09-11 19:24    [W:0.235 / U:0.696 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site