Re: [PATCH RFC] driver core: Reprobe consumer if it was unbound by dropped device_link

[Daniel Vetter]

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