[PATCH] misc: rtsx: Set setting_reg2 before use.

[PATCH] misc: rtsx: Set setting_reg2 before use.

[Tom Rix]

The clang build fails with
rts5261.c:406:13: error: variable 'setting_reg2' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
        } else if (efuse_valid == 0) {
                   ^~~~~~~~~~~~~~~~

setting_reg2 is set in this block
  if (efuse_valid == 2 || efuse_valid == 3) {
..
  } else if (efuse_valid == 0) {
    // default
..
  }
But efuse_valid can also have a value of 1.
Change the 'else if' to 'else' to make the second block the default.

Fixes: b1c5f3085149 ("misc: rtsx: add rts5261 efuse function")
Signed-off-by: Tom Rix <trix@redhat.com>
---
 drivers/misc/cardreader/rts5261.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/cardreader/rts5261.c b/drivers/misc/cardreader/rts5261.c
index 749cc5a46d13..f22634b14dc8 100644
--- a/drivers/misc/cardreader/rts5261.c
+++ b/drivers/misc/cardreader/rts5261.c
@@ -403,7 +403,7 @@ static void rts5261_init_from_hw(struct rtsx_pcr *pcr)
 			setting_reg1 = PCR_SETTING_REG4;
 			setting_reg2 = PCR_SETTING_REG5;
 		}
-	} else if (efuse_valid == 0) {
+	} else {
 		// default
 		setting_reg1 = PCR_SETTING_REG1;
 		setting_reg2 = PCR_SETTING_REG2;
-- 
2.27.0

Re: [PATCH] misc: rtsx: Set setting_reg2 before use.

[Nathan Chancellor]

On Mon, May 16, 2022 at 09:00:47AM -0400, Tom Rix wrote:
> The clang build fails with
> rts5261.c:406:13: error: variable 'setting_reg2' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
>         } else if (efuse_valid == 0) {
>                    ^~~~~~~~~~~~~~~~
> 
> setting_reg2 is set in this block
>   if (efuse_valid == 2 || efuse_valid == 3) {
> ..
>   } else if (efuse_valid == 0) {
>     // default
> ..
>   }
> But efuse_valid can also have a value of 1.
> Change the 'else if' to 'else' to make the second block the default.
> 
> Fixes: b1c5f3085149 ("misc: rtsx: add rts5261 efuse function")
> Signed-off-by: Tom Rix <trix@redhat.com>

I am not sure if this fix is correct from a functional standpoint (i.e.
is treating efuse_valid == 1 the same as efuse_valid == 0 correct?) but
it is better than not handling this value altogether. For what it's
worth:

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

As a side note, it is unfortunate that this change made it into -next
when there was an outstanding report about this warning:

https://lore.kernel.org/202205100220.WyAyhKap-lkp@intel.com/

> ---
>  drivers/misc/cardreader/rts5261.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/cardreader/rts5261.c b/drivers/misc/cardreader/rts5261.c
> index 749cc5a46d13..f22634b14dc8 100644
> --- a/drivers/misc/cardreader/rts5261.c
> +++ b/drivers/misc/cardreader/rts5261.c
> @@ -403,7 +403,7 @@ static void rts5261_init_from_hw(struct rtsx_pcr *pcr)
>  			setting_reg1 = PCR_SETTING_REG4;
>  			setting_reg2 = PCR_SETTING_REG5;
>  		}
> -	} else if (efuse_valid == 0) {
> +	} else {
>  		// default
>  		setting_reg1 = PCR_SETTING_REG1;
>  		setting_reg2 = PCR_SETTING_REG2;
> -- 
> 2.27.0
> 

Cheers,
Nathan

Re: [PATCH] misc: rtsx: Set setting_reg2 before use.

[Tom Rix]

On 5/16/22 8:56 AM, Nathan Chancellor wrote:
> On Mon, May 16, 2022 at 09:00:47AM -0400, Tom Rix wrote:
>> The clang build fails with
>> rts5261.c:406:13: error: variable 'setting_reg2' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
>>          } else if (efuse_valid == 0) {
>>                     ^~~~~~~~~~~~~~~~
>>
>> setting_reg2 is set in this block
>>    if (efuse_valid == 2 || efuse_valid == 3) {
>> ..
>>    } else if (efuse_valid == 0) {
>>      // default
>> ..
>>    }
>> But efuse_valid can also have a value of 1.
>> Change the 'else if' to 'else' to make the second block the default.
>>
>> Fixes: b1c5f3085149 ("misc: rtsx: add rts5261 efuse function")
>> Signed-off-by: Tom Rix <trix@redhat.com>
> I am not sure if this fix is correct from a functional standpoint (i.e.
> is treating efuse_valid == 1 the same as efuse_valid == 0 correct?) but
> it is better than not handling this value altogether. For what it's
> worth:

I looked at how the code used to work, this seemed better than 
initializing to NULL.

>
> Reviewed-by: Nathan Chancellor <nathan@kernel.org>
>
> As a side note, it is unfortunate that this change made it into -next
> when there was an outstanding report about this warning:

 From the clang side, this is a build break and my static analysis infra 
goes down.

These build breaks seem to happening every week, is there a precommit 
clang gating test that could be done for -next ?

Tom

>
> https://lore.kernel.org/202205100220.WyAyhKap-lkp@intel.com/
>
>> ---
>>   drivers/misc/cardreader/rts5261.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/misc/cardreader/rts5261.c b/drivers/misc/cardreader/rts5261.c
>> index 749cc5a46d13..f22634b14dc8 100644
>> --- a/drivers/misc/cardreader/rts5261.c
>> +++ b/drivers/misc/cardreader/rts5261.c
>> @@ -403,7 +403,7 @@ static void rts5261_init_from_hw(struct rtsx_pcr *pcr)
>>   			setting_reg1 = PCR_SETTING_REG4;
>>   			setting_reg2 = PCR_SETTING_REG5;
>>   		}
>> -	} else if (efuse_valid == 0) {
>> +	} else {
>>   		// default
>>   		setting_reg1 = PCR_SETTING_REG1;
>>   		setting_reg2 = PCR_SETTING_REG2;
>> -- 
>> 2.27.0
>>
> Cheers,
> Nathan
>

Re: [PATCH] misc: rtsx: Set setting_reg2 before use.

[Nick Desaulniers]

On Mon, May 16, 2022 at 10:06 AM Tom Rix <trix@redhat.com> wrote:
>
>
> On 5/16/22 8:56 AM, Nathan Chancellor wrote:
> > On Mon, May 16, 2022 at 09:00:47AM -0400, Tom Rix wrote:
> >> The clang build fails with
> >> rts5261.c:406:13: error: variable 'setting_reg2' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
> >>          } else if (efuse_valid == 0) {
> >>                     ^~~~~~~~~~~~~~~~
> >>
> >> setting_reg2 is set in this block
> >>    if (efuse_valid == 2 || efuse_valid == 3) {
> >> ..
> >>    } else if (efuse_valid == 0) {
> >>      // default
> >> ..
> >>    }
> >> But efuse_valid can also have a value of 1.
> >> Change the 'else if' to 'else' to make the second block the default.
> >>
> >> Fixes: b1c5f3085149 ("misc: rtsx: add rts5261 efuse function")
> >> Signed-off-by: Tom Rix <trix@redhat.com>
> > I am not sure if this fix is correct from a functional standpoint (i.e.
> > is treating efuse_valid == 1 the same as efuse_valid == 0 correct?) but
> > it is better than not handling this value altogether. For what it's
> > worth:
>
> I looked at how the code used to work, this seemed better than
> initializing to NULL.
>
> >
> > Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> >
> > As a side note, it is unfortunate that this change made it into -next
> > when there was an outstanding report about this warning:
>
>  From the clang side, this is a build break and my static analysis infra
> goes down.
>
> These build breaks seem to happening every week, is there a precommit
> clang gating test that could be done for -next ?

Probably worth asking Stephen, though I don't think there's _any_
gating (i.e. presubmit testing) for -next, since that'd increase the
build capacity needed. -next is tested post-merge (i.e. post submit
testing) IIUC.

>
> Tom
>
> >
> > https://lore.kernel.org/202205100220.WyAyhKap-lkp@intel.com/
> >
> >> ---
> >>   drivers/misc/cardreader/rts5261.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/misc/cardreader/rts5261.c b/drivers/misc/cardreader/rts5261.c
> >> index 749cc5a46d13..f22634b14dc8 100644
> >> --- a/drivers/misc/cardreader/rts5261.c
> >> +++ b/drivers/misc/cardreader/rts5261.c
> >> @@ -403,7 +403,7 @@ static void rts5261_init_from_hw(struct rtsx_pcr *pcr)
> >>                      setting_reg1 = PCR_SETTING_REG4;
> >>                      setting_reg2 = PCR_SETTING_REG5;
> >>              }
> >> -    } else if (efuse_valid == 0) {
> >> +    } else {
> >>              // default
> >>              setting_reg1 = PCR_SETTING_REG1;
> >>              setting_reg2 = PCR_SETTING_REG2;
> >> --
> >> 2.27.0
> >>
> > Cheers,
> > Nathan
> >
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] misc: rtsx: Set setting_reg2 before use.

[Nathan Chancellor]

On Mon, May 16, 2022 at 02:22:50PM -0700, Nick Desaulniers wrote:
> On Mon, May 16, 2022 at 10:06 AM Tom Rix <trix@redhat.com> wrote:
> >
> >
> > On 5/16/22 8:56 AM, Nathan Chancellor wrote:
> > > On Mon, May 16, 2022 at 09:00:47AM -0400, Tom Rix wrote:
> > >> The clang build fails with
> > >> rts5261.c:406:13: error: variable 'setting_reg2' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
> > >>          } else if (efuse_valid == 0) {
> > >>                     ^~~~~~~~~~~~~~~~
> > >>
> > >> setting_reg2 is set in this block
> > >>    if (efuse_valid == 2 || efuse_valid == 3) {
> > >> ..
> > >>    } else if (efuse_valid == 0) {
> > >>      // default
> > >> ..
> > >>    }
> > >> But efuse_valid can also have a value of 1.
> > >> Change the 'else if' to 'else' to make the second block the default.
> > >>
> > >> Fixes: b1c5f3085149 ("misc: rtsx: add rts5261 efuse function")
> > >> Signed-off-by: Tom Rix <trix@redhat.com>
> > > I am not sure if this fix is correct from a functional standpoint (i.e.
> > > is treating efuse_valid == 1 the same as efuse_valid == 0 correct?) but
> > > it is better than not handling this value altogether. For what it's
> > > worth:
> >
> > I looked at how the code used to work, this seemed better than
> > initializing to NULL.
> >
> > >
> > > Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> > >
> > > As a side note, it is unfortunate that this change made it into -next
> > > when there was an outstanding report about this warning:
> >
> >  From the clang side, this is a build break and my static analysis infra
> > goes down.
> >
> > These build breaks seem to happening every week, is there a precommit
> > clang gating test that could be done for -next ?
> 
> Probably worth asking Stephen, though I don't think there's _any_
> gating (i.e. presubmit testing) for -next, since that'd increase the
> build capacity needed. -next is tested post-merge (i.e. post submit
> testing) IIUC.

Stephen does do build testing during the merges ("Between each merge,
the tree was built with a ppc64_defconfig for powerpc, an allmodconfig
for x86_64, a multi_v7_defconfig for arm and a native build of
tools/perf"), in addition to testing after the final tree is done.

https://lore.kernel.org/20220516205718.2c5a52f9@canb.auug.org.au/

Increasing the "during merge" testing is not a reasonable request (IMO)
due to the additional time it would add to the entire -next process.  I
am not sure that having Stephen do builds with clang post merge is any
different from all the existing build coverage that we already have (the
tree is already done at that point), other than maybe people listen to a
human complaining more than a robot... :)

Cheers,
Nathan

Re: [PATCH] misc: rtsx: Set setting_reg2 before use.

[Kai-Heng Feng]

On Tue, May 17, 2022 at 1:06 AM Tom Rix <trix@redhat.com> wrote:
>
>
> On 5/16/22 8:56 AM, Nathan Chancellor wrote:
> > On Mon, May 16, 2022 at 09:00:47AM -0400, Tom Rix wrote:
> >> The clang build fails with
> >> rts5261.c:406:13: error: variable 'setting_reg2' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
> >>          } else if (efuse_valid == 0) {
> >>                     ^~~~~~~~~~~~~~~~
> >>
> >> setting_reg2 is set in this block
> >>    if (efuse_valid == 2 || efuse_valid == 3) {
> >> ..
> >>    } else if (efuse_valid == 0) {
> >>      // default
> >> ..
> >>    }
> >> But efuse_valid can also have a value of 1.
> >> Change the 'else if' to 'else' to make the second block the default.
> >>
> >> Fixes: b1c5f3085149 ("misc: rtsx: add rts5261 efuse function")
> >> Signed-off-by: Tom Rix <trix@redhat.com>
> > I am not sure if this fix is correct from a functional standpoint (i.e.
> > is treating efuse_valid == 1 the same as efuse_valid == 0 correct?) but
> > it is better than not handling this value altogether. For what it's
> > worth:
>
> I looked at how the code used to work, this seemed better than
> initializing to NULL.

Or maybe use a single if block?

u16 setting_reg1 =PCR_SETTING_REG1 , setting_reg2 = PCR_SETTING_REG2;
...
if ((efuse_valid == 2 || efuse_valid == 3) && (valid != 3) {
    setting_reg1 = PCR_SETTING_REG4;
    setting_reg2 = PCR_SETTING_REG5;
}

Kai-Heng

>
> >
> > Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> >
> > As a side note, it is unfortunate that this change made it into -next
> > when there was an outstanding report about this warning:
>
>  From the clang side, this is a build break and my static analysis infra
> goes down.
>
> These build breaks seem to happening every week, is there a precommit
> clang gating test that could be done for -next ?
>
> Tom
>
> >
> > https://lore.kernel.org/202205100220.WyAyhKap-lkp@intel.com/
> >
> >> ---
> >>   drivers/misc/cardreader/rts5261.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/misc/cardreader/rts5261.c b/drivers/misc/cardreader/rts5261.c
> >> index 749cc5a46d13..f22634b14dc8 100644
> >> --- a/drivers/misc/cardreader/rts5261.c
> >> +++ b/drivers/misc/cardreader/rts5261.c
> >> @@ -403,7 +403,7 @@ static void rts5261_init_from_hw(struct rtsx_pcr *pcr)
> >>                      setting_reg1 = PCR_SETTING_REG4;
> >>                      setting_reg2 = PCR_SETTING_REG5;
> >>              }
> >> -    } else if (efuse_valid == 0) {
> >> +    } else {
> >>              // default
> >>              setting_reg1 = PCR_SETTING_REG1;
> >>              setting_reg2 = PCR_SETTING_REG2;
> >> --
> >> 2.27.0
> >>
> > Cheers,
> > Nathan
> >
>

RE: [PATCH] misc: rtsx: Set setting_reg2 before use.

[Ricky WU]

> -----Original Message-----
> From: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Sent: Tuesday, May 17, 2022 9:53 AM
> To: Tom Rix <trix@redhat.com>
> Cc: Nathan Chancellor <nathan@kernel.org>; arnd@arndb.de;
> gregkh@linuxfoundation.org; ndesaulniers@google.com; Ricky WU
> <ricky_wu@realtek.com>; linux-kernel@vger.kernel.org; llvm@lists.linux.dev
> Subject: Re: [PATCH] misc: rtsx: Set setting_reg2 before use.
> 
> On Tue, May 17, 2022 at 1:06 AM Tom Rix <trix@redhat.com> wrote:
> >
> >
> > On 5/16/22 8:56 AM, Nathan Chancellor wrote:
> > > On Mon, May 16, 2022 at 09:00:47AM -0400, Tom Rix wrote:
> > >> The clang build fails with
> > >> rts5261.c:406:13: error: variable 'setting_reg2' is used uninitialized
> whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
> > >>          } else if (efuse_valid == 0) {
> > >>                     ^~~~~~~~~~~~~~~~
> > >>
> > >> setting_reg2 is set in this block
> > >>    if (efuse_valid == 2 || efuse_valid == 3) { ..
> > >>    } else if (efuse_valid == 0) {
> > >>      // default
> > >> ..
> > >>    }
> > >> But efuse_valid can also have a value of 1.
> > >> Change the 'else if' to 'else' to make the second block the default.
> > >>
> > >> Fixes: b1c5f3085149 ("misc: rtsx: add rts5261 efuse function")
> > >> Signed-off-by: Tom Rix <trix@redhat.com>
> > > I am not sure if this fix is correct from a functional standpoint (i.e.
> > > is treating efuse_valid == 1 the same as efuse_valid == 0 correct?)
> > > but it is better than not handling this value altogether. For what
> > > it's
> > > worth:
> >
> > I looked at how the code used to work, this seemed better than
> > initializing to NULL.
> 
> Or maybe use a single if block?
> 
> u16 setting_reg1 =PCR_SETTING_REG1 , setting_reg2 =
> PCR_SETTING_REG2; ...
> if ((efuse_valid == 2 || efuse_valid == 3) && (valid != 3) {
>     setting_reg1 = PCR_SETTING_REG4;
>     setting_reg2 = PCR_SETTING_REG5;
> }
> 
> Kai-Heng
> 
> >
> > >
> > > Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> > >
> > > As a side note, it is unfortunate that this change made it into
> > > -next when there was an outstanding report about this warning:
> >
> >  From the clang side, this is a build break and my static analysis
> > infra goes down.
> >
> > These build breaks seem to happening every week, is there a precommit
> > clang gating test that could be done for -next ?
> >
> > Tom
> >
> > >
> > > https://lore.kernel.org/202205100220.WyAyhKap-lkp@intel.com/
> > >
> > >> ---
> > >>   drivers/misc/cardreader/rts5261.c | 2 +-
> > >>   1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/misc/cardreader/rts5261.c
> > >> b/drivers/misc/cardreader/rts5261.c
> > >> index 749cc5a46d13..f22634b14dc8 100644
> > >> --- a/drivers/misc/cardreader/rts5261.c
> > >> +++ b/drivers/misc/cardreader/rts5261.c
> > >> @@ -403,7 +403,7 @@ static void rts5261_init_from_hw(struct rtsx_pcr
> *pcr)
> > >>                      setting_reg1 = PCR_SETTING_REG4;
> > >>                      setting_reg2 = PCR_SETTING_REG5;
> > >>              }
> > >> -    } else if (efuse_valid == 0) {
> > >> +    } else {
> > >>              // default
> > >>              setting_reg1 = PCR_SETTING_REG1;
> > >>              setting_reg2 = PCR_SETTING_REG2;

Sorry for the trouble 
here can be changed to 
...
} else if (efuse_valid == 0) {
		// default
		setting_reg1 = PCR_SETTING_REG1;
		setting_reg2 = PCR_SETTING_REG2;
} else {
 return;
}
Because other values are invalid


> > >> --
> > >> 2.27.0
> > >>
> > > Cheers,
> > > Nathan
> > >
> >
> ------Please consider the environment before printing this e-mail.

Re: [PATCH] misc: rtsx: Set setting_reg2 before use.

[Nathan Chancellor]

On Tue, May 17, 2022 at 08:10:17AM +0000, Ricky WU wrote:
> > -----Original Message-----
> > From: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > Sent: Tuesday, May 17, 2022 9:53 AM
> > To: Tom Rix <trix@redhat.com>
> > Cc: Nathan Chancellor <nathan@kernel.org>; arnd@arndb.de;
> > gregkh@linuxfoundation.org; ndesaulniers@google.com; Ricky WU
> > <ricky_wu@realtek.com>; linux-kernel@vger.kernel.org; llvm@lists.linux.dev
> > Subject: Re: [PATCH] misc: rtsx: Set setting_reg2 before use.
> > 
> > On Tue, May 17, 2022 at 1:06 AM Tom Rix <trix@redhat.com> wrote:
> > >
> > >
> > > On 5/16/22 8:56 AM, Nathan Chancellor wrote:
> > > > On Mon, May 16, 2022 at 09:00:47AM -0400, Tom Rix wrote:
> > > >> The clang build fails with
> > > >> rts5261.c:406:13: error: variable 'setting_reg2' is used uninitialized
> > whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
> > > >>          } else if (efuse_valid == 0) {
> > > >>                     ^~~~~~~~~~~~~~~~
> > > >>
> > > >> setting_reg2 is set in this block
> > > >>    if (efuse_valid == 2 || efuse_valid == 3) { ..
> > > >>    } else if (efuse_valid == 0) {
> > > >>      // default
> > > >> ..
> > > >>    }
> > > >> But efuse_valid can also have a value of 1.
> > > >> Change the 'else if' to 'else' to make the second block the default.
> > > >>
> > > >> Fixes: b1c5f3085149 ("misc: rtsx: add rts5261 efuse function")
> > > >> Signed-off-by: Tom Rix <trix@redhat.com>
> > > > I am not sure if this fix is correct from a functional standpoint (i.e.
> > > > is treating efuse_valid == 1 the same as efuse_valid == 0 correct?)
> > > > but it is better than not handling this value altogether. For what
> > > > it's
> > > > worth:
> > >
> > > I looked at how the code used to work, this seemed better than
> > > initializing to NULL.
> > 
> > Or maybe use a single if block?
> > 
> > u16 setting_reg1 =PCR_SETTING_REG1 , setting_reg2 =
> > PCR_SETTING_REG2; ...
> > if ((efuse_valid == 2 || efuse_valid == 3) && (valid != 3) {
> >     setting_reg1 = PCR_SETTING_REG4;
> >     setting_reg2 = PCR_SETTING_REG5;
> > }
> > 
> > Kai-Heng
> > 
> > >
> > > >
> > > > Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> > > >
> > > > As a side note, it is unfortunate that this change made it into
> > > > -next when there was an outstanding report about this warning:
> > >
> > >  From the clang side, this is a build break and my static analysis
> > > infra goes down.
> > >
> > > These build breaks seem to happening every week, is there a precommit
> > > clang gating test that could be done for -next ?
> > >
> > > Tom
> > >
> > > >
> > > > https://lore.kernel.org/202205100220.WyAyhKap-lkp@intel.com/
> > > >
> > > >> ---
> > > >>   drivers/misc/cardreader/rts5261.c | 2 +-
> > > >>   1 file changed, 1 insertion(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/drivers/misc/cardreader/rts5261.c
> > > >> b/drivers/misc/cardreader/rts5261.c
> > > >> index 749cc5a46d13..f22634b14dc8 100644
> > > >> --- a/drivers/misc/cardreader/rts5261.c
> > > >> +++ b/drivers/misc/cardreader/rts5261.c
> > > >> @@ -403,7 +403,7 @@ static void rts5261_init_from_hw(struct rtsx_pcr
> > *pcr)
> > > >>                      setting_reg1 = PCR_SETTING_REG4;
> > > >>                      setting_reg2 = PCR_SETTING_REG5;
> > > >>              }
> > > >> -    } else if (efuse_valid == 0) {
> > > >> +    } else {
> > > >>              // default
> > > >>              setting_reg1 = PCR_SETTING_REG1;
> > > >>              setting_reg2 = PCR_SETTING_REG2;
> 
> Sorry for the trouble 
> here can be changed to 
> ...
> } else if (efuse_valid == 0) {
> 		// default
> 		setting_reg1 = PCR_SETTING_REG1;
> 		setting_reg2 = PCR_SETTING_REG2;
> } else {
>  return;
> }
> Because other values are invalid

Tom, were you going to send a v2 of this?

Cheers,
Nathan

Re: [PATCH] misc: rtsx: Set setting_reg2 before use.

[Tom Rix]

On 5/19/22 1:57 PM, Nathan Chancellor wrote:
> On Tue, May 17, 2022 at 08:10:17AM +0000, Ricky WU wrote:
>>> -----Original Message-----
>>> From: Kai-Heng Feng <kai.heng.feng@canonical.com>
>>> Sent: Tuesday, May 17, 2022 9:53 AM
>>> To: Tom Rix <trix@redhat.com>
>>> Cc: Nathan Chancellor <nathan@kernel.org>; arnd@arndb.de;
>>> gregkh@linuxfoundation.org; ndesaulniers@google.com; Ricky WU
>>> <ricky_wu@realtek.com>; linux-kernel@vger.kernel.org; llvm@lists.linux.dev
>>> Subject: Re: [PATCH] misc: rtsx: Set setting_reg2 before use.
>>>
>>> On Tue, May 17, 2022 at 1:06 AM Tom Rix <trix@redhat.com> wrote:
>>>>
>>>> On 5/16/22 8:56 AM, Nathan Chancellor wrote:
>>>>> On Mon, May 16, 2022 at 09:00:47AM -0400, Tom Rix wrote:
>>>>>> The clang build fails with
>>>>>> rts5261.c:406:13: error: variable 'setting_reg2' is used uninitialized
>>> whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
>>>>>>           } else if (efuse_valid == 0) {
>>>>>>                      ^~~~~~~~~~~~~~~~
>>>>>>
>>>>>> setting_reg2 is set in this block
>>>>>>     if (efuse_valid == 2 || efuse_valid == 3) { ..
>>>>>>     } else if (efuse_valid == 0) {
>>>>>>       // default
>>>>>> ..
>>>>>>     }
>>>>>> But efuse_valid can also have a value of 1.
>>>>>> Change the 'else if' to 'else' to make the second block the default.
>>>>>>
>>>>>> Fixes: b1c5f3085149 ("misc: rtsx: add rts5261 efuse function")
>>>>>> Signed-off-by: Tom Rix <trix@redhat.com>
>>>>> I am not sure if this fix is correct from a functional standpoint (i.e.
>>>>> is treating efuse_valid == 1 the same as efuse_valid == 0 correct?)
>>>>> but it is better than not handling this value altogether. For what
>>>>> it's
>>>>> worth:
>>>> I looked at how the code used to work, this seemed better than
>>>> initializing to NULL.
>>> Or maybe use a single if block?
>>>
>>> u16 setting_reg1 =PCR_SETTING_REG1 , setting_reg2 =
>>> PCR_SETTING_REG2; ...
>>> if ((efuse_valid == 2 || efuse_valid == 3) && (valid != 3) {
>>>      setting_reg1 = PCR_SETTING_REG4;
>>>      setting_reg2 = PCR_SETTING_REG5;
>>> }
>>>
>>> Kai-Heng
>>>
>>>>> Reviewed-by: Nathan Chancellor <nathan@kernel.org>
>>>>>
>>>>> As a side note, it is unfortunate that this change made it into
>>>>> -next when there was an outstanding report about this warning:
>>>>   From the clang side, this is a build break and my static analysis
>>>> infra goes down.
>>>>
>>>> These build breaks seem to happening every week, is there a precommit
>>>> clang gating test that could be done for -next ?
>>>>
>>>> Tom
>>>>
>>>>> https://lore.kernel.org/202205100220.WyAyhKap-lkp@intel.com/
>>>>>
>>>>>> ---
>>>>>>    drivers/misc/cardreader/rts5261.c | 2 +-
>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/misc/cardreader/rts5261.c
>>>>>> b/drivers/misc/cardreader/rts5261.c
>>>>>> index 749cc5a46d13..f22634b14dc8 100644
>>>>>> --- a/drivers/misc/cardreader/rts5261.c
>>>>>> +++ b/drivers/misc/cardreader/rts5261.c
>>>>>> @@ -403,7 +403,7 @@ static void rts5261_init_from_hw(struct rtsx_pcr
>>> *pcr)
>>>>>>                       setting_reg1 = PCR_SETTING_REG4;
>>>>>>                       setting_reg2 = PCR_SETTING_REG5;
>>>>>>               }
>>>>>> -    } else if (efuse_valid == 0) {
>>>>>> +    } else {
>>>>>>               // default
>>>>>>               setting_reg1 = PCR_SETTING_REG1;
>>>>>>               setting_reg2 = PCR_SETTING_REG2;
>> Sorry for the trouble
>> here can be changed to
>> ...
>> } else if (efuse_valid == 0) {
>> 		// default
>> 		setting_reg1 = PCR_SETTING_REG1;
>> 		setting_reg2 = PCR_SETTING_REG2;
>> } else {
>>   return;
>> }
>> Because other values are invalid
> Tom, were you going to send a v2 of this?

No.

Miquèl has the best fix.

https://lore.kernel.org/lkml/20220518170920.4db16990@xps-13/

Tom


>
> Cheers,
> Nathan
>

Re: [PATCH] misc: rtsx: Set setting_reg2 before use.

[Nathan Chancellor]

On Thu, May 19, 2022 at 02:18:59PM -0700, Tom Rix wrote:
> 
> On 5/19/22 1:57 PM, Nathan Chancellor wrote:
> > On Tue, May 17, 2022 at 08:10:17AM +0000, Ricky WU wrote:
> > > > -----Original Message-----
> > > > From: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > > > Sent: Tuesday, May 17, 2022 9:53 AM
> > > > To: Tom Rix <trix@redhat.com>
> > > > Cc: Nathan Chancellor <nathan@kernel.org>; arnd@arndb.de;
> > > > gregkh@linuxfoundation.org; ndesaulniers@google.com; Ricky WU
> > > > <ricky_wu@realtek.com>; linux-kernel@vger.kernel.org; llvm@lists.linux.dev
> > > > Subject: Re: [PATCH] misc: rtsx: Set setting_reg2 before use.
> > > > 
> > > > On Tue, May 17, 2022 at 1:06 AM Tom Rix <trix@redhat.com> wrote:
> > > > > 
> > > > > On 5/16/22 8:56 AM, Nathan Chancellor wrote:
> > > > > > On Mon, May 16, 2022 at 09:00:47AM -0400, Tom Rix wrote:
> > > > > > > The clang build fails with
> > > > > > > rts5261.c:406:13: error: variable 'setting_reg2' is used uninitialized
> > > > whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
> > > > > > >           } else if (efuse_valid == 0) {
> > > > > > >                      ^~~~~~~~~~~~~~~~
> > > > > > > 
> > > > > > > setting_reg2 is set in this block
> > > > > > >     if (efuse_valid == 2 || efuse_valid == 3) { ..
> > > > > > >     } else if (efuse_valid == 0) {
> > > > > > >       // default
> > > > > > > ..
> > > > > > >     }
> > > > > > > But efuse_valid can also have a value of 1.
> > > > > > > Change the 'else if' to 'else' to make the second block the default.
> > > > > > > 
> > > > > > > Fixes: b1c5f3085149 ("misc: rtsx: add rts5261 efuse function")
> > > > > > > Signed-off-by: Tom Rix <trix@redhat.com>
> > > > > > I am not sure if this fix is correct from a functional standpoint (i.e.
> > > > > > is treating efuse_valid == 1 the same as efuse_valid == 0 correct?)
> > > > > > but it is better than not handling this value altogether. For what
> > > > > > it's
> > > > > > worth:
> > > > > I looked at how the code used to work, this seemed better than
> > > > > initializing to NULL.
> > > > Or maybe use a single if block?
> > > > 
> > > > u16 setting_reg1 =PCR_SETTING_REG1 , setting_reg2 =
> > > > PCR_SETTING_REG2; ...
> > > > if ((efuse_valid == 2 || efuse_valid == 3) && (valid != 3) {
> > > >      setting_reg1 = PCR_SETTING_REG4;
> > > >      setting_reg2 = PCR_SETTING_REG5;
> > > > }
> > > > 
> > > > Kai-Heng
> > > > 
> > > > > > Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> > > > > > 
> > > > > > As a side note, it is unfortunate that this change made it into
> > > > > > -next when there was an outstanding report about this warning:
> > > > >   From the clang side, this is a build break and my static analysis
> > > > > infra goes down.
> > > > > 
> > > > > These build breaks seem to happening every week, is there a precommit
> > > > > clang gating test that could be done for -next ?
> > > > > 
> > > > > Tom
> > > > > 
> > > > > > https://lore.kernel.org/202205100220.WyAyhKap-lkp@intel.com/
> > > > > > 
> > > > > > > ---
> > > > > > >    drivers/misc/cardreader/rts5261.c | 2 +-
> > > > > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/misc/cardreader/rts5261.c
> > > > > > > b/drivers/misc/cardreader/rts5261.c
> > > > > > > index 749cc5a46d13..f22634b14dc8 100644
> > > > > > > --- a/drivers/misc/cardreader/rts5261.c
> > > > > > > +++ b/drivers/misc/cardreader/rts5261.c
> > > > > > > @@ -403,7 +403,7 @@ static void rts5261_init_from_hw(struct rtsx_pcr
> > > > *pcr)
> > > > > > >                       setting_reg1 = PCR_SETTING_REG4;
> > > > > > >                       setting_reg2 = PCR_SETTING_REG5;
> > > > > > >               }
> > > > > > > -    } else if (efuse_valid == 0) {
> > > > > > > +    } else {
> > > > > > >               // default
> > > > > > >               setting_reg1 = PCR_SETTING_REG1;
> > > > > > >               setting_reg2 = PCR_SETTING_REG2;
> > > Sorry for the trouble
> > > here can be changed to
> > > ...
> > > } else if (efuse_valid == 0) {
> > > 		// default
> > > 		setting_reg1 = PCR_SETTING_REG1;
> > > 		setting_reg2 = PCR_SETTING_REG2;
> > > } else {
> > >   return;
> > > }
> > > Because other values are invalid
> > Tom, were you going to send a v2 of this?
> 
> No.
> 
> Miquèl has the best fix.
> 
> https://lore.kernel.org/lkml/20220518170920.4db16990@xps-13/

Different warning/patch I think? There are too many of these flying
around as of late :( this one doesn't have a new version from what I can
tell.

I do see a fix for that one available:

https://lore.kernel.org/20220519132300.424095-1-miquel.raynal@bootlin.com/

Cheers,
Nathan