Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()

[Marcel Apfelbaum]

On 12/21/2015 04:59 AM, Cao jin wrote:
> On 12/20/2015 07:21 PM, Marcel Apfelbaum wrote:
> > On 12/20/2015 12:48 PM, Cao jin wrote:
> > > Hi,
> > >
> > > On 12/20/2015 06:22 PM, Marcel Apfelbaum wrote:
> [...]
> > > > > +
> > > > > +err_register_bus:
> > > > > +    object_unref(OBJECT(ds));
> > > > > +    object_unref(OBJECT(bds));
> > > > > +    object_unref(OBJECT(bus));
> > > >
> > > >
> > > > The order should be in the reverse order of creation:
> > > >      bds, bus, ds
> > >
> > > Ok, I can do that. But it seems the order here doesn`t matter? Is
> > > there dependency among these three?
> >
> > Yes, there is a dependency:
> > At first the pxb host (ds) is created, then the bus (bus) is created as
> > host's child (see pci_bus_new)
> > and in the end a pci bridge (bds) is attached to the bus (see qdev_create).
>
> Yup...thanks for reminding, I did read the code trying to find the parent 
> relationship...but seem I didn`t read it thoroughly:-[
>
> > By the way, indeed you should call object_unparent at least for the
> > pxb_host(ds), but you may want to
> > check the others parent relationship as well.
>
> yes, but I think you are saying: object_unparent(bus), right? the relationship 
> seems is:
>    pxb host-->(child property)bus-->(link property)bds
>
> Another question: because some of this series is CCed to qemu-trivial(which means: 
> reviewed-by?) by other maintainer, so next time, do I need to send the whole series with 
> "v2", or the rest?

Hi,

Since the patches are not related, the ones cc-ed to qemu-trivial will be taken 
by the maintainer of trivial patches,
for the rest you should prepare a V2 to be reviewed by the corresponding 
sub-tree maintainer.

CC to qemu-trivial does not mean "reviewed-by", it just implies the
patch is simple enough to go through the trivial tree and does not need to go 
through the sub-tree maintainer.

Thanks,
Marcel

> > > > >   }
> > > > >
> > > > >   static void pxb_dev_exitfn(PCIDevice *pci_dev)
> > > > > @@ -259,7 +263,7 @@ static void pxb_dev_class_init(ObjectClass *klass,
> > > > > void *data)
> > > > >       DeviceClass *dc = DEVICE_CLASS(klass);
> > > > >       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> > > > >
> > > > > -    k->init = pxb_dev_initfn;
> > > > > +    k->realize = pxb_dev_realize;
> > > > >       k->exit = pxb_dev_exitfn;
> > > >
> > > > If init is converted to realize, maybe the exit should be converted to
> > > > unrealize?
> > >
> > > Yup, I agree with you from the point that the names should be antonym.
> > > But it seems there is no PCIDeviceClass.unrealize:(
> >
> > You are right. The pci_qdev_unrealize ultimately calls exit. But the
> > same goes for init, pci_qdev_realize calls for pc->realize.
> > This is the reason I chose to use init/exit instead of the strange
> > realize/exit.
> >
> > But since the intention is to get rid of init, I am not against it.
> >
> > > And I am also not aware why there is no comment for .exit while there
> > > is for .init. It is appreciated if somebody could tell me the history
> > > O:-)
> >
> > I'll add Markus, Andreas and Michael (the PCI maintainer), maybe they
> > have a better insight to this.
> >
> > On the other hand you should continue with the patch and leave the
> > "unrealize" until we'll know more :)
>
> Got it;)
>
> > Thanks,
> > Marcel
> >
> > > > Thanks,
> > > > Marcel
> > > >
> > > > >       k->vendor_id = PCI_VENDOR_ID_REDHAT;
> > > > >       k->device_id = PCI_DEVICE_ID_REDHAT_PXB;
> > > >
> > > >
> > > >
> > > > .
> >
> >
> >
> > .