Re: [PATCH 1/7] spapr/pci: Correct "does not support hotplugging error messages

[Markus Armbruster]

BALATON Zoltan <balaton@eik.bme.hu> writes:

> On Wed, 1 Nov 2023, Daniel Henrique Barboza wrote:
>> On 10/31/23 08:10, Markus Armbruster wrote:
>>> When dynamic-reconfiguration is off, hot plug / unplug can fail with
>>> "Bus 'spapr-pci-host-bridge' does not support hotplugging".
>>> spapr-pci-host-bridge is a device, not a bus.  Report the name of the
>>> bus it provides instead: 'pci.0'.
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>
>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>
>> Feel free to queue it up. Thanks,
>>
>>
>> Daniel
>>
>>>   hw/ppc/spapr_pci.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>>> index 370c5a90f2..ebb32ad90b 100644
>>> --- a/hw/ppc/spapr_pci.c
>>> +++ b/hw/ppc/spapr_pci.c
>>> @@ -1551,7 +1551,7 @@ static void spapr_pci_pre_plug(HotplugHandler 
>>> *plug_handler,
>>>            */
>>>           if (plugged_dev->hotplugged) {
>>>               error_setg(errp, QERR_BUS_NO_HOTPLUG,
>>> -                       object_get_typename(OBJECT(phb)));
>>> +                       phb->parent_obj.bus->qbus.name);
>
> I could not find it mentioned in the docs but it was said the parent pointer 
> is private and one should not access it but cast to the parent object 
> instead. Or here may even use pci_get_bus(pdev) maybe after moving the 
> asserts before it to make sure the device is valid. But I don't mind so you 
> can commit it as it is if nobody notices.

pci_get_bus() returns the bus the device is plugged into as a PCI bus.
We need the bus the device provides.  Besides, @phb is plugged into the
main system bus.  pci_get_bus(PCI_DEVICE(phb)) would pass the main
system bus to PCI_BUS(), which is not good.

I can offer

                            PCI_HOST_BRIDGE(phb)->bus->qbus.name);

Looks like a wash to me, but if maintainers like it better, I'll change
to it.

>
> Regards,
> BALATON Zoltan
>
>>>               return;
>>>           }
>>>       }
>>> @@ -1672,7 +1672,7 @@ static void spapr_pci_unplug_request(HotplugHandler 
>>> *plug_handler,
>>>         if (!phb->dr_enabled) {
>>>           error_setg(errp, QERR_BUS_NO_HOTPLUG,
>>> -                   object_get_typename(OBJECT(phb)));
>>> +                   phb->parent_obj.bus->qbus.name);
>>>           return;
>>>       }
>>> 
>>
>>