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

Wolfram Sang
2018-09-11T23:39:33Z

[-- Attachment #1: Type: text/plain, Size: 1859 bytes --]

Hi Robin,

> > 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 ;)

I see. Yes, I was aware that the misuse of this dangling pointer is
somewhat academical. Yet, it was easy to fix and clearing this pointer
is good programming practice, I'd say. I agree that clearing the pointer
in __device_release_driver is a good option, too, if documentation about
its expected life cycle (== get's cleared on unbind) is clear about
that. Probably that life cycle confusion led to the more complicated
code in the exynos_drm driver. I will look into all of that tomorrow.

> 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

I was wondering about that, yes.

> there's a fair chance of this getting fundamentally rejigged at some point
> anyway.

Makes sense. Yet, as this change is gonna be small, I think it's still
nice to have.

Thanks for the input!

Wolfram

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]