From: Daniel Vetter <daniel@ffwll.ch> To: Mihail Atanassov <Mihail.Atanassov@arm.com> Cc: "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>, nd <nd@arm.com>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Russell King <rmk+kernel@armlinux.org.uk> Subject: Re: [PATCH 29/30] drm/bridge: add support for device links to bridge Date: Tue, 26 Nov 2019 15:35:34 +0100 [thread overview] Message-ID: <20191126143534.GW29965@phenom.ffwll.local> (raw) In-Reply-To: <20191126131541.47393-30-mihail.atanassov@arm.com> On Tue, Nov 26, 2019 at 01:16:26PM +0000, Mihail Atanassov wrote: > From: Russell King <rmk+kernel@armlinux.org.uk> > > 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@armlinux.org.uk> > Tested-by: Mihail Atanassov <mihail.atanassov@arm.com> > [reworked to use drm_bridge_init() for setting bridge->device] > Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com> 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? - 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. 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) > + 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@samsung.com>"); > 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); > > -- > 2.23.0 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Vetter <daniel@ffwll.ch> To: Mihail Atanassov <Mihail.Atanassov@arm.com> Cc: David Airlie <airlied@linux.ie>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Russell King <rmk+kernel@armlinux.org.uk>, "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>, nd <nd@arm.com> Subject: Re: [PATCH 29/30] drm/bridge: add support for device links to bridge Date: Tue, 26 Nov 2019 15:35:34 +0100 [thread overview] Message-ID: <20191126143534.GW29965@phenom.ffwll.local> (raw) Message-ID: <20191126143534.rUFa9g17_94ML1MYqgPKL9gzd4xWNPV5EMgiOsQUVYw@z> (raw) In-Reply-To: <20191126131541.47393-30-mihail.atanassov@arm.com> On Tue, Nov 26, 2019 at 01:16:26PM +0000, Mihail Atanassov wrote: > From: Russell King <rmk+kernel@armlinux.org.uk> > > 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@armlinux.org.uk> > Tested-by: Mihail Atanassov <mihail.atanassov@arm.com> > [reworked to use drm_bridge_init() for setting bridge->device] > Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com> 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? - 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. 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) > + 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@samsung.com>"); > 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); > > -- > 2.23.0 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2019-11-26 14:35 UTC|newest] Thread overview: 110+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-26 13:15 [PATCH 00/30] drm/bridge: Add device links for lifetime control Mihail Atanassov 2019-11-26 13:15 ` Mihail Atanassov 2019-11-26 13:15 ` Mihail Atanassov 2019-11-26 13:15 ` [PATCH 01/30] drm: Introduce drm_bridge_init() Mihail Atanassov 2019-11-26 13:15 ` Mihail Atanassov 2019-11-26 14:26 ` Daniel Vetter 2019-11-26 14:26 ` Daniel Vetter 2019-11-26 15:55 ` Mihail Atanassov 2019-11-26 15:55 ` Mihail Atanassov 2019-11-26 17:04 ` Daniel Vetter 2019-11-26 17:04 ` Daniel Vetter 2019-11-26 19:24 ` Sam Ravnborg 2019-11-26 19:24 ` Sam Ravnborg 2019-11-27 11:05 ` Mihail Atanassov 2019-11-27 11:05 ` Mihail Atanassov 2019-11-27 11:05 ` Mihail Atanassov 2019-11-27 11:31 ` Daniel Vetter 2019-11-27 11:31 ` Daniel Vetter 2019-12-02 5:55 ` [01/30] " james qian wang (Arm Technology China) 2019-12-02 5:55 ` james qian wang (Arm Technology China) 2019-12-02 8:49 ` Daniel Vetter 2019-12-02 8:49 ` Daniel Vetter 2019-12-03 6:12 ` james qian wang (Arm Technology China) 2019-12-03 6:12 ` james qian wang (Arm Technology China) 2019-11-26 13:16 ` [PATCH 02/30] drm/bridge: adv7511: Use drm_bridge_init() Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` [PATCH 03/30] drm/bridge: anx6345: " Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` [PATCH 04/30] drm/bridge: anx78xx: " Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` [PATCH 05/30] drm/bridge: cdns: " Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` [PATCH 06/30] drm/bridge: dumb-vga-dac: " Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` [PATCH 07/30] drm/bridge: lvds-encoder: " Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` [PATCH 08/30] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: " Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` [PATCH 09/30] drm/bridge: nxp-ptn3460: " Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` [PATCH 10/30] drm/bridge: panel: " Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` [PATCH 11/30] drm/bridge: ps8622: " Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` [PATCH 12/30] drm/bridge: sii902x: " Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` [PATCH 13/30] gpu: drm: bridge: sii9234: " Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` [PATCH 14/30] drm/bridge: sil_sii8620: " Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` [PATCH 15/30] drm/bridge: dw-hdmi: " Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` [PATCH 17/30] drm/bridge: tc358764: " Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` [PATCH 16/30] drm/bridge/synopsys: dsi: " Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` [PATCH 18/30] drm/bridge: tc358767: " Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` [PATCH 19/30] drm/bridge: thc63: " Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` [PATCH 20/30] drm/bridge: ti-sn65dsi86: " Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` [PATCH 21/30] drm/bridge: ti-tfp410: " Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` [PATCH 22/30] drm/exynos: mic: " Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-12-03 4:54 ` Inki Dae 2019-12-03 4:54 ` Inki Dae 2019-12-03 4:54 ` Inki Dae 2019-11-26 13:16 ` [PATCH 23/30] drm/i2c: tda998x: " Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` [PATCH 24/30] drm/mcde: dsi: " Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-28 9:08 ` Linus Walleij 2019-11-28 9:08 ` Linus Walleij 2019-11-26 13:16 ` [PATCH 25/30] drm/mediatek: hdmi: " Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` [PATCH 26/30] drm: rcar-du: lvds: " Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` [PATCH 27/30] drm: rcar-du: lvds: Don't set drm_bridge private pointer Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 13:16 ` [PATCH 28/30] drm/sti: sti_vdo: Use drm_bridge_init() Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 19:37 ` Sam Ravnborg 2019-11-26 19:37 ` Sam Ravnborg 2019-11-27 11:02 ` Mihail Atanassov 2019-11-27 11:02 ` Mihail Atanassov 2019-11-27 16:19 ` Sam Ravnborg 2019-11-27 16:19 ` Sam Ravnborg 2019-11-27 16:30 ` Benjamin Gaignard 2019-11-27 16:30 ` Benjamin Gaignard 2019-11-26 13:16 ` [PATCH 29/30] drm/bridge: add support for device links to bridge Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 14:35 ` Daniel Vetter [this message] 2019-11-26 14:35 ` Daniel Vetter 2019-11-26 15:55 ` Mihail Atanassov 2019-11-26 15:55 ` Mihail Atanassov 2019-11-28 15:33 ` Mihail Atanassov 2019-11-28 15:33 ` Mihail Atanassov 2019-11-26 13:16 ` [PATCH 30/30] drm/komeda: Use drm_bridge interface for pipe outputs Mihail Atanassov 2019-11-26 13:16 ` Mihail Atanassov 2019-11-26 15:27 ` [PATCH 00/30] drm/bridge: Add device links for lifetime control Russell King - ARM Linux admin 2019-11-26 15:27 ` Russell King - ARM Linux admin 2019-11-26 15:27 ` Russell King - ARM Linux admin 2019-11-26 15:55 ` Mihail Atanassov 2019-11-26 15:55 ` Mihail Atanassov 2019-11-26 15:55 ` Mihail Atanassov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20191126143534.GW29965@phenom.ffwll.local \ --to=daniel@ffwll.ch \ --cc=Mihail.Atanassov@arm.com \ --cc=airlied@linux.ie \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux-kernel@vger.kernel.org \ --cc=maarten.lankhorst@linux.intel.com \ --cc=mripard@kernel.org \ --cc=nd@arm.com \ --cc=rmk+kernel@armlinux.org.uk \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.