Re: [PATCH v3 8/8] leds: leds-mt6323: Add support for WLEDs and MT6332

[AngeloGioacchino Del Regno]

Il 21/06/23 23:31, Nathan Chancellor ha scritto:
> Hi Angelo,
>
> On Thu, Jun 01, 2023 at 01:08:13PM +0200, AngeloGioacchino Del Regno wrote:
> > Add basic code to turn on and off WLEDs and wire up MT6332 support
> > to take advantage of it.
> > This is a simple approach due to the aforementioned PMIC supporting
> > only on/off status so, at the time of writing, it is impossible for me
> > to validate more advanced functionality due to lack of hardware.
> >
> > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
>
> After this patch as commit 9bb0a9e0626c ("leds: leds-mt6323: Add support
> for WLEDs and MT6332") in -next, I see the following warnings from
> clang, which are basically flagging potential kernel Control Flow
> Integrity [1] violations that will be visible at runtime (this warning
> is not enabled for the kernel yet but we would like it to be):
>
>    drivers/leds/leds-mt6323.c:598:49: error: incompatible function pointer types assigning to 'int (*)(struct led_classdev *, enum led_brightness)' from 'int (struct led_classdev *, unsigned int)' [-Werror,-Wincompatible-function-pointer-types-strict]
>      598 |                         leds->led[reg]->cdev.brightness_set_blocking =
>          |                                                                      ^
>      599 |                                                 mt6323_wled_set_brightness;
>          |                                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~
>    drivers/leds/leds-mt6323.c:600:40: error: incompatible function pointer types assigning to 'enum led_brightness (*)(struct led_classdev *)' from 'unsigned int (struct led_classdev *)' [-Werror,-Wincompatible-function-pointer-types-strict]
>      600 |                         leds->led[reg]->cdev.brightness_get =
>          |                                                             ^
>      601 |                                                 mt6323_get_wled_brightness;
>          |                                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~
>    2 errors generated.
>
>  From what I can tell/understand, 'enum led_brightness' is obsolete and
> the value that is passed via ->brightness_set_blocking() is an 'unsigned
> int' as well but it seems 'enum led_brightness' is used as the parameter
> in a lot of different callback implementations, so the prototype cannot
> be easily updated without a lot of extra work. Is there any reason not
> to just do something like this to avoid this issue?

I don't think that this would bring any issue to the table.

The rework will possibly be done globally, for all drivers, when time comes... so
feel free to send the proposed patch.

Thanks,
Angelo

> [1]: https://lwn.net/Articles/898040/
>
> Cheers,
> Nathan
>
> diff --git a/drivers/leds/leds-mt6323.c b/drivers/leds/leds-mt6323.c
> index e8fecfc2e90a..24f35bdb55fb 100644
> --- a/drivers/leds/leds-mt6323.c
> +++ b/drivers/leds/leds-mt6323.c
> @@ -76,7 +76,7 @@ struct mt6323_led {
>   	int			id;
>   	struct mt6323_leds	*parent;
>   	struct led_classdev	cdev;
> -	unsigned int		current_brightness;
> +	enum led_brightness	current_brightness;
>   };
>   
>   /**
> @@ -451,7 +451,7 @@ static int mtk_wled_hw_off(struct led_classdev *cdev)
>   	return 0;
>   }
>   
> -static unsigned int mt6323_get_wled_brightness(struct led_classdev *cdev)
> +static enum led_brightness mt6323_get_wled_brightness(struct led_classdev *cdev)
>   {
>   	struct mt6323_led *led = container_of(cdev, struct mt6323_led, cdev);
>   	struct mt6323_leds *leds = led->parent;
> @@ -471,7 +471,7 @@ static unsigned int mt6323_get_wled_brightness(struct led_classdev *cdev)
>   }
>   
>   static int mt6323_wled_set_brightness(struct led_classdev *cdev,
> -				      unsigned int brightness)
> +				      enum led_brightness brightness)
>   {
>   	struct mt6323_led *led = container_of(cdev, struct mt6323_led, cdev);
>   	struct mt6323_leds *leds = led->parent;