Re: [RFC PATCH 0/2] dma-mapping: introduce helper for setting dma_parms

[Robin Murphy]

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.

Robin.

> 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://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.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(+)