On Tuesday, 26 November 2019 14:35:34 GMT Daniel Vetter wrote:
> On Tue, Nov 26, 2019 at 01:16:26PM +0000, Mihail Atanassov wrote:
> > From: Russell King <rmk+kernel@xxxxxxxxxxxxxxx>
> >
> > Bridge devices have been a potential for kernel oops as their lifetime
> > is independent of the DRM device that they are bound to. Hence, if a
> > bridge device is unbound while the parent DRM device is using them, the
> > parent happily continues to use the bridge device, calling the driver
> > and accessing its objects that have been freed.
> >
> > This can cause kernel memory corruption and kernel oops.
> >
> > To control this, use device links to ensure that the parent DRM device
> > is unbound when the bridge device is unbound, and when the bridge
> > device is re-bound, automatically rebind the parent DRM device.
> >
> > Signed-off-by: Russell King <rmk+kernel@xxxxxxxxxxxxxxx>
> > Tested-by: Mihail Atanassov <mihail.atanassov@xxxxxxx>
> > [reworked to use drm_bridge_init() for setting bridge->device]
> > Signed-off-by: Mihail Atanassov <mihail.atanassov@xxxxxxx>
>
> So I thought the big plan was to put the device_link setup into
> drm_bridge_attach, so that it's done for everyone. And we could then
> slowly go through the existing drivers that use the component framework to
> get this handled correctly.
>
> So my questions:
> - is there a problem if we add the device_link for everyone?
Possibly not, but I didn't want to stir the entire pot :). This is
safer in the sense that it's an opt-in, so a lower chance of
regressions in code and setups that I can't possibly test. If you
think it's worth sticking it in the existing code paths, I can
certainly do that too.
> - is there an issue if we only add it at drm_bridge_attach time? I kinda
> assumed that it's not needed before that (EPROBE_DEFER should handle
> load dependencies as before), but it could be that some drivers ask for
> a bridge and then check more stuff and then drop the bridge without
> calling drm_bridge_attach. We probably don't have a case like this yet,
> but better robust than sorry.
I think there would be a race there:
- bridge driver calls drm_bridge_add() in their probe()
- client driver calls of_drm_find_bridge() in their probe()
- bridge driver gets removed, calls drm_bridge_remove()
- client driver uses the now invalid drm_bridge pointer from above to
do drm_bridge_attach()
With of_drm_bridge_find_devlink(), you get the device_link inside the
bridge_lock so the reference to the drm_bridge will either be valid, or
your driver gets removed right after it's probed, so that the bridge
can be removed, too.
In patch 30/30 I use both the _devlink and the non-_devlink versions
of of_drm_find_bridge(), but I guess there's no harm adding another
refcount on the link, it'll get destroyed if the bridge is removed
regardless, although that may need a DL_FLAG_AUTOREMOVE_CONSUMER.
>
> Anyway, I scrolled through the bridge patches, looked all good, huge
> thanks for tackling this! Once we have some agreement on the bigger
> questions here I'll try to go through them and review.
>
> Cheers, Daniel
> > ---
> > drivers/gpu/drm/drm_bridge.c | 49 ++++++++++++++++++++++++++----------
> > include/drm/drm_bridge.h | 4 +++
> > 2 files changed, 40 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index cbe680aa6eac..e1f8db84651a 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"
> > @@ -109,6 +110,7 @@ void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> > bridge->encoder = NULL;
> > bridge->next = NULL;
> >
> > + bridge->device = dev;
> > #ifdef CONFIG_OF
> > bridge->of_node = dev->of_node;
> > #endif
> > @@ -492,6 +494,32 @@ void drm_atomic_bridge_enable(struct drm_bridge *bridge,
> > EXPORT_SYMBOL(drm_atomic_bridge_enable);
> >
> > #ifdef CONFIG_OF
> > +static struct drm_bridge *drm_bridge_find(struct drm_device *dev,
> > + struct device_node *np, bool link)
> > +{
> > + struct drm_bridge *bridge, *found = NULL;
> > + struct device_link *dl;
> > +
> > + mutex_lock(&bridge_lock);
> > +
> > + list_for_each_entry(bridge, &bridge_list, list)
> > + if (bridge->of_node == np) {
> > + found = bridge;
> > + break;
> > + }
> > +
> > + if (found && link) {
> > + dl = device_link_add(dev->dev, found->device,
> > + DL_FLAG_AUTOPROBE_CONSUMER);
> > + if (!dl)mutex
> > + found = NULL;
> > + }
> > +
> > + mutex_unlock(&bridge_lock);
> > +
> > + return found;
> > +}
> > +
> > /**
> > * of_drm_find_bridge - find the bridge corresponding to the device node in
> > * the global bridge list
> > @@ -503,21 +531,16 @@ EXPORT_SYMBOL(drm_atomic_bridge_enable);
> > */
> > struct drm_bridge *of_drm_find_bridge(struct device_node *np)
> > {
> > - struct drm_bridge *bridge;
> > -
> > - mutex_lock(&bridge_lock);
> > -
> > - list_for_each_entry(bridge, &bridge_list, list) {
> > - if (bridge->of_node == np) {
> > - mutex_unlock(&bridge_lock);
> > - return bridge;
> > - }
> > - }
> > -
> > - mutex_unlock(&bridge_lock);
> > - return NULL;
> > + return drm_bridge_find(NULL, np, false);
> > }
> > EXPORT_SYMBOL(of_drm_find_bridge);
> > +
> > +struct drm_bridge *of_drm_find_bridge_devlink(struct drm_device *dev,
> > + struct device_node *np)
> > +{
> > + return drm_bridge_find(dev, np, true);
> > +}
> > +EXPORT_SYMBOL(of_drm_find_bridge_devlink);
> > #endif
> >
> > MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@xxxxxxxxxxx>");
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index d6d9d5301551..68b27c69cc3d 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -382,6 +382,8 @@ struct drm_bridge {
> > struct drm_encoder *encoder;
> > /** @next: the next bridge in the encoder chain */
> > struct drm_bridge *next;
> > + /** @device: Linux driver model device */
> > + struct device *device;
> > #ifdef CONFIG_OF
> > /** @of_node: device node pointer to the bridge */
> > struct device_node *of_node;
> > @@ -407,6 +409,8 @@ void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> > const struct drm_bridge_timings *timings,
> > void *driver_private);
> > struct drm_bridge *of_drm_find_bridge(struct device_node *np);
> > +struct drm_bridge *of_drm_find_bridge_devlink(struct drm_device *dev,
> > + struct device_node *np);
> > int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
> > struct drm_bridge *previous);
> >
>
>
--
Mihail
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
