[PATCH] usb: atm: cxacru: Zero initialize bp in cxacru_heavy_init()

[PATCH] usb: atm: cxacru: Zero initialize bp in cxacru_heavy_init()

[Nathan Chancellor]

After a recent change in clang to expose uninitialized warnings from
const variables [1], there is a warning in cxacru_heavy_init():

  drivers/usb/atm/cxacru.c:1104:6: error: variable 'bp' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
   1104 |         if (instance->modem_type->boot_rom_patch) {
        |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  drivers/usb/atm/cxacru.c:1113:39: note: uninitialized use occurs here
   1113 |         cxacru_upload_firmware(instance, fw, bp);
        |                                              ^~
  drivers/usb/atm/cxacru.c:1104:2: note: remove the 'if' if its condition is always true
   1104 |         if (instance->modem_type->boot_rom_patch) {
        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  drivers/usb/atm/cxacru.c:1095:32: note: initialize the variable 'bp' to silence this warning
   1095 |         const struct firmware *fw, *bp;
        |                                       ^
        |                                        = NULL

This warning occurs in clang's frontend before inlining occurs, so it
cannot notice that bp is only used within cxacru_upload_firmware() under
the same condition that initializes it in cxacru_heavy_init(). Just
initialize bp to NULL to silence the warning without functionally
changing the code, which is what happens with modern compilers when they
support '-ftrivial-auto-var-init=zero' (CONFIG_INIT_STACK_ALL_ZERO=y).

Cc: stable@vger.kernel.org
Fixes: 1b0e61465234 ("[PATCH] USB ATM: driver for the Conexant AccessRunner chipset cxacru")
Closes: https://github.com/ClangBuiltLinux/linux/issues/2102
Link: https://github.com/llvm/llvm-project/commit/2464313eef01c5b1edf0eccf57a32cdee01472c7 [1]
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 drivers/usb/atm/cxacru.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
index a12ab90b3db7..b7c3b224a759 100644
--- a/drivers/usb/atm/cxacru.c
+++ b/drivers/usb/atm/cxacru.c
@@ -1092,7 +1092,7 @@ static int cxacru_find_firmware(struct cxacru_data *instance,
 static int cxacru_heavy_init(struct usbatm_data *usbatm_instance,
 			     struct usb_interface *usb_intf)
 {
-	const struct firmware *fw, *bp;
+	const struct firmware *fw, *bp = NULL;
 	struct cxacru_data *instance = usbatm_instance->driver_data;
 	int ret = cxacru_find_firmware(instance, "fw", &fw);
 

---
base-commit: fdfa018c6962c86d2faa183187669569be4d513f
change-id: 20250715-usb-cxacru-fix-clang-21-uninit-warning-9430d96c6bc1

Best regards,
--  
Nathan Chancellor <nathan@kernel.org>

Re: [PATCH] usb: atm: cxacru: Zero initialize bp in cxacru_heavy_init()

[Greg Kroah-Hartman]

On Tue, Jul 15, 2025 at 01:33:32PM -0700, Nathan Chancellor wrote:
> After a recent change in clang to expose uninitialized warnings from
> const variables [1], there is a warning in cxacru_heavy_init():
> 
>   drivers/usb/atm/cxacru.c:1104:6: error: variable 'bp' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
>    1104 |         if (instance->modem_type->boot_rom_patch) {
>         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   drivers/usb/atm/cxacru.c:1113:39: note: uninitialized use occurs here
>    1113 |         cxacru_upload_firmware(instance, fw, bp);
>         |                                              ^~
>   drivers/usb/atm/cxacru.c:1104:2: note: remove the 'if' if its condition is always true
>    1104 |         if (instance->modem_type->boot_rom_patch) {
>         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   drivers/usb/atm/cxacru.c:1095:32: note: initialize the variable 'bp' to silence this warning
>    1095 |         const struct firmware *fw, *bp;
>         |                                       ^
>         |                                        = NULL
> 
> This warning occurs in clang's frontend before inlining occurs, so it
> cannot notice that bp is only used within cxacru_upload_firmware() under
> the same condition that initializes it in cxacru_heavy_init(). Just
> initialize bp to NULL to silence the warning without functionally
> changing the code, which is what happens with modern compilers when they
> support '-ftrivial-auto-var-init=zero' (CONFIG_INIT_STACK_ALL_ZERO=y).

We generally do not want to paper over compiler bugs, when our code is
correct, so why should we do that here?  Why not fix clang instead?

thanks,

greg k-h

Re: [PATCH] usb: atm: cxacru: Zero initialize bp in cxacru_heavy_init()

[Nathan Chancellor]

On Wed, Jul 16, 2025 at 07:06:50AM +0200, Greg Kroah-Hartman wrote:
> On Tue, Jul 15, 2025 at 01:33:32PM -0700, Nathan Chancellor wrote:
> > After a recent change in clang to expose uninitialized warnings from
> > const variables [1], there is a warning in cxacru_heavy_init():
> > 
> >   drivers/usb/atm/cxacru.c:1104:6: error: variable 'bp' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
> >    1104 |         if (instance->modem_type->boot_rom_patch) {
> >         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >   drivers/usb/atm/cxacru.c:1113:39: note: uninitialized use occurs here
> >    1113 |         cxacru_upload_firmware(instance, fw, bp);
> >         |                                              ^~
> >   drivers/usb/atm/cxacru.c:1104:2: note: remove the 'if' if its condition is always true
> >    1104 |         if (instance->modem_type->boot_rom_patch) {
> >         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >   drivers/usb/atm/cxacru.c:1095:32: note: initialize the variable 'bp' to silence this warning
> >    1095 |         const struct firmware *fw, *bp;
> >         |                                       ^
> >         |                                        = NULL
> > 
> > This warning occurs in clang's frontend before inlining occurs, so it
> > cannot notice that bp is only used within cxacru_upload_firmware() under
> > the same condition that initializes it in cxacru_heavy_init(). Just
> > initialize bp to NULL to silence the warning without functionally
> > changing the code, which is what happens with modern compilers when they
> > support '-ftrivial-auto-var-init=zero' (CONFIG_INIT_STACK_ALL_ZERO=y).
> 
> We generally do not want to paper over compiler bugs, when our code is
> correct, so why should we do that here?  Why not fix clang instead?

I would not really call this a compiler bug. It IS passed uninitialized
to this function and while the uninitialized value is not actually used,
clang has no way of knowing that at this point in its pipeline, so I
don't think warning in this case is unreasonable. This type of warning
is off for GCC because of how unreliable it was when it is done in the
middle end with optimizations. Furthermore, it is my understanding based
on [1] that just the passing of an uninitialized variable in this manner
is UB.

[1]: https://lore.kernel.org/20220614214039.GA25951@gate.crashing.org/

Cheers,
Nathan

Re: [PATCH] usb: atm: cxacru: Zero initialize bp in cxacru_heavy_init()

[Greg Kroah-Hartman]

On Tue, Jul 15, 2025 at 10:24:50PM -0700, Nathan Chancellor wrote:
> On Wed, Jul 16, 2025 at 07:06:50AM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Jul 15, 2025 at 01:33:32PM -0700, Nathan Chancellor wrote:
> > > After a recent change in clang to expose uninitialized warnings from
> > > const variables [1], there is a warning in cxacru_heavy_init():
> > > 
> > >   drivers/usb/atm/cxacru.c:1104:6: error: variable 'bp' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
> > >    1104 |         if (instance->modem_type->boot_rom_patch) {
> > >         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >   drivers/usb/atm/cxacru.c:1113:39: note: uninitialized use occurs here
> > >    1113 |         cxacru_upload_firmware(instance, fw, bp);
> > >         |                                              ^~
> > >   drivers/usb/atm/cxacru.c:1104:2: note: remove the 'if' if its condition is always true
> > >    1104 |         if (instance->modem_type->boot_rom_patch) {
> > >         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >   drivers/usb/atm/cxacru.c:1095:32: note: initialize the variable 'bp' to silence this warning
> > >    1095 |         const struct firmware *fw, *bp;
> > >         |                                       ^
> > >         |                                        = NULL
> > > 
> > > This warning occurs in clang's frontend before inlining occurs, so it
> > > cannot notice that bp is only used within cxacru_upload_firmware() under
> > > the same condition that initializes it in cxacru_heavy_init(). Just
> > > initialize bp to NULL to silence the warning without functionally
> > > changing the code, which is what happens with modern compilers when they
> > > support '-ftrivial-auto-var-init=zero' (CONFIG_INIT_STACK_ALL_ZERO=y).
> > 
> > We generally do not want to paper over compiler bugs, when our code is
> > correct, so why should we do that here?  Why not fix clang instead?
> 
> I would not really call this a compiler bug. It IS passed uninitialized
> to this function and while the uninitialized value is not actually used,
> clang has no way of knowing that at this point in its pipeline, so I
> don't think warning in this case is unreasonable. This type of warning
> is off for GCC because of how unreliable it was when it is done in the
> middle end with optimizations. Furthermore, it is my understanding based
> on [1] that just the passing of an uninitialized variable in this manner
> is UB.
> 
> [1]: https://lore.kernel.org/20220614214039.GA25951@gate.crashing.org/

Ah, I see now what you are referring to, sorry.  I'll go queue this up
now, thanks.

greg k-h

Re: [PATCH] usb: atm: cxacru: Zero initialize bp in cxacru_heavy_init()

[Greg Kroah-Hartman]

On Tue, Jul 15, 2025 at 10:24:50PM -0700, Nathan Chancellor wrote:
> On Wed, Jul 16, 2025 at 07:06:50AM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Jul 15, 2025 at 01:33:32PM -0700, Nathan Chancellor wrote:
> > > After a recent change in clang to expose uninitialized warnings from
> > > const variables [1], there is a warning in cxacru_heavy_init():
> > > 
> > >   drivers/usb/atm/cxacru.c:1104:6: error: variable 'bp' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
> > >    1104 |         if (instance->modem_type->boot_rom_patch) {
> > >         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >   drivers/usb/atm/cxacru.c:1113:39: note: uninitialized use occurs here
> > >    1113 |         cxacru_upload_firmware(instance, fw, bp);
> > >         |                                              ^~
> > >   drivers/usb/atm/cxacru.c:1104:2: note: remove the 'if' if its condition is always true
> > >    1104 |         if (instance->modem_type->boot_rom_patch) {
> > >         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >   drivers/usb/atm/cxacru.c:1095:32: note: initialize the variable 'bp' to silence this warning
> > >    1095 |         const struct firmware *fw, *bp;
> > >         |                                       ^
> > >         |                                        = NULL
> > > 
> > > This warning occurs in clang's frontend before inlining occurs, so it
> > > cannot notice that bp is only used within cxacru_upload_firmware() under
> > > the same condition that initializes it in cxacru_heavy_init(). Just
> > > initialize bp to NULL to silence the warning without functionally
> > > changing the code, which is what happens with modern compilers when they
> > > support '-ftrivial-auto-var-init=zero' (CONFIG_INIT_STACK_ALL_ZERO=y).
> > 
> > We generally do not want to paper over compiler bugs, when our code is
> > correct, so why should we do that here?  Why not fix clang instead?
> 
> I would not really call this a compiler bug. It IS passed uninitialized
> to this function and while the uninitialized value is not actually used,
> clang has no way of knowing that at this point in its pipeline, so I
> don't think warning in this case is unreasonable.

No, I take it back, it is unreasonable :)

At runtime, there never is a uninitialized use of this pointer, the
first time it is used, it is intended to be filled in if this is a "boot
rom patch":
	ret = cxacru_find_firmware(instance, "bp", &bp);

Then if that call fails, the function exits, great.

Then later on, this is called:
	cxacru_upload_firmware(instance, fw, bp);
so either bp IS valid, or it's still uninitialized, fair enough.

But then cxacru_upload_firmware() does the same check for "is this a
boot rom patch" and only then does it reference the variable.

And when it references it, it does NOT check if it is valid or not, so
even if you do pre-initialize this to NULL, surely some other static
checker is going to come along and say "Hey, you just dereferenced a
NULL pointer, this needs to be fixed!" when that too is just not true at
all.

So the logic here is all "safe" for now, and if you set this to NULL,
you are just papering over the fact that it is right, AND setting us up
to get another patch that actually does nothing, while feeling like the
submitter just fixed a security bug, demanding a CVE for an impossible
code path :)

So let's leave this for now because:

> This type of warning
> is off for GCC because of how unreliable it was when it is done in the
> middle end with optimizations. Furthermore, it is my understanding based
> on [1] that just the passing of an uninitialized variable in this manner
> is UB.
> 
> [1]: https://lore.kernel.org/20220614214039.GA25951@gate.crashing.org/

As gcc can't handle this either, it seems that clang also can't handle
it.  So turning this on for the kernel surely is going to trip it up in
other places than just this one driver.

If you _really_ want to fix this, refactor the code to be more sane and
obvious from a C parsing standpoint, but really, it isn't that complex
for a human to read and understand, and I see why it was written this
way.

As for the UB argument, bah, I don't care, sane compilers will do the
right thing, i.e. pass in the uninitialized value, or if we turned on
the 0-fill stack option, will be NULL anyway, otherwise why do we have
that option if not to "solve" the UB issue?).

thanks,

greg k-h

Re: [PATCH] usb: atm: cxacru: Zero initialize bp in cxacru_heavy_init()

[Nathan Chancellor]

On Wed, Jul 16, 2025 at 10:00:10AM +0200, Greg Kroah-Hartman wrote:
> No, I take it back, it is unreasonable :)
> 
> At runtime, there never is a uninitialized use of this pointer, the
> first time it is used, it is intended to be filled in if this is a "boot
> rom patch":
> 	ret = cxacru_find_firmware(instance, "bp", &bp);
> 
> Then if that call fails, the function exits, great.
> 
> Then later on, this is called:
> 	cxacru_upload_firmware(instance, fw, bp);
> so either bp IS valid, or it's still uninitialized, fair enough.
> 
> But then cxacru_upload_firmware() does the same check for "is this a
> boot rom patch" and only then does it reference the variable.

Right but how would you know this if you were unable to look at what's
inside cxacru_upload_firmware()? That's basically what is happening with
clang, it is only able to look at cxacru_heavy_init().

> And when it references it, it does NOT check if it is valid or not, so
> even if you do pre-initialize this to NULL, surely some other static
> checker is going to come along and say "Hey, you just dereferenced a
> NULL pointer, this needs to be fixed!" when that too is just not true at
> all.

If a static checker has the ability to see the NULL passed to
cxacru_upload_firmware() from cxacru_heavy_init(), I would expect it to
notice the identical conditions but point taken :)

> So the logic here is all "safe" for now, and if you set this to NULL,
> you are just papering over the fact that it is right, AND setting us up
> to get another patch that actually does nothing, while feeling like the
> submitter just fixed a security bug, demanding a CVE for an impossible
> code path :)

Wouldn this be sufficient to avoid such a situation?

diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
index b7c3b224a759..fcff092fe826 100644
--- a/drivers/usb/atm/cxacru.c
+++ b/drivers/usb/atm/cxacru.c
@@ -1026,7 +1026,7 @@ static void cxacru_upload_firmware(struct cxacru_data *instance,
 	}
 
 	/* Boot ROM patch */
-	if (instance->modem_type->boot_rom_patch) {
+	if (instance->modem_type->boot_rom_patch && bp) {
 		usb_info(usbatm, "loading boot ROM patch\n");
 		ret = cxacru_fw(usb_dev, FW_WRITE_MEM, 0x2, 0x0, BR_ADDR, bp->data, bp->size);
 		if (ret) {


> So let's leave this for now because:
> 
> > This type of warning
> > is off for GCC because of how unreliable it was when it is done in the
> > middle end with optimizations. Furthermore, it is my understanding based
> > on [1] that just the passing of an uninitialized variable in this manner
> > is UB.
> > 
> > [1]: https://lore.kernel.org/20220614214039.GA25951@gate.crashing.org/
> 
> As gcc can't handle this either, it seems that clang also can't handle
> it.  So turning this on for the kernel surely is going to trip it up in
> other places than just this one driver.

I turned this warning on in 5.3 in commit 3a61925e91ba ("kbuild: Enable
-Wuninitialized"), so it is already enabled and it has found many, many
legitimate instances in doing so, just go run 'git log
--grep=Wuninitialized' or 'git log --grep=Wsometimes-uninitialized' in
the kernel sources. While there have been other places in the kernel
where this warning has been falsely triggered such as here, I have
rarely received pushback from maintainers on fixes to silence them
because the majority of them are legitimate (and the false positive
fixes usually result in more robust code). For example, the
strengthening of the warning in clang-21 resulted in what I would
consider legitimate fixes:

https://lore.kernel.org/20250715-mt7996-fix-uninit-const-pointer-v1-1-b5d8d11d7b78@kernel.org/
https://lore.kernel.org/20250715-net-phonet-fix-uninit-const-pointer-v1-1-8efd1bd188b3@kernel.org/
https://lore.kernel.org/20250715-drm-msm-fix-const-uninit-warning-v1-1-d6a366fd9a32@kernel.org/
https://lore.kernel.org/20250715-riscv-uaccess-fix-self-init-val-v1-1-82b8e911f120@kernel.org/
https://lore.kernel.org/20250715-trace_probe-fix-const-uninit-warning-v1-1-98960f91dd04@kernel.org/
https://lore.kernel.org/20250715-sdca_interrupts-fix-const-uninit-warning-v1-1-cc031c913499@kernel.org/

> If you _really_ want to fix this, refactor the code to be more sane and
> obvious from a C parsing standpoint, but really, it isn't that complex
> for a human to read and understand, and I see why it was written this
> way.

Yes, I agree that it is not complex or hard to understand, so I would
rather not refactor it, but I do need this fixed so that allmodconfig
builds (which enable -Werror by default) with clang-21 do not break.
Won't Android eventually hit this when they get a newer compiler?

> As for the UB argument, bah, I don't care, sane compilers will do the
> right thing, i.e. pass in the uninitialized value, or if we turned on
> the 0-fill stack option, will be NULL anyway, otherwise why do we have
> that option if not to "solve" the UB issue?).

As far as I understand it, clang adds "noundef" to function parameters
when lowering to LLVM IR, which codifies that passing an uninitialized
value is UB. I suspect that cxacru_upload_firmware() gets inlined so
that ends up not mattering in this case but it could in others.

While '-ftrivial-auto-var-init=zero' does "solve" the UB issue, I see it
more of a mitigation against missed initializations, not as a
replacement for ensuring variables are consistently initialized, as zero
might not be the expected initialization. Since that is the default for
the kernel when compilers support it, why not just take this patch with
that fixup above to make it consistent? I would be happy to send a v2 if
you would be okay with it.

Cheers,
Nathan

Re: [PATCH] usb: atm: cxacru: Zero initialize bp in cxacru_heavy_init()

[Greg Kroah-Hartman]

On Wed, Jul 16, 2025 at 08:43:04AM -0700, Nathan Chancellor wrote:
> On Wed, Jul 16, 2025 at 10:00:10AM +0200, Greg Kroah-Hartman wrote:
> > No, I take it back, it is unreasonable :)
> > 
> > At runtime, there never is a uninitialized use of this pointer, the
> > first time it is used, it is intended to be filled in if this is a "boot
> > rom patch":
> > 	ret = cxacru_find_firmware(instance, "bp", &bp);
> > 
> > Then if that call fails, the function exits, great.
> > 
> > Then later on, this is called:
> > 	cxacru_upload_firmware(instance, fw, bp);
> > so either bp IS valid, or it's still uninitialized, fair enough.
> > 
> > But then cxacru_upload_firmware() does the same check for "is this a
> > boot rom patch" and only then does it reference the variable.
> 
> Right but how would you know this if you were unable to look at what's
> inside cxacru_upload_firmware()? That's basically what is happening with
> clang, it is only able to look at cxacru_heavy_init().

True, and it's also unable to look into cxacru_upload_firmware() :)

> > And when it references it, it does NOT check if it is valid or not, so
> > even if you do pre-initialize this to NULL, surely some other static
> > checker is going to come along and say "Hey, you just dereferenced a
> > NULL pointer, this needs to be fixed!" when that too is just not true at
> > all.
> 
> If a static checker has the ability to see the NULL passed to
> cxacru_upload_firmware() from cxacru_heavy_init(), I would expect it to
> notice the identical conditions but point taken :)



> 
> > So the logic here is all "safe" for now, and if you set this to NULL,
> > you are just papering over the fact that it is right, AND setting us up
> > to get another patch that actually does nothing, while feeling like the
> > submitter just fixed a security bug, demanding a CVE for an impossible
> > code path :)
> 
> Wouldn this be sufficient to avoid such a situation?
> 
> diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
> index b7c3b224a759..fcff092fe826 100644
> --- a/drivers/usb/atm/cxacru.c
> +++ b/drivers/usb/atm/cxacru.c
> @@ -1026,7 +1026,7 @@ static void cxacru_upload_firmware(struct cxacru_data *instance,
>  	}
>  
>  	/* Boot ROM patch */
> -	if (instance->modem_type->boot_rom_patch) {
> +	if (instance->modem_type->boot_rom_patch && bp) {
>  		usb_info(usbatm, "loading boot ROM patch\n");
>  		ret = cxacru_fw(usb_dev, FW_WRITE_MEM, 0x2, 0x0, BR_ADDR, bp->data, bp->size);
>  		if (ret) {

That's what a follow-on patch would generate thinking they were actually
fixing a bug, but again, that's a pointless check!

> > So let's leave this for now because:
> > 
> > > This type of warning
> > > is off for GCC because of how unreliable it was when it is done in the
> > > middle end with optimizations. Furthermore, it is my understanding based
> > > on [1] that just the passing of an uninitialized variable in this manner
> > > is UB.
> > > 
> > > [1]: https://lore.kernel.org/20220614214039.GA25951@gate.crashing.org/
> > 
> > As gcc can't handle this either, it seems that clang also can't handle
> > it.  So turning this on for the kernel surely is going to trip it up in
> > other places than just this one driver.
> 
> I turned this warning on in 5.3 in commit 3a61925e91ba ("kbuild: Enable
> -Wuninitialized"), so it is already enabled and it has found many, many
> legitimate instances in doing so, just go run 'git log
> --grep=Wuninitialized' or 'git log --grep=Wsometimes-uninitialized' in
> the kernel sources. While there have been other places in the kernel
> where this warning has been falsely triggered such as here, I have
> rarely received pushback from maintainers on fixes to silence them
> because the majority of them are legitimate (and the false positive
> fixes usually result in more robust code). For example, the
> strengthening of the warning in clang-21 resulted in what I would
> consider legitimate fixes:
> 
> https://lore.kernel.org/20250715-mt7996-fix-uninit-const-pointer-v1-1-b5d8d11d7b78@kernel.org/
> https://lore.kernel.org/20250715-net-phonet-fix-uninit-const-pointer-v1-1-8efd1bd188b3@kernel.org/
> https://lore.kernel.org/20250715-drm-msm-fix-const-uninit-warning-v1-1-d6a366fd9a32@kernel.org/
> https://lore.kernel.org/20250715-riscv-uaccess-fix-self-init-val-v1-1-82b8e911f120@kernel.org/
> https://lore.kernel.org/20250715-trace_probe-fix-const-uninit-warning-v1-1-98960f91dd04@kernel.org/
> https://lore.kernel.org/20250715-sdca_interrupts-fix-const-uninit-warning-v1-1-cc031c913499@kernel.org/
> 
> > If you _really_ want to fix this, refactor the code to be more sane and
> > obvious from a C parsing standpoint, but really, it isn't that complex
> > for a human to read and understand, and I see why it was written this
> > way.
> 
> Yes, I agree that it is not complex or hard to understand, so I would
> rather not refactor it, but I do need this fixed so that allmodconfig
> builds (which enable -Werror by default) with clang-21 do not break.
> Won't Android eventually hit this when they get a newer compiler?

I have no idea what Android uses for their compiler.  Usually when they
run into issues like this, for their 'allmodconfig' builds, they just
apply a "CONFIG_BROKEN" patch to disable the old/unneeded driver
entirely.

> > As for the UB argument, bah, I don't care, sane compilers will do the
> > right thing, i.e. pass in the uninitialized value, or if we turned on
> > the 0-fill stack option, will be NULL anyway, otherwise why do we have
> > that option if not to "solve" the UB issue?).
> 
> As far as I understand it, clang adds "noundef" to function parameters
> when lowering to LLVM IR, which codifies that passing an uninitialized
> value is UB. I suspect that cxacru_upload_firmware() gets inlined so
> that ends up not mattering in this case but it could in others.
> 
> While '-ftrivial-auto-var-init=zero' does "solve" the UB issue, I see it
> more of a mitigation against missed initializations, not as a
> replacement for ensuring variables are consistently initialized, as zero
> might not be the expected initialization. Since that is the default for
> the kernel when compilers support it, why not just take this patch with
> that fixup above to make it consistent? I would be happy to send a v2 if
> you would be okay with it.

I'm really loath to take it, sorry.  I'd prefer that if the compiler
can't figure it out, we should rewrite it to make it more "obvious" as
to what is going on here so that both people, and the compiler, can
understand it easier.

Just setting the variable to NULL does neither of those things, except
to shut up a false-positive, not making it more obvious to the compiler
as to what really is going on.

thanks,

greg k-h

Re: [PATCH] usb: atm: cxacru: Zero initialize bp in cxacru_heavy_init()

[Nathan Chancellor]

On Wed, Jul 16, 2025 at 06:08:47PM +0200, Greg Kroah-Hartman wrote:
> I have no idea what Android uses for their compiler.  Usually when they
> run into issues like this, for their 'allmodconfig' builds, they just
> apply a "CONFIG_BROKEN" patch to disable the old/unneeded driver
> entirely.

Well I can say for certain that they do use clang but if that's their
preferred solution to situations such as this, so be it.

> I'm really loath to take it, sorry.  I'd prefer that if the compiler
> can't figure it out, we should rewrite it to make it more "obvious" as
> to what is going on here so that both people, and the compiler, can
> understand it easier.
> 
> Just setting the variable to NULL does neither of those things, except
> to shut up a false-positive, not making it more obvious to the compiler
> as to what really is going on.

Alright. We could just manually inline cxacru_upload_firmware() into
cxacru_heavy_init() since that is the only place where it is called so
that clang can see via the control flow graph that bp is only
initialized and used under the same condition. This resolves the warning
for me as well.

diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
index a12ab90b3db7..68a8e9de8b4f 100644
--- a/drivers/usb/atm/cxacru.c
+++ b/drivers/usb/atm/cxacru.c
@@ -980,25 +980,60 @@ static int cxacru_fw(struct usb_device *usb_dev, enum cxacru_fw_request fw,
 	return ret;
 }
 
-static void cxacru_upload_firmware(struct cxacru_data *instance,
-				   const struct firmware *fw,
-				   const struct firmware *bp)
+
+static int cxacru_find_firmware(struct cxacru_data *instance,
+				char *phase, const struct firmware **fw_p)
 {
-	int ret;
+	struct usbatm_data *usbatm = instance->usbatm;
+	struct device *dev = &usbatm->usb_intf->dev;
+	char buf[16];
+
+	sprintf(buf, "cxacru-%s.bin", phase);
+	usb_dbg(usbatm, "cxacru_find_firmware: looking for %s\n", buf);
+
+	if (request_firmware(fw_p, buf, dev)) {
+		usb_dbg(usbatm, "no stage %s firmware found\n", phase);
+		return -ENOENT;
+	}
+
+	usb_info(usbatm, "found firmware %s\n", buf);
+
+	return 0;
+}
+
+static int cxacru_heavy_init(struct usbatm_data *usbatm_instance,
+			     struct usb_interface *usb_intf)
+{
+	const struct firmware *fw, *bp;
+	struct cxacru_data *instance = usbatm_instance->driver_data;
 	struct usbatm_data *usbatm = instance->usbatm;
 	struct usb_device *usb_dev = usbatm->usb_dev;
 	__le16 signature[] = { usb_dev->descriptor.idVendor,
 			       usb_dev->descriptor.idProduct };
 	__le32 val;
+	int ret;
 
-	usb_dbg(usbatm, "%s\n", __func__);
+	ret = cxacru_find_firmware(instance, "fw", &fw);
+	if (ret) {
+		usb_warn(usbatm_instance, "firmware (cxacru-fw.bin) unavailable (system misconfigured?)\n");
+		return ret;
+	}
+
+	if (instance->modem_type->boot_rom_patch) {
+		ret = cxacru_find_firmware(instance, "bp", &bp);
+		if (ret) {
+			usb_warn(usbatm_instance, "boot ROM patch (cxacru-bp.bin) unavailable (system misconfigured?)\n");
+			release_firmware(fw);
+			return ret;
+		}
+	}
 
 	/* FirmwarePllFClkValue */
 	val = cpu_to_le32(instance->modem_type->pll_f_clk);
 	ret = cxacru_fw(usb_dev, FW_WRITE_MEM, 0x2, 0x0, PLLFCLK_ADDR, (u8 *) &val, 4);
 	if (ret) {
 		usb_err(usbatm, "FirmwarePllFClkValue failed: %d\n", ret);
-		return;
+		goto done;
 	}
 
 	/* FirmwarePllBClkValue */
@@ -1006,7 +1041,7 @@ static void cxacru_upload_firmware(struct cxacru_data *instance,
 	ret = cxacru_fw(usb_dev, FW_WRITE_MEM, 0x2, 0x0, PLLBCLK_ADDR, (u8 *) &val, 4);
 	if (ret) {
 		usb_err(usbatm, "FirmwarePllBClkValue failed: %d\n", ret);
-		return;
+		goto done;
 	}
 
 	/* Enable SDRAM */
@@ -1014,7 +1049,7 @@ static void cxacru_upload_firmware(struct cxacru_data *instance,
 	ret = cxacru_fw(usb_dev, FW_WRITE_MEM, 0x2, 0x0, SDRAMEN_ADDR, (u8 *) &val, 4);
 	if (ret) {
 		usb_err(usbatm, "Enable SDRAM failed: %d\n", ret);
-		return;
+		goto done;
 	}
 
 	/* Firmware */
@@ -1022,7 +1057,7 @@ static void cxacru_upload_firmware(struct cxacru_data *instance,
 	ret = cxacru_fw(usb_dev, FW_WRITE_MEM, 0x2, 0x0, FW_ADDR, fw->data, fw->size);
 	if (ret) {
 		usb_err(usbatm, "Firmware upload failed: %d\n", ret);
-		return;
+		goto done;
 	}
 
 	/* Boot ROM patch */
@@ -1031,7 +1066,7 @@ static void cxacru_upload_firmware(struct cxacru_data *instance,
 		ret = cxacru_fw(usb_dev, FW_WRITE_MEM, 0x2, 0x0, BR_ADDR, bp->data, bp->size);
 		if (ret) {
 			usb_err(usbatm, "Boot ROM patching failed: %d\n", ret);
-			return;
+			goto done;
 		}
 	}
 
@@ -1039,7 +1074,7 @@ static void cxacru_upload_firmware(struct cxacru_data *instance,
 	ret = cxacru_fw(usb_dev, FW_WRITE_MEM, 0x2, 0x0, SIG_ADDR, (u8 *) signature, 4);
 	if (ret) {
 		usb_err(usbatm, "Signature storing failed: %d\n", ret);
-		return;
+		goto done;
 	}
 
 	usb_info(usbatm, "starting device\n");
@@ -1051,7 +1086,7 @@ static void cxacru_upload_firmware(struct cxacru_data *instance,
 	}
 	if (ret) {
 		usb_err(usbatm, "Passing control to firmware failed: %d\n", ret);
-		return;
+		goto done;
 	}
 
 	/* Delay to allow firmware to start up. */
@@ -1065,53 +1100,10 @@ static void cxacru_upload_firmware(struct cxacru_data *instance,
 	ret = cxacru_cm(instance, CM_REQUEST_CARD_GET_STATUS, NULL, 0, NULL, 0);
 	if (ret < 0) {
 		usb_err(usbatm, "modem failed to initialize: %d\n", ret);
-		return;
-	}
-}
-
-static int cxacru_find_firmware(struct cxacru_data *instance,
-				char *phase, const struct firmware **fw_p)
-{
-	struct usbatm_data *usbatm = instance->usbatm;
-	struct device *dev = &usbatm->usb_intf->dev;
-	char buf[16];
-
-	sprintf(buf, "cxacru-%s.bin", phase);
-	usb_dbg(usbatm, "cxacru_find_firmware: looking for %s\n", buf);
-
-	if (request_firmware(fw_p, buf, dev)) {
-		usb_dbg(usbatm, "no stage %s firmware found\n", phase);
-		return -ENOENT;
-	}
-
-	usb_info(usbatm, "found firmware %s\n", buf);
-
-	return 0;
-}
-
-static int cxacru_heavy_init(struct usbatm_data *usbatm_instance,
-			     struct usb_interface *usb_intf)
-{
-	const struct firmware *fw, *bp;
-	struct cxacru_data *instance = usbatm_instance->driver_data;
-	int ret = cxacru_find_firmware(instance, "fw", &fw);
-
-	if (ret) {
-		usb_warn(usbatm_instance, "firmware (cxacru-fw.bin) unavailable (system misconfigured?)\n");
-		return ret;
+		goto done;
 	}
 
-	if (instance->modem_type->boot_rom_patch) {
-		ret = cxacru_find_firmware(instance, "bp", &bp);
-		if (ret) {
-			usb_warn(usbatm_instance, "boot ROM patch (cxacru-bp.bin) unavailable (system misconfigured?)\n");
-			release_firmware(fw);
-			return ret;
-		}
-	}
-
-	cxacru_upload_firmware(instance, fw, bp);
-
+done:
 	if (instance->modem_type->boot_rom_patch)
 		release_firmware(bp);
 	release_firmware(fw);