[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <5a6ad008-61c6-4f65-82b3-a4e538cbb9e8@akamai.com>
Date: Fri, 24 Jan 2025 14:27:53 -0500
From: Jason Baron <jbaron@...mai.com>
To: "Zhuo, Qiuxu" <qiuxu.zhuo@...el.com>, Arnd Bergmann <arnd@...nel.org>,
Borislav Petkov <bp@...en8.de>, "Luck, Tony" <tony.luck@...el.com>
Cc: Arnd Bergmann <arnd@...db.de>, James Morse <james.morse@....com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Robert Richter
<rric@...nel.org>, James Ye <jye836@...il.com>,
"linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] EDAC/ie31200: work around false positive build warning
On 1/22/25 9:18 AM, Zhuo, Qiuxu wrote:
> !-------------------------------------------------------------------|
> This Message Is From an External Sender
> This message came from outside your organization.
> |-------------------------------------------------------------------!
>
>> From: Arnd Bergmann <arnd@...nel.org>
>> [...]
>> Subject: [PATCH] EDAC/ie31200: work around false positive build warning
>>
>> From: Arnd Bergmann <arnd@...db.de>
>>
>> gcc-14 produces a bogus warning in some configurations:
>>
>> drivers/edac/ie31200_edac.c: In function 'ie31200_probe1.isra':
>> drivers/edac/ie31200_edac.c:412:26: error: 'dimm_info' is used uninitialized
>> [-Werror=uninitialized]
>> 412 | struct dimm_data
>> dimm_info[IE31200_CHANNELS][IE31200_DIMMS_PER_CHANNEL];
>> | ^~~~~~~~~
>> drivers/edac/ie31200_edac.c:412:26: note: 'dimm_info' declared here
>> 412 | struct dimm_data
>> dimm_info[IE31200_CHANNELS][IE31200_DIMMS_PER_CHANNEL];
>> | ^~~~~~~~~
>>
>> I don't see any way the unintialized access could really happen here, but I can
>
> It looks like gcc-14 isn't smart enough to recognize that this is a false positive build warning :-).
> Actually, the dim_info[][] array is always initialized by populate_dimm_info().
>
>> see why the compiler gets confused by the two loops.
>>
>> Instead, rework the two nested loops to only read the addr_decode registers
>> and then keep only one instance of the dimm info structure.
>>
>> Signed-off-by: Arnd Bergmann <arnd@...db.de>
Seems fine with the comment adjustment.
Thanks.
Acked-by: Jason Baron <jbaron@...mai.com>
>> ---
>> drivers/edac/ie31200_edac.c | 27 +++++++++++++--------------
>> 1 file changed, 13 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/edac/ie31200_edac.c b/drivers/edac/ie31200_edac.c index
>> 4fc16922dc1a..b0ccc54dc66a 100644
>> --- a/drivers/edac/ie31200_edac.c
>> +++ b/drivers/edac/ie31200_edac.c
>> @@ -409,10 +409,9 @@ static int ie31200_probe1(struct pci_dev *pdev, int
>> dev_idx)
>> int i, j, ret;
>> struct mem_ctl_info *mci = NULL;
>> struct edac_mc_layer layers[2];
>> - struct dimm_data
>> dimm_info[IE31200_CHANNELS][IE31200_DIMMS_PER_CHANNEL];
>> void __iomem *window;
>> struct ie31200_priv *priv;
>> - u32 addr_decode, mad_offset;
>> + u32 addr_decode[IE31200_CHANNELS], mad_offset;
>>
>> /*
>> * Kaby Lake, Coffee Lake seem to work like Skylake. Please re-visit
>> @@ -472,17 +471,9 @@ static int ie31200_probe1(struct pci_dev *pdev, int
>> dev_idx)
>>
>> /* populate DIMM info */
>
> This comment also needs to be moved to just above the new location of populate_dimm_info().
>
>> for (i = 0; i < IE31200_CHANNELS; i++) {
>> - addr_decode = readl(window + mad_offset +
>> + addr_decode[i] = readl(window + mad_offset +
>> (i * 4));
>> - edac_dbg(0, "addr_decode: 0x%x\n", addr_decode);
>> - for (j = 0; j < IE31200_DIMMS_PER_CHANNEL; j++) {
>> - populate_dimm_info(&dimm_info[i][j], addr_decode,
>> j,
>> - skl);
>> - edac_dbg(0, "size: 0x%x, rank: %d, width: %d\n",
>> - dimm_info[i][j].size,
>> - dimm_info[i][j].dual_rank,
>> - dimm_info[i][j].x16_width);
>> - }
>> + edac_dbg(0, "addr_decode: 0x%x\n", addr_decode[i]);
>> }
>>
>> /*
>> @@ -493,14 +484,22 @@ static int ie31200_probe1(struct pci_dev *pdev, int
>> dev_idx)
>> */
>> for (i = 0; i < IE31200_DIMMS_PER_CHANNEL; i++) {
>> for (j = 0; j < IE31200_CHANNELS; j++) {
>> + struct dimm_data dimm_info;
>> struct dimm_info *dimm;
>> unsigned long nr_pages;
>>
>> - nr_pages = IE31200_PAGES(dimm_info[j][i].size, skl);
>
> Move the comment here.
>
>> + populate_dimm_info(&dimm_info, addr_decode[j], i,
>> + skl);
>> + edac_dbg(0, "size: 0x%x, rank: %d, width: %d\n",
>> + dimm_info.size,
>> + dimm_info.dual_rank,
>> + dimm_info.x16_width);
>> +
>> + nr_pages = IE31200_PAGES(dimm_info.size, skl);
>> if (nr_pages == 0)
>> continue;
>>
>> - if (dimm_info[j][i].dual_rank) {
>> + if (dimm_info.dual_rank) {
>> nr_pages = nr_pages / 2;
>> dimm = edac_get_dimm(mci, (i * 2) + 1, j, 0);
>> dimm->nr_pages = nr_pages;
>> --
>> 2.39.5
>>
>
Powered by blists - more mailing lists