| Message ID | 20190523052918.1129-1-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/8] spapr: Clean up device node name generation for PCI devices | expand |
On Thu, 23 May 2019 15:29:11 +1000 David Gibson <david@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 <david@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 <groug@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);
On Thu, May 23, 2019 at 03:29:11PM +1000, David Gibson 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 <david@gibson.dropbear.id.au> The threading is broken here btw. I was CC'd but it's mostly PPC stuff. I like how pci_XX functions that are not in pci.c are going away :) Acked-by: Michael S. Tsirkin <mst@redhat.com> > --- > 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; > + 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); > -- > 2.21.0
On Tue, May 28, 2019 at 11:23:54PM -0400, Michael S. Tsirkin wrote: > On Thu, May 23, 2019 at 03:29:11PM +1000, David Gibson 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 <david@gibson.dropbear.id.au> > > The threading is broken here btw. > > I was CC'd but it's mostly PPC stuff. > I like how pci_XX functions that are not in pci.c are > going away :) > > Acked-by: Michael S. Tsirkin <mst@redhat.com> and that's for the whole patchset. > > > --- > > 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; > > + 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); > > -- > > 2.21.0
On Fri, May 24, 2019 at 03:32:19PM +0200, Greg Kurz wrote: > On Thu, 23 May 2019 15:29:11 +1000 > David Gibson <david@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 <david@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 * ... Uh... by mistake. I'll remove that. > Apart from that, LGTM. > > Reviewed-by: Greg Kurz <groug@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); >
On Tue, May 28, 2019 at 11:23:54PM -0400, Michael S. Tsirkin wrote: > On Thu, May 23, 2019 at 03:29:11PM +1000, David Gibson 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 <david@gibson.dropbear.id.au> > > The threading is broken here btw. I'm not entirely sure what you mean by that. I did forget to add a cover letter. > I was CC'd but it's mostly PPC stuff. Yeah, this was just for reference, since it is PCI stuff for ppc. > I like how pci_XX functions that are not in pci.c are > going away :) You're welcome. > Acked-by: Michael S. Tsirkin <mst@redhat.com> Thanks.
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; + 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);
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 <david@gibson.dropbear.id.au> --- hw/ppc/spapr_pci.c | 43 +++++++++++++++++-------------------------- 1 file changed, 17 insertions(+), 26 deletions(-)