From: Lee Jones <lee@kernel.org> To: Nick Desaulniers <ndesaulniers@google.com> Cc: Nathan Chancellor <nathan@kernel.org>, sean.wang@mediatek.com, pavel@ucw.cz, matthias.bgg@gmail.com, angelogioacchino.delregno@collabora.com, trix@redhat.com, linux-leds@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, llvm@lists.linux.dev, patches@lists.linux.dev Subject: Re: [PATCH] leds: leds-mt6323: Adjust return/parameter types in wled get/set callbacks Date: Thu, 22 Jun 2023 17:54:05 +0100 [thread overview] Message-ID: <20230622165405.GX10378@google.com> (raw) In-Reply-To: <CAKwvOd=jHrch4a-AZgXmScGKW2Fs4MHwH0iaW_8PgR=iYfvrEg@mail.gmail.com> On Thu, 22 Jun 2023, Nick Desaulniers wrote: > On Thu, Jun 22, 2023 at 9:12 AM Nathan Chancellor <nathan@kernel.org> wrote: > > > > Clang's kernel Control Flow Integrity (kCFI) is a compiler-based > > security mitigation that ensures the target of an indirect function call > > matches the expected type of the call and trapping if they do not match > > exactly. The warning -Wincompatible-function-pointer-types-strict aims > > to catch these issues at compile time, which reveals: > > > > 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. > > > > While 'unsigned int' is ABI compatible with 'enum led_brightness' (hence > > no warning from -Wincompatible-function-pointer-types) and the callers > > of these callbacks use/pass the values as 'unsigned int', the mismatch > > between the prototype and the called function will trip kCFI at runtime. > > > > Change the types in the implementations to match the prototypes, clearing > > up the warning and avoiding kCFI failures. > > > > Fixes: 9bb0a9e0626c ("leds: leds-mt6323: Add support for WLEDs and MT6332") > > Signed-off-by: Nathan Chancellor <nathan@kernel.org> > > Thanks for the patch! Consider additionally having > mt6323_get_wled_brightness return LED_OFF rather than 0 in its > ternary. https://elixir.bootlin.com/linux/latest/source/include/linux/leds.h#L32 Perhaps this is not relevant for this older driver though? I'd really like some more information from Pavel on the history. > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > > > --- > > drivers/leds/leds-mt6323.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > 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; > > > > --- > > base-commit: 7bd932d9adbcc5c5370d968bdb0b00385606bf3a > > change-id: 20230621-mt6323-wled-wincompatible-function-pointer-types-strict-334f06d92ffb > > > > Best regards, > > -- > > Nathan Chancellor <nathan@kernel.org> > > > > > > > -- > Thanks, > ~Nick Desaulniers -- Lee Jones [李琼斯]
WARNING: multiple messages have this Message-ID (diff)
From: Lee Jones <lee@kernel.org> To: Nick Desaulniers <ndesaulniers@google.com> Cc: Nathan Chancellor <nathan@kernel.org>, sean.wang@mediatek.com, pavel@ucw.cz, matthias.bgg@gmail.com, angelogioacchino.delregno@collabora.com, trix@redhat.com, linux-leds@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, llvm@lists.linux.dev, patches@lists.linux.dev Subject: Re: [PATCH] leds: leds-mt6323: Adjust return/parameter types in wled get/set callbacks Date: Thu, 22 Jun 2023 17:54:05 +0100 [thread overview] Message-ID: <20230622165405.GX10378@google.com> (raw) In-Reply-To: <CAKwvOd=jHrch4a-AZgXmScGKW2Fs4MHwH0iaW_8PgR=iYfvrEg@mail.gmail.com> On Thu, 22 Jun 2023, Nick Desaulniers wrote: > On Thu, Jun 22, 2023 at 9:12 AM Nathan Chancellor <nathan@kernel.org> wrote: > > > > Clang's kernel Control Flow Integrity (kCFI) is a compiler-based > > security mitigation that ensures the target of an indirect function call > > matches the expected type of the call and trapping if they do not match > > exactly. The warning -Wincompatible-function-pointer-types-strict aims > > to catch these issues at compile time, which reveals: > > > > 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. > > > > While 'unsigned int' is ABI compatible with 'enum led_brightness' (hence > > no warning from -Wincompatible-function-pointer-types) and the callers > > of these callbacks use/pass the values as 'unsigned int', the mismatch > > between the prototype and the called function will trip kCFI at runtime. > > > > Change the types in the implementations to match the prototypes, clearing > > up the warning and avoiding kCFI failures. > > > > Fixes: 9bb0a9e0626c ("leds: leds-mt6323: Add support for WLEDs and MT6332") > > Signed-off-by: Nathan Chancellor <nathan@kernel.org> > > Thanks for the patch! Consider additionally having > mt6323_get_wled_brightness return LED_OFF rather than 0 in its > ternary. https://elixir.bootlin.com/linux/latest/source/include/linux/leds.h#L32 Perhaps this is not relevant for this older driver though? I'd really like some more information from Pavel on the history. > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > > > --- > > drivers/leds/leds-mt6323.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > 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; > > > > --- > > base-commit: 7bd932d9adbcc5c5370d968bdb0b00385606bf3a > > change-id: 20230621-mt6323-wled-wincompatible-function-pointer-types-strict-334f06d92ffb > > > > Best regards, > > -- > > Nathan Chancellor <nathan@kernel.org> > > > > > > > -- > Thanks, > ~Nick Desaulniers -- Lee Jones [李琼斯] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-06-22 16:54 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-06-22 16:12 [PATCH] leds: leds-mt6323: Adjust return/parameter types in wled get/set callbacks Nathan Chancellor 2023-06-22 16:12 ` Nathan Chancellor 2023-06-22 16:43 ` Nick Desaulniers 2023-06-22 16:43 ` Nick Desaulniers 2023-06-22 16:54 ` Lee Jones [this message] 2023-06-22 16:54 ` Lee Jones 2023-06-23 8:04 ` AngeloGioacchino Del Regno 2023-06-23 8:04 ` AngeloGioacchino Del Regno 2023-06-26 9:10 ` Lee Jones 2023-06-26 9:10 ` Lee Jones
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20230622165405.GX10378@google.com \ --to=lee@kernel.org \ --cc=angelogioacchino.delregno@collabora.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-leds@vger.kernel.org \ --cc=linux-mediatek@lists.infradead.org \ --cc=llvm@lists.linux.dev \ --cc=matthias.bgg@gmail.com \ --cc=nathan@kernel.org \ --cc=ndesaulniers@google.com \ --cc=patches@lists.linux.dev \ --cc=pavel@ucw.cz \ --cc=sean.wang@mediatek.com \ --cc=trix@redhat.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.