diff mbox series

[v2] backlight: propagate errors from get_brightness()

Message ID 20210907124751.6404-1-linux@weissschuh.net (mailing list archive)
State New, archived
Headers show
Series [v2] backlight: propagate errors from get_brightness() | expand

Commit Message

Thomas Weißschuh Sept. 7, 2021, 12:47 p.m. UTC
backlight.h documents "struct backlight_ops->get_brightness()" to return
a negative errno on failure.
So far these errors have not been handled in the backlight core.
This leads to negative values being exposed through sysfs although only
positive values are documented to be reported.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---

v1: https://lore.kernel.org/dri-devel/20210906215525.15418-1-linux@weissschuh.net/

v1 -> v2:
* use dev_err() instead of dev_warn() (Daniel Thompson)
* Finish logging format string with newline (Daniel Thompson)
* Log errno via dedicated error pointer format (Daniel Thompson)

 drivers/video/backlight/backlight.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)


base-commit: 79fad92f2e596f5a8dd085788a24f540263ef887

Comments

Daniel Thompson Sept. 7, 2021, 1:10 p.m. UTC | #1
On Tue, Sep 07, 2021 at 02:47:51PM +0200, Thomas Weißschuh wrote:
> backlight.h documents "struct backlight_ops->get_brightness()" to return
> a negative errno on failure.
> So far these errors have not been handled in the backlight core.
> This leads to negative values being exposed through sysfs although only
> positive values are documented to be reported.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


> ---
> 
> v1: https://lore.kernel.org/dri-devel/20210906215525.15418-1-linux@weissschuh.net/
> 
> v1 -> v2:
> * use dev_err() instead of dev_warn() (Daniel Thompson)
> * Finish logging format string with newline (Daniel Thompson)
> * Log errno via dedicated error pointer format (Daniel Thompson)
> 
>  drivers/video/backlight/backlight.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index 537fe1b376ad..4658cfb75aa2 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -292,10 +292,13 @@ static ssize_t actual_brightness_show(struct device *dev,
>  	struct backlight_device *bd = to_backlight_device(dev);
>  
>  	mutex_lock(&bd->ops_lock);
> -	if (bd->ops && bd->ops->get_brightness)
> -		rc = sprintf(buf, "%d\n", bd->ops->get_brightness(bd));
> -	else
> +	if (bd->ops && bd->ops->get_brightness) {
> +		rc = bd->ops->get_brightness(bd);
> +		if (rc >= 0)
> +			rc = sprintf(buf, "%d\n", rc);
> +	} else {
>  		rc = sprintf(buf, "%d\n", bd->props.brightness);
> +	}
>  	mutex_unlock(&bd->ops_lock);
>  
>  	return rc;
> @@ -381,9 +384,18 @@ ATTRIBUTE_GROUPS(bl_device);
>  void backlight_force_update(struct backlight_device *bd,
>  			    enum backlight_update_reason reason)
>  {
> +	int brightness;
> +
>  	mutex_lock(&bd->ops_lock);
> -	if (bd->ops && bd->ops->get_brightness)
> -		bd->props.brightness = bd->ops->get_brightness(bd);
> +	if (bd->ops && bd->ops->get_brightness) {
> +		brightness = bd->ops->get_brightness(bd);
> +		if (brightness >= 0)
> +			bd->props.brightness = brightness;
> +		else
> +			dev_err(&bd->dev,
> +				"Could not update brightness from device: %pe\n",
> +				ERR_PTR(brightness));
> +	}
>  	mutex_unlock(&bd->ops_lock);
>  	backlight_generate_event(bd, reason);
>  }
> 
> base-commit: 79fad92f2e596f5a8dd085788a24f540263ef887
> -- 
> 2.33.0
>
Thomas Weißschuh Sept. 7, 2021, 2:52 p.m. UTC | #2
On 2021-09-07T14:10+0100, Daniel Thompson wrote:
> On Tue, Sep 07, 2021 at 02:47:51PM +0200, Thomas Weißschuh wrote:
> > backlight.h documents "struct backlight_ops->get_brightness()" to return
> > a negative errno on failure.
> > So far these errors have not been handled in the backlight core.
> > This leads to negative values being exposed through sysfs although only
> > positive values are documented to be reported.
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> 
> Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>

Thanks!

Thomas
Thomas Weißschuh Sept. 21, 2021, 2:50 p.m. UTC | #3
On 2021-09-07T14:47+0200, Thomas Weißschuh wrote:
> backlight.h documents "struct backlight_ops->get_brightness()" to return
> a negative errno on failure.
> So far these errors have not been handled in the backlight core.
> This leads to negative values being exposed through sysfs although only
> positive values are documented to be reported.

> [..]

Friendly ping.
Lee Jones Sept. 21, 2021, 2:56 p.m. UTC | #4
On Tue, 21 Sep 2021, Thomas Weißschuh wrote:

> On 2021-09-07T14:47+0200, Thomas Weißschuh wrote:
> > backlight.h documents "struct backlight_ops->get_brightness()" to return
> > a negative errno on failure.
> > So far these errors have not been handled in the backlight core.
> > This leads to negative values being exposed through sysfs although only
> > positive values are documented to be reported.
> 
> > [..]
> 
> Friendly ping.

Don't do that.  If you think the submission has been forgotten about
(it hasn't), then please submit a [RESEND].  As it happens, this is on
my TOREVEW list.  I just need to get around to it post-vacation.
Lee Jones Sept. 23, 2021, 9:48 a.m. UTC | #5
On Tue, 07 Sep 2021, Thomas Weißschuh wrote:

> backlight.h documents "struct backlight_ops->get_brightness()" to return
> a negative errno on failure.
> So far these errors have not been handled in the backlight core.
> This leads to negative values being exposed through sysfs although only
> positive values are documented to be reported.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
> 
> v1: https://lore.kernel.org/dri-devel/20210906215525.15418-1-linux@weissschuh.net/
> 
> v1 -> v2:
> * use dev_err() instead of dev_warn() (Daniel Thompson)
> * Finish logging format string with newline (Daniel Thompson)
> * Log errno via dedicated error pointer format (Daniel Thompson)
> 
>  drivers/video/backlight/backlight.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)

Applied, thanks.
Thomas Weißschuh Sept. 23, 2021, 10:49 a.m. UTC | #6
On 2021-09-23T10:48+0100, Lee Jones wrote:
> On Tue, 07 Sep 2021, Thomas Weißschuh wrote:
> 
> > backlight.h documents "struct backlight_ops->get_brightness()" to return
> > a negative errno on failure.
> > So far these errors have not been handled in the backlight core.
> > This leads to negative values being exposed through sysfs although only
> > positive values are documented to be reported.
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> > 
> > v1: https://lore.kernel.org/dri-devel/20210906215525.15418-1-linux@weissschuh.net/
> > 
> > v1 -> v2:
> > * use dev_err() instead of dev_warn() (Daniel Thompson)
> > * Finish logging format string with newline (Daniel Thompson)
> > * Log errno via dedicated error pointer format (Daniel Thompson)
> > 
> >  drivers/video/backlight/backlight.c | 22 +++++++++++++++++-----
> >  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> Applied, thanks.

Hi Lee,

thanks!

Also I'm sorry about my nagging before.
I was not aware you were on vacation and saw you respond to other mails.

Thomas
Lee Jones Sept. 23, 2021, 1:43 p.m. UTC | #7
On Thu, 23 Sep 2021, Thomas Weißschuh wrote:

> On 2021-09-23T10:48+0100, Lee Jones wrote:
> > On Tue, 07 Sep 2021, Thomas Weißschuh wrote:
> > 
> > > backlight.h documents "struct backlight_ops->get_brightness()" to return
> > > a negative errno on failure.
> > > So far these errors have not been handled in the backlight core.
> > > This leads to negative values being exposed through sysfs although only
> > > positive values are documented to be reported.
> > > 
> > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > > ---
> > > 
> > > v1: https://lore.kernel.org/dri-devel/20210906215525.15418-1-linux@weissschuh.net/
> > > 
> > > v1 -> v2:
> > > * use dev_err() instead of dev_warn() (Daniel Thompson)
> > > * Finish logging format string with newline (Daniel Thompson)
> > > * Log errno via dedicated error pointer format (Daniel Thompson)
> > > 
> > >  drivers/video/backlight/backlight.c | 22 +++++++++++++++++-----
> > >  1 file changed, 17 insertions(+), 5 deletions(-)
> > 
> > Applied, thanks.
> 
> Hi Lee,
> 
> thanks!
> 
> Also I'm sorry about my nagging before.

No worries.

> I was not aware you were on vacation and saw you respond to other mails.

They were in the queue before this one.

I had hundreds of emails to get through on my return!
diff mbox series

Patch

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 537fe1b376ad..4658cfb75aa2 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -292,10 +292,13 @@  static ssize_t actual_brightness_show(struct device *dev,
 	struct backlight_device *bd = to_backlight_device(dev);
 
 	mutex_lock(&bd->ops_lock);
-	if (bd->ops && bd->ops->get_brightness)
-		rc = sprintf(buf, "%d\n", bd->ops->get_brightness(bd));
-	else
+	if (bd->ops && bd->ops->get_brightness) {
+		rc = bd->ops->get_brightness(bd);
+		if (rc >= 0)
+			rc = sprintf(buf, "%d\n", rc);
+	} else {
 		rc = sprintf(buf, "%d\n", bd->props.brightness);
+	}
 	mutex_unlock(&bd->ops_lock);
 
 	return rc;
@@ -381,9 +384,18 @@  ATTRIBUTE_GROUPS(bl_device);
 void backlight_force_update(struct backlight_device *bd,
 			    enum backlight_update_reason reason)
 {
+	int brightness;
+
 	mutex_lock(&bd->ops_lock);
-	if (bd->ops && bd->ops->get_brightness)
-		bd->props.brightness = bd->ops->get_brightness(bd);
+	if (bd->ops && bd->ops->get_brightness) {
+		brightness = bd->ops->get_brightness(bd);
+		if (brightness >= 0)
+			bd->props.brightness = brightness;
+		else
+			dev_err(&bd->dev,
+				"Could not update brightness from device: %pe\n",
+				ERR_PTR(brightness));
+	}
 	mutex_unlock(&bd->ops_lock);
 	backlight_generate_event(bd, reason);
 }