2023-07-27 14:10:55

by Colin Ian King

Subject: potential return of uninitialized variable ret in function skx_upi_topology_cb

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 <[email protected]>
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

2023-07-31 15:14:39

by Alexander Antonov

Subject: Re: potential return of uninitialized variable ret in function skx_upi_topology_cb

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 <[email protected]>
> 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