+CC: Rafael J. Wysocki <rafael@xxxxxxxxxx> On 08.01.2019 16:07, Russell King - ARM Linux wrote: > On Tue, Jan 08, 2019 at 03:33:54PM +0100, Andrzej Hajda wrote: >> On 08.01.2019 14:21, Russell King - ARM Linux wrote: >>> On Tue, Jan 08, 2019 at 01:27:56PM +0100, Andrzej Hajda wrote: >>>> On 08.01.2019 12:38, Russell King - ARM Linux wrote: >>>>> On Tue, Jan 08, 2019 at 12:25:34PM +0100, Andrzej Hajda wrote: >>>>>> Issues with device links have nothing to do with hotplugging, they are >>>>>> generic - lifetime of the objects (drm_bridge, drm_panel) is just >>>>>> slightly different of lifetime of device links, and this is racy even if >>>>>> you do not want hotplugging. Moreover since drm_dev is not device (has >>>>>> no associated struct device) assuming we can reuse its parent to create >>>>>> device link results in circular dependencies. >>>>> How about having the device links created depending on whether the >>>>> main drm driver wants them or not - that would mean that Exynos >>>>> could continue avoiding them, but others that want them can have >>>>> the links? >>>> That should be OK for Exynos. But regardless of Exynos device_links at >>>> the current state will not work correctly with bridges/panels as I >>>> described earlier. >>> However, I'm not sure you're correct with your interpretation of the >>> documentation. Firstly, the documentation says: >>> >>> Another example for an inconsistent state would be a device link that >>> represents a driver presence dependency, yet is added from the consumer's >>> ->probe callback while the supplier hasn't probed yet: Had the driver core >>> known about the device link earlier, it wouldn't have probed the consumer >>> in the first place. The onus is thus on the consumer to check presence of >>> the supplier after adding the link, and defer probing on non-presence. >>> >>> This is fine - if we add the device link from of_drm_find_bridge(), we >>> will be in the consumer's ->probe function, and the supplier must have >>> been probed for us to find the struct drm_bridge. >> >> Supplier usually is registered in it's probe time, so there is short >> period of time when supplier is available, but the probe is not yet >> finished - quite unsafe, but not impossible, especially if there exists >> some kind of notifications about resource appearance (MIPI-DSI case). > At some point during the supplier probe, the resource becomes available > to consumers. At that point, device links can be setup before the > supplier has finished probing. So any driver that provides resources > to another driver will, at some point during the provider's probe, > make resources available, and therefore be a candidate for device links > to be created _before_ the probe function has returned. > > What is a problem is if the provider publishes resources, and then fails > its probe function, causing the resource to disappear. But creating link to not-probed provider is still incorrect usage from device_links framework PoV, and my tests showed indeed that device link created before end of provider's probe does not work at all - and since it is stated in the documentation I guess it is by design. > > Taking DRM bridge, drm_bridge_add() returns void - it never fails. > Most bridge drivers do drm_bridge_add() as the very last step before > returning zero from their probe function. There are a few exceptions. > > megachips-stdpxxxx-ge-b850v3-fw.c is already buggy - it calls > devm_request_threaded_irq(), which if it fails, the bridge is left added > to the global list of bridges. Since this structure was allocated with > devm_kzalloc(), it will be freed when the probe function fails. So, > any failure here (eg a deferred probe because the IRQ controller is not > available) already creates a latent bug just waiting to bite. > > tc358764.c is another case where the bridge is published prior to > initialisation completion, but it looks like that's under the control > of the "host" (consumer) driver. > > mtk_hdmi.c enables clocks for audio after registering the bridge, which > may fail, but are unlikely to. However, moving them prior to > drm_bridge_add() probably won't hurt and avoids the "published > interfaces which then vanish" problem. > > Then we have exynos_drm_mic.c and tda998x_drv.c, but I think the > latter's error path after drm_bridge_add() can be eliminated if we > transitioned to device links instead of the component helper. > > Outside DRM, take regulators - at some point during a regulator > supplier's probe function, the resource will be published, and as soon > as it is, it's available for regulator_get() to find it and setup a > device link before the probe function has finished. > > >From what I can tell, in the situation where a supplier makes resources > available, the consumer binds to the resource, and then the supplier > goes away, the device link will remain, and the consumer will not be > unbound, which leads to an unexpected state. The solution to this is > the age old kernel rule of "don't publish your interfaces until you've > completed initialisation". IOW, publish at a point where you aren't > going to fail. > What about complex devices which wants to publish multiple interfaces? I can imagine in some cases it could be impossible to stick to this rule. Regards Andrzej _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
