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

[Jyri Sarha]

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.

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