On Fri, 14 Mar 2025 19:08:22 +0100 Maxime Ripard <mripard@xxxxxxxxxx> wrote: > On Fri, Mar 14, 2025 at 11:31:18AM +0100, Luca Ceresoli wrote: > > Many functions get a drm_bridge pointer, only use it in the function body > > (or a smaller scope such as a loop body), and don't store it. In these > > cases they always need to drm_bridge_put() it before returning (or exiting > > the scope). > > > > Some of those functions have complex code paths with multiple return points > > or loop break/continue. This makes adding drm_bridge_put() in the right > > places tricky, ugly and error prone in case of future code changes. > > > > Others use the bridge pointer in the return statement and would need to > > split the return line to fit the drm_bridge_put, which is a bit annoying: > > > > -return some_thing(bridge); > > +ret = some_thing(bridge); > > +drm_bridge_put(bridge); > > +return ret; > > > > To make it easier for all of them to put the bridge reference correctly > > without complicating code, define a scope-based cleanup action to be used > > with __free(). > > > > Signed-off-by: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx> > > > > --- > > > > This patch was added in v7. > > --- > > include/drm/drm_bridge.h | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > > index 5c1e2b9cafb12eb429d1f5d3ef312e6cf9b54f47..a5accd64c364ebb57903ae1e7459034ad9ebf4f3 100644 > > --- a/include/drm/drm_bridge.h > > +++ b/include/drm/drm_bridge.h > > @@ -23,6 +23,7 @@ > > #ifndef __DRM_BRIDGE_H__ > > #define __DRM_BRIDGE_H__ > > > > +#include <linux/cleanup.h> > > #include <linux/ctype.h> > > #include <linux/list.h> > > #include <linux/mutex.h> > > @@ -995,6 +996,9 @@ static inline struct drm_bridge *drm_bridge_put(struct drm_bridge *bridge) > > return bridge; > > } > > > > +/* Cleanup action for use with __free() */ > > +DEFINE_FREE(drm_bridge_put, struct drm_bridge *, if (_T) drm_bridge_put(_T)) > > + > > IIRC, drm_bridge_put already checks for the pointer being null before > dereferencing it, right? Then we can probably simplify the macro here. drm_bridge_put() does the NULL-check, yes. However I have kept the 'if' here for consistency with all other DEFINE_FREE()s in the kernel which also have an 'if' even when the called function does it as well. Moreover, as per discussion after patch 2, in the next iteration I'm moving drm_bridge_put() back to a an exported symbol instead of an inline. So the 'if' here will save a useless function call on NULL. For the two above reasons I'm rather inclined to keeping this line as is. > Either way, > > Reviewed-by: Maxime Ripard <mripard@xxxxxxxxxx> Luca -- Luca Ceresoli, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
