Patchwork [01/15] qdev: do not reset a device until the parent has been initialized

login
register
mail settings
Submitter Paolo Bonzini
Date Dec. 17, 2012, 4:24 p.m.
Message ID <1355761490-10073-2-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/206909/
State New
Headers show

Comments

Paolo Bonzini - Dec. 17, 2012, 4:24 p.m.
When a device creates a bus and more devices as part of its init callback, the
child device could be reset while the parent is still only partly initialized.
In this case, the right thing to do is to delay resetting the child.  Do not
do it at all in qdev_init, instead use qdev_reset_all to reset already-created
devices when the state goes from CREATED to INITIALIZED.

This happens when hotplugging a usb-storage device.  Without this patch,
initialization of a hotplugged usb-storage device would run in pre-order.
Initialization of a coldplugged usb-storage device would run according to
qdev_reset_all semantics (pre-order right now, post-order later in the
series).  This patch makes things consistent.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Michael S. Tsirkin - Dec. 17, 2012, 4:52 p.m.
On Mon, Dec 17, 2012 at 05:24:36PM +0100, Paolo Bonzini wrote:
> When a device creates a bus and more devices as part of its init callback, the
> child device could be reset while the parent is still only partly initialized.
> In this case, the right thing to do is to delay resetting the child.  Do not
> do it at all in qdev_init, instead use qdev_reset_all to reset already-created
> devices when the state goes from CREATED to INITIALIZED.
> 
> This happens when hotplugging a usb-storage device.  Without this patch,
> initialization of a hotplugged usb-storage device would run in pre-order.
> Initialization of a coldplugged usb-storage device would run according to
> qdev_reset_all semantics (pre-order right now, post-order later in the
> series).

Is this a problem or just not pretty?

>  This patch makes things consistent.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/qdev.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 599382c..2fdf4ac 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -176,8 +176,9 @@ int qdev_init(DeviceState *dev)
>                                         dev->alias_required_for_version);
>      }
>      dev->state = DEV_STATE_INITIALIZED;
> -    if (dev->hotplugged) {
> -        device_reset(dev);
> +    if (dev->hotplugged &&
> +        dev->parent_bus->parent->state == DEV_STATE_INITIALIZED) {
> +        qdev_reset_all(dev);

Let's add a comment explaining the why of this test, and when
will reset happen if it does not trigger here.

>      }
>      return 0;
>  }
> -- 
> 1.8.0.2
>
Michael S. Tsirkin - Dec. 17, 2012, 4:53 p.m.
On Mon, Dec 17, 2012 at 06:52:03PM +0200, Michael S. Tsirkin wrote:
> On Mon, Dec 17, 2012 at 05:24:36PM +0100, Paolo Bonzini wrote:
> > When a device creates a bus and more devices as part of its init callback, the
> > child device could be reset while the parent is still only partly initialized.
> > In this case, the right thing to do is to delay resetting the child.  Do not
> > do it at all in qdev_init, instead use qdev_reset_all to reset already-created
> > devices when the state goes from CREATED to INITIALIZED.
> > 
> > This happens when hotplugging a usb-storage device.  Without this patch,
> > initialization of a hotplugged usb-storage device would run in pre-order.
> > Initialization of a coldplugged usb-storage device would run according to
> > qdev_reset_all semantics (pre-order right now, post-order later in the
> > series).
> 
> Is this a problem or just not pretty?
> 
> >  This patch makes things consistent.
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  hw/qdev.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/qdev.c b/hw/qdev.c
> > index 599382c..2fdf4ac 100644
> > --- a/hw/qdev.c
> > +++ b/hw/qdev.c
> > @@ -176,8 +176,9 @@ int qdev_init(DeviceState *dev)
> >                                         dev->alias_required_for_version);
> >      }
> >      dev->state = DEV_STATE_INITIALIZED;
> > -    if (dev->hotplugged) {
> > -        device_reset(dev);
> > +    if (dev->hotplugged &&
> > +        dev->parent_bus->parent->state == DEV_STATE_INITIALIZED) {
> > +        qdev_reset_all(dev);
> 
> Let's add a comment explaining the why of this test, and when
> will reset happen if it does not trigger here.

Also - shouldn't device init be delayed as well?
What do you think?

> >      }
> >      return 0;
> >  }
> > -- 
> > 1.8.0.2
> >
Paolo Bonzini - Dec. 17, 2012, 5:06 p.m.
Il 17/12/2012 17:53, Michael S. Tsirkin ha scritto:
>>> This happens when hotplugging a usb-storage device.  Without this patch,
>>> initialization of a hotplugged usb-storage device would run in pre-order.
>>> Initialization of a coldplugged usb-storage device would run according to
>>> qdev_reset_all semantics (pre-order right now, post-order later in the
>>> series).
>>
>> Is this a problem or just not pretty?

Not pretty.  The child device doesn't exist until the parent has been
initialized, it doesn't make sense to reset it.

I can add an assertion to make it a problem, of course. :)

>>>  This patch makes things consistent.
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>  hw/qdev.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/qdev.c b/hw/qdev.c
>>> index 599382c..2fdf4ac 100644
>>> --- a/hw/qdev.c
>>> +++ b/hw/qdev.c
>>> @@ -176,8 +176,9 @@ int qdev_init(DeviceState *dev)
>>>                                         dev->alias_required_for_version);
>>>      }
>>>      dev->state = DEV_STATE_INITIALIZED;
>>> -    if (dev->hotplugged) {
>>> -        device_reset(dev);
>>> +    if (dev->hotplugged &&
>>> +        dev->parent_bus->parent->state == DEV_STATE_INITIALIZED) {
>>> +        qdev_reset_all(dev);
>>
>> Let's add a comment explaining the why of this test, and when
>> will reset happen if it does not trigger here.
> 
> Also - shouldn't device init be delayed as well?
> What do you think?

Initialization should run in pre-order (unlike reset): the parent needs
to be ready in order to start the child.  What you typically have, is
that the parent device initializes itself, and then calls
qdev_create/qdev_init on the child before returning.  So it is already
being delayed.  The delay is explicit in program flow, not hidden in
qdev semantics.

Paolo


>>>      }
>>>      return 0;
>>>  }
>>> -- 
>>> 1.8.0.2
>>>
Andreas Färber - Dec. 17, 2012, 9:57 p.m.
Am 17.12.2012 17:24, schrieb Paolo Bonzini:
> When a device creates a bus and more devices as part of its init callback, the
> child device could be reset while the parent is still only partly initialized.
> In this case, the right thing to do is to delay resetting the child.  Do not
> do it at all in qdev_init, instead use qdev_reset_all to reset already-created
> devices when the state goes from CREATED to INITIALIZED.
> 
> This happens when hotplugging a usb-storage device.  Without this patch,
> initialization of a hotplugged usb-storage device would run in pre-order.
> Initialization of a coldplugged usb-storage device would run according to
> qdev_reset_all semantics (pre-order right now, post-order later in the
> series).  This patch makes things consistent.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/qdev.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 599382c..2fdf4ac 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -176,8 +176,9 @@ int qdev_init(DeviceState *dev)
>                                         dev->alias_required_for_version);
>      }
>      dev->state = DEV_STATE_INITIALIZED;
> -    if (dev->hotplugged) {
> -        device_reset(dev);
> +    if (dev->hotplugged &&
> +        dev->parent_bus->parent->state == DEV_STATE_INITIALIZED) {

This assumes that parent_bus != NULL, when we are trying to let our
fallback bus SysBus die out. The CPU in the current proposal is not a
SysBus device, to avoid compiling SysBus unnecessarily into *-user.

Your direct access into the parent's state also conflicts with my QOM
realize series IIUC.

So please at least add appropriate checks to do the below reset only
when there is a bus and leave the old behavior otherwise.

Andreas

> +        qdev_reset_all(dev);
>      }
>      return 0;
>  }
>

Patch

diff --git a/hw/qdev.c b/hw/qdev.c
index 599382c..2fdf4ac 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -176,8 +176,9 @@  int qdev_init(DeviceState *dev)
                                        dev->alias_required_for_version);
     }
     dev->state = DEV_STATE_INITIALIZED;
-    if (dev->hotplugged) {
-        device_reset(dev);
+    if (dev->hotplugged &&
+        dev->parent_bus->parent->state == DEV_STATE_INITIALIZED) {
+        qdev_reset_all(dev);
     }
     return 0;
 }