| Message ID | 20220519164830.3934669-1-ptosi@google.com |
|---|---|
| State | Accepted |
| Commit | f2ebaaa9f38dddddefaf2e616a9fc489fe8b4021 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | pci: Handle failed calloc in decode_regions() | expand |
On 19.05.22 18:48, Pierre-Clément Tosi wrote: > Add a check for calloc() failing to allocate the requested memory. > > Make decode_regions() return an error code. > > Cc: Bin Meng <bmeng.cn@gmail.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Stefan Roese <sr@denx.de> > Signed-off-by: Pierre-Clément Tosi <ptosi@google.com> Reviewed-by: Stefan Roese <sr@denx.de> Thanks, Stefan > --- > drivers/pci/pci-uclass.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c > index 970ee1adf1..2c85e78a13 100644 > --- a/drivers/pci/pci-uclass.c > +++ b/drivers/pci/pci-uclass.c > @@ -954,7 +954,7 @@ int pci_bind_bus_devices(struct udevice *bus) > return 0; > } > > -static void decode_regions(struct pci_controller *hose, ofnode parent_node, > +static int decode_regions(struct pci_controller *hose, ofnode parent_node, > ofnode node) > { > int pci_addr_cells, addr_cells, size_cells; > @@ -968,7 +968,7 @@ static void decode_regions(struct pci_controller *hose, ofnode parent_node, > prop = ofnode_get_property(node, "ranges", &len); > if (!prop) { > debug("%s: Cannot decode regions\n", __func__); > - return; > + return -EINVAL; > } > > pci_addr_cells = ofnode_read_simple_addr_cells(node); > @@ -986,6 +986,8 @@ static void decode_regions(struct pci_controller *hose, ofnode parent_node, > max_regions = len / cells_per_record + CONFIG_NR_DRAM_BANKS; > hose->regions = (struct pci_region *) > calloc(1, max_regions * sizeof(struct pci_region)); > + if (!hose->regions) > + return -ENOMEM; > > for (i = 0; i < max_regions; i++, len -= cells_per_record) { > u64 pci_addr, addr, size; > @@ -1053,7 +1055,7 @@ static void decode_regions(struct pci_controller *hose, ofnode parent_node, > /* Add a region for our local memory */ > bd = gd->bd; > if (!bd) > - return; > + return 0; > > for (i = 0; i < CONFIG_NR_DRAM_BANKS; ++i) { > if (bd->bi_dram[i].size) { > @@ -1068,7 +1070,7 @@ static void decode_regions(struct pci_controller *hose, ofnode parent_node, > } > } > > - return; > + return 0; > } > > static int pci_uclass_pre_probe(struct udevice *bus) > @@ -1097,7 +1099,10 @@ static int pci_uclass_pre_probe(struct udevice *bus) > /* For bridges, use the top-level PCI controller */ > if (!device_is_on_pci_bus(bus)) { > hose->ctlr = bus; > - decode_regions(hose, dev_ofnode(bus->parent), dev_ofnode(bus)); > + ret = decode_regions(hose, dev_ofnode(bus->parent), > + dev_ofnode(bus)); > + if (ret) > + return ret; > } else { > struct pci_controller *parent_hose; > Viele Grüße, Stefan Roese
On Thu, May 19, 2022 at 05:48:30PM +0100, Pierre-Clément Tosi wrote: > Add a check for calloc() failing to allocate the requested memory. > > Make decode_regions() return an error code. > > Cc: Bin Meng <bmeng.cn@gmail.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Stefan Roese <sr@denx.de> > Signed-off-by: Pierre-Clément Tosi <ptosi@google.com> > Reviewed-by: Stefan Roese <sr@denx.de> Applied to u-boot/next, thanks!
Quoting Pierre-Clément Tosi <ptosi@google.com>: > Add a check for calloc() failing to allocate the requested memory. > > Make decode_regions() return an error code. > > Cc: Bin Meng <bmeng.cn@gmail.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Stefan Roese <sr@denx.de> > Signed-off-by: Pierre-Clément Tosi <ptosi@google.com> > --- Hi, We upgraded from v2022.04 -> v2022.10, and this PATCH breaks PCI (scsi/sata) support for x86_64. I have seen this in qemu, i havn't had the time to test this on real hardware. Steps to reproduce: $ make efi-x86_payload64_defconfig && make -j32 && scripts/build-efi.sh -sPrp Br, /Sean > drivers/pci/pci-uclass.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c > index 970ee1adf1..2c85e78a13 100644 > --- a/drivers/pci/pci-uclass.c > +++ b/drivers/pci/pci-uclass.c > @@ -954,7 +954,7 @@ int pci_bind_bus_devices(struct udevice *bus) > return 0; > } > > -static void decode_regions(struct pci_controller *hose, ofnode parent_node, > +static int decode_regions(struct pci_controller *hose, ofnode parent_node, > ofnode node) > { > int pci_addr_cells, addr_cells, size_cells; > @@ -968,7 +968,7 @@ static void decode_regions(struct pci_controller > *hose, ofnode parent_node, > prop = ofnode_get_property(node, "ranges", &len); > if (!prop) { > debug("%s: Cannot decode regions\n", __func__); > - return; > + return -EINVAL; > } > > pci_addr_cells = ofnode_read_simple_addr_cells(node); > @@ -986,6 +986,8 @@ static void decode_regions(struct pci_controller > *hose, ofnode parent_node, > max_regions = len / cells_per_record + CONFIG_NR_DRAM_BANKS; > hose->regions = (struct pci_region *) > calloc(1, max_regions * sizeof(struct pci_region)); > + if (!hose->regions) > + return -ENOMEM; > > for (i = 0; i < max_regions; i++, len -= cells_per_record) { > u64 pci_addr, addr, size; > @@ -1053,7 +1055,7 @@ static void decode_regions(struct > pci_controller *hose, ofnode parent_node, > /* Add a region for our local memory */ > bd = gd->bd; > if (!bd) > - return; > + return 0; > > for (i = 0; i < CONFIG_NR_DRAM_BANKS; ++i) { > if (bd->bi_dram[i].size) { > @@ -1068,7 +1070,7 @@ static void decode_regions(struct > pci_controller *hose, ofnode parent_node, > } > } > > - return; > + return 0; > } > > static int pci_uclass_pre_probe(struct udevice *bus) > @@ -1097,7 +1099,10 @@ static int pci_uclass_pre_probe(struct udevice *bus) > /* For bridges, use the top-level PCI controller */ > if (!device_is_on_pci_bus(bus)) { > hose->ctlr = bus; > - decode_regions(hose, dev_ofnode(bus->parent), dev_ofnode(bus)); > + ret = decode_regions(hose, dev_ofnode(bus->parent), > + dev_ofnode(bus)); > + if (ret) > + return ret; > } else { > struct pci_controller *parent_hose; > > -- > 2.36.1.124.g0e6072fb45-goog
Hi, On Fri, Dec 02, 2022 at 08:38:37PM +0100, sean@geanix.com wrote: > > Quoting Pierre-Clément Tosi <ptosi@google.com>: > > > Add a check for calloc() failing to allocate the requested memory. > > > > Make decode_regions() return an error code. > > > > Cc: Bin Meng <bmeng.cn@gmail.com> > > Cc: Simon Glass <sjg@chromium.org> > > Cc: Stefan Roese <sr@denx.de> > > Signed-off-by: Pierre-Clément Tosi <ptosi@google.com> > > --- > > Hi, > > We upgraded from v2022.04 -> v2022.10, and this PATCH breaks PCI (scsi/sata) > support for x86_64. > I have seen this in qemu, i havn't had the time to test this on real hardware. > > Steps to reproduce: > $ make efi-x86_payload64_defconfig && make -j32 && scripts/build-efi.sh -sPrp Decompiling the resulting u-boot.dtb shows pci { compatible = "pci-x86"; u-boot,dm-pre-reloc; }; which isn't supported by the patch as we return -EINVAL when missing "ranges". The rationale here was that the property seemed mandatory (see [1] and [2]); was this a misunderstanding of potential corner cases? OTOH, I see that most DTS using "pci-x86" (a pseudo-device intended to act as a PCI bus) actually provide "ranges". In particular, a comment added by 0ced70a0bb7a ("x86: tpl: Add a fake PCI bus") states that The BARs are allocated statically in the device tree. So I'll let others comment on this but either: - arch/x86/dts/efi-x86_payload.dts (and a few other DTS) is missing "ranges"; or - pci_uclass_pre_probe() should treat UCLASS_SIMPLE_BUS differently. [1]: https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/host-generic-pci.txt [2]: IEEE Std 1275-1994 > > Br, > /Sean > > > drivers/pci/pci-uclass.c | 15 ++++++++++----- > > 1 file changed, 10 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c > > index 970ee1adf1..2c85e78a13 100644 > > --- a/drivers/pci/pci-uclass.c > > +++ b/drivers/pci/pci-uclass.c > > @@ -954,7 +954,7 @@ int pci_bind_bus_devices(struct udevice *bus) > > return 0; > > } > > > > -static void decode_regions(struct pci_controller *hose, ofnode parent_node, > > +static int decode_regions(struct pci_controller *hose, ofnode parent_node, > > ofnode node) > > { > > int pci_addr_cells, addr_cells, size_cells; > > @@ -968,7 +968,7 @@ static void decode_regions(struct pci_controller > > *hose, ofnode parent_node, > > prop = ofnode_get_property(node, "ranges", &len); > > if (!prop) { > > debug("%s: Cannot decode regions\n", __func__); > > - return; > > + return -EINVAL; > > } > > > > pci_addr_cells = ofnode_read_simple_addr_cells(node); > > @@ -986,6 +986,8 @@ static void decode_regions(struct pci_controller > > *hose, ofnode parent_node, > > max_regions = len / cells_per_record + CONFIG_NR_DRAM_BANKS; > > hose->regions = (struct pci_region *) > > calloc(1, max_regions * sizeof(struct pci_region)); > > + if (!hose->regions) > > + return -ENOMEM; > > > > for (i = 0; i < max_regions; i++, len -= cells_per_record) { > > u64 pci_addr, addr, size; > > @@ -1053,7 +1055,7 @@ static void decode_regions(struct pci_controller > > *hose, ofnode parent_node, > > /* Add a region for our local memory */ > > bd = gd->bd; > > if (!bd) > > - return; > > + return 0; > > > > for (i = 0; i < CONFIG_NR_DRAM_BANKS; ++i) { > > if (bd->bi_dram[i].size) { > > @@ -1068,7 +1070,7 @@ static void decode_regions(struct pci_controller > > *hose, ofnode parent_node, > > } > > } > > > > - return; > > + return 0; > > } > > > > static int pci_uclass_pre_probe(struct udevice *bus) > > @@ -1097,7 +1099,10 @@ static int pci_uclass_pre_probe(struct udevice *bus) > > /* For bridges, use the top-level PCI controller */ > > if (!device_is_on_pci_bus(bus)) { > > hose->ctlr = bus; > > - decode_regions(hose, dev_ofnode(bus->parent), dev_ofnode(bus)); > > + ret = decode_regions(hose, dev_ofnode(bus->parent), > > + dev_ofnode(bus)); > > + if (ret) > > + return ret; > > } else { > > struct pci_controller *parent_hose; > > > > -- > > 2.36.1.124.g0e6072fb45-goog > > >
Am So., 4. Dez. 2022 um 22:22 Uhr schrieb Pierre-Clément Tosi <ptosi@google.com>: > > Hi, > > On Fri, Dec 02, 2022 at 08:38:37PM +0100, sean@geanix.com wrote: > > > > Quoting Pierre-Clément Tosi <ptosi@google.com>: > > > > > Add a check for calloc() failing to allocate the requested memory. > > > > > > Make decode_regions() return an error code. > > > > > > Cc: Bin Meng <bmeng.cn@gmail.com> > > > Cc: Simon Glass <sjg@chromium.org> > > > Cc: Stefan Roese <sr@denx.de> > > > Signed-off-by: Pierre-Clément Tosi <ptosi@google.com> > > > --- > > > > Hi, > > > > We upgraded from v2022.04 -> v2022.10, and this PATCH breaks PCI (scsi/sata) > > support for x86_64. > > I have seen this in qemu, i havn't had the time to test this on real hardware. > > > > Steps to reproduce: > > $ make efi-x86_payload64_defconfig && make -j32 && scripts/build-efi.sh -sPrp > Breaks my use case too and is basically a revert of https://source.denx.de/u-boot/u-boot/-/commit/f2825f6ec0bb50e7bd9376828a32212f1961f979 In my use case we are using coreboot for different Intel SoCs with a generic U-Boot payload. > Decompiling the resulting u-boot.dtb shows > > pci { > compatible = "pci-x86"; > u-boot,dm-pre-reloc; > }; > Yes.. that is what can be found here: https://source.denx.de/u-boot/u-boot/-/blob/master/arch/x86/dts/coreboot.dts#L34 > which isn't supported by the patch as we return -EINVAL when missing "ranges". > The rationale here was that the property seemed mandatory (see [1] and [2]); was > this a misunderstanding of potential corner cases? > At the moment I would like to revert this change. > OTOH, I see that most DTS using "pci-x86" (a pseudo-device intended to act as a > PCI bus) actually provide "ranges". In particular, a comment added by > 0ced70a0bb7a ("x86: tpl: Add a fake PCI bus") states that > > The BARs are allocated statically in the device tree. > > So I'll let others comment on this but either: > > - arch/x86/dts/efi-x86_payload.dts (and a few other DTS) is missing "ranges"; or > - pci_uclass_pre_probe() should treat UCLASS_SIMPLE_BUS differently. > > [1]: https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/host-generic-pci.txt > [2]: IEEE Std 1275-1994 > > > > > Br, > > /Sean > > > > > drivers/pci/pci-uclass.c | 15 ++++++++++----- > > > 1 file changed, 10 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c > > > index 970ee1adf1..2c85e78a13 100644 > > > --- a/drivers/pci/pci-uclass.c > > > +++ b/drivers/pci/pci-uclass.c > > > @@ -954,7 +954,7 @@ int pci_bind_bus_devices(struct udevice *bus) > > > return 0; > > > } > > > > > > -static void decode_regions(struct pci_controller *hose, ofnode parent_node, > > > +static int decode_regions(struct pci_controller *hose, ofnode parent_node, > > > ofnode node) > > > { > > > int pci_addr_cells, addr_cells, size_cells; > > > @@ -968,7 +968,7 @@ static void decode_regions(struct pci_controller > > > *hose, ofnode parent_node, > > > prop = ofnode_get_property(node, "ranges", &len); > > > if (!prop) { > > > debug("%s: Cannot decode regions\n", __func__); > > > - return; > > > + return -EINVAL; > > > } > > > > > > pci_addr_cells = ofnode_read_simple_addr_cells(node); > > > @@ -986,6 +986,8 @@ static void decode_regions(struct pci_controller > > > *hose, ofnode parent_node, > > > max_regions = len / cells_per_record + CONFIG_NR_DRAM_BANKS; > > > hose->regions = (struct pci_region *) > > > calloc(1, max_regions * sizeof(struct pci_region)); > > > + if (!hose->regions) > > > + return -ENOMEM; > > > > > > for (i = 0; i < max_regions; i++, len -= cells_per_record) { > > > u64 pci_addr, addr, size; > > > @@ -1053,7 +1055,7 @@ static void decode_regions(struct pci_controller > > > *hose, ofnode parent_node, > > > /* Add a region for our local memory */ > > > bd = gd->bd; > > > if (!bd) > > > - return; > > > + return 0; > > > > > > for (i = 0; i < CONFIG_NR_DRAM_BANKS; ++i) { > > > if (bd->bi_dram[i].size) { > > > @@ -1068,7 +1070,7 @@ static void decode_regions(struct pci_controller > > > *hose, ofnode parent_node, > > > } > > > } > > > > > > - return; > > > + return 0; > > > } > > > > > > static int pci_uclass_pre_probe(struct udevice *bus) > > > @@ -1097,7 +1099,10 @@ static int pci_uclass_pre_probe(struct udevice *bus) > > > /* For bridges, use the top-level PCI controller */ > > > if (!device_is_on_pci_bus(bus)) { > > > hose->ctlr = bus; > > > - decode_regions(hose, dev_ofnode(bus->parent), dev_ofnode(bus)); > > > + ret = decode_regions(hose, dev_ofnode(bus->parent), > > > + dev_ofnode(bus)); > > > + if (ret) > > > + return ret; > > > } else { > > > struct pci_controller *parent_hose; > > > > > > -- > > > 2.36.1.124.g0e6072fb45-goog > > > > > > > > -- > Pierre
Hi, On Tue, Mar 21, 2023 at 08:57:18AM +0100, Christian Gmeiner wrote: > Am So., 4. Dez. 2022 um 22:22 Uhr schrieb Pierre-Clément Tosi > <ptosi@google.com>: > > > > Hi, > > > > On Fri, Dec 02, 2022 at 08:38:37PM +0100, sean@geanix.com wrote: > > > > > > Quoting Pierre-Clément Tosi <ptosi@google.com>: > > > > > > > Add a check for calloc() failing to allocate the requested memory. > > > > > > > > Make decode_regions() return an error code. > > > > > > > > Cc: Bin Meng <bmeng.cn@gmail.com> > > > > Cc: Simon Glass <sjg@chromium.org> > > > > Cc: Stefan Roese <sr@denx.de> > > > > Signed-off-by: Pierre-Clément Tosi <ptosi@google.com> > > > > --- > > > > > > Hi, > > > > > > We upgraded from v2022.04 -> v2022.10, and this PATCH breaks PCI (scsi/sata) > > > support for x86_64. > > > I have seen this in qemu, i havn't had the time to test this on real hardware. > > > > > > Steps to reproduce: > > > $ make efi-x86_payload64_defconfig && make -j32 && scripts/build-efi.sh -sPrp > > > > Breaks my use case too and is basically a revert of > https://source.denx.de/u-boot/u-boot/-/commit/f2825f6ec0bb50e7bd9376828a32212f1961f979 > In my use case we are using coreboot for different Intel SoCs with a > generic U-Boot payload. > > > Decompiling the resulting u-boot.dtb shows > > > > pci { > > compatible = "pci-x86"; > > u-boot,dm-pre-reloc; > > }; > > > > Yes.. that is what can be found here: > https://source.denx.de/u-boot/u-boot/-/blob/master/arch/x86/dts/coreboot.dts#L34 > > > > which isn't supported by the patch as we return -EINVAL when missing "ranges". > > The rationale here was that the property seemed mandatory (see [1] and [2]); was > > this a misunderstanding of potential corner cases? > > > > At the moment I would like to revert this change. > Thanks for sharing such a corner case. IMO, normal operation shouldn't rely on errors being silently skipped because return values are being blindly ignored. Instead, we could limit EINVAL to libfdt errors that aren't FDT_ERR_NOTFOUND, which would address your use-case, where "ranges" is missing from the DT node. Is your issue fixed by this patch: https://patchwork.ozlabs.org/project/uboot/patch/20230220194927.476708-8-sjg@chromium.org/ ? > > OTOH, I see that most DTS using "pci-x86" (a pseudo-device intended to act as a > > PCI bus) actually provide "ranges". In particular, a comment added by > > 0ced70a0bb7a ("x86: tpl: Add a fake PCI bus") states that > > > > The BARs are allocated statically in the device tree. > > > > So I'll let others comment on this but either: > > > > - arch/x86/dts/efi-x86_payload.dts (and a few other DTS) is missing "ranges"; or > > - pci_uclass_pre_probe() should treat UCLASS_SIMPLE_BUS differently. > > > > [1]: https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/host-generic-pci.txt > > [2]: IEEE Std 1275-1994 > > > > > > > > Br, > > > /Sean > > > > > > > drivers/pci/pci-uclass.c | 15 ++++++++++----- > > > > 1 file changed, 10 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c > > > > index 970ee1adf1..2c85e78a13 100644 > > > > --- a/drivers/pci/pci-uclass.c > > > > +++ b/drivers/pci/pci-uclass.c > > > > @@ -954,7 +954,7 @@ int pci_bind_bus_devices(struct udevice *bus) > > > > return 0; > > > > } > > > > > > > > -static void decode_regions(struct pci_controller *hose, ofnode parent_node, > > > > +static int decode_regions(struct pci_controller *hose, ofnode parent_node, > > > > ofnode node) > > > > { > > > > int pci_addr_cells, addr_cells, size_cells; > > > > @@ -968,7 +968,7 @@ static void decode_regions(struct pci_controller > > > > *hose, ofnode parent_node, > > > > prop = ofnode_get_property(node, "ranges", &len); > > > > if (!prop) { > > > > debug("%s: Cannot decode regions\n", __func__); > > > > - return; > > > > + return -EINVAL; > > > > } > > > > > > > > pci_addr_cells = ofnode_read_simple_addr_cells(node); > > > > @@ -986,6 +986,8 @@ static void decode_regions(struct pci_controller > > > > *hose, ofnode parent_node, > > > > max_regions = len / cells_per_record + CONFIG_NR_DRAM_BANKS; > > > > hose->regions = (struct pci_region *) > > > > calloc(1, max_regions * sizeof(struct pci_region)); > > > > + if (!hose->regions) > > > > + return -ENOMEM; > > > > > > > > for (i = 0; i < max_regions; i++, len -= cells_per_record) { > > > > u64 pci_addr, addr, size; > > > > @@ -1053,7 +1055,7 @@ static void decode_regions(struct pci_controller > > > > *hose, ofnode parent_node, > > > > /* Add a region for our local memory */ > > > > bd = gd->bd; > > > > if (!bd) > > > > - return; > > > > + return 0; > > > > > > > > for (i = 0; i < CONFIG_NR_DRAM_BANKS; ++i) { > > > > if (bd->bi_dram[i].size) { > > > > @@ -1068,7 +1070,7 @@ static void decode_regions(struct pci_controller > > > > *hose, ofnode parent_node, > > > > } > > > > } > > > > > > > > - return; > > > > + return 0; > > > > } > > > > > > > > static int pci_uclass_pre_probe(struct udevice *bus) > > > > @@ -1097,7 +1099,10 @@ static int pci_uclass_pre_probe(struct udevice *bus) > > > > /* For bridges, use the top-level PCI controller */ > > > > if (!device_is_on_pci_bus(bus)) { > > > > hose->ctlr = bus; > > > > - decode_regions(hose, dev_ofnode(bus->parent), dev_ofnode(bus)); > > > > + ret = decode_regions(hose, dev_ofnode(bus->parent), > > > > + dev_ofnode(bus)); > > > > + if (ret) > > > > + return ret; > > > > } else { > > > > struct pci_controller *parent_hose; > > > > > > > > -- > > > > 2.36.1.124.g0e6072fb45-goog > > > > > > > > > > > > > -- > > Pierre > > > > -- > greets > -- > Christian Gmeiner, MSc > > https://christian-gmeiner.info/privacypolicy Thanks
Hi > > Hi, > > On Tue, Mar 21, 2023 at 08:57:18AM +0100, Christian Gmeiner wrote: > > Am So., 4. Dez. 2022 um 22:22 Uhr schrieb Pierre-Clément Tosi > > <ptosi@google.com>: > > > > > > Hi, > > > > > > On Fri, Dec 02, 2022 at 08:38:37PM +0100, sean@geanix.com wrote: > > > > > > > > Quoting Pierre-Clément Tosi <ptosi@google.com>: > > > > > > > > > Add a check for calloc() failing to allocate the requested memory. > > > > > > > > > > Make decode_regions() return an error code. > > > > > > > > > > Cc: Bin Meng <bmeng.cn@gmail.com> > > > > > Cc: Simon Glass <sjg@chromium.org> > > > > > Cc: Stefan Roese <sr@denx.de> > > > > > Signed-off-by: Pierre-Clément Tosi <ptosi@google.com> > > > > > --- > > > > > > > > Hi, > > > > > > > > We upgraded from v2022.04 -> v2022.10, and this PATCH breaks PCI (scsi/sata) > > > > support for x86_64. > > > > I have seen this in qemu, i havn't had the time to test this on real hardware. > > > > > > > > Steps to reproduce: > > > > $ make efi-x86_payload64_defconfig && make -j32 && scripts/build-efi.sh -sPrp > > > > > > > Breaks my use case too and is basically a revert of > > https://source.denx.de/u-boot/u-boot/-/commit/f2825f6ec0bb50e7bd9376828a32212f1961f979 > > In my use case we are using coreboot for different Intel SoCs with a > > generic U-Boot payload. > > > > > Decompiling the resulting u-boot.dtb shows > > > > > > pci { > > > compatible = "pci-x86"; > > > u-boot,dm-pre-reloc; > > > }; > > > > > > > Yes.. that is what can be found here: > > https://source.denx.de/u-boot/u-boot/-/blob/master/arch/x86/dts/coreboot.dts#L34 > > > > > > > which isn't supported by the patch as we return -EINVAL when missing "ranges". > > > The rationale here was that the property seemed mandatory (see [1] and [2]); was > > > this a misunderstanding of potential corner cases? > > > > > > > At the moment I would like to revert this change. > > > > Thanks for sharing such a corner case. > > IMO, normal operation shouldn't rely on errors being silently skipped because > return values are being blindly ignored. Instead, we could limit EINVAL to > libfdt errors that aren't FDT_ERR_NOTFOUND, which would address your use-case, > where "ranges" is missing from the DT node. > > Is your issue fixed by this patch: > > https://patchwork.ozlabs.org/project/uboot/patch/20230220194927.476708-8-sjg@chromium.org/ > Yes .. the regression is fixed with this patch \o/
diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c index 970ee1adf1..2c85e78a13 100644 --- a/drivers/pci/pci-uclass.c +++ b/drivers/pci/pci-uclass.c @@ -954,7 +954,7 @@ int pci_bind_bus_devices(struct udevice *bus) return 0; } -static void decode_regions(struct pci_controller *hose, ofnode parent_node, +static int decode_regions(struct pci_controller *hose, ofnode parent_node, ofnode node) { int pci_addr_cells, addr_cells, size_cells; @@ -968,7 +968,7 @@ static void decode_regions(struct pci_controller *hose, ofnode parent_node, prop = ofnode_get_property(node, "ranges", &len); if (!prop) { debug("%s: Cannot decode regions\n", __func__); - return; + return -EINVAL; } pci_addr_cells = ofnode_read_simple_addr_cells(node); @@ -986,6 +986,8 @@ static void decode_regions(struct pci_controller *hose, ofnode parent_node, max_regions = len / cells_per_record + CONFIG_NR_DRAM_BANKS; hose->regions = (struct pci_region *) calloc(1, max_regions * sizeof(struct pci_region)); + if (!hose->regions) + return -ENOMEM; for (i = 0; i < max_regions; i++, len -= cells_per_record) { u64 pci_addr, addr, size; @@ -1053,7 +1055,7 @@ static void decode_regions(struct pci_controller *hose, ofnode parent_node, /* Add a region for our local memory */ bd = gd->bd; if (!bd) - return; + return 0; for (i = 0; i < CONFIG_NR_DRAM_BANKS; ++i) { if (bd->bi_dram[i].size) { @@ -1068,7 +1070,7 @@ static void decode_regions(struct pci_controller *hose, ofnode parent_node, } } - return; + return 0; } static int pci_uclass_pre_probe(struct udevice *bus) @@ -1097,7 +1099,10 @@ static int pci_uclass_pre_probe(struct udevice *bus) /* For bridges, use the top-level PCI controller */ if (!device_is_on_pci_bus(bus)) { hose->ctlr = bus; - decode_regions(hose, dev_ofnode(bus->parent), dev_ofnode(bus)); + ret = decode_regions(hose, dev_ofnode(bus->parent), + dev_ofnode(bus)); + if (ret) + return ret; } else { struct pci_controller *parent_hose;
Add a check for calloc() failing to allocate the requested memory. Make decode_regions() return an error code. Cc: Bin Meng <bmeng.cn@gmail.com> Cc: Simon Glass <sjg@chromium.org> Cc: Stefan Roese <sr@denx.de> Signed-off-by: Pierre-Clément Tosi <ptosi@google.com> --- drivers/pci/pci-uclass.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)