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
