Re: [PATCH v9 2/6] video: add of helper for videomode

[Steffen Trumtrar <s.trumtrar@pengutronix.de>]

On Wed, Nov 14, 2012 at 01:00:45PM +0100, Thierry Reding wrote:
> On Wed, Nov 14, 2012 at 12:43:19PM +0100, Steffen Trumtrar wrote:
> [...]
> > +display-timings bindings
> > +============
> > +
> > +display timings node
> 
> I didn't express myself very clearly here =). The way I think this
> should be written is "display-timings node".
> 
> > +required properties:
> > + - hactive, vactive: Display resolution
> > + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
> > +   in pixels
> > +   vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
> > +   lines
> > + - clock-frequency: displayclock in Hz
> 
> I still think "displayclock" should be two words: "display clock".
> 
> > +/**
> > + * of_get_display_timings - parse all display_timing entries from a device_node
> > + * @np: device_node with the subnodes
> > + **/
> > +struct display_timings *of_get_display_timings(struct device_node *np)
> > +{
> [...]
> > +	disp->num_timings = 0;
> > +	disp->native_mode = 0;
> > +
> > +	for_each_child_of_node(timings_np, entry) {
> > +		struct display_timing *dt;
> > +
> > +		dt = of_get_display_timing(entry);
> > +		if (!dt) {
> > +			/* to not encourage wrong devicetrees, fail in case of an error */
> > +			pr_err("%s: error in timing %d\n", __func__, disp->num_timings+1);
> > +			goto timingfail;
> 
> I believe you're still potentially leaking memory here. In case you have
> 5 entries for instance, and the last one fails to parse, then this will
> cause the memory allocated for the 4 correct entries to be lost.
> 
> Can't you just call display_timings_release() in this case and then jump
> to dispfail? That would still leak the native_mode device node. Maybe it
> would be better to keep timingfail but modify it to free the display
> timings with display_timings_release() instead? See below.
> 
> > +		}
> > +
> > +		if (native_mode = entry)
> > +			disp->native_mode = disp->num_timings;
> > +
> > +		disp->timings[disp->num_timings] = dt;
> > +		disp->num_timings++;
> > +	}
> > +	of_node_put(timings_np);
> > +	of_node_put(native_mode);
> > +
> > +	if (disp->num_timings > 0)
> > +		pr_info("%s: got %d timings. Using timing #%d as default\n", __func__,
> > +			disp->num_timings , disp->native_mode + 1);
> > +	else {
> > +		pr_err("%s: no valid timings specified\n", __func__);
> > +		display_timings_release(disp);
> > +		return NULL;
> > +	}
> > +	return disp;
> > +
> > +timingfail:
> > +	if (native_mode)
> > +		of_node_put(native_mode);
> > +	kfree(disp->timings);
> 
> Call display_timings_release(disp) instead of kfree(disp->timings)?
> 

Yes. That would be the appropriate way to fail here. Done.

> > diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
> > new file mode 100644
> > index 0000000..4138758
> > --- /dev/null
> > +++ b/include/linux/of_videomode.h
> > @@ -0,0 +1,16 @@
> > +/*
> > + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > + *
> > + * videomode of-helpers
> > + *
> > + * This file is released under the GPLv2
> > + */
> > +
> > +#ifndef __LINUX_OF_VIDEOMODE_H
> > +#define __LINUX_OF_VIDEOMODE_H
> > +
> > +#include <linux/videomode.h>
> > +#include <linux/of.h>
> > +
> > +int of_get_videomode(struct device_node *np, struct videomode *vm, int index);
> > +#endif /* __LINUX_OF_VIDEOMODE_H */
> 
> Nit: should have a blank line before #endif.
> 
> Thierry



> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |