[PATCH] leds: core: use deferred probing if default trigger isn't available yet

[PATCH] leds: core: use deferred probing if default trigger isn't available yet

[Heiner Kallweit]

When registering a LED device we have the option to set a default trigger.
Depending on load order of drivers this trigger may not be available yet.
(affected LED device in my case: a DT-configured GPIO LED)
So far if the default trigger can't be found this error is silently
ignored.

Let's change this to return EPROBE_DEFER if the default trigger can't be
found. This gives the system the chance to probe the LED device later
once the trigger is available.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
---
 drivers/leds/led-class.c    |  6 +++++-
 drivers/leds/led-triggers.c | 11 ++++++++---
 include/linux/leds.h        |  5 +++--
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index f2b0a80a..efe4f5a3 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -295,7 +295,11 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
 	led_init_core(led_cdev);
 
 #ifdef CONFIG_LEDS_TRIGGERS
-	led_trigger_set_default(led_cdev);
+	ret = led_trigger_set_default(led_cdev);
+	if (ret) {
+		led_classdev_unregister(led_cdev);
+		return ret;
+	}
 #endif
 
 	dev_dbg(parent, "Registered led device: %s\n",
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 431123b0..bad9e986 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -157,21 +157,26 @@ void led_trigger_remove(struct led_classdev *led_cdev)
 }
 EXPORT_SYMBOL_GPL(led_trigger_remove);
 
-void led_trigger_set_default(struct led_classdev *led_cdev)
+int led_trigger_set_default(struct led_classdev *led_cdev)
 {
 	struct led_trigger *trig;
+	int ret = -EPROBE_DEFER;
 
 	if (!led_cdev->default_trigger)
-		return;
+		return 0;
 
 	down_read(&triggers_list_lock);
 	down_write(&led_cdev->trigger_lock);
 	list_for_each_entry(trig, &trigger_list, next_trig) {
-		if (!strcmp(led_cdev->default_trigger, trig->name))
+		if (!strcmp(led_cdev->default_trigger, trig->name)) {
 			led_trigger_set(led_cdev, trig);
+			ret = 0;
+		}
 	}
 	up_write(&led_cdev->trigger_lock);
 	up_read(&triggers_list_lock);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(led_trigger_set_default);
 
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 38c0bd7c..3e36ce31 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -280,7 +280,7 @@ extern void led_trigger_blink_oneshot(struct led_trigger *trigger,
 				      unsigned long *delay_on,
 				      unsigned long *delay_off,
 				      int invert);
-extern void led_trigger_set_default(struct led_classdev *led_cdev);
+extern int led_trigger_set_default(struct led_classdev *led_cdev);
 extern void led_trigger_set(struct led_classdev *led_cdev,
 			struct led_trigger *trigger);
 extern void led_trigger_remove(struct led_classdev *led_cdev);
@@ -326,7 +326,8 @@ static inline void led_trigger_blink_oneshot(struct led_trigger *trigger,
 				      unsigned long *delay_on,
 				      unsigned long *delay_off,
 				      int invert) {}
-static inline void led_trigger_set_default(struct led_classdev *led_cdev) {}
+static inline int led_trigger_set_default(struct led_classdev *led_cdev)
+							{ return 0; }
 static inline void led_trigger_set(struct led_classdev *led_cdev,
 				struct led_trigger *trigger) {}
 static inline void led_trigger_remove(struct led_classdev *led_cdev) {}
-- 
2.11.1

Re: [PATCH] leds: core: use deferred probing if default trigger isn't available yet

[Jacek Anaszewski]

Hi Heiner,

Thanks for the patch. I have one comment below.

On 02/22/2017 09:35 PM, Heiner Kallweit wrote:
> When registering a LED device we have the option to set a default trigger.
> Depending on load order of drivers this trigger may not be available yet.
> (affected LED device in my case: a DT-configured GPIO LED)
> So far if the default trigger can't be found this error is silently
> ignored.
> 
> Let's change this to return EPROBE_DEFER if the default trigger can't be
> found. This gives the system the chance to probe the LED device later
> once the trigger is available.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> ---
>  drivers/leds/led-class.c    |  6 +++++-
>  drivers/leds/led-triggers.c | 11 ++++++++---
>  include/linux/leds.h        |  5 +++--
>  3 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index f2b0a80a..efe4f5a3 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -295,7 +295,11 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
>  	led_init_core(led_cdev);
>  
>  #ifdef CONFIG_LEDS_TRIGGERS
> -	led_trigger_set_default(led_cdev);
> +	ret = led_trigger_set_default(led_cdev);
> +	if (ret) {
> +		led_classdev_unregister(led_cdev);
> +		return ret;
> +	}
>  #endif
>  
>  	dev_dbg(parent, "Registered led device: %s\n",
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index 431123b0..bad9e986 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -157,21 +157,26 @@ void led_trigger_remove(struct led_classdev *led_cdev)
>  }
>  EXPORT_SYMBOL_GPL(led_trigger_remove);
>  
> -void led_trigger_set_default(struct led_classdev *led_cdev)
> +int led_trigger_set_default(struct led_classdev *led_cdev)
>  {
>  	struct led_trigger *trig;
> +	int ret = -EPROBE_DEFER;
>  
>  	if (!led_cdev->default_trigger)
> -		return;
> +		return 0;
>  
>  	down_read(&triggers_list_lock);
>  	down_write(&led_cdev->trigger_lock);
>  	list_for_each_entry(trig, &trigger_list, next_trig) {
> -		if (!strcmp(led_cdev->default_trigger, trig->name))
> +		if (!strcmp(led_cdev->default_trigger, trig->name)) {
>  			led_trigger_set(led_cdev, trig);
> +			ret = 0;

I wonder why we don't break the loop after matching the trigger?

I think that we can add break here while we are at it since LED Trigger
core doesn't allow for registering two triggers with the same name.

Would you mind sending an update and mention it also in the commit
message?

Best regards,
Jacek Anaszewski

> +		}
>  	}
>  	up_write(&led_cdev->trigger_lock);
>  	up_read(&triggers_list_lock);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(led_trigger_set_default);
>  
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 38c0bd7c..3e36ce31 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -280,7 +280,7 @@ extern void led_trigger_blink_oneshot(struct led_trigger *trigger,
>  				      unsigned long *delay_on,
>  				      unsigned long *delay_off,
>  				      int invert);
> -extern void led_trigger_set_default(struct led_classdev *led_cdev);
> +extern int led_trigger_set_default(struct led_classdev *led_cdev);
>  extern void led_trigger_set(struct led_classdev *led_cdev,
>  			struct led_trigger *trigger);
>  extern void led_trigger_remove(struct led_classdev *led_cdev);
> @@ -326,7 +326,8 @@ static inline void led_trigger_blink_oneshot(struct led_trigger *trigger,
>  				      unsigned long *delay_on,
>  				      unsigned long *delay_off,
>  				      int invert) {}
> -static inline void led_trigger_set_default(struct led_classdev *led_cdev) {}
> +static inline int led_trigger_set_default(struct led_classdev *led_cdev)
> +							{ return 0; }
>  static inline void led_trigger_set(struct led_classdev *led_cdev,
>  				struct led_trigger *trigger) {}
>  static inline void led_trigger_remove(struct led_classdev *led_cdev) {}
> 

Re: [PATCH] leds: core: use deferred probing if default trigger isn't available yet

[Heiner Kallweit]

Am 23.02.2017 um 22:04 schrieb Jacek Anaszewski:
> Hi Heiner,
> 
> Thanks for the patch. I have one comment below.
> 
> On 02/22/2017 09:35 PM, Heiner Kallweit wrote:
>> When registering a LED device we have the option to set a default trigger.
>> Depending on load order of drivers this trigger may not be available yet.
>> (affected LED device in my case: a DT-configured GPIO LED)
>> So far if the default trigger can't be found this error is silently
>> ignored.
>>
>> Let's change this to return EPROBE_DEFER if the default trigger can't be
>> found. This gives the system the chance to probe the LED device later
>> once the trigger is available.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> ---
>>  drivers/leds/led-class.c    |  6 +++++-
>>  drivers/leds/led-triggers.c | 11 ++++++++---
>>  include/linux/leds.h        |  5 +++--
>>  3 files changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>> index f2b0a80a..efe4f5a3 100644
>> --- a/drivers/leds/led-class.c
>> +++ b/drivers/leds/led-class.c
>> @@ -295,7 +295,11 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
>>  	led_init_core(led_cdev);
>>  
>>  #ifdef CONFIG_LEDS_TRIGGERS
>> -	led_trigger_set_default(led_cdev);
>> +	ret = led_trigger_set_default(led_cdev);
>> +	if (ret) {
>> +		led_classdev_unregister(led_cdev);
>> +		return ret;
>> +	}
>>  #endif
>>  
>>  	dev_dbg(parent, "Registered led device: %s\n",
>> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
>> index 431123b0..bad9e986 100644
>> --- a/drivers/leds/led-triggers.c
>> +++ b/drivers/leds/led-triggers.c
>> @@ -157,21 +157,26 @@ void led_trigger_remove(struct led_classdev *led_cdev)
>>  }
>>  EXPORT_SYMBOL_GPL(led_trigger_remove);
>>  
>> -void led_trigger_set_default(struct led_classdev *led_cdev)
>> +int led_trigger_set_default(struct led_classdev *led_cdev)
>>  {
>>  	struct led_trigger *trig;
>> +	int ret = -EPROBE_DEFER;
>>  
>>  	if (!led_cdev->default_trigger)
>> -		return;
>> +		return 0;
>>  
>>  	down_read(&triggers_list_lock);
>>  	down_write(&led_cdev->trigger_lock);
>>  	list_for_each_entry(trig, &trigger_list, next_trig) {
>> -		if (!strcmp(led_cdev->default_trigger, trig->name))
>> +		if (!strcmp(led_cdev->default_trigger, trig->name)) {
>>  			led_trigger_set(led_cdev, trig);
>> +			ret = 0;
> 
> I wonder why we don't break the loop after matching the trigger?
> 
> I think that we can add break here while we are at it since LED Trigger
> core doesn't allow for registering two triggers with the same name.
> 
Indeed.

> Would you mind sending an update and mention it also in the commit
> message?
> 
Fine with me if we can silently improve the existing code ..

> Best regards,
> Jacek Anaszewski
> 
>> +		}
>>  	}
>>  	up_write(&led_cdev->trigger_lock);
>>  	up_read(&triggers_list_lock);
>> +
>> +	return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(led_trigger_set_default);
>>  
>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>> index 38c0bd7c..3e36ce31 100644
>> --- a/include/linux/leds.h
>> +++ b/include/linux/leds.h
>> @@ -280,7 +280,7 @@ extern void led_trigger_blink_oneshot(struct led_trigger *trigger,
>>  				      unsigned long *delay_on,
>>  				      unsigned long *delay_off,
>>  				      int invert);
>> -extern void led_trigger_set_default(struct led_classdev *led_cdev);
>> +extern int led_trigger_set_default(struct led_classdev *led_cdev);
>>  extern void led_trigger_set(struct led_classdev *led_cdev,
>>  			struct led_trigger *trigger);
>>  extern void led_trigger_remove(struct led_classdev *led_cdev);
>> @@ -326,7 +326,8 @@ static inline void led_trigger_blink_oneshot(struct led_trigger *trigger,
>>  				      unsigned long *delay_on,
>>  				      unsigned long *delay_off,
>>  				      int invert) {}
>> -static inline void led_trigger_set_default(struct led_classdev *led_cdev) {}
>> +static inline int led_trigger_set_default(struct led_classdev *led_cdev)
>> +							{ return 0; }
>>  static inline void led_trigger_set(struct led_classdev *led_cdev,
>>  				struct led_trigger *trigger) {}
>>  static inline void led_trigger_remove(struct led_classdev *led_cdev) {}
>>
> 

Re: [PATCH] leds: core: use deferred probing if default trigger isn't available yet

[Jacek Anaszewski]

On 02/23/2017 10:25 PM, Heiner Kallweit wrote:
> Am 23.02.2017 um 22:04 schrieb Jacek Anaszewski:
>> Hi Heiner,
>>
>> Thanks for the patch. I have one comment below.
>>
>> On 02/22/2017 09:35 PM, Heiner Kallweit wrote:
>>> When registering a LED device we have the option to set a default trigger.
>>> Depending on load order of drivers this trigger may not be available yet.
>>> (affected LED device in my case: a DT-configured GPIO LED)
>>> So far if the default trigger can't be found this error is silently
>>> ignored.
>>>
>>> Let's change this to return EPROBE_DEFER if the default trigger can't be
>>> found. This gives the system the chance to probe the LED device later
>>> once the trigger is available.
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>> ---
>>>  drivers/leds/led-class.c    |  6 +++++-
>>>  drivers/leds/led-triggers.c | 11 ++++++++---
>>>  include/linux/leds.h        |  5 +++--
>>>  3 files changed, 16 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>>> index f2b0a80a..efe4f5a3 100644
>>> --- a/drivers/leds/led-class.c
>>> +++ b/drivers/leds/led-class.c
>>> @@ -295,7 +295,11 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
>>>  	led_init_core(led_cdev);
>>>  
>>>  #ifdef CONFIG_LEDS_TRIGGERS
>>> -	led_trigger_set_default(led_cdev);
>>> +	ret = led_trigger_set_default(led_cdev);
>>> +	if (ret) {
>>> +		led_classdev_unregister(led_cdev);
>>> +		return ret;
>>> +	}
>>>  #endif
>>>  
>>>  	dev_dbg(parent, "Registered led device: %s\n",
>>> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
>>> index 431123b0..bad9e986 100644
>>> --- a/drivers/leds/led-triggers.c
>>> +++ b/drivers/leds/led-triggers.c
>>> @@ -157,21 +157,26 @@ void led_trigger_remove(struct led_classdev *led_cdev)
>>>  }
>>>  EXPORT_SYMBOL_GPL(led_trigger_remove);
>>>  
>>> -void led_trigger_set_default(struct led_classdev *led_cdev)
>>> +int led_trigger_set_default(struct led_classdev *led_cdev)
>>>  {
>>>  	struct led_trigger *trig;
>>> +	int ret = -EPROBE_DEFER;
>>>  
>>>  	if (!led_cdev->default_trigger)
>>> -		return;
>>> +		return 0;
>>>  
>>>  	down_read(&triggers_list_lock);
>>>  	down_write(&led_cdev->trigger_lock);
>>>  	list_for_each_entry(trig, &trigger_list, next_trig) {
>>> -		if (!strcmp(led_cdev->default_trigger, trig->name))
>>> +		if (!strcmp(led_cdev->default_trigger, trig->name)) {
>>>  			led_trigger_set(led_cdev, trig);
>>> +			ret = 0;
>>
>> I wonder why we don't break the loop after matching the trigger?
>>
>> I think that we can add break here while we are at it since LED Trigger
>> core doesn't allow for registering two triggers with the same name.
>>
> Indeed.
> 
>> Would you mind sending an update and mention it also in the commit
>> message?
>>
> Fine with me if we can silently improve the existing code ..

We shouldn't enclose unrelated changes in a patch, but in this
case you're modifying the condition body anyway, so adding suitable
comment in the commit message should be enough.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] leds: core: use deferred probing if default trigger isn't available yet

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1734 bytes --]

On Wed 2017-02-22 21:35:52, Heiner Kallweit wrote:
> When registering a LED device we have the option to set a default trigger.
> Depending on load order of drivers this trigger may not be available yet.
> (affected LED device in my case: a DT-configured GPIO LED)
> So far if the default trigger can't be found this error is silently
> ignored.
> 
> Let's change this to return EPROBE_DEFER if the default trigger can't be
> found. This gives the system the chance to probe the LED device later
> once the trigger is available.

I see a lot of EPROBE_DEFERs on N900, and it is quite nasty, as it
spams a log a lot.

Rather then re-trying LED registration few times, could we make sure
leds are always registered after triggers or something like that?

     	 		   	 	     	       	    Pavel


> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> ---
>  drivers/leds/led-class.c    |  6 +++++-
>  drivers/leds/led-triggers.c | 11 ++++++++---
>  include/linux/leds.h        |  5 +++--
>  3 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index f2b0a80a..efe4f5a3 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -295,7 +295,11 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
>  	led_init_core(led_cdev);
>  
>  #ifdef CONFIG_LEDS_TRIGGERS
> -	led_trigger_set_default(led_cdev);
> +	ret = led_trigger_set_default(led_cdev);
> +	if (ret) {
> +		led_classdev_unregister(led_cdev);
> +		return ret;
> +	}

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] leds: core: use deferred probing if default trigger isn't available yet

[Heiner Kallweit]

Am 23.02.2017 um 22:08 schrieb Pavel Machek:
> On Wed 2017-02-22 21:35:52, Heiner Kallweit wrote:
>> When registering a LED device we have the option to set a default trigger.
>> Depending on load order of drivers this trigger may not be available yet.
>> (affected LED device in my case: a DT-configured GPIO LED)
>> So far if the default trigger can't be found this error is silently
>> ignored.
>>
>> Let's change this to return EPROBE_DEFER if the default trigger can't be
>> found. This gives the system the chance to probe the LED device later
>> once the trigger is available.
> 
> I see a lot of EPROBE_DEFERs on N900, and it is quite nasty, as it
> spams a log a lot.
> 
Usually error messages are printed only if there is an error and it is not
EPROBE_DEFER. However indeed there still may be several drivers not
taking into account that a subsystem they depend on may return
EPROBE_DEFER and this should not be treated as "hard error".

> Rather then re-trying LED registration few times, could we make sure
> leds are always registered after triggers or something like that?
> 
I'm afraid if guaranteeing a particular order would be possible w/o
significant effort then the whole deferred probing concept wouldn't exist.

I could imagine that we can try reordering definitions in the DTS to
ensure a certain load order. But this might be somewhat fragile.
So using the existing concept of deferred probing seems to me to be
the cleaner solution.

Heiner

>      	 		   	 	     	       	    Pavel
> 
> 
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> ---
>>  drivers/leds/led-class.c    |  6 +++++-
>>  drivers/leds/led-triggers.c | 11 ++++++++---
>>  include/linux/leds.h        |  5 +++--
>>  3 files changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>> index f2b0a80a..efe4f5a3 100644
>> --- a/drivers/leds/led-class.c
>> +++ b/drivers/leds/led-class.c
>> @@ -295,7 +295,11 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
>>  	led_init_core(led_cdev);
>>  
>>  #ifdef CONFIG_LEDS_TRIGGERS
>> -	led_trigger_set_default(led_cdev);
>> +	ret = led_trigger_set_default(led_cdev);
>> +	if (ret) {
>> +		led_classdev_unregister(led_cdev);
>> +		return ret;
>> +	}
> 

Re: [PATCH] leds: core: use deferred probing if default trigger isn't available yet

[Jacek Anaszewski]

On 02/23/2017 10:23 PM, Heiner Kallweit wrote:
> Am 23.02.2017 um 22:08 schrieb Pavel Machek:
>> On Wed 2017-02-22 21:35:52, Heiner Kallweit wrote:
>>> When registering a LED device we have the option to set a default trigger.
>>> Depending on load order of drivers this trigger may not be available yet.
>>> (affected LED device in my case: a DT-configured GPIO LED)
>>> So far if the default trigger can't be found this error is silently
>>> ignored.
>>>
>>> Let's change this to return EPROBE_DEFER if the default trigger can't be
>>> found. This gives the system the chance to probe the LED device later
>>> once the trigger is available.
>>
>> I see a lot of EPROBE_DEFERs on N900, and it is quite nasty, as it
>> spams a log a lot.
>>
> Usually error messages are printed only if there is an error and it is not
> EPROBE_DEFER. However indeed there still may be several drivers not
> taking into account that a subsystem they depend on may return
> EPROBE_DEFER and this should not be treated as "hard error".
> 
>> Rather then re-trying LED registration few times, could we make sure
>> leds are always registered after triggers or something like that?
>>
> I'm afraid if guaranteeing a particular order would be possible w/o
> significant effort then the whole deferred probing concept wouldn't exist.
> 
> I could imagine that we can try reordering definitions in the DTS to
> ensure a certain load order. But this might be somewhat fragile.
> So using the existing concept of deferred probing seems to me to be
> the cleaner solution.

We could go for a solution similar to v4l2-async.c mechanism which
allows for deferring video pipeline linking until all video pipeline
entities are probed, but in the LED subsystem case I think it would be
an overengineering. V4L2 media device has to wait for completion of
probing of several v4l2 drivers, whereas here we've got to wait for
a single driver. EPROBE_DEFER seems to fit for that ideally.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] leds: core: use deferred probing if default trigger isn't available yet

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2403 bytes --]

On Sun 2017-02-26 18:10:56, Jacek Anaszewski wrote:
> On 02/23/2017 10:23 PM, Heiner Kallweit wrote:
> > Am 23.02.2017 um 22:08 schrieb Pavel Machek:
> >> On Wed 2017-02-22 21:35:52, Heiner Kallweit wrote:
> >>> When registering a LED device we have the option to set a default trigger.
> >>> Depending on load order of drivers this trigger may not be available yet.
> >>> (affected LED device in my case: a DT-configured GPIO LED)
> >>> So far if the default trigger can't be found this error is silently
> >>> ignored.
> >>>
> >>> Let's change this to return EPROBE_DEFER if the default trigger can't be
> >>> found. This gives the system the chance to probe the LED device later
> >>> once the trigger is available.
> >>
> >> I see a lot of EPROBE_DEFERs on N900, and it is quite nasty, as it
> >> spams a log a lot.
> >>
> > Usually error messages are printed only if there is an error and it is not
> > EPROBE_DEFER. However indeed there still may be several drivers not
> > taking into account that a subsystem they depend on may return
> > EPROBE_DEFER and this should not be treated as "hard error".
> > 
> >> Rather then re-trying LED registration few times, could we make sure
> >> leds are always registered after triggers or something like that?
> >>
> > I'm afraid if guaranteeing a particular order would be possible w/o
> > significant effort then the whole deferred probing concept wouldn't exist.
> > 
> > I could imagine that we can try reordering definitions in the DTS to
> > ensure a certain load order. But this might be somewhat fragile.
> > So using the existing concept of deferred probing seems to me to be
> > the cleaner solution.
> 
> We could go for a solution similar to v4l2-async.c mechanism which
> allows for deferring video pipeline linking until all video pipeline
> entities are probed, but in the LED subsystem case I think it would be
> an overengineering. V4L2 media device has to wait for completion of
> probing of several v4l2 drivers, whereas here we've got to wait for
> a single driver. EPROBE_DEFER seems to fit for that ideally.

I was more thinking of ... just initialize all triggers earlier then
all LEDs?

Initcalls do have few levels we could use..
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] leds: core: use deferred probing if default trigger isn't available yet

[Jacek Anaszewski]

On 02/26/2017 10:00 PM, Pavel Machek wrote:
> On Sun 2017-02-26 18:10:56, Jacek Anaszewski wrote:
>> On 02/23/2017 10:23 PM, Heiner Kallweit wrote:
>>> Am 23.02.2017 um 22:08 schrieb Pavel Machek:
>>>> On Wed 2017-02-22 21:35:52, Heiner Kallweit wrote:
>>>>> When registering a LED device we have the option to set a default trigger.
>>>>> Depending on load order of drivers this trigger may not be available yet.
>>>>> (affected LED device in my case: a DT-configured GPIO LED)
>>>>> So far if the default trigger can't be found this error is silently
>>>>> ignored.
>>>>>
>>>>> Let's change this to return EPROBE_DEFER if the default trigger can't be
>>>>> found. This gives the system the chance to probe the LED device later
>>>>> once the trigger is available.
>>>>
>>>> I see a lot of EPROBE_DEFERs on N900, and it is quite nasty, as it
>>>> spams a log a lot.
>>>>
>>> Usually error messages are printed only if there is an error and it is not
>>> EPROBE_DEFER. However indeed there still may be several drivers not
>>> taking into account that a subsystem they depend on may return
>>> EPROBE_DEFER and this should not be treated as "hard error".
>>>
>>>> Rather then re-trying LED registration few times, could we make sure
>>>> leds are always registered after triggers or something like that?
>>>>
>>> I'm afraid if guaranteeing a particular order would be possible w/o
>>> significant effort then the whole deferred probing concept wouldn't exist.
>>>
>>> I could imagine that we can try reordering definitions in the DTS to
>>> ensure a certain load order. But this might be somewhat fragile.
>>> So using the existing concept of deferred probing seems to me to be
>>> the cleaner solution.
>>
>> We could go for a solution similar to v4l2-async.c mechanism which
>> allows for deferring video pipeline linking until all video pipeline
>> entities are probed, but in the LED subsystem case I think it would be
>> an overengineering. V4L2 media device has to wait for completion of
>> probing of several v4l2 drivers, whereas here we've got to wait for
>> a single driver. EPROBE_DEFER seems to fit for that ideally.
> 
> I was more thinking of ... just initialize all triggers earlier then
> all LEDs?
> 
> Initcalls do have few levels we could use..

All LED triggers are registered from initialization functions
(marked with __init), and there are some LED class drivers that
are also probed from similar functions. We could e.g. try to move
triggers to subsys_initcall, but it would require thorough testing
to assure that we're not breaking anything.

-- 
Best regards,
Jacek Anaszewski