Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <9fa13591-38dc-5dbf-5271-5c2bd364f16f@linux.intel.com>
Date:   Mon, 31 Jul 2023 16:02:09 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: potential return of uninitialized variable ret in function
 skx_upi_topology_cb
To:     "Colin King (gmail)" <colin.i.king@gmail.com>
Cc:     linux-perf-users@vger.kernel.org,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <21e2ea66-9524-8e34-a2f0-8e16b9617d68@gmail.com>
Content-Language: en-US
From:   Alexander Antonov <alexander.antonov@linux.intel.com>
In-Reply-To: <21e2ea66-9524-8e34-a2f0-8e16b9617d68@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Colin,
Thank you so much for reporting the issue. Seems fix wasn't picked up 
before, I will resend it.

Thank you


On 7/27/2023 3:55 PM, Colin King (gmail) wrote:
> Hi,
>
> Static analysis with cppcheck has detected a potential return of an 
> uninitialized variable in function skx_upi_topology_cb. The issue was 
> introduced with commit:
>
> commit 4cfce57fa42d277497cd2c425021312eae2f223c
> Author: Alexander Antonov <alexander.antonov@linux.intel.com>
> Date:   Thu Nov 17 12:28:28 2022 +0000
>
>     perf/x86/intel/uncore: Enable UPI topology discovery for Skylake 
> Server
>
> static int skx_upi_topology_cb(struct intel_uncore_type *type, int 
> segment,
>                                 int die, u64 cpu_bus_msr)
> {
>         int idx, ret;
>
> ^^ ret is not initialized
>
>         struct intel_uncore_topology *upi;
>         unsigned int devfn;
>         struct pci_dev *dev = NULL;
>         u8 bus = cpu_bus_msr >> (3 * BUS_NUM_STRIDE);
>
>         for (idx = 0; idx < type->num_boxes; idx++) {
>                 upi = &type->topology[die][idx];
>                 devfn = PCI_DEVFN(SKX_UPI_REGS_ADDR_DEVICE_LINK0 + idx,
>                                   SKX_UPI_REGS_ADDR_FUNCTION);
>                 dev = pci_get_domain_bus_and_slot(segment, bus, devfn);
>
> ^^ dev may be null, so ret is never assigned
>
>                 if (dev) {
>                         ret = upi_fill_topology(dev, upi, idx);
>                         if (ret)
>                                 break;
>                 }
>         }
>
>         pci_dev_put(dev);
>         return ret;
> }
>
> I suspect this probably is very unlikely, but it would be useful to 
> have ret initialized to some value to avoid garbage being returned. 
> Not sure what the best value is to set as as default in this corner case.
>
> Colin