| Message ID | 1409247613-14232-3-git-send-email-gustavo@padovan.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 28 Aug 2014, Gustavo Padovan <gustavo@padovan.org> wrote: > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > If the save_encoder_crtcs or save_connector_encoders allocation fail > we need to free everything we have already allocated. There is no memleak, because the caller of intel_set_config_save_state() checks the return value, and subsequently handles the error with a call to intel_set_config_free(), which does the right thing. I know one could argue this should be done within intel_set_config_save_state() but I'm not convinced. I'd let this be as it is. Just in case Daniel disagrees with me here and wants to merge, the patch does look correct. So r-b for correctness, but nak on merging from me. ;) BR, Jani. > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > --- > drivers/gpu/drm/i915/intel_display.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 05937fe..a8a8abe 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11018,13 +11018,13 @@ static int intel_set_config_save_state(struct drm_device *dev, > kcalloc(dev->mode_config.num_encoder, > sizeof(struct drm_crtc *), GFP_KERNEL); > if (!config->save_encoder_crtcs) > - return -ENOMEM; > + goto free_crtc; > > config->save_connector_encoders = > kcalloc(dev->mode_config.num_connector, > sizeof(struct drm_encoder *), GFP_KERNEL); > if (!config->save_connector_encoders) > - return -ENOMEM; > + goto free_encoder; > > /* Copy data. Note that driver private data is not affected. > * Should anything bad happen only the expected state is > @@ -11046,6 +11046,12 @@ static int intel_set_config_save_state(struct drm_device *dev, > } > > return 0; > + > +free_encoder: > + kfree(config->save_encoder_crtcs); > +free_crtc: > + kfree(config->save_crtc_enabled); > + return -ENOMEM; > } > > static void intel_set_config_restore_state(struct drm_device *dev, > -- > 1.9.3 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Aug 29, 2014 at 10:38:43AM +0300, Jani Nikula wrote: > On Thu, 28 Aug 2014, Gustavo Padovan <gustavo@padovan.org> wrote: > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > > If the save_encoder_crtcs or save_connector_encoders allocation fail > > we need to free everything we have already allocated. > > There is no memleak, because the caller of intel_set_config_save_state() > checks the return value, and subsequently handles the error with a call > to intel_set_config_free(), which does the right thing. > > I know one could argue this should be done within > intel_set_config_save_state() but I'm not convinced. I'd let this be as > it is. > > Just in case Daniel disagrees with me here and wants to merge, the patch > does look correct. So r-b for correctness, but nak on merging from > me. ;) You just said it triggers a double free... And you would be right. -Chris
On Fri, 29 Aug 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Fri, Aug 29, 2014 at 10:38:43AM +0300, Jani Nikula wrote: >> On Thu, 28 Aug 2014, Gustavo Padovan <gustavo@padovan.org> wrote: >> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >> > >> > If the save_encoder_crtcs or save_connector_encoders allocation fail >> > we need to free everything we have already allocated. >> >> There is no memleak, because the caller of intel_set_config_save_state() >> checks the return value, and subsequently handles the error with a call >> to intel_set_config_free(), which does the right thing. >> >> I know one could argue this should be done within >> intel_set_config_save_state() but I'm not convinced. I'd let this be as >> it is. >> >> Just in case Daniel disagrees with me here and wants to merge, the patch >> does look correct. So r-b for correctness, but nak on merging from >> me. ;) > > You just said it triggers a double free... And you would be right. Thanks Chris. I'll retract any hint of r-b I was giving. NAK. BR, Jani. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 05937fe..a8a8abe 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11018,13 +11018,13 @@ static int intel_set_config_save_state(struct drm_device *dev, kcalloc(dev->mode_config.num_encoder, sizeof(struct drm_crtc *), GFP_KERNEL); if (!config->save_encoder_crtcs) - return -ENOMEM; + goto free_crtc; config->save_connector_encoders = kcalloc(dev->mode_config.num_connector, sizeof(struct drm_encoder *), GFP_KERNEL); if (!config->save_connector_encoders) - return -ENOMEM; + goto free_encoder; /* Copy data. Note that driver private data is not affected. * Should anything bad happen only the expected state is @@ -11046,6 +11046,12 @@ static int intel_set_config_save_state(struct drm_device *dev, } return 0; + +free_encoder: + kfree(config->save_encoder_crtcs); +free_crtc: + kfree(config->save_crtc_enabled); + return -ENOMEM; } static void intel_set_config_restore_state(struct drm_device *dev,