Re: [PATCH] pci: Handle failed calloc in decode_regions()

["Pierre-Clément Tosi" <ptosi@google.com>]

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

-- 
Pierre