Re: [PATCH 03/11] ppc/pnv: use dev->parent_bus->parent to get the PHB

[Daniel Henrique Barboza]

On 6/14/22 09:10, Cédric Le Goater wrote:
> On 6/13/22 17:44, Daniel Henrique Barboza wrote:
> > It is not advisable to execute an object_dynamic_cast() to poke into
> > bus->qbus.parent and follow it up with a C cast into the PnvPHB type we
> > think we got.
> >
> > A better way is to access the PnvPHB object via a QOM macro accessing
> > the existing parent links of the DeviceState. For a given
> > pnv-phb3/4-root-port 'dev', dev->parent_bus will give us the PHB bus,
> > and dev->parent_bus->parent is the PHB. Use the adequate QOM macro to
> > assert the type, and keep the NULL check in case we didn't get the
> > object we were expecting.
> >
> > Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
> > ---
> >   hw/pci-host/pnv_phb3.c | 10 +++++++---
> >   hw/pci-host/pnv_phb4.c | 10 +++++++---
> >   2 files changed, 14 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> > index 4ba660f8b9..7901d8172c 100644
> > --- a/hw/pci-host/pnv_phb3.c
> > +++ b/hw/pci-host/pnv_phb3.c
> > @@ -1139,12 +1139,16 @@ static void pnv_phb3_root_port_realize(DeviceState 
> > *dev, Error **errp)
> >   {
> >       PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev);
> >       PCIDevice *pci = PCI_DEVICE(dev);
> > -    PCIBus *bus = pci_get_bus(pci);
> >       PnvPHB3 *phb = NULL;
> >       Error *local_err = NULL;
> > -    phb = (PnvPHB3 *) object_dynamic_cast(OBJECT(bus->qbus.parent),
> > -                                          TYPE_PNV_PHB3);
> > +    /*
> > +     * dev->parent_bus gives access to the pnv-phb-root bus.
> > +     * The PnvPHB3 is the owner (parent) of the bus.
> > +     */
> > +    if (dev && dev->parent_bus) {
> > +        phb = PNV_PHB3(dev->parent_bus->parent);
> > +    }
>
> Couldn't we simply use :
>
>        phb = PNV_PHB3(bus);
>
> ?

No. This will give us a reference to a pnv-phb3-root object.


Getting a reference to the PHB by using bus->parent happens in other parts of
code, such as:


hw/pci-host/gpex-acpi.c:            crs = 
build_crs(PCI_HOST_BRIDGE(BUS(bus)->parent), &crs_range_set,
hw/pci-bridge/pci_expander_bridge.c:    main_host = 
PCI_HOST_BRIDGE(pxb_dev_base->parent_bus->parent);


So I believe we're not out of line here.


Thanks,


Daniel


> Thanks,
>
> C.
>
> >       if (!phb) {
> >           error_setg(errp,
> > diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> > index ffd9d8a947..bae9398d86 100644
> > --- a/hw/pci-host/pnv_phb4.c
> > +++ b/hw/pci-host/pnv_phb4.c
> > @@ -1782,12 +1782,16 @@ static void pnv_phb4_root_port_realize(DeviceState 
> > *dev, Error **errp)
> >   {
> >       PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev);
> >       PCIDevice *pci = PCI_DEVICE(dev);
> > -    PCIBus *bus = pci_get_bus(pci);
> >       PnvPHB4 *phb = NULL;
> >       Error *local_err = NULL;
> > -    phb = (PnvPHB4 *) object_dynamic_cast(OBJECT(bus->qbus.parent),
> > -                                          TYPE_PNV_PHB4);
> > +    /*
> > +     * dev->parent_bus gives access to the pnv-phb-root bus.
> > +     * The PnvPHB4 is the owner (parent) of the bus.
> > +     */
> > +    if (dev && dev->parent_bus) {
> > +        phb = PNV_PHB4(dev->parent_bus->parent);
> > +    }
> >       if (!phb) {
> >           error_setg(errp, "%s must be connected to pnv-phb4 buses", dev->id);