On Wed, Apr 12, 2017 at 10:46:30AM +0300, Ville Syrjälä wrote:
> On Wed, Apr 12, 2017 at 06:59:55AM +0200, Takashi Iwai wrote:
> > On Tue, 11 Apr 2017 23:27:07 +0200,
> > Chris Wilson wrote:
> > >
> > > On Tue, Apr 11, 2017 at 10:20:56PM +0100, Chris Wilson wrote:
> > > > On Tue, Apr 11, 2017 at 11:01:57PM +0200, Takashi Iwai wrote:
> > > > > On Tue, 11 Apr 2017 22:41:12 +0200,
> > > > > Chris Wilson wrote:
> > > > > Oh, this fell into a crack as it was sent just before my vacation.
> > > > >
> > > > > About the change:
> > > > >
> > > > > > > ---
> > > > > > > drivers/gpu/drm/i915/intel_lpe_audio.c | 8 ++++++--
> > > > > > > 1 file changed, 6 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > > > > > index 7a5b41b1c024..32000902a204 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > > > > > @@ -131,8 +131,12 @@ lpe_audio_platdev_create(struct drm_i915_private *dev_priv)
> > > > > > >
> > > > > > > static void lpe_audio_platdev_destroy(struct drm_i915_private *dev_priv)
> > > > > > > {
> > > > > > > - platform_device_unregister(dev_priv->lpe_audio.platdev);
> > > > > > > - kfree(dev_priv->lpe_audio.platdev->dev.dma_mask);
> > > > > > > + struct platform_device *platdev = dev_priv->lpe_audio.platdev;
> > > > > > > +
> > > > > > > + kfree(platdev->dev.dma_mask);
> > > > > > > + platdev->dev.dma_mask = NULL;
> > > > > > > +
> > > > > > > + platform_device_unregister(platdev);
> > > > >
> > > > > I'm not sure whether it's good idea to fiddle dma_mask bits before the
> > > > > unregister call. Interestingly, this is the only driver that calls
> > > > > kfree() for pdev's dma_mask. Either we do something wrong, or
> > > > > everyone forgot this?
> > > >
> > > > Afaict, everyone else chose the blissful ignorance strategy and we are
> > > > the only fools to try and free the dma_mask.
> > > >
> > > > Would you feel more comfortable with:
> > > >
> > > > void *mask = platdev->dev.dma_mask;
> > > >
> > > > platform_device_unregister(platdev);
> > > >
> > > > /* XXX see platform_device_register_full():
> > > > * "This memory isn't freed when the device is put."
> > > > * It's not clear why it hasn't been fixed in a decade...
> > > > */
> > > > kfree(mask);
> > >
> > > Still has the issue that unregister may not the final put, it should but
> > > still...
> > >
> > > I think we just leak the memory. We are not the owner and so shouldn't
> > > be fiddling around trying to free it.
> >
> > Agreed, IMO, we should handle better in the platform device side.
>
> Maybe just use dma_coerce_mask_and_coherent() and avoid the stupid
> kmalloc()?
diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c b/drivers/gpu/drm/i915/intel_lpe_audio.c
index d19053353a2b..a4c20490c48b 100644
--- a/drivers/gpu/drm/i915/intel_lpe_audio.c
+++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
@@ -108,7 +108,6 @@ lpe_audio_platdev_create(struct drm_i915_private *dev_priv)
pinfo.num_res = 2;
pinfo.data = pdata;
pinfo.size_data = sizeof(*pdata);
- pinfo.dma_mask = DMA_BIT_MASK(32);
spin_lock_init(&pdata->lpe_audio_slock);
@@ -119,6 +118,8 @@ lpe_audio_platdev_create(struct drm_i915_private *dev_priv)
goto err;
}
+ dma_coerce_mask_and_coherent(&platdev->dev, DMA_BIT_MASK(32));
+
kfree(rsc);
return platdev;
I think.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
