Hi Russell, On Tuesday, 22 October 2019 15:53:29 BST Russell King - ARM Linux admin wrote: > On Tue, Oct 22, 2019 at 03:42:07PM +0100, Russell King - ARM Linux admin wrote: > > On Tue, Oct 22, 2019 at 10:50:42AM +0200, Daniel Vetter wrote: > > > On Tue, Oct 22, 2019 at 10:48 AM Russell King - ARM Linux admin > > > <linux@xxxxxxxxxxxxxxx> wrote: > > > > I had a patches, which is why I raised the problem with the core: > > > > > > > > 6961edfee26d bridge hacks using device links > > > > > > > > but it never went further than an experiment at the time because of the > > > > problems in the core. As it was a hack, it never got posted. Seems > > > > that kernel tree (for the cubox) is still 5.2 based, so has a lot of > > > > patches and might need updating to a more recent base before anything > > > > can be tested. > > > > > > > > > For reference, the panel patch: > > > > > > https://patchwork.kernel.org/patch/10364873/ > > > > > > And the huge discussion around bridges, that resulted in Rafael > > > Wyzocki fixing all the core issues: > > > > > > https://www.spinics.net/lists/dri-devel/msg201927.html > > > > > > James, do you want to look into this for bridges? > > > > I can now confirm that it does work. > > Something like this - it's based off an older kernel, so may be missing > some of the bridge drivers, but should be sufficient for people to test > with. Thanks for the patch, I tested to the limit that our driver allows at the moment -- rmmod'ing the bridge while the driver is not in use -- and I don't see any issues there. komeda successfully gets removed then re-probed once the bridge reappears. For that part, Tested-by: Mihail Atanassov <mihail.atanassov@xxxxxxx> I couldn't sadly check a hot unplug since we have an mm bug or two that I need to sort out first. If anyone else has a hot-unplug capable driver and can test, it'd be good to ensure that also functions properly. > > 8<==== > From: Russell King <rmk+kernel@xxxxxxxxxxxxxxx> > Subject: [PATCH] drm/bridge: add support for device links to bridge > > 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> > --- > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 1 + > drivers/gpu/drm/bridge/analogix-anx78xx.c | 1 + > drivers/gpu/drm/bridge/dumb-vga-dac.c | 1 + > drivers/gpu/drm/bridge/lvds-encoder.c | 1 + > .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 1 + > drivers/gpu/drm/bridge/nxp-ptn3460.c | 1 + > drivers/gpu/drm/bridge/panel.c | 1 + > drivers/gpu/drm/bridge/parade-ps8622.c | 1 + > drivers/gpu/drm/bridge/sii902x.c | 1 + > drivers/gpu/drm/bridge/sii9234.c | 1 + > drivers/gpu/drm/bridge/sil-sii8620.c | 1 + > drivers/gpu/drm/bridge/tc358767.c | 1 + > drivers/gpu/drm/bridge/ti-tfp410.c | 1 + > drivers/gpu/drm/drm_bridge.c | 48 ++++++++++++++----- > drivers/gpu/drm/i2c/tda998x_drv.c | 1 + > include/drm/drm_bridge.h | 4 ++ > 16 files changed, 53 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > index f6d2681f6927..6a5906da58ea 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > @@ -1217,6 +1217,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) > goto err_unregister_cec; > > adv7511->bridge.funcs = &adv7511_bridge_funcs; > + adv7511->bridge.device = dev; > adv7511->bridge.of_node = dev->of_node; > > drm_bridge_add(&adv7511->bridge); > diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix-anx78xx.c > index 3c7cc5af735c..77ff17c38037 100644 > --- a/drivers/gpu/drm/bridge/analogix-anx78xx.c > +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c > @@ -1323,6 +1323,7 @@ static int anx78xx_i2c_probe(struct i2c_client *client, > > mutex_init(&anx78xx->lock); > > + anx78xx->bridge.device = &client->dev; > #if IS_ENABLED(CONFIG_OF) > anx78xx->bridge.of_node = client->dev.of_node; > #endif > diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c > index d32885b906ae..40169920560e 100644 > --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c > +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c > @@ -202,6 +202,7 @@ static int dumb_vga_probe(struct platform_device *pdev) > } > > vga->bridge.funcs = &dumb_vga_bridge_funcs; > + vga->bridge.device = &pdev->dev; > vga->bridge.of_node = pdev->dev.of_node; > vga->bridge.timings = of_device_get_match_data(&pdev->dev); > > diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c > index 2ab2c234f26c..5012be35a5fb 100644 > --- a/drivers/gpu/drm/bridge/lvds-encoder.c > +++ b/drivers/gpu/drm/bridge/lvds-encoder.c > @@ -115,6 +115,7 @@ static int lvds_encoder_probe(struct platform_device *pdev) > * to look up. > */ > lvds_encoder->bridge.of_node = dev->of_node; > + lvds_encoder->bridge.device = dev; > lvds_encoder->bridge.funcs = &funcs; > drm_bridge_add(&lvds_encoder->bridge); > > diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c > index 79311f8354bd..e211c57f6f56 100644 > --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c > +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c > @@ -304,6 +304,7 @@ static int stdp4028_ge_b850v3_fw_probe(struct i2c_client *stdp4028_i2c, > > /* drm bridge initialization */ > ge_b850v3_lvds_ptr->bridge.funcs = &ge_b850v3_lvds_funcs; > + ge_b850v3_lvds_ptr->bridge.device = dev; > ge_b850v3_lvds_ptr->bridge.of_node = dev->of_node; > drm_bridge_add(&ge_b850v3_lvds_ptr->bridge); > > diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c > index 98bc650b8c95..00097e314575 100644 > --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c > +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c > @@ -323,6 +323,7 @@ static int ptn3460_probe(struct i2c_client *client, > } > > ptn_bridge->bridge.funcs = &ptn3460_bridge_funcs; > + ptn_bridge->bridge.device = dev; > ptn_bridge->bridge.of_node = dev->of_node; > drm_bridge_add(&ptn_bridge->bridge); > > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c > index b12ae3a4c5f1..eab7126f0d61 100644 > --- a/drivers/gpu/drm/bridge/panel.c > +++ b/drivers/gpu/drm/bridge/panel.c > @@ -168,6 +168,7 @@ struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel, > panel_bridge->panel = panel; > > panel_bridge->bridge.funcs = &panel_bridge_bridge_funcs; > + panel_bridge->bridge.device = panel->dev; > #ifdef CONFIG_OF > panel_bridge->bridge.of_node = panel->dev->of_node; > #endif > diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c b/drivers/gpu/drm/bridge/parade-ps8622.c > index 2d88146e4836..ff79df0ff183 100644 > --- a/drivers/gpu/drm/bridge/parade-ps8622.c > +++ b/drivers/gpu/drm/bridge/parade-ps8622.c > @@ -589,6 +589,7 @@ static int ps8622_probe(struct i2c_client *client, > } > > ps8622->bridge.funcs = &ps8622_bridge_funcs; > + ps8622->bridge.device = dev; > ps8622->bridge.of_node = dev->of_node; > drm_bridge_add(&ps8622->bridge); > > diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c > index dd7aa466b280..ef768b149bee 100644 > --- a/drivers/gpu/drm/bridge/sii902x.c > +++ b/drivers/gpu/drm/bridge/sii902x.c > @@ -991,6 +991,7 @@ static int sii902x_probe(struct i2c_client *client, > } > > sii902x->bridge.funcs = &sii902x_bridge_funcs; > + sii902x->bridge.device = dev; > sii902x->bridge.of_node = dev->of_node; > sii902x->bridge.timings = &default_sii902x_timings; > drm_bridge_add(&sii902x->bridge); > diff --git a/drivers/gpu/drm/bridge/sii9234.c b/drivers/gpu/drm/bridge/sii9234.c > index 25d4ad8c7ad6..824ffaeff16e 100644 > --- a/drivers/gpu/drm/bridge/sii9234.c > +++ b/drivers/gpu/drm/bridge/sii9234.c > @@ -936,6 +936,7 @@ static int sii9234_probe(struct i2c_client *client, > i2c_set_clientdata(client, ctx); > > ctx->bridge.funcs = &sii9234_bridge_funcs; > + ctx->bridge.device = dev; > ctx->bridge.of_node = dev->of_node; > drm_bridge_add(&ctx->bridge); > > diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c > index bd3165ee5354..5bc56c5f6826 100644 > --- a/drivers/gpu/drm/bridge/sil-sii8620.c > +++ b/drivers/gpu/drm/bridge/sil-sii8620.c > @@ -2333,6 +2333,7 @@ static int sii8620_probe(struct i2c_client *client, > i2c_set_clientdata(client, ctx); > > ctx->bridge.funcs = &sii8620_bridge_funcs; > + ctx->bridge.device = dev; > ctx->bridge.of_node = dev->of_node; > drm_bridge_add(&ctx->bridge); > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c > index 13ade28a36a8..d62c6925c5fe 100644 > --- a/drivers/gpu/drm/bridge/tc358767.c > +++ b/drivers/gpu/drm/bridge/tc358767.c > @@ -1526,6 +1526,7 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) > return ret; > > tc->bridge.funcs = &tc_bridge_funcs; > + tc->bridge.device = dev; > tc->bridge.of_node = dev->of_node; > drm_bridge_add(&tc->bridge); > > diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c > index dbf35c7bc85e..2f9899d7d4b4 100644 > --- a/drivers/gpu/drm/bridge/ti-tfp410.c > +++ b/drivers/gpu/drm/bridge/ti-tfp410.c > @@ -326,6 +326,7 @@ static int tfp410_init(struct device *dev, bool i2c) > dev_set_drvdata(dev, dvi); > > dvi->bridge.funcs = &tfp410_bridge_funcs; > + dvi->bridge.device = dev; > dvi->bridge.of_node = dev->of_node; > dvi->bridge.timings = &dvi->timings; > dvi->dev = dev; > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index cba537c99e43..b4561ce63a49 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" > @@ -463,6 +464,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 > @@ -474,21 +501,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/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c > index e79507fb225f..5d4122bcf7ff 100644 > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c > @@ -2201,6 +2201,7 @@ static int tda998x_create(struct device *dev) > } > > priv->bridge.funcs = &tda998x_bridge_funcs; > + priv->bridge.device = dev; > #ifdef CONFIG_OF > priv->bridge.of_node = dev->of_node; > #endif > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > index 7616f6562fe4..f8a3af42a382 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; > @@ -403,6 +405,8 @@ struct drm_bridge { > void drm_bridge_add(struct drm_bridge *bridge); > void drm_bridge_remove(struct drm_bridge *bridge); > 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
