On Thu, 23 May 2019 15:29:11 +1000
David Gibson <da...@gibson.dropbear.id.au> wrote:
> spapr_populate_pci_child_dt() adds a 'name' property to the device tree
> node for PCI devices. This is never necessary for a flattened device tree,
> it is implicit in the name added when the node is constructed. In fact
> anything we do add to a 'name' property will be overwritten with something
> derived from the structural name in the guest firmware (but in fact it is
> exactly the same bytes).
>
> So, remove that. In addition, pci_get_node_name() is very simple, so fold
> it into its (also simple) sole caller spapr_create_pci_child_dt().
>
> While we're there rename pci_find_device_name() to the shorter and more
> accurate dt_name_from_class().
>
> Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
> ---
> hw/ppc/spapr_pci.c | 43 +++++++++++++++++--------------------------
> 1 file changed, 17 insertions(+), 26 deletions(-)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 97961b0128..b2db46ef1d 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1173,8 +1173,8 @@ static const PCIClass pci_classes[] = {
> { "data-processing-controller", spc_subclass },
> };
>
> -static const char *pci_find_device_name(uint8_t class, uint8_t subclass,
> - uint8_t iface)
> +static const char *dt_name_from_class(uint8_t class, uint8_t subclass,
> + uint8_t iface)
> {
> const PCIClass *pclass;
> const PCISubClass *psubclass;
> @@ -1216,23 +1216,6 @@ static const char *pci_find_device_name(uint8_t class,
> uint8_t subclass,
> return name;
> }
>
> -static gchar *pci_get_node_name(PCIDevice *dev)
> -{
> - int slot = PCI_SLOT(dev->devfn);
> - int func = PCI_FUNC(dev->devfn);
> - uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3);
> - const char *name;
> -
> - name = pci_find_device_name((ccode >> 16) & 0xff, (ccode >> 8) & 0xff,
> - ccode & 0xff);
> -
> - if (func != 0) {
> - return g_strdup_printf("%s@%x,%x", name, slot, func);
> - } else {
> - return g_strdup_printf("%s@%x", name, slot);
> - }
> -}
> -
> static uint32_t spapr_phb_get_pci_drc_index(SpaprPhbState *phb,
> PCIDevice *pdev);
>
> @@ -1300,11 +1283,6 @@ static void spapr_populate_pci_child_dt(PCIDevice
> *dev, void *fdt, int offset,
> _FDT(fdt_setprop(fdt, offset, "udf-supported", NULL, 0));
> }
>
> - _FDT(fdt_setprop_string(fdt, offset, "name",
> - pci_find_device_name((ccode >> 16) & 0xff,
> - (ccode >> 8) & 0xff,
> - ccode & 0xff)));
> -
> buf = spapr_phb_get_loc_code(sphb, dev);
> _FDT(fdt_setprop_string(fdt, offset, "ibm,loc-code", buf));
> g_free(buf);
> @@ -1348,10 +1326,23 @@ static int spapr_create_pci_child_dt(SpaprPhbState
> *phb, PCIDevice *dev,
> void *fdt, int node_offset)
> {
> int offset;
> - gchar *nodename;
> + const gchar *basename;
> + char *nodename;
Not sure why you're changing nodename to be a char * instead of a gchar * ...
Apart from that, LGTM.
Reviewed-by: Greg Kurz <gr...@kaod.org>
> + int slot = PCI_SLOT(dev->devfn);
> + int func = PCI_FUNC(dev->devfn);
> + uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3);
> +
> + basename = dt_name_from_class((ccode >> 16) & 0xff, (ccode >> 8) & 0xff,
> + ccode & 0xff);
> +
> + if (func != 0) {
> + nodename = g_strdup_printf("%s@%x,%x", basename, slot, func);
> + } else {
> + nodename = g_strdup_printf("%s@%x", basename, slot);
> + }
>
> - nodename = pci_get_node_name(dev);
> _FDT(offset = fdt_add_subnode(fdt, node_offset, nodename));
> +
> g_free(nodename);
>
> spapr_populate_pci_child_dt(dev, fdt, offset, phb);
