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 <[email protected]>
---
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
On Mon, May 16, 2022 at 02:22:50PM -0700, Nick Desaulniers wrote:
> On Mon, May 16, 2022 at 10:06 AM Tom Rix <[email protected]> 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 <[email protected]>
> > > 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 <[email protected]>
> > >
> > > 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/[email protected]/
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
On Mon, May 16, 2022 at 10:06 AM Tom Rix <[email protected]> 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 <[email protected]>
> > 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 <[email protected]>
> >
> > 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/[email protected]/
> >
> >> ---
> >> 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
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 <[email protected]>
> 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 <[email protected]>
>
> 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/[email protected]/
>
>> ---
>> 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
>
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 <[email protected]>
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 <[email protected]>
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/[email protected]/
> ---
> 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
> -----Original Message-----
> From: Kai-Heng Feng <[email protected]>
> Sent: Tuesday, May 17, 2022 9:53 AM
> To: Tom Rix <[email protected]>
> Cc: Nathan Chancellor <[email protected]>; [email protected];
> [email protected]; [email protected]; Ricky WU
> <[email protected]>; [email protected]; [email protected]
> Subject: Re: [PATCH] misc: rtsx: Set setting_reg2 before use.
>
> On Tue, May 17, 2022 at 1:06 AM Tom Rix <[email protected]> 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 <[email protected]>
> > > 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 <[email protected]>
> > >
> > > 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/[email protected]/
> > >
> > >> ---
> > >> 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.
On Tue, May 17, 2022 at 1:06 AM Tom Rix <[email protected]> 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 <[email protected]>
> > 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 <[email protected]>
> >
> > 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/[email protected]/
> >
> >> ---
> >> 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
> >
>
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 <[email protected]>
>>> Sent: Tuesday, May 17, 2022 9:53 AM
>>> To: Tom Rix <[email protected]>
>>> Cc: Nathan Chancellor <[email protected]>; [email protected];
>>> [email protected]; [email protected]; Ricky WU
>>> <[email protected]>; [email protected]; [email protected]
>>> Subject: Re: [PATCH] misc: rtsx: Set setting_reg2 before use.
>>>
>>> On Tue, May 17, 2022 at 1:06 AM Tom Rix <[email protected]> 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 <[email protected]>
>>>>> 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 <[email protected]>
>>>>>
>>>>> 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/[email protected]/
>>>>>
>>>>>> ---
>>>>>> 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
>
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 <[email protected]>
> > > > Sent: Tuesday, May 17, 2022 9:53 AM
> > > > To: Tom Rix <[email protected]>
> > > > Cc: Nathan Chancellor <[email protected]>; [email protected];
> > > > [email protected]; [email protected]; Ricky WU
> > > > <[email protected]>; [email protected]; [email protected]
> > > > Subject: Re: [PATCH] misc: rtsx: Set setting_reg2 before use.
> > > >
> > > > On Tue, May 17, 2022 at 1:06 AM Tom Rix <[email protected]> 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 <[email protected]>
> > > > > > 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 <[email protected]>
> > > > > >
> > > > > > 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/[email protected]/
> > > > > >
> > > > > > > ---
> > > > > > > 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/[email protected]/
Cheers,
Nathan
On Tue, May 17, 2022 at 08:10:17AM +0000, Ricky WU wrote:
> > -----Original Message-----
> > From: Kai-Heng Feng <[email protected]>
> > Sent: Tuesday, May 17, 2022 9:53 AM
> > To: Tom Rix <[email protected]>
> > Cc: Nathan Chancellor <[email protected]>; [email protected];
> > [email protected]; [email protected]; Ricky WU
> > <[email protected]>; [email protected]; [email protected]
> > Subject: Re: [PATCH] misc: rtsx: Set setting_reg2 before use.
> >
> > On Tue, May 17, 2022 at 1:06 AM Tom Rix <[email protected]> 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 <[email protected]>
> > > > 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 <[email protected]>
> > > >
> > > > 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/[email protected]/
> > > >
> > > >> ---
> > > >> 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