On Wed, Nov 29, 2017 at 08:11:53PM +0000, Russell King - ARM Linux wrote:
> On Wed, Nov 29, 2017 at 03:03:09PM -0500, Sean Paul wrote:
> > On Wed, Nov 29, 2017 at 07:16:43PM +0000, Russell King - ARM Linux wrote:
> > > On Wed, Nov 29, 2017 at 09:53:42AM -0500, Sean Paul wrote:
> > > > On Wed, Nov 29, 2017 at 11:45:43AM +0000, Russell King wrote:
> > > > > Fix the leak of the CRTC structure in the failure paths of
> > > > > armada_drm_crtc_create().
> > > > >
> > > > > Signed-off-by: Russell King <rmk+kernel@xxxxxxxxxxxxxxx>
> > > > > ---
> > > > > drivers/gpu/drm/armada/armada_crtc.c | 10 ++++++++--
> > > > > 1 file changed, 8 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c
> > > > > index 2e065facdce7..844d488b6654 100644
> > > > > --- a/drivers/gpu/drm/armada/armada_crtc.c
> > > > > +++ b/drivers/gpu/drm/armada/armada_crtc.c
> > > > > @@ -1246,11 +1246,14 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
> > > > > dcrtc->crtc.port = port;
> > > > >
> > > > > primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> > > > > - if (!primary)
> > > > > + if (!primary) {
> > > > > + kfree(dcrtc);
> > > > > return -ENOMEM;
> > > > > + }
> > > > >
> > > > > ret = armada_drm_plane_init(primary);
> > > > > if (ret) {
> > > > > + kfree(dcrtc);
> > > > > kfree(primary);
> > > > > return ret;
> > > > > }
> > > > > @@ -1262,14 +1265,17 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
> > > > > NULL,
> > > > > DRM_PLANE_TYPE_PRIMARY, NULL);
> > > > > if (ret) {
> > > > > + kfree(dcrtc);
> > > > > kfree(primary);
> > > > > return ret;
> > > > > }
> > > > >
> > > > > ret = drm_crtc_init_with_planes(drm, &dcrtc->crtc, &primary->base, NULL,
> > > > > &armada_crtc_funcs, NULL);
> > > > > - if (ret)
> > > > > + if (ret) {
> > > > > + kfree(dcrtc);
> > > > > goto err_crtc_init;
> > > >
> > > > The mixed use of cleaning up at every exit and the lone label is a bit messy and
> > > > prone to bugs such as the one you're fixing. How about you label everything
> > > > and then just clean up once?
> > >
> > > It's not that simple - see alternative patch below. How we clean
> > > stuff up depends on how far through the initialisation we are, and
> > > that doesn't lend itself to a sequential cleanup methodology.
> >
> > It can be that simple, depending on how you setup your initialization sequence.
> > For instance, if armada_plane_init did the allocation as well as
> > universal_plane_init (like armada_overlay_plane_create already does), then you
> > wouldn't need to worry about kfree(primary) at all.
>
> Two problems with that:
>
> 1. The underlying plane structures for the primary and overlay planes
> are different.
>
> 2. the calls to drm_universal_plane_init() are rather different:
>
> - primary:
> ret = drm_universal_plane_init(drm, &primary->base, 0,
> &armada_primary_plane_funcs,
> armada_primary_formats,
> ARRAY_SIZE(armada_primary_formats),
> NULL,
> DRM_PLANE_TYPE_PRIMARY, NULL);
>
> - overlay:
> ret = drm_universal_plane_init(dev, &dplane->base.base, crtcs,
> &armada_ovl_plane_funcs,
> armada_ovl_formats,
> ARRAY_SIZE(armada_ovl_formats),
> NULL,
> DRM_PLANE_TYPE_OVERLAY, NULL);
>
> Moving the drm_universal_plane_init() call into armada_drm_plane_init()
> in a patch to fix a leak doesn't make sense - you're actually asking for
> a massive rewrite of the code (I do have patches which I'm intending to
> send after this series which do quite a lot of re-organisation of this
> plane code, but I want to get the fixes sorted before I post that.)
Well, not a massive rewrite, but more code than the current patch. At any
rate, I just thought it would be helpful to suggest ways of improving your
cleanup paths.
I didn't know you had a bigger reorg coming down the pipe. Feel free to
take or leave the advice as you see fit!
Sean
>
> --
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
> According to speedtest.net: 8.21Mbps down 510kbps up
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
