| Message ID | 20230213191510.2237360-1-trix@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | EDAC/amd64: remove unneeded call to reserve_mc_sibling_devs() | expand |
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
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.
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
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 >
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) { ------
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 --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);
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(-)