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

Markus Armbruster posted 7 patches 1 year ago
Maintainers: Nicholas Piggin <npiggin@gmail.com>, Daniel Henrique Barboza <danielhb413@gmail.com>, "Cédric Le Goater" <clg@kaod.org>, David Gibson <david@gibson.dropbear.id.au>, Harsh Prateek Bora <harshpb@linux.ibm.com>, "Dr. David Alan Gilbert" <dave@treblig.org>, Jason Wang <jasowang@redhat.com>, Michael Roth <michael.roth@amd.com>, Konstantin Kostiuk <kkostiuk@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, David Hildenbrand <david@redhat.com>, Gerd Hoffmann <kraxel@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>
[PATCH 1/7] spapr/pci: Correct "does not support hotplugging error messages
Posted by Markus Armbruster 1 year ago
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>
---
 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);
             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;
     }
 
-- 
2.41.0
Re: [PATCH 1/7] spapr/pci: Correct "does not support hotplugging error messages
Posted by Daniel Henrique Barboza 1 year ago

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);
>               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;
>       }
>
Re: [PATCH 1/7] spapr/pci: Correct "does not support hotplugging error messages
Posted by BALATON Zoltan 1 year ago
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.

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;
>>       }
>> 
>
>
Re: [PATCH 1/7] spapr/pci: Correct "does not support hotplugging error messages
Posted by Markus Armbruster 1 year ago
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;
>>>       }
>>> 
>>
>>