On Wed, Aug 15, 2018 at 09:49:58AM +0300, Jyri Sarha wrote: > On 14/08/18 17:17, Daniel Vetter wrote: > > On Mon, Feb 26, 2018 at 8:52 AM, Jyri Sarha <jsarha@xxxxxx> wrote: > >> On 25/02/18 11:22, Lukas Wunner wrote: > >>> On Thu, Feb 22, 2018 at 07:42:46PM +0200, Jyri Sarha wrote: > >>>> Put consumer device to deferred probe list if it is unbound due to a > >>>> dropped link to a supplier. > >>>> > >>>> When a device link supplier is unbound (either manually or because one > >>>> of its own suppliers was unbound), its consumers are unbound as > >>>> well. Currently if the supplier binds again after this the consumer > >>>> does not automatically probe again. With this patch it does. > >>> > >>> Yes I think this makes sense, based on the rationale that the consumer > >>> was automatically unbound, so by symmetry it should also be automatically > >>> rebound. > >>> > >>> The only thing I don't understand is you wrote in an earlier e-mail of a > >>> difference in behavior depending on whether driver_deferred_probe_add() > >>> is called before or after device_release_driver_internal(). > >>> That's really odd, it shouldn't make a difference. > >>> > >> > >> In that version there was a couple of other bugs elsewhere in the > >> system. I tried to reproduce that situation again multiple times, but I > >> could not (even write those bugs back in there, and move the link > >> creation back to panel bridge code). But even from reading the code that > >> difference did not make any sense. I suspect a heisen-bug, after I have > >> read the code and understood that the crash is not possible, it does not > >> happen anymore :). > >> > >> With the current version I could not find any difference in behaviour > >> depending on the order of device_release_driver_internal() and > >> driver_deferred_probe_add() calls. I just thought having them in this > >> order just lookes nicer. > >> > >> I stress tested the code by unloading and loading the panel driver in a > >> tight loop, for several minutes, but it simply wont crash (not in this > >> setup anyway). The probe of tilcdc eventually gracefully fails in a CMA > >> failure. > > > > Is this still moving or stuck? Pinging since Sean just brought up the > > drm_bridge lifetime issues, and I think it'd be nice if we could solve > > those by using device_link, like we've added already for drm_panel. > > From my very layman understanding of the discussions, device_link not > > quite working for deferred probing is the blocker for this plan. > > > > I would say stuck. As there is has not been any traffic regarding this > patch after Lukas' reply. And I've been busy with other things since > then. Perhaps I did not know to cc the right people. > > A reviewed-by or tested-by from someone would probably help to get this > spinning again. I am happy to do changes or improvements etc., but for > that I would need suggestions. > > As far as I can tell the patch is good to apply as it is, but then again > I am no device core expert. I do think the cc list is good and has all the stakeholders on it (Greg/Rafeal are the main ones I think). Maybe this also didn't get much attention since it's not (yet) a big problem in drm? Only drm_panel uses device_link, the discussion to add that for drm_bridge also seems to have died down. But from my far-away observer vantage point (we don't use device_link nor panel or bridge in drm/i915) it does all sound like a good approach. Interest from i915 side might go up, since we're using component already and device_link will probably happen eventually too (for better handling the suspend/resume ordering of i915 vs. snd-hda and other stuff), and then I guess we'll need this? Otoh we're not big on EPROBE_DEFER in x86 :-) Cheers, Daniel > > Best regards, > Jyri > > > Also adding Sean. > > -Daniel > > > >> > >> Best regards, > >> Jyri > >> > >>> Thanks, > >>> > >>> Lukas > >>> > >>>> > >>>> If this patch is not acceptable as such, how about adding this > >>>> behavior behind a new device link flag? > >>>> > >>>> The idea to this patch was gotten from this post by Lucas Wunner: > >>>> https://www.spinics.net/lists/dri-devel/msg166318.html > >>>> > >>>> Part of the code and the description is borrowed from him. > >>>> > >>>> cc: Lukas Wunner <lukas@xxxxxxxxx> > >>>> cc: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx> > >>>> cc: Thierry Reding <thierry.reding@xxxxxxxxx> > >>>> Signed-off-by: Jyri Sarha <jsarha@xxxxxx> > >>>> --- > >>>> drivers/base/base.h | 1 + > >>>> drivers/base/core.c | 2 ++ > >>>> drivers/base/dd.c | 2 +- > >>>> 3 files changed, 4 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/base/base.h b/drivers/base/base.h > >>>> index d800de6..39370eb 100644 > >>>> --- a/drivers/base/base.h > >>>> +++ b/drivers/base/base.h > >>>> @@ -114,6 +114,7 @@ extern void device_release_driver_internal(struct device *dev, > >>>> > >>>> extern void driver_detach(struct device_driver *drv); > >>>> extern int driver_probe_device(struct device_driver *drv, struct device *dev); > >>>> +extern void driver_deferred_probe_add(struct device *dev); > >>>> extern void driver_deferred_probe_del(struct device *dev); > >>>> static inline int driver_match_device(struct device_driver *drv, > >>>> struct device *dev) > >>>> diff --git a/drivers/base/core.c b/drivers/base/core.c > >>>> index b2261f9..0964ed5 100644 > >>>> --- a/drivers/base/core.c > >>>> +++ b/drivers/base/core.c > >>>> @@ -570,6 +570,8 @@ void device_links_unbind_consumers(struct device *dev) > >>>> > >>>> device_release_driver_internal(consumer, NULL, > >>>> consumer->parent); > >>>> + driver_deferred_probe_add(consumer); > >>>> + > >>>> put_device(consumer); > >>>> goto start; > >>>> } > >>>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c > >>>> index de6fd09..846ae78 100644 > >>>> --- a/drivers/base/dd.c > >>>> +++ b/drivers/base/dd.c > >>>> @@ -140,7 +140,7 @@ static void deferred_probe_work_func(struct work_struct *work) > >>>> } > >>>> static DECLARE_WORK(deferred_probe_work, deferred_probe_work_func); > >>>> > >>>> -static void driver_deferred_probe_add(struct device *dev) > >>>> +void driver_deferred_probe_add(struct device *dev) > >>>> { > >>>> mutex_lock(&deferred_probe_mutex); > >>>> if (list_empty(&dev->p->deferred_probe)) { > >> > >> > >> -- > >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > >> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki > >> _______________________________________________ > >> dri-devel mailing list > >> dri-devel@xxxxxxxxxxxxxxxxxxxxx > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > > > > > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
