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

[Pierre-Clément Tosi]

Hi,

On Fri, Dec 02, 2022 at 08:38:37PM +0100, sean at geanix.com wrote:
> 
> Quoting Pierre-Clément Tosi <ptosi at 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 at gmail.com>
> > Cc: Simon Glass <sjg at chromium.org>
> > Cc: Stefan Roese <sr at denx.de>
> > Signed-off-by: Pierre-Clément Tosi <ptosi at 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
> 
> 
> 

-- 
Pierre