| Message ID | 20180426223139.16740-25-peda@axentia.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 27, 2018 at 12:31:39AM +0200, Peter Rosin wrote: > If the bridge supplier is unbound, this will bring the bridge consumer > down along with the bridge. Thus, there will no longer linger any > dangling pointers from the bridge consumer (the drm_device) to some > non-existent bridge supplier. > > Signed-off-by: Peter Rosin <peda@axentia.se> Minus the ->owner bikeshed I brought up in the previous patch I agree with this approach as the best way to move forward for now. Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> One small suggestion below, for merging I'd say pls get Jyri's review/tested-by too, since you're both working on the same problem it seems. Aside: Do you want commit rights to drm-misc to be able to push work like this? Cheers, Daniel > --- > drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++ > include/drm/drm_bridge.h | 2 ++ > 2 files changed, 20 insertions(+) > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index a038da696802..f0c79043ec43 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -26,6 +26,7 @@ > #include <linux/mutex.h> > > #include <drm/drm_bridge.h> > +#include <drm/drm_device.h> > #include <drm/drm_encoder.h> > > #include "drm_crtc_internal.h" > @@ -124,12 +125,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, > if (bridge->dev) > return -EBUSY; > > + if (encoder->dev->dev != bridge->owner) { You might end up with a NULL encoder->dev->dev. Perhaps check that and bail with a WARN_ON? > + bridge->link = device_link_add(encoder->dev->dev, > + bridge->owner, 0); > + if (!bridge->link) { > + dev_err(bridge->owner, "failed to link bridge to %s\n", > + dev_name(encoder->dev->dev)); > + return -EINVAL; > + } > + } > + > bridge->dev = encoder->dev; > bridge->encoder = encoder; > > if (bridge->funcs->attach) { > ret = bridge->funcs->attach(bridge); > if (ret < 0) { > + if (bridge->link) > + device_link_del(bridge->link); > + bridge->link = NULL; > bridge->dev = NULL; > bridge->encoder = NULL; > return ret; > @@ -156,6 +170,10 @@ void drm_bridge_detach(struct drm_bridge *bridge) > if (bridge->funcs->detach) > bridge->funcs->detach(bridge); > > + if (bridge->link) > + device_link_del(bridge->link); > + bridge->link = NULL; > + > bridge->dev = NULL; > } > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > index 3bc659f3e7d2..9a386559a41a 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -261,6 +261,7 @@ struct drm_bridge_timings { > * @list: to keep track of all added bridges > * @timings: the timing specification for the bridge, if any (may > * be NULL) > + * @link: drm consumer <-> bridge supplier > * @funcs: control functions > * @driver_private: pointer to the bridge driver's internal context > */ > @@ -271,6 +272,7 @@ struct drm_bridge { > struct drm_bridge *next; > struct list_head list; > const struct drm_bridge_timings *timings; > + struct device_link *link; > > const struct drm_bridge_funcs *funcs; > void *driver_private; > -- > 2.11.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 2018-04-30 17:32, Daniel Vetter wrote: > On Fri, Apr 27, 2018 at 12:31:39AM +0200, Peter Rosin wrote: >> If the bridge supplier is unbound, this will bring the bridge consumer >> down along with the bridge. Thus, there will no longer linger any >> dangling pointers from the bridge consumer (the drm_device) to some >> non-existent bridge supplier. >> >> Signed-off-by: Peter Rosin <peda@axentia.se> > > Minus the ->owner bikeshed I brought up in the previous patch I agree with > this approach as the best way to move forward for now. > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> Thanks, let's see if Laurent is also on-board... > One small suggestion below, for merging I'd say pls get Jyri's > review/tested-by too, since you're both working on the same problem it > seems. Yes, I too would be very happy to see a tested-by from someone. > Aside: Do you want commit rights to drm-misc to be able to push work like > this? If that makes life easier, sure. Cheers, Peter
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index a038da696802..f0c79043ec43 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -26,6 +26,7 @@ #include <linux/mutex.h> #include <drm/drm_bridge.h> +#include <drm/drm_device.h> #include <drm/drm_encoder.h> #include "drm_crtc_internal.h" @@ -124,12 +125,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, if (bridge->dev) return -EBUSY; + if (encoder->dev->dev != bridge->owner) { + bridge->link = device_link_add(encoder->dev->dev, + bridge->owner, 0); + if (!bridge->link) { + dev_err(bridge->owner, "failed to link bridge to %s\n", + dev_name(encoder->dev->dev)); + return -EINVAL; + } + } + bridge->dev = encoder->dev; bridge->encoder = encoder; if (bridge->funcs->attach) { ret = bridge->funcs->attach(bridge); if (ret < 0) { + if (bridge->link) + device_link_del(bridge->link); + bridge->link = NULL; bridge->dev = NULL; bridge->encoder = NULL; return ret; @@ -156,6 +170,10 @@ void drm_bridge_detach(struct drm_bridge *bridge) if (bridge->funcs->detach) bridge->funcs->detach(bridge); + if (bridge->link) + device_link_del(bridge->link); + bridge->link = NULL; + bridge->dev = NULL; } diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 3bc659f3e7d2..9a386559a41a 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -261,6 +261,7 @@ struct drm_bridge_timings { * @list: to keep track of all added bridges * @timings: the timing specification for the bridge, if any (may * be NULL) + * @link: drm consumer <-> bridge supplier * @funcs: control functions * @driver_private: pointer to the bridge driver's internal context */ @@ -271,6 +272,7 @@ struct drm_bridge { struct drm_bridge *next; struct list_head list; const struct drm_bridge_timings *timings; + struct device_link *link; const struct drm_bridge_funcs *funcs; void *driver_private;
If the bridge supplier is unbound, this will bring the bridge consumer down along with the bridge. Thus, there will no longer linger any dangling pointers from the bridge consumer (the drm_device) to some non-existent bridge supplier. Signed-off-by: Peter Rosin <peda@axentia.se> --- drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++ include/drm/drm_bridge.h | 2 ++ 2 files changed, 20 insertions(+)