diff mbox

[for-2.4] watchdog/diag288: correctly register for system reset requests

Message ID 20150708123140.65e40003.cornelia.huck@de.ibm.com
State New
Headers show

Commit Message

Cornelia Huck July 8, 2015, 10:31 a.m. UTC 
On Wed, 8 Jul 2015 09:45:05 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Tue, 7 Jul 2015 14:11:18 -0700
> Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:

> > Yes I see. I think it is a core code bug though and we want to avoid
> > having to patch individual devs based on their system level
> > connectivity. I'm looking at qbus_realize, and there, there is code to
> > register a reset for orphaned busses. So we have precedent for lazily
> > setting up a reset for an orphaned bus at realize time, just not for
> > indiv. devs. We can do the same.
> > 
> > I think this can be added to device_set_realized(). If a devices
> > parent is not a bus, then register its reset individually to catch-all
> > these. 
> 
> Solving this in the core sounds good, but do you already have some kind
> of patch ready? :) As we're pretty late in the cycle, it might make
> sense to merge the existing fix for diag288 first, and switch to a
> generic solution later on.

OTOH, this is less code than I expected. With the following code, I see
the diag288 reset callback called on system reset. If this looks good,
I can resend as a proper patch; we can reduce Xu's patch to the
io_subsystem_reset() part in that case. Opinions?

Comments

Cornelia Huck July 9, 2015, 12:41 p.m. UTC | #1 
On Wed, 8 Jul 2015 12:31:40 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Wed, 8 Jul 2015 09:45:05 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> 
> > On Tue, 7 Jul 2015 14:11:18 -0700
> > Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> 
> > > Yes I see. I think it is a core code bug though and we want to avoid
> > > having to patch individual devs based on their system level
> > > connectivity. I'm looking at qbus_realize, and there, there is code to
> > > register a reset for orphaned busses. So we have precedent for lazily
> > > setting up a reset for an orphaned bus at realize time, just not for
> > > indiv. devs. We can do the same.
> > > 
> > > I think this can be added to device_set_realized(). If a devices
> > > parent is not a bus, then register its reset individually to catch-all
> > > these. 
> > 
> > Solving this in the core sounds good, but do you already have some kind
> > of patch ready? :) As we're pretty late in the cycle, it might make
> > sense to merge the existing fix for diag288 first, and switch to a
> > generic solution later on.
> 
> OTOH, this is less code than I expected. With the following code, I see
> the diag288 reset callback called on system reset. If this looks good,
> I can resend as a proper patch; we can reduce Xu's patch to the
> io_subsystem_reset() part in that case. Opinions?

Ping? Does this make sense?

> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index b2f404a..5c7c27b 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -1018,6 +1018,13 @@ static bool device_get_realized(Object *obj, Error **errp)
>      return dev->realized;
>  }
>  
> +static void do_device_reset(void *opaque)
> +{
> +    DeviceState *dev = opaque;
> +
> +    device_reset(dev);
> +}
> +
>  static void device_set_realized(Object *obj, bool value, Error **errp)
>  {
>      DeviceState *dev = DEVICE(obj);
> @@ -1061,6 +1068,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>              goto post_realize_fail;
>          }
>  
> +        if (!dev->parent_bus) {
> +            /* Make sure that reset is called for bus-less devices. */
> +            qemu_register_reset(do_device_reset, dev);
> +        }
> +
>          if (qdev_get_vmsd(dev)) {
>              vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
>                                             dev->instance_id_alias,
> @@ -1094,6 +1106,9 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>          }
>          dev->pending_deleted_event = true;
>          DEVICE_LISTENER_CALL(unrealize, Reverse, dev);
> +        if (!dev->parent_bus) {
> +            qemu_unregister_reset(do_device_reset, dev);
> +        }
>      }
>  
>      if (local_err != NULL) {
Peter Crosthwaite July 9, 2015, 4:16 p.m. UTC | #2 
On Thu, Jul 9, 2015 at 5:41 AM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> On Wed, 8 Jul 2015 12:31:40 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>
>> On Wed, 8 Jul 2015 09:45:05 +0200
>> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>>
>> > On Tue, 7 Jul 2015 14:11:18 -0700
>> > Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>>
>> > > Yes I see. I think it is a core code bug though and we want to avoid
>> > > having to patch individual devs based on their system level
>> > > connectivity. I'm looking at qbus_realize, and there, there is code to
>> > > register a reset for orphaned busses. So we have precedent for lazily
>> > > setting up a reset for an orphaned bus at realize time, just not for
>> > > indiv. devs. We can do the same.
>> > >
>> > > I think this can be added to device_set_realized(). If a devices
>> > > parent is not a bus, then register its reset individually to catch-all
>> > > these.
>> >
>> > Solving this in the core sounds good, but do you already have some kind
>> > of patch ready? :) As we're pretty late in the cycle, it might make
>> > sense to merge the existing fix for diag288 first, and switch to a
>> > generic solution later on.
>>
>> OTOH, this is less code than I expected. With the following code, I see
>> the diag288 reset callback called on system reset. If this looks good,
>> I can resend as a proper patch; we can reduce Xu's patch to the
>> io_subsystem_reset() part in that case. Opinions?
>

I'm for sending that new core patch, as I'm suspicious I can make it
fail in other places than your diag case. Runtime reset is poorly
exercised code so I think you are going to pickup half-a-dozen
bugfixes here.

Regards,
Peter

> Ping? Does this make sense?
>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index b2f404a..5c7c27b 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -1018,6 +1018,13 @@ static bool device_get_realized(Object *obj, Error **errp)
>>      return dev->realized;
>>  }
>>
>> +static void do_device_reset(void *opaque)
>> +{
>> +    DeviceState *dev = opaque;
>> +
>> +    device_reset(dev);
>> +}
>> +
>>  static void device_set_realized(Object *obj, bool value, Error **errp)
>>  {
>>      DeviceState *dev = DEVICE(obj);
>> @@ -1061,6 +1068,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>>              goto post_realize_fail;
>>          }
>>
>> +        if (!dev->parent_bus) {
>> +            /* Make sure that reset is called for bus-less devices. */
>> +            qemu_register_reset(do_device_reset, dev);
>> +        }
>> +
>>          if (qdev_get_vmsd(dev)) {
>>              vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
>>                                             dev->instance_id_alias,
>> @@ -1094,6 +1106,9 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>>          }
>>          dev->pending_deleted_event = true;
>>          DEVICE_LISTENER_CALL(unrealize, Reverse, dev);
>> +        if (!dev->parent_bus) {
>> +            qemu_unregister_reset(do_device_reset, dev);
>> +        }
>>      }
>>
>>      if (local_err != NULL) {
>
>
Peter Maydell July 14, 2015, 1:51 p.m. UTC | #3 
On 9 July 2015 at 17:16, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> On Thu, Jul 9, 2015 at 5:41 AM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>> On Wed, 8 Jul 2015 12:31:40 +0200
>> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>>> OTOH, this is less code than I expected. With the following code, I see
>>> the diag288 reset callback called on system reset. If this looks good,
>>> I can resend as a proper patch; we can reduce Xu's patch to the
>>> io_subsystem_reset() part in that case. Opinions?
>>
>
> I'm for sending that new core patch, as I'm suspicious I can make it
> fail in other places than your diag case. Runtime reset is poorly
> exercised code so I think you are going to pickup half-a-dozen
> bugfixes here.

FWIW this patch would mean we would try to call dc->reset on
the ARM CPU devices; the only reason this doesn't cause a
problem is that those devices don't happen to register a
dc->reset function pointer. (It's this sort of "now we start
calling a reset method on a device that was probably doing its
own reset via another path" that meant I wasn't too keen on it
for 2.4, though quite possibly it would work out better than
the ad-hoc reset we have currently in the longer term...)

-- PMM
diff mbox

Patch

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index b2f404a..5c7c27b 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1018,6 +1018,13 @@  static bool device_get_realized(Object *obj, Error **errp)
     return dev->realized;
 }
 
+static void do_device_reset(void *opaque)
+{
+    DeviceState *dev = opaque;
+
+    device_reset(dev);
+}
+
 static void device_set_realized(Object *obj, bool value, Error **errp)
 {
     DeviceState *dev = DEVICE(obj);
@@ -1061,6 +1068,11 @@  static void device_set_realized(Object *obj, bool value, Error **errp)
             goto post_realize_fail;
         }
 
+        if (!dev->parent_bus) {
+            /* Make sure that reset is called for bus-less devices. */
+            qemu_register_reset(do_device_reset, dev);
+        }
+
         if (qdev_get_vmsd(dev)) {
             vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
                                            dev->instance_id_alias,
@@ -1094,6 +1106,9 @@  static void device_set_realized(Object *obj, bool value, Error **errp)
         }
         dev->pending_deleted_event = true;
         DEVICE_LISTENER_CALL(unrealize, Reverse, dev);
+        if (!dev->parent_bus) {
+            qemu_unregister_reset(do_device_reset, dev);
+        }
     }
 
     if (local_err != NULL) {