diff mbox series

EDAC/amd64: remove unneeded call to reserve_mc_sibling_devs()

Message ID 20230213191510.2237360-1-trix@redhat.com
State New
Headers
Series EDAC/amd64: remove unneeded call to reserve_mc_sibling_devs() |

Commit Message

Tom Rix Feb. 13, 2023, 7:15 p.m. UTC 
  cpp_check reports
drivers/edac/amd64_edac.c:3943:37: error: Uninitialized variable: pci_id1 [uninitvar]
 ret = reserve_mc_sibling_devs(pvt, pci_id1, pci_id2);
                                    ^
drivers/edac/amd64_edac.c:3943:46: error: Uninitialized variable: pci_id2 [uninitvar]
 ret = reserve_mc_sibling_devs(pvt, pci_id1, pci_id2);
                                             ^
The call to reserve_mc_sibling_devs() will not fail because
  if (pvt->umc)
    return 0;

reserve_mc_sibling_devs() is only called by hw_info_get() and pvt->umc is only set
in hw_info_get(), so with fam >= 0x17, the call to reserver_mc_siblings will
just return, so the call the call is not needed.  And when that call is moved
the check for umc is not needed.

Signed-off-by: Tom Rix <trix@redhat.com>
---
 drivers/edac/amd64_edac.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

Comments

Yazen Ghannam Feb. 13, 2023, 8:12 p.m. UTC | #1 
On Mon, Feb 13, 2023 at 11:15:10AM -0800, Tom Rix wrote:
> cpp_check reports
> drivers/edac/amd64_edac.c:3943:37: error: Uninitialized variable: pci_id1 [uninitvar]
>  ret = reserve_mc_sibling_devs(pvt, pci_id1, pci_id2);
>                                     ^
> drivers/edac/amd64_edac.c:3943:46: error: Uninitialized variable: pci_id2 [uninitvar]
>  ret = reserve_mc_sibling_devs(pvt, pci_id1, pci_id2);
>                                              ^
> The call to reserve_mc_sibling_devs() will not fail because
>   if (pvt->umc)
>     return 0;
> 
> reserve_mc_sibling_devs() is only called by hw_info_get() and pvt->umc is only set
> in hw_info_get(), so with fam >= 0x17, the call to reserver_mc_siblings will
> just return, so the call the call is not needed.  And when that call is moved
> the check for umc is not needed.
> 
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---

Link to similar patch from Nathan:
https://lore.kernel.org/linux-edac/20230213-amd64_edac-wsometimes-uninitialized-v1-1-5bde32b89e02@kernel.org/

Hi Tom and Nathan,

These errors are encountered when extra warnings are enabled, correct?

I think the following patch would resolve this issue. This is part of a set
that isn't fully applied.
https://lore.kernel.org/linux-edac/20230127170419.1824692-12-yazen.ghannam@amd.com/

Boris,
Do you think one of these patches should be applied or just hold off until the
entire original set is applied?

As for myself, I'll start including builds with extra warnings enabled for
each patch in my workflow. Currently I do a regular build for each patch and a
build with extra warnings for the entire set.

Thanks,
Yazen
Borislav Petkov Feb. 13, 2023, 8:23 p.m. UTC | #2 
On Mon, Feb 13, 2023 at 08:12:38PM +0000, Yazen Ghannam wrote:
> These errors are encountered when extra warnings are enabled, correct?

It says so in the warning which one it is: -Werror,-Wsometimes-uninitialized

Don't know if we enable that one for clang with W= or Nathan adds
additional switches.

> I think the following patch would resolve this issue. This is part of a set
> that isn't fully applied.
> https://lore.kernel.org/linux-edac/20230127170419.1824692-12-yazen.ghannam@amd.com/
> 
> Boris,
> Do you think one of these patches should be applied or just hold off until the
> entire original set is applied?

I still wanted to go through the rest but I'm not sure I'll have time
for it before the merge window. So unless this is breaking some silly
testing scenario, I'd say I'll leave things as they are.

Once I take yours, that silly false positive will go away and we can
forget about it.

> As for myself, I'll start including builds with extra warnings enabled
> for each patch in my workflow. Currently I do a regular build for each
> patch and a build with extra warnings for the entire set.

Dunno, I'd say with false positives we have bigger fish to fry...

Thx.
Nathan Chancellor Feb. 13, 2023, 8:28 p.m. UTC | #3 
On Mon, Feb 13, 2023 at 09:23:47PM +0100, Borislav Petkov wrote:
> On Mon, Feb 13, 2023 at 08:12:38PM +0000, Yazen Ghannam wrote:
> > These errors are encountered when extra warnings are enabled, correct?
> 
> It says so in the warning which one it is: -Werror,-Wsometimes-uninitialized
> 
> Don't know if we enable that one for clang with W= or Nathan adds
> additional switches.

-Wsometimes-uninitialized is part of clang's -Wall so it is on by
default in all builds, regardless of W=

-Werror comes from CONFIG_WERROR, which is enabled with allmodconfig.

> > I think the following patch would resolve this issue. This is part of a set
> > that isn't fully applied.
> > https://lore.kernel.org/linux-edac/20230127170419.1824692-12-yazen.ghannam@amd.com/
> > 
> > Boris,
> > Do you think one of these patches should be applied or just hold off until the
> > entire original set is applied?
> 
> I still wanted to go through the rest but I'm not sure I'll have time
> for it before the merge window. So unless this is breaking some silly
> testing scenario, I'd say I'll leave things as they are.

This breaks allmodconfig with clang, so it would be great if one of
these solutions was applied in the meantime.

Cheers,
Nathan
Tom Rix Feb. 13, 2023, 9:17 p.m. UTC | #4 
On 2/13/23 12:28 PM, Nathan Chancellor wrote:
> On Mon, Feb 13, 2023 at 09:23:47PM +0100, Borislav Petkov wrote:
>> On Mon, Feb 13, 2023 at 08:12:38PM +0000, Yazen Ghannam wrote:
>>> These errors are encountered when extra warnings are enabled, correct?
>> It says so in the warning which one it is: -Werror,-Wsometimes-uninitialized
>>
>> Don't know if we enable that one for clang with W= or Nathan adds
>> additional switches.
> -Wsometimes-uninitialized is part of clang's -Wall so it is on by
> default in all builds, regardless of W=
>
> -Werror comes from CONFIG_WERROR, which is enabled with allmodconfig.
>
>>> I think the following patch would resolve this issue. This is part of a set
>>> that isn't fully applied.
>>> https://lore.kernel.org/linux-edac/20230127170419.1824692-12-yazen.ghannam@amd.com/
>>>
>>> Boris,
>>> Do you think one of these patches should be applied or just hold off until the
>>> entire original set is applied?
>> I still wanted to go through the rest but I'm not sure I'll have time
>> for it before the merge window. So unless this is breaking some silly
>> testing scenario, I'd say I'll leave things as they are.
> This breaks allmodconfig with clang, so it would be great if one of
> these solutions was applied in the meantime.

This happens at least on allyesconfig clang W=1,2, i do not know about 
default, it's in a bad state as well.

It would be great if the clang build was working.

Nathan's patch is fine, go with that.

Tom

>
> Cheers,
> Nathan
>
Yazen Ghannam Feb. 13, 2023, 10:11 p.m. UTC | #5 
On Mon, Feb 13, 2023 at 01:17:51PM -0800, Tom Rix wrote:
> 
> On 2/13/23 12:28 PM, Nathan Chancellor wrote:
> > On Mon, Feb 13, 2023 at 09:23:47PM +0100, Borislav Petkov wrote:
> > > On Mon, Feb 13, 2023 at 08:12:38PM +0000, Yazen Ghannam wrote:
> > > > These errors are encountered when extra warnings are enabled, correct?
> > > It says so in the warning which one it is: -Werror,-Wsometimes-uninitialized
> > > 
> > > Don't know if we enable that one for clang with W= or Nathan adds
> > > additional switches.
> > -Wsometimes-uninitialized is part of clang's -Wall so it is on by
> > default in all builds, regardless of W=
> > 
> > -Werror comes from CONFIG_WERROR, which is enabled with allmodconfig.
> > 
> > > > I think the following patch would resolve this issue. This is part of a set
> > > > that isn't fully applied.
> > > > https://lore.kernel.org/linux-edac/20230127170419.1824692-12-yazen.ghannam@amd.com/
> > > > 
> > > > Boris,
> > > > Do you think one of these patches should be applied or just hold off until the
> > > > entire original set is applied?
> > > I still wanted to go through the rest but I'm not sure I'll have time
> > > for it before the merge window. So unless this is breaking some silly
> > > testing scenario, I'd say I'll leave things as they are.
> > This breaks allmodconfig with clang, so it would be great if one of
> > these solutions was applied in the meantime.
> 
> This happens at least on allyesconfig clang W=1,2, i do not know about
> default, it's in a bad state as well.
>

Yes, this breaks on a default clang build. I just used "make LLVM=1" with the
same config I used before, and I see the error.

GCC doesn't seem to have a comparable warning to "-Wsometimes-uninitialized".
I went back and tried W=123 and no warnings in this code.

Building with clang was straightforward, so I'll try to include it in my
workflow in the future.

> It would be great if the clang build was working.
> 
> Nathan's patch is fine, go with that.
>

I agree Nathan's patch is fine, but would you all be okay with a simpler
change? Initializing the variables (as below) will silence the warnings, and
we know this is a false positive. Eventually this function will be reworked,
so a trivial workaround seems okay. What do y'all think?

Thanks,
Yazen

------

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 1c4bef1cdf28..5b42533f306a 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -3928,7 +3928,7 @@ static const struct attribute_group
*amd64_edac_attr_groups[] = {

 static int hw_info_get(struct amd64_pvt *pvt)
  {
  -       u16 pci_id1, pci_id2;
  +       u16 pci_id1 = 0, pci_id2 = 0;
          int ret;

	          if (pvt->fam >= 0x17) {

------
Nathan Chancellor Feb. 13, 2023, 10:16 p.m. UTC | #6 
On Mon, Feb 13, 2023 at 10:11:38PM +0000, Yazen Ghannam wrote:
> On Mon, Feb 13, 2023 at 01:17:51PM -0800, Tom Rix wrote:
> > 
> > On 2/13/23 12:28 PM, Nathan Chancellor wrote:
> > > On Mon, Feb 13, 2023 at 09:23:47PM +0100, Borislav Petkov wrote:
> > > > On Mon, Feb 13, 2023 at 08:12:38PM +0000, Yazen Ghannam wrote:
> > > > > These errors are encountered when extra warnings are enabled, correct?
> > > > It says so in the warning which one it is: -Werror,-Wsometimes-uninitialized
> > > > 
> > > > Don't know if we enable that one for clang with W= or Nathan adds
> > > > additional switches.
> > > -Wsometimes-uninitialized is part of clang's -Wall so it is on by
> > > default in all builds, regardless of W=
> > > 
> > > -Werror comes from CONFIG_WERROR, which is enabled with allmodconfig.
> > > 
> > > > > I think the following patch would resolve this issue. This is part of a set
> > > > > that isn't fully applied.
> > > > > https://lore.kernel.org/linux-edac/20230127170419.1824692-12-yazen.ghannam@amd.com/
> > > > > 
> > > > > Boris,
> > > > > Do you think one of these patches should be applied or just hold off until the
> > > > > entire original set is applied?
> > > > I still wanted to go through the rest but I'm not sure I'll have time
> > > > for it before the merge window. So unless this is breaking some silly
> > > > testing scenario, I'd say I'll leave things as they are.
> > > This breaks allmodconfig with clang, so it would be great if one of
> > > these solutions was applied in the meantime.
> > 
> > This happens at least on allyesconfig clang W=1,2, i do not know about
> > default, it's in a bad state as well.
> >
> 
> Yes, this breaks on a default clang build. I just used "make LLVM=1" with the
> same config I used before, and I see the error.
> 
> GCC doesn't seem to have a comparable warning to "-Wsometimes-uninitialized".
> I went back and tried W=123 and no warnings in this code.

GCC's -Wmaybe-uninitialized uses interprocedural analysis I believe,
which would allow it to see that it is not possible for these variables
to be used uninitialized. However, that type of analysis can go wrong
with optimizations pretty quickly, so it was disabled for the kernel
under normal builds and W=1; W=2 will show those instances again but
again, I would not expect there to be one here.

> Building with clang was straightforward, so I'll try to include it in my
> workflow in the future.
> 
> > It would be great if the clang build was working.
> > 
> > Nathan's patch is fine, go with that.
> >
> 
> I agree Nathan's patch is fine, but would you all be okay with a simpler
> change? Initializing the variables (as below) will silence the warnings, and
> we know this is a false positive. Eventually this function will be reworked,
> so a trivial workaround seems okay. What do y'all think?

I have no objections. Will you send the patch? Consider it

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

in advanced.

Thanks for taking a further look at this problem!

Cheers,
Nathan

> 
> Thanks,
> Yazen
> 
> ------
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index 1c4bef1cdf28..5b42533f306a 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -3928,7 +3928,7 @@ static const struct attribute_group
> *amd64_edac_attr_groups[] = {
> 
>  static int hw_info_get(struct amd64_pvt *pvt)
>   {
>   -       u16 pci_id1, pci_id2;
>   +       u16 pci_id1 = 0, pci_id2 = 0;
>           int ret;
> 
> 	          if (pvt->fam >= 0x17) {
> 
> ------
diff mbox series

Patch

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 1c4bef1cdf28..f6d50561c106 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -3179,9 +3179,6 @@  static void decode_umc_error(int node_id, struct mce *m)
 static int
 reserve_mc_sibling_devs(struct amd64_pvt *pvt, u16 pci_id1, u16 pci_id2)
 {
-	if (pvt->umc)
-		return 0;
-
 	/* Reserve the ADDRESS MAP Device */
 	pvt->F1 = pci_get_related_function(pvt->F3->vendor, pci_id1, pvt->F3);
 	if (!pvt->F1) {
@@ -3938,11 +3935,11 @@  static int hw_info_get(struct amd64_pvt *pvt)
 	} else {
 		pci_id1 = fam_type->f1_id;
 		pci_id2 = fam_type->f2_id;
-	}
 
-	ret = reserve_mc_sibling_devs(pvt, pci_id1, pci_id2);
-	if (ret)
-		return ret;
+		ret = reserve_mc_sibling_devs(pvt, pci_id1, pci_id2);
+		if (ret)
+			return ret;
+	}
 
 	read_mc_regs(pvt);