* [PATCH RFC] driver core: Reprobe consumer if it was unbound by dropped device_link
[not found] <cover.1519321145.git.jsarha@ti.com>
@ 2018-02-22 17:42 ` Jyri Sarha
2018-02-25 9:22 ` Lukas Wunner
0 siblings, 1 reply; 6+ messages in thread
From: Jyri Sarha @ 2018-02-22 17:42 UTC (permalink / raw)
To: linux-kernel, dri-devel
Cc: tomi.valkeinen, thierry.reding, lukas, gregkh, Jyri Sarha,
Rafael J . Wysocki
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.
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@wunner.de>
cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
cc: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] driver core: Reprobe consumer if it was unbound by dropped device_link
2018-02-22 17:42 ` [PATCH RFC] driver core: Reprobe consumer if it was unbound by dropped device_link Jyri Sarha
@ 2018-02-25 9:22 ` Lukas Wunner
2018-02-26 7:52 ` Jyri Sarha
0 siblings, 1 reply; 6+ messages in thread
From: Lukas Wunner @ 2018-02-25 9:22 UTC (permalink / raw)
To: Jyri Sarha
Cc: linux-kernel, dri-devel, tomi.valkeinen, thierry.reding, gregkh,
Rafael J . Wysocki
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.
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@wunner.de>
> cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> cc: Thierry Reding <thierry.reding@gmail.com>
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
> 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)) {
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] driver core: Reprobe consumer if it was unbound by dropped device_link
2018-02-25 9:22 ` Lukas Wunner
@ 2018-02-26 7:52 ` Jyri Sarha
2018-08-14 14:17 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Jyri Sarha @ 2018-02-26 7:52 UTC (permalink / raw)
To: Lukas Wunner
Cc: linux-kernel, dri-devel, tomi.valkeinen, thierry.reding, gregkh,
Rafael J . Wysocki
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.
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@wunner.de>
>> cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> cc: Thierry Reding <thierry.reding@gmail.com>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> ---
>> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] driver core: Reprobe consumer if it was unbound by dropped device_link
2018-02-26 7:52 ` Jyri Sarha
@ 2018-08-14 14:17 ` Daniel Vetter
2018-08-15 6:49 ` Jyri Sarha
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2018-08-14 14:17 UTC (permalink / raw)
To: Jyri Sarha, Sean Paul
Cc: Lukas Wunner, Greg KH, Rafael J . Wysocki,
Linux Kernel Mailing List, dri-devel, Tomi Valkeinen,
Thierry Reding
On Mon, Feb 26, 2018 at 8:52 AM, Jyri Sarha <jsarha@ti.com> 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.
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@wunner.de>
>>> cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> cc: Thierry Reding <thierry.reding@gmail.com>
>>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>>> ---
>>> 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] driver core: Reprobe consumer if it was unbound by dropped device_link
2018-08-14 14:17 ` Daniel Vetter
@ 2018-08-15 6:49 ` Jyri Sarha
2018-08-15 8:49 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Jyri Sarha @ 2018-08-15 6:49 UTC (permalink / raw)
To: Daniel Vetter, Sean Paul
Cc: Lukas Wunner, Greg KH, Rafael J . Wysocki,
Linux Kernel Mailing List, dri-devel, Tomi Valkeinen,
Thierry Reding
On 14/08/18 17:17, Daniel Vetter wrote:
> On Mon, Feb 26, 2018 at 8:52 AM, Jyri Sarha <jsarha@ti.com> 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@wunner.de>
>>>> cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>> cc: Thierry Reding <thierry.reding@gmail.com>
>>>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>>>> ---
>>>> 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@lists.freedesktop.org
>> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] driver core: Reprobe consumer if it was unbound by dropped device_link
2018-08-15 6:49 ` Jyri Sarha
@ 2018-08-15 8:49 ` Daniel Vetter
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2018-08-15 8:49 UTC (permalink / raw)
To: Jyri Sarha
Cc: Daniel Vetter, Sean Paul, Lukas Wunner, Greg KH,
Rafael J . Wysocki, Linux Kernel Mailing List, dri-devel,
Tomi Valkeinen, Thierry Reding
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@ti.com> 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@wunner.de>
> >>>> cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>> cc: Thierry Reding <thierry.reding@gmail.com>
> >>>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> >>>> ---
> >>>> 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@lists.freedesktop.org
> >> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-08-15 8:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <cover.1519321145.git.jsarha@ti.com>
2018-02-22 17:42 ` [PATCH RFC] driver core: Reprobe consumer if it was unbound by dropped device_link Jyri Sarha
2018-02-25 9:22 ` Lukas Wunner
2018-02-26 7:52 ` Jyri Sarha
2018-08-14 14:17 ` Daniel Vetter
2018-08-15 6:49 ` Jyri Sarha
2018-08-15 8:49 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox