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

[Marcel Apfelbaum <marcel@redhat.com>]

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;
>>>>>
>>>>
>>>>
>>>>
>>>> .
>>>>
>>>
>>
>>
>>
>> .
>>
>