[CC Lukas and linux-pm]
On Mon, Jan 14, 2019 at 1:32 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>
> On Fri, Jan 11, 2019 at 3:49 PM Russell King - ARM Linux
> <linux@xxxxxxxxxxxxxxx> wrote:
[cut]
> >
> > This thread is discussing how to deal with Armada DRM, its use of the
> > component helper, TDA998x's hybrid use of the component helper and
> > DRM bridges.
> >
> > Currently, DRM bridges register into the DRM system and are added to
> > a global list of bridges. When a DRM driver binds, it looks up the
> > DRM bridge and attaches to it. There is no way in generic DRM code
> > for the DRM driver to know if the DRM bridge is unbound from DRM,
> > consequently a DRM driver may continue trying to call functions in
> > the DRM bridge driver using memory that has already been freed.
> >
> > We had similar issues with imx-drm, and a number of years ago at a
> > kernel summit, this was discussed, and I proposed a system which is
> > now known as the component helper. This handles the problem of a
> > multi-component system.
> >
> > However, DRM bridge was already established, and there appears to be
> > no desire to convert DRM bridges and DRM drivers to use the component
> > helper.
> >
> > We are presently in the situation where Armada DRM is incompatible
> > with the DRM bridges way of doing things, and making it compatible
> > essentially means introducing potential use-after-free bugs into the
> > code.
> >
> > Device links in their stateful form appear to provide an alternative
> > to the component helper, in that a stateful device link will remove
> > consumers of a resource when the supplier is going away - which is
> > exactly the problem which the component helper is solving. The
> > difference is that device links look like being a cleaner solution.
> >
> > Just like the component helper, a stateful link would unbind the
> > consumer of a resource when the supplier goes away - which is exactly
> > the behaviour we're wanting.
> >
> > The problem is that the connection between various drivers is only
> > really known when the drivers obtain their resources, and the
> > following can happen:
> >
> > supplier consumer
> >
> > probe()
> > alloc
> > probe()
> > publish
> > obtain supplier's resource
> > return
> >
> > Where things go wrong is if a stateful link is created when the
> > consumer obtains the suppliers resource before the supplier has
> > finished probing - which from what's been said is illegal.
>
> It just doesn't work (which means "invalid" rather than "illegal" I
> guess, but whatever :-)).
>
> Admittedly, the original design didn't take this particular case into
> account and I'm not actually sure how it can be taken into account
> either.
>
> If the link is created by the consumer in the scenario above, its
> status will be updated twice in a row after the consumer probe returns
> and after the supplier probe returns. It looks like this update would
> need to work regardless of the order it which this happened which
> sounds somewhat challenging. I would need to think about that.
So if I'm not mistaken it can be made work with the help of the
(completely untested) attached patch (of course, the documentation
would need to be updated too).
The key observation here is that it should be fine to create a link
from the consumer driver's probe while the supplier is still probing
if the consumer has some way to find out that the supplier is
functional at that point (like when it has published itself already in
your example above). The initial state of the link can be "consumer
probe" in that case and I don't see a reason why that might not work.
---
drivers/base/core.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
Index: linux-pm/drivers/base/core.c
===================================================================
--- linux-pm.orig/drivers/base/core.c
+++ linux-pm/drivers/base/core.c
@@ -260,6 +260,17 @@ struct device_link *device_link_add(stru
link->status = DL_STATE_NONE;
} else {
switch (supplier->links.status) {
+ case DL_DEV_PROBING:
+ /*
+ * A consumer driver can create a link to a supplier
+ * that also is probing as long as it knows that the
+ * supplier is already functional (for example, it has
+ * just acquired some resources from the supplier).
+ */
+ link->status = consumer->links.status == DL_DEV_PROBING ?
+ DL_STATE_CONSUMER_PROBE :
+ DL_STATE_DORMANT;
+ break;
case DL_DEV_DRIVER_BOUND:
switch (consumer->links.status) {
case DL_DEV_PROBING:
@@ -474,6 +485,14 @@ void device_links_driver_bound(struct de
if (link->flags & DL_FLAG_STATELESS)
continue;
+ /*
+ * Links created during consumer probe may be in the "consumer
+ * probe" state to start with if the supplier is still probing
+ * when they are created. Skip them here.
+ */
+ if (link->status == DL_STATE_CONSUMER_PROBE)
+ continue;
+
WARN_ON(link->status != DL_STATE_DORMANT);
WRITE_ONCE(link->status, DL_STATE_AVAILABLE);
}
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
