From: Russell King - ARM Linux <linux@armlinux.org.uk> To: Peter Rosin <peda@axentia.se> Cc: linux-kernel@vger.kernel.org, David Airlie <airlied@linux.ie>, Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Nicolas Ferre <nicolas.ferre@microchip.com>, Alexandre Belloni <alexandre.belloni@bootlin.com>, Boris Brezillon <boris.brezillon@free-electrons.com>, Jyri Sarha <jsarha@ti.com>, Tomi Valkeinen <tomi.valkeinen@ti.com>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Jacopo Mondi <jacopo+renesas@jmondi.org>, dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge Date: Tue, 24 Apr 2018 09:08:33 +0100 [thread overview] Message-ID: <20180424080833.GJ16141@n2100.armlinux.org.uk> (raw) In-Reply-To: <5d6866d0-75bc-4de8-9b87-4fee5430e9dd@axentia.se> On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote: > On 2018-04-23 18:08, Russell King - ARM Linux wrote: > > On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote: > >> static int tda998x_remove(struct i2c_client *client) > >> { > >> - component_del(&client->dev, &tda998x_ops); > >> + struct device *dev = &client->dev; > >> + struct tda998x_bridge *bridge = dev_get_drvdata(dev); > >> + > >> + drm_bridge_remove(&bridge->bridge); > >> + component_del(dev, &tda998x_ops); > >> + > > > > I'd like to ask a rather fundamental question about DRM bridge support, > > because I suspect that there's a major fsckup here. > > > > The above is the function that deals with the TDA998x device being > > unbound from the driver. With the component API, this results in the > > DRM device correctly being torn down, because one of the hardware > > devices has gone. > > > > With DRM bridge, the bridge is merely removed from the list of > > bridges: > > > > void drm_bridge_remove(struct drm_bridge *bridge) > > { > > mutex_lock(&bridge_lock); > > list_del_init(&bridge->list); > > mutex_unlock(&bridge_lock); > > } > > EXPORT_SYMBOL(drm_bridge_remove); > > > > and the memory backing the "struct tda998x_bridge" (which contains > > the struct drm_bridge) will be freed by the devm subsystem. > > > > However, there is no notification into the rest of the DRM subsystem > > that the device has gone away. Worse, the memory that is still in > > use by DRM has now been freed, so further use of the DRM device > > results in a use-after-free bug. > > > > This is really not good, and to me looks like a fundamental problem > > with the DRM bridge code. I see nothing in the DRM bridge code that > > deals with the lifetime of a "DRM bridge" or indeed the lifetime of > > the actual device itself. > > > > So, from what I can see, there seems to be a fundamental lifetime > > issue with the design of the DRM bridge code. This needs to be > > fixed. > > Oh crap. A gigantic can of worms... Yes, it's especially annoying for me, having put the effort in to the component helper to cover all these cases. > Would a patch (completely untested btw) along this line of thinking make > any difference whatsoever? It looks interesting - from what I can see of the device links code, it would have the effect of unbinding the DRM device just before TDA998x is unbound, so that's an improvement. However, from what I can see, the link vanishes at that point (as DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results in nothing further happening - the link will be recreated, but there appears to be nothing that triggers the "consumer" to rebind at that point. Maybe I've missed something? -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up
WARNING: multiple messages have this Message-ID (diff)
From: linux@armlinux.org.uk (Russell King - ARM Linux) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge Date: Tue, 24 Apr 2018 09:08:33 +0100 [thread overview] Message-ID: <20180424080833.GJ16141@n2100.armlinux.org.uk> (raw) In-Reply-To: <5d6866d0-75bc-4de8-9b87-4fee5430e9dd@axentia.se> On Tue, Apr 24, 2018 at 08:58:42AM +0200, Peter Rosin wrote: > On 2018-04-23 18:08, Russell King - ARM Linux wrote: > > On Mon, Apr 23, 2018 at 09:23:00AM +0200, Peter Rosin wrote: > >> static int tda998x_remove(struct i2c_client *client) > >> { > >> - component_del(&client->dev, &tda998x_ops); > >> + struct device *dev = &client->dev; > >> + struct tda998x_bridge *bridge = dev_get_drvdata(dev); > >> + > >> + drm_bridge_remove(&bridge->bridge); > >> + component_del(dev, &tda998x_ops); > >> + > > > > I'd like to ask a rather fundamental question about DRM bridge support, > > because I suspect that there's a major fsckup here. > > > > The above is the function that deals with the TDA998x device being > > unbound from the driver. With the component API, this results in the > > DRM device correctly being torn down, because one of the hardware > > devices has gone. > > > > With DRM bridge, the bridge is merely removed from the list of > > bridges: > > > > void drm_bridge_remove(struct drm_bridge *bridge) > > { > > mutex_lock(&bridge_lock); > > list_del_init(&bridge->list); > > mutex_unlock(&bridge_lock); > > } > > EXPORT_SYMBOL(drm_bridge_remove); > > > > and the memory backing the "struct tda998x_bridge" (which contains > > the struct drm_bridge) will be freed by the devm subsystem. > > > > However, there is no notification into the rest of the DRM subsystem > > that the device has gone away. Worse, the memory that is still in > > use by DRM has now been freed, so further use of the DRM device > > results in a use-after-free bug. > > > > This is really not good, and to me looks like a fundamental problem > > with the DRM bridge code. I see nothing in the DRM bridge code that > > deals with the lifetime of a "DRM bridge" or indeed the lifetime of > > the actual device itself. > > > > So, from what I can see, there seems to be a fundamental lifetime > > issue with the design of the DRM bridge code. This needs to be > > fixed. > > Oh crap. A gigantic can of worms... Yes, it's especially annoying for me, having put the effort in to the component helper to cover all these cases. > Would a patch (completely untested btw) along this line of thinking make > any difference whatsoever? It looks interesting - from what I can see of the device links code, it would have the effect of unbinding the DRM device just before TDA998x is unbound, so that's an improvement. However, from what I can see, the link vanishes at that point (as DL_FLAG_AUTOREMOVE is set), and re-binding the TDA998x device results in nothing further happening - the link will be recreated, but there appears to be nothing that triggers the "consumer" to rebind at that point. Maybe I've missed something? -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up
next prev parent reply other threads:[~2018-04-24 8:08 UTC|newest] Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-04-23 7:22 [PATCH v4 0/8] Add tda998x (HDMI) support to atmel-hlcdc Peter Rosin 2018-04-23 7:22 ` Peter Rosin 2018-04-23 7:22 ` [PATCH v4 1/8] dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185 Peter Rosin 2018-04-23 7:22 ` [PATCH v4 1/8] dt-bindings: display: bridge: lvds-transmitter: add ti, ds90c185 Peter Rosin 2018-04-23 7:22 ` [PATCH v4 2/8] dt-bindings: display: atmel: optional video-interface of endpoints Peter Rosin 2018-04-23 7:22 ` Peter Rosin 2018-04-23 17:41 ` Boris Brezillon 2018-04-23 17:41 ` Boris Brezillon 2018-04-23 17:41 ` Boris Brezillon 2018-04-27 14:27 ` Rob Herring 2018-04-27 14:27 ` Rob Herring 2018-04-23 7:22 ` [PATCH v4 3/8] drm/atmel-hlcdc: support bus-width (12/16/18/24) in endpoint nodes Peter Rosin 2018-04-23 7:22 ` Peter Rosin 2018-04-23 17:40 ` Boris Brezillon 2018-04-23 17:40 ` Boris Brezillon 2018-04-23 17:40 ` Boris Brezillon 2018-04-23 7:22 ` [PATCH v4 4/8] drm/i2c: tda998x: find the drm_device via the drm_connector Peter Rosin 2018-04-23 7:22 ` Peter Rosin 2018-04-23 7:22 ` [PATCH v4 5/8] drm/i2c: tda998x: split tda998x_encoder_dpms into enable/disable Peter Rosin 2018-04-23 7:22 ` Peter Rosin 2018-04-23 7:22 ` [PATCH v4 6/8] drm/i2c: tda998x: split encoder and component functions from the work Peter Rosin 2018-04-23 7:22 ` Peter Rosin 2018-04-23 7:23 ` [PATCH v4 7/8] drm/i2c: tda998x: register as a drm bridge Peter Rosin 2018-04-23 7:23 ` Peter Rosin 2018-04-23 16:08 ` Russell King - ARM Linux 2018-04-23 16:08 ` Russell King - ARM Linux 2018-04-24 6:58 ` Peter Rosin 2018-04-24 6:58 ` Peter Rosin 2018-04-24 8:08 ` Russell King - ARM Linux [this message] 2018-04-24 8:08 ` Russell King - ARM Linux 2018-04-24 10:14 ` Peter Rosin 2018-04-24 10:14 ` Peter Rosin 2018-04-24 13:26 ` Peter Rosin 2018-04-24 13:26 ` Peter Rosin 2018-04-24 16:04 ` Jyri Sarha 2018-04-24 16:04 ` Jyri Sarha 2018-04-24 16:04 ` Jyri Sarha 2018-04-24 17:06 ` Russell King - ARM Linux 2018-04-24 17:06 ` Russell King - ARM Linux 2018-04-24 18:25 ` Jyri Sarha 2018-04-24 18:25 ` Jyri Sarha 2018-04-24 18:25 ` Jyri Sarha 2018-04-24 23:25 ` Russell King - ARM Linux 2018-04-24 23:25 ` Russell King - ARM Linux 2018-04-25 20:01 ` Jyri Sarha 2018-04-25 20:01 ` Jyri Sarha 2018-04-25 20:01 ` Jyri Sarha 2018-07-06 10:03 ` Russell King - ARM Linux 2018-07-06 10:03 ` Russell King - ARM Linux 2018-07-06 10:03 ` Russell King - ARM Linux 2018-07-06 12:43 ` Russell King - ARM Linux 2018-07-06 12:43 ` Russell King - ARM Linux 2018-07-17 15:57 ` Russell King - ARM Linux 2018-07-17 15:57 ` Russell King - ARM Linux 2018-08-28 17:49 ` Peter Rosin 2018-08-28 17:49 ` Peter Rosin 2018-08-28 18:14 ` Russell King - ARM Linux 2018-08-28 18:14 ` Russell King - ARM Linux 2018-04-25 9:09 ` Peter Rosin 2018-04-25 9:09 ` Peter Rosin 2018-04-23 7:23 ` [RFC PATCH v4 8/8] drm/tilcdc: decomponentize now that tda998x is a bridge Peter Rosin 2018-04-23 7:23 ` Peter Rosin
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=20180424080833.GJ16141@n2100.armlinux.org.uk \ --to=linux@armlinux.org.uk \ --cc=airlied@linux.ie \ --cc=alexandre.belloni@bootlin.com \ --cc=boris.brezillon@free-electrons.com \ --cc=devicetree@vger.kernel.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=jacopo+renesas@jmondi.org \ --cc=jsarha@ti.com \ --cc=laurent.pinchart@ideasonboard.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=nicolas.ferre@microchip.com \ --cc=peda@axentia.se \ --cc=robh+dt@kernel.org \ --cc=tomi.valkeinen@ti.com \ /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.