Re: [PATCH v4 11/14] video: backlight: lm3533_bl: Improve logic of sysfs functions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] sysfs show functions `show_als_en` and `show_linear` return prematurely when the tested bits are set, breaking the sysfs read and returning an empty/uninitialized buffer.
--

commit 549a7f316072e9f93bfcd3d182e73739a35c0c7a
Author: Svyatoslav Ryhel <clamor95@gmail.com>

video: backlight: lm3533_bl: Improve logic of sysfs functions

Simplify the sysfs logic of properties by switching to macros and proper
regmap helpers.

> diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
> --- a/drivers/video/backlight/lm3533_bl.c
> +++ b/drivers/video/backlight/lm3533_bl.c

[ ... ]

> @@ -85,88 +87,68 @@ static ssize_t show_als_channel(struct device *dev,
>  	return scnprintf(buf, PAGE_SIZE, "%u\n", channel);
>  }
>  
> -static ssize_t show_als_en(struct device *dev,
> -				struct device_attribute *attr, char *buf)
> +static ssize_t show_als_en(struct device *dev, struct device_attribute *attr,
> +			   char *buf)
>  {
>  	struct lm3533_bl *bl = dev_get_drvdata(dev);
>  	int ctrlbank = lm3533_bl_get_ctrlbank_id(bl);
> -	u32 val;
> -	u8 mask;
> -	bool enable;
>  	int ret;
>  
> -	ret = regmap_read(bl->regmap, LM3533_REG_CTRLBANK_AB_BCONF, &val);
> +	ret = regmap_test_bits(bl->regmap, LM3533_REG_CTRLBANK_AB_BCONF,
> +			       CTRLBANK_AB_BCONF_ALS(ctrlbank));
>  	if (ret)
>  		return ret;

[Severity: High]
Does this error check prematurely exit the function when the bit is set?

regmap_test_bits() returns 1 if the specified bit is set. If the bit is set, 
the if (ret) condition evaluates to true, causing the function to return 1
immediately instead of formatting the buffer.

This could break the sysfs API contract and return a single null byte to
userspace instead of the expected string. Should this check be if (ret < 0)
instead?

>  
> -	mask = 1 << (2 * ctrlbank);
> -	enable = val & mask;
> -
> -	return scnprintf(buf, PAGE_SIZE, "%d\n", enable);
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", ret);
>  }

[ ... ]

> -static ssize_t show_linear(struct device *dev,
> -				struct device_attribute *attr, char *buf)
> +static ssize_t show_linear(struct device *dev, struct device_attribute *attr,
> +			   char *buf)
>  {
>  	struct lm3533_bl *bl = dev_get_drvdata(dev);
> -	u32 val;
> -	u8 mask;
> -	int linear;
> +	int ctrlbank = lm3533_bl_get_ctrlbank_id(bl);
>  	int ret;
>  
> -	ret = regmap_read(bl->regmap, LM3533_REG_CTRLBANK_AB_BCONF, &val);
> +	ret = regmap_test_bits(bl->regmap, LM3533_REG_CTRLBANK_AB_BCONF,
> +			       CTRLBANK_AB_BCONF_MODE(ctrlbank));
>  	if (ret)
>  		return ret;

[Severity: High]
Could this have the same issue as show_als_en() above?

When the linear mode bit is set, regmap_test_bits() returns 1, triggering
this error path and returning 1 directly instead of calling scnprintf().

>  
> -	mask = 1 << (2 * lm3533_bl_get_ctrlbank_id(bl) + 1);
> -
> -	if (val & mask)
> -		linear = 1;
> -	else
> -		linear = 0;
> -
> -	return scnprintf(buf, PAGE_SIZE, "%x\n", linear);
> +	return scnprintf(buf, PAGE_SIZE, "%x\n", ret);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260606045738.21050-1-clamor95@gmail.com?part=11