On Mon, Sep 06, 2021 at 11:55:25PM +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@xxxxxxxxxxxxxx>
Change looks good overall but I've raised a few quibbles about the new
error message below.
> ---
> 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..d681962f8509 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_warn(&bd->dev,
> + "Could not update brightness from device: errno = %d",
> + -brightness);
The format string is missing a \n and should it really be a dev_warn()?
Is dev_err() more appropriate?
Also please print the error symbolically (which is self explaining
meaning we don't need the errno prefix):
"Could not update brightness from device: %pe\n",
ERR_PTR(brightness)
Daniel.
> + }
> mutex_unlock(&bd->ops_lock);
> backlight_generate_event(bd, reason);
> }
>
> base-commit: 79fad92f2e596f5a8dd085788a24f540263ef887
> --
> 2.33.0
>
