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

[Wolfram Sang]

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: signature.asc
Description: PGP signature