[v2] qdev/core: Can not replug device on bus that allows one device

[Qemu-devel] [PATCH v2] qdev/core: Can not replug device on bus that allows one device

Posted by Tony Krowiak 5 years, 4 months ago

If the maximum number of devices allowed on a bus is 1 and a device
which is plugged into the bus is subsequently unplugged, attempting to replug
the device fails with error "Bus 'xxx' does not support hotplugging".
The "error" is detected in the qbus_is_full(BusState *bus) function
(qdev_monitor.c) because bus->max_index >= bus_class->max_dev. The
root of the problem is that the bus->max_index is not decremented when a device
is unplugged from the bus. This patch fixes that problem.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 hw/core/qdev.c         | 3 +++
 include/hw/qdev-core.h | 1 +
 qdev-monitor.c         | 2 +-
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 6b3cc55b27c2..956923f33520 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -59,6 +59,8 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
             snprintf(name, sizeof(name), "child[%d]", kid->index);
             QTAILQ_REMOVE(&bus->children, kid, sibling);
 
+            bus->num_children--;
+
             /* This gives back ownership of kid->child back to us.  */
             object_property_del(OBJECT(bus), name, NULL);
             object_unref(OBJECT(kid->child));
@@ -73,6 +75,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
     char name[32];
     BusChild *kid = g_malloc0(sizeof(*kid));
 
+    bus->num_children++;
     kid->index = bus->max_index++;
     kid->child = child;
     object_ref(OBJECT(kid->child));
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index a24d0dd566e3..521f0a947ead 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -206,6 +206,7 @@ struct BusState {
     HotplugHandler *hotplug_handler;
     int max_index;
     bool realized;
+    int num_children;
     QTAILQ_HEAD(ChildrenHead, BusChild) children;
     QLIST_ENTRY(BusState) sibling;
 };
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 07147c63bf8b..45a8ba49644c 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -414,7 +414,7 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem)
 static inline bool qbus_is_full(BusState *bus)
 {
     BusClass *bus_class = BUS_GET_CLASS(bus);
-    return bus_class->max_dev && bus->max_index >= bus_class->max_dev;
+    return bus_class->max_dev && bus->num_children >= bus_class->max_dev;
 }
 
 /*
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2] qdev/core: Can not replug device on bus that allows one device

Posted by Pierre Morel 5 years, 4 months ago

On 13/12/2018 17:26, Tony Krowiak wrote:
> If the maximum number of devices allowed on a bus is 1 and a device
> which is plugged into the bus is subsequently unplugged, attempting to replug
> the device fails with error "Bus 'xxx' does not support hotplugging".
> The "error" is detected in the qbus_is_full(BusState *bus) function
> (qdev_monitor.c) because bus->max_index >= bus_class->max_dev. The
> root of the problem is that the bus->max_index is not decremented when a device
> is unplugged from the bus. This patch fixes that problem.



I think that the root of the problem is that the max_index is not the 
right measure to determine the number of devices currently plugged.

With the comment amended in this direction:

Reviewed-by: Pierre Morel<pmorel@linux.ibm.com>

(resent after address correction)



> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>   hw/core/qdev.c         | 3 +++
>   include/hw/qdev-core.h | 1 +
>   qdev-monitor.c         | 2 +-
>   3 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 6b3cc55b27c2..956923f33520 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -59,6 +59,8 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
>               snprintf(name, sizeof(name), "child[%d]", kid->index);
>               QTAILQ_REMOVE(&bus->children, kid, sibling);
> 
> +            bus->num_children--;
> +
>               /* This gives back ownership of kid->child back to us.  */
>               object_property_del(OBJECT(bus), name, NULL);
>               object_unref(OBJECT(kid->child));
> @@ -73,6 +75,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
>       char name[32];
>       BusChild *kid = g_malloc0(sizeof(*kid));
> 
> +    bus->num_children++;
>       kid->index = bus->max_index++;
>       kid->child = child;
>       object_ref(OBJECT(kid->child));
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index a24d0dd566e3..521f0a947ead 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -206,6 +206,7 @@ struct BusState {
>       HotplugHandler *hotplug_handler;
>       int max_index;
>       bool realized;
> +    int num_children;
>       QTAILQ_HEAD(ChildrenHead, BusChild) children;
>       QLIST_ENTRY(BusState) sibling;
>   };
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 07147c63bf8b..45a8ba49644c 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -414,7 +414,7 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem)
>   static inline bool qbus_is_full(BusState *bus)
>   {
>       BusClass *bus_class = BUS_GET_CLASS(bus);
> -    return bus_class->max_dev && bus->max_index >= bus_class->max_dev;
> +    return bus_class->max_dev && bus->num_children >= bus_class->max_dev;
>   }
> 
>   /*
> 


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v2] qdev/core: Can not replug device on bus that allows one device

Posted by Halil Pasic 5 years, 4 months ago

On Thu, 13 Dec 2018 11:26:42 -0500
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> If the maximum number of devices allowed on a bus is 1 and a device
> which is plugged into the bus is subsequently unplugged, attempting to replug
> the device fails with error "Bus 'xxx' does not support hotplugging".
> The "error" is detected in the qbus_is_full(BusState *bus) function
> (qdev_monitor.c) because bus->max_index >= bus_class->max_dev. The
> root of the problem is that the bus->max_index is not decremented when a device
> is unplugged from the bus. This patch fixes that problem.
> 

As Pierre has pointed out, the commit message is stale and inaccurate.
Subject could be better as well. I mean the problem is not limited to
buses that allow only one device. Each bus that can get full looses
capacity with every plug/unplug op pair. With that fixed:

Reviewed-by: Halil Pasic <pasic@linux.ibm.com>

> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>  hw/core/qdev.c         | 3 +++
>  include/hw/qdev-core.h | 1 +
>  qdev-monitor.c         | 2 +-
>  3 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 6b3cc55b27c2..956923f33520 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -59,6 +59,8 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
>              snprintf(name, sizeof(name), "child[%d]", kid->index);
>              QTAILQ_REMOVE(&bus->children, kid, sibling);
>  
> +            bus->num_children--;
> +
>              /* This gives back ownership of kid->child back to us.  */
>              object_property_del(OBJECT(bus), name, NULL);
>              object_unref(OBJECT(kid->child));
> @@ -73,6 +75,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
>      char name[32];
>      BusChild *kid = g_malloc0(sizeof(*kid));
>  
> +    bus->num_children++;
>      kid->index = bus->max_index++;
>      kid->child = child;
>      object_ref(OBJECT(kid->child));
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index a24d0dd566e3..521f0a947ead 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -206,6 +206,7 @@ struct BusState {
>      HotplugHandler *hotplug_handler;
>      int max_index;
>      bool realized;
> +    int num_children;
>      QTAILQ_HEAD(ChildrenHead, BusChild) children;
>      QLIST_ENTRY(BusState) sibling;
>  };
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 07147c63bf8b..45a8ba49644c 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -414,7 +414,7 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem)
>  static inline bool qbus_is_full(BusState *bus)
>  {
>      BusClass *bus_class = BUS_GET_CLASS(bus);
> -    return bus_class->max_dev && bus->max_index >= bus_class->max_dev;
> +    return bus_class->max_dev && bus->num_children >= bus_class->max_dev;
>  }
>  
>  /*

Re: [Qemu-devel] [PATCH v2] qdev/core: Can not replug device on bus that allows one device

Posted by Tony Krowiak 5 years, 4 months ago

On 12/14/18 7:09 AM, Halil Pasic wrote:
> On Thu, 13 Dec 2018 11:26:42 -0500
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> 
>> If the maximum number of devices allowed on a bus is 1 and a device
>> which is plugged into the bus is subsequently unplugged, attempting to replug
>> the device fails with error "Bus 'xxx' does not support hotplugging".
>> The "error" is detected in the qbus_is_full(BusState *bus) function
>> (qdev_monitor.c) because bus->max_index >= bus_class->max_dev. The
>> root of the problem is that the bus->max_index is not decremented when a device
>> is unplugged from the bus. This patch fixes that problem.
>>
> 
> As Pierre has pointed out, the commit message is stale and inaccurate.
> Subject could be better as well. I mean the problem is not limited to
> buses that allow only one device. Each bus that can get full looses
> capacity with every plug/unplug op pair. With that fixed:
> 
> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>

I'll make the changes recommended by you and Pierre.

> 
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> ---
>>   hw/core/qdev.c         | 3 +++
>>   include/hw/qdev-core.h | 1 +
>>   qdev-monitor.c         | 2 +-
>>   3 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 6b3cc55b27c2..956923f33520 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -59,6 +59,8 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
>>               snprintf(name, sizeof(name), "child[%d]", kid->index);
>>               QTAILQ_REMOVE(&bus->children, kid, sibling);
>>   
>> +            bus->num_children--;
>> +
>>               /* This gives back ownership of kid->child back to us.  */
>>               object_property_del(OBJECT(bus), name, NULL);
>>               object_unref(OBJECT(kid->child));
>> @@ -73,6 +75,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
>>       char name[32];
>>       BusChild *kid = g_malloc0(sizeof(*kid));
>>   
>> +    bus->num_children++;
>>       kid->index = bus->max_index++;
>>       kid->child = child;
>>       object_ref(OBJECT(kid->child));
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index a24d0dd566e3..521f0a947ead 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -206,6 +206,7 @@ struct BusState {
>>       HotplugHandler *hotplug_handler;
>>       int max_index;
>>       bool realized;
>> +    int num_children;
>>       QTAILQ_HEAD(ChildrenHead, BusChild) children;
>>       QLIST_ENTRY(BusState) sibling;
>>   };
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 07147c63bf8b..45a8ba49644c 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -414,7 +414,7 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem)
>>   static inline bool qbus_is_full(BusState *bus)
>>   {
>>       BusClass *bus_class = BUS_GET_CLASS(bus);
>> -    return bus_class->max_dev && bus->max_index >= bus_class->max_dev;
>> +    return bus_class->max_dev && bus->num_children >= bus_class->max_dev;
>>   }
>>   
>>   /*
>

Re: [Qemu-devel] [PATCH v2] qdev/core: Can not replug device on bus that allows one device

Posted by Pierre Morel 5 years, 4 months ago

On 13/12/2018 17:26, Tony Krowiak wrote:
> If the maximum number of devices allowed on a bus is 1 and a device
> which is plugged into the bus is subsequently unplugged, attempting to replug
> the device fails with error "Bus 'xxx' does not support hotplugging".
> The "error" is detected in the qbus_is_full(BusState *bus) function
> (qdev_monitor.c) because bus->max_index >= bus_class->max_dev. The
> root of the problem is that the bus->max_index is not decremented when a device
> is unplugged from the bus. This patch fixes that problem.

I think that the root of the problem is that the max_index is not the 
right measure to determine the number of devices currently plugged.

With the comment amended in this direction:

Reviewed-by: Pierre Morel<pmorel@linux.ibm.com>


> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>   hw/core/qdev.c         | 3 +++
>   include/hw/qdev-core.h | 1 +
>   qdev-monitor.c         | 2 +-
>   3 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 6b3cc55b27c2..956923f33520 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -59,6 +59,8 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
>               snprintf(name, sizeof(name), "child[%d]", kid->index);
>               QTAILQ_REMOVE(&bus->children, kid, sibling);
> 
> +            bus->num_children--;
> +
>               /* This gives back ownership of kid->child back to us.  */
>               object_property_del(OBJECT(bus), name, NULL);
>               object_unref(OBJECT(kid->child));
> @@ -73,6 +75,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
>       char name[32];
>       BusChild *kid = g_malloc0(sizeof(*kid));
> 
> +    bus->num_children++;
>       kid->index = bus->max_index++;
>       kid->child = child;
>       object_ref(OBJECT(kid->child));
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index a24d0dd566e3..521f0a947ead 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -206,6 +206,7 @@ struct BusState {
>       HotplugHandler *hotplug_handler;
>       int max_index;
>       bool realized;
> +    int num_children;
>       QTAILQ_HEAD(ChildrenHead, BusChild) children;
>       QLIST_ENTRY(BusState) sibling;
>   };
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 07147c63bf8b..45a8ba49644c 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -414,7 +414,7 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem)
>   static inline bool qbus_is_full(BusState *bus)
>   {
>       BusClass *bus_class = BUS_GET_CLASS(bus);
> -    return bus_class->max_dev && bus->max_index >= bus_class->max_dev;
> +    return bus_class->max_dev && bus->num_children >= bus_class->max_dev;
>   }
> 
>   /*
> 


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v2] qdev/core: Can not replug device on bus that allows one device

Posted by Tony Krowiak 5 years, 4 months ago

On 12/14/18 4:16 AM, Pierre Morel wrote:
> On 13/12/2018 17:26, Tony Krowiak wrote:
>> If the maximum number of devices allowed on a bus is 1 and a device
>> which is plugged into the bus is subsequently unplugged, attempting to 
>> replug
>> the device fails with error "Bus 'xxx' does not support hotplugging".
>> The "error" is detected in the qbus_is_full(BusState *bus) function
>> (qdev_monitor.c) because bus->max_index >= bus_class->max_dev. The
>> root of the problem is that the bus->max_index is not decremented when 
>> a device
>> is unplugged from the bus. This patch fixes that problem.
> 
> I think that the root of the problem is that the max_index is not the 
> right measure to determine the number of devices currently plugged.
> 
> With the comment amended in this direction:

Good point, I should have changed that when I refactored this fix.

> 
> Reviewed-by: Pierre Morel<pmorel@linux.ibm.com>
> 
> 
>>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> ---
>>   hw/core/qdev.c         | 3 +++
>>   include/hw/qdev-core.h | 1 +
>>   qdev-monitor.c         | 2 +-
>>   3 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 6b3cc55b27c2..956923f33520 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -59,6 +59,8 @@ static void bus_remove_child(BusState *bus, 
>> DeviceState *child)
>>               snprintf(name, sizeof(name), "child[%d]", kid->index);
>>               QTAILQ_REMOVE(&bus->children, kid, sibling);
>>
>> +            bus->num_children--;
>> +
>>               /* This gives back ownership of kid->child back to us.  */
>>               object_property_del(OBJECT(bus), name, NULL);
>>               object_unref(OBJECT(kid->child));
>> @@ -73,6 +75,7 @@ static void bus_add_child(BusState *bus, DeviceState 
>> *child)
>>       char name[32];
>>       BusChild *kid = g_malloc0(sizeof(*kid));
>>
>> +    bus->num_children++;
>>       kid->index = bus->max_index++;
>>       kid->child = child;
>>       object_ref(OBJECT(kid->child));
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index a24d0dd566e3..521f0a947ead 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -206,6 +206,7 @@ struct BusState {
>>       HotplugHandler *hotplug_handler;
>>       int max_index;
>>       bool realized;
>> +    int num_children;
>>       QTAILQ_HEAD(ChildrenHead, BusChild) children;
>>       QLIST_ENTRY(BusState) sibling;
>>   };
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 07147c63bf8b..45a8ba49644c 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -414,7 +414,7 @@ static DeviceState *qbus_find_dev(BusState *bus, 
>> char *elem)
>>   static inline bool qbus_is_full(BusState *bus)
>>   {
>>       BusClass *bus_class = BUS_GET_CLASS(bus);
>> -    return bus_class->max_dev && bus->max_index >= bus_class->max_dev;
>> +    return bus_class->max_dev && bus->num_children >= 
>> bus_class->max_dev;
>>   }
>>
>>   /*
>>
> 
>