On Mon, May 14, 2018 at 10:40 PM, Peter Rosin <p...@axentia.se> wrote: > On 2018-05-14 18:28, Daniel Vetter wrote: >> On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote: >>> On 2018-05-10 10:10, Andrzej Hajda wrote: >>>> On 04.05.2018 15:52, 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 <p...@axentia.se> >>>>> --- >>>>> 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 78d186b6831b..0259f0a3ff27 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" >>>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, >>>>> struct drm_bridge *bridge, >>>>> if (bridge->dev) >>>>> return -EBUSY; >>>>> >>>>> + if (encoder->dev->dev != bridge->odev) { >>>> >>>> I wonder why device_link_add does not handle this case (self dependency) >>>> silently as noop, as it seems to be a correct behavior. >>> >>> It's kind-of a silly corner-case though, so perfectly understandable >>> that it isn't handled. >>> >>>>> + bridge->link = device_link_add(encoder->dev->dev, >>>>> + bridge->odev, 0); >>>>> + if (!bridge->link) { >>>>> + dev_err(bridge->odev, "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; >>>>> @@ -159,6 +173,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 b656e505d11e..804189c63a4c 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 >>>> >>>> Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer >>>> to the bridge" would be better. >>> >>> I meant "<->" to indicate that the link is bidirectional, not that the >>> relationship is in any way symmetric. I wasn't aware of any implication >>> of a symmetric relationship when using "<->", do you have a reference? >>> But I guess the different arrow notations in math are somewhat overloaded >>> and that someone at some point must have used "<->" to indicate a >>> symmetric relationship... >> >> Yeah I agree with Andrzej here, for me <-> implies a symmetric >> relationship. Spelling it out like Andrzej suggested sounds like the >> better idea. >> -Daniel > > Ok, I guess that means I have to do a v3 after all. Or can this > trivial documentation update be done by the committer? I hate to > spam everyone with another volley... > > Or perhaps I should squash patches 2-23 that are all rather similar > and mechanic? I separated them to allow for easier review from > individual driver maintainers, but that didn't seem to happen > anyway...
Do another volley of the full set, or in-reply-to to just replace the patch that needs to be respun (but some people don't like that). When resending just make sure you're picking up all the acks/r-bs you have already. -Daniel > Cheers, > Peter > >> >>> >>>> Anyway: >>>> Reviewed-by: Andrzej Hajda <a.ha...@samsung.com> >>> >>> Thanks! >>> >>> Cheers, >>> Peter >>> >>>> -- >>>> Regards >>>> Andrzej >>>> >>>>> * @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; >>>> >>>> >>> >> > > _______________________________________________ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch