[Qemu-devel] [PATCH] watchdog: 6300esb: add exit function

[Qemu-devel] [PATCH] watchdog: 6300esb: add exit function

[Li Qiang]

From: Li Qiang <liqiang6-s@360.cn>

When the Intel 6300ESB watchdog is hot unplug. The timer allocated
in realize isn't freed thus leaking memory leak. This patch avoid
this through adding the exit function.

Signed-off-by: Li Qiang <liqiang6-s@360.cn>
---
 hw/watchdog/wdt_i6300esb.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
index a83d951..49b3cd1 100644
--- a/hw/watchdog/wdt_i6300esb.c
+++ b/hw/watchdog/wdt_i6300esb.c
@@ -428,6 +428,14 @@ static void i6300esb_realize(PCIDevice *dev, Error **errp)
     /* qemu_register_coalesced_mmio (addr, 0x10); ? */
 }
 
+static void i6300esb_exit(PCIDevice *dev)
+{
+    I6300State *d = WATCHDOG_I6300ESB_DEVICE(dev);
+
+    timer_del(d->timer);
+    timer_free(d->timer);
+}
+
 static WatchdogTimerModel model = {
     .wdt_name = "i6300esb",
     .wdt_description = "Intel 6300ESB",
@@ -441,6 +449,7 @@ static void i6300esb_class_init(ObjectClass *klass, void *data)
     k->config_read = i6300esb_config_read;
     k->config_write = i6300esb_config_write;
     k->realize = i6300esb_realize;
+    k->exit = i6300esb_exit;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = PCI_DEVICE_ID_INTEL_ESB_9;
     k->class_id = PCI_CLASS_SYSTEM_OTHER;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] watchdog: 6300esb: add exit function

[Richard W.M. Jones]

On Mon, Nov 28, 2016 at 05:49:04PM -0800, Li Qiang wrote:
> From: Li Qiang <liqiang6-s@360.cn>
> 
> When the Intel 6300ESB watchdog is hot unplug. The timer allocated
> in realize isn't freed thus leaking memory leak. This patch avoid
> this through adding the exit function.

I will just note that the real hardware is not hot-pluggable.  However
we don't need to stick to the real hardware capabilities, so that's OK.

> Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> ---
>  hw/watchdog/wdt_i6300esb.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
> index a83d951..49b3cd1 100644
> --- a/hw/watchdog/wdt_i6300esb.c
> +++ b/hw/watchdog/wdt_i6300esb.c
> @@ -428,6 +428,14 @@ static void i6300esb_realize(PCIDevice *dev, Error **errp)
>      /* qemu_register_coalesced_mmio (addr, 0x10); ? */
>  }
>  
> +static void i6300esb_exit(PCIDevice *dev)
> +{
> +    I6300State *d = WATCHDOG_I6300ESB_DEVICE(dev);
> +
> +    timer_del(d->timer);
> +    timer_free(d->timer);
> +}
> +
>  static WatchdogTimerModel model = {
>      .wdt_name = "i6300esb",
>      .wdt_description = "Intel 6300ESB",
> @@ -441,6 +449,7 @@ static void i6300esb_class_init(ObjectClass *klass, void *data)
>      k->config_read = i6300esb_config_read;
>      k->config_write = i6300esb_config_write;
>      k->realize = i6300esb_realize;
> +    k->exit = i6300esb_exit;

The wdt_diag288.c file seems to use k->unrealize for this purpose.
I don't know which is correct however.

Rich.

>      k->vendor_id = PCI_VENDOR_ID_INTEL;
>      k->device_id = PCI_DEVICE_ID_INTEL_ESB_9;
>      k->class_id = PCI_CLASS_SYSTEM_OTHER;
> -- 
> 1.8.3.1

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

Re: [Qemu-devel] [PATCH] watchdog: 6300esb: add exit function

[Li Qiang]

Hi

2016-11-29 16:39 GMT+08:00 Richard W.M. Jones <rjones@redhat.com>:

> On Mon, Nov 28, 2016 at 05:49:04PM -0800, Li Qiang wrote:
> > From: Li Qiang <liqiang6-s@360.cn>
> >
> > When the Intel 6300ESB watchdog is hot unplug. The timer allocated
> > in realize isn't freed thus leaking memory leak. This patch avoid
> > this through adding the exit function.
>
> I will just note that the real hardware is not hot-pluggable.  However
> we don't need to stick to the real hardware capabilities, so that's OK.
>
>

If the hardware is not hot-pluggable, we can set dc->hotpluggable = false.


> Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> > ---
> >  hw/watchdog/wdt_i6300esb.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
> > index a83d951..49b3cd1 100644
> > --- a/hw/watchdog/wdt_i6300esb.c
> > +++ b/hw/watchdog/wdt_i6300esb.c
> > @@ -428,6 +428,14 @@ static void i6300esb_realize(PCIDevice *dev, Error
> **errp)
> >      /* qemu_register_coalesced_mmio (addr, 0x10); ? */
> >  }
> >
> > +static void i6300esb_exit(PCIDevice *dev)
> > +{
> > +    I6300State *d = WATCHDOG_I6300ESB_DEVICE(dev);
> > +
> > +    timer_del(d->timer);
> > +    timer_free(d->timer);
> > +}
> > +
> >  static WatchdogTimerModel model = {
> >      .wdt_name = "i6300esb",
> >      .wdt_description = "Intel 6300ESB",
> > @@ -441,6 +449,7 @@ static void i6300esb_class_init(ObjectClass *klass,
> void *data)
> >      k->config_read = i6300esb_config_read;
> >      k->config_write = i6300esb_config_write;
> >      k->realize = i6300esb_realize;
> > +    k->exit = i6300esb_exit;
>
> The wdt_diag288.c file seems to use k->unrealize for this purpose.
> I don't know which is correct however.
>
>
>
wdt_diag288.c uses dc->unrealize for DeviceClass, while k->exit if for
PCIDeviceClass.
I have tested this patch, it's ok.

So we should make it  not hotpluggable or just remain this?

Thanks

Li Qiang

>
> >      k->vendor_id = PCI_VENDOR_ID_INTEL;
> >      k->device_id = PCI_DEVICE_ID_INTEL_ESB_9;
> >      k->class_id = PCI_CLASS_SYSTEM_OTHER;
> > --
> > 1.8.3.1
>
> --
> Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~
> rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> virt-p2v converts physical machines to virtual machines.  Boot with a
> live CD or over the network (PXE) and turn machines into KVM guests.
> http://libguestfs.org/virt-v2v
>

Re: [Qemu-devel] [PATCH] watchdog: 6300esb: add exit function

[Richard W.M. Jones]

On Tue, Nov 29, 2016 at 04:56:55PM +0800, Li Qiang wrote:
> Hi
> 
> 2016-11-29 16:39 GMT+08:00 Richard W.M. Jones <rjones@redhat.com>:
> 
> > On Mon, Nov 28, 2016 at 05:49:04PM -0800, Li Qiang wrote:
> > > From: Li Qiang <liqiang6-s@360.cn>
> > >
> > > When the Intel 6300ESB watchdog is hot unplug. The timer allocated
> > > in realize isn't freed thus leaking memory leak. This patch avoid
> > > this through adding the exit function.
> >
> > I will just note that the real hardware is not hot-pluggable.  However
> > we don't need to stick to the real hardware capabilities, so that's OK.
> >
> >
> 
> If the hardware is not hot-pluggable, we can set dc->hotpluggable = false.

No no no!  The hardware hasn't been made for a decade or two,
we don't need to stick with what the real hardware did.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [Qemu-devel] [PATCH] watchdog: 6300esb: add exit function

[Li Qiang]

2016-11-29 17:00 GMT+08:00 Richard W.M. Jones <rjones@redhat.com>:

> On Tue, Nov 29, 2016 at 04:56:55PM +0800, Li Qiang wrote:
> > Hi
> >
> > 2016-11-29 16:39 GMT+08:00 Richard W.M. Jones <rjones@redhat.com>:
> >
> > > On Mon, Nov 28, 2016 at 05:49:04PM -0800, Li Qiang wrote:
> > > > From: Li Qiang <liqiang6-s@360.cn>
> > > >
> > > > When the Intel 6300ESB watchdog is hot unplug. The timer allocated
> > > > in realize isn't freed thus leaking memory leak. This patch avoid
> > > > this through adding the exit function.
> > >
> > > I will just note that the real hardware is not hot-pluggable.  However
> > > we don't need to stick to the real hardware capabilities, so that's OK.
> > >
> > >
> >
> > If the hardware is not hot-pluggable, we can set dc->hotpluggable =
> false.
>
> No no no!  The hardware hasn't been made for a decade or two,
> we don't need to stick with what the real hardware did.
>
>
OK, then I think this patch is OK.


> Rich.
>
> --
> Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~
> rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> libguestfs lets you edit virtual machines.  Supports shell scripting,
> bindings from many languages.  http://libguestfs.org
>

Re: [Qemu-devel] [PATCH] watchdog: 6300esb: add exit function

[Markus Armbruster]

Li Qiang <liq3ea@gmail.com> writes:

> Hi
>
> 2016-11-29 16:39 GMT+08:00 Richard W.M. Jones <rjones@redhat.com>:
>
>> On Mon, Nov 28, 2016 at 05:49:04PM -0800, Li Qiang wrote:
>> > From: Li Qiang <liqiang6-s@360.cn>
>> >
>> > When the Intel 6300ESB watchdog is hot unplug. The timer allocated
>> > in realize isn't freed thus leaking memory leak. This patch avoid
>> > this through adding the exit function.
>>
>> I will just note that the real hardware is not hot-pluggable.  However
>> we don't need to stick to the real hardware capabilities, so that's OK.
>>
>>
>
> If the hardware is not hot-pluggable, we can set dc->hotpluggable = false.
>
>
>> Signed-off-by: Li Qiang <liqiang6-s@360.cn>
>> > ---
>> >  hw/watchdog/wdt_i6300esb.c | 9 +++++++++
>> >  1 file changed, 9 insertions(+)
>> >
>> > diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
>> > index a83d951..49b3cd1 100644
>> > --- a/hw/watchdog/wdt_i6300esb.c
>> > +++ b/hw/watchdog/wdt_i6300esb.c
>> > @@ -428,6 +428,14 @@ static void i6300esb_realize(PCIDevice *dev, Error
>> **errp)
>> >      /* qemu_register_coalesced_mmio (addr, 0x10); ? */
>> >  }
>> >
>> > +static void i6300esb_exit(PCIDevice *dev)
>> > +{
>> > +    I6300State *d = WATCHDOG_I6300ESB_DEVICE(dev);
>> > +
>> > +    timer_del(d->timer);
>> > +    timer_free(d->timer);
>> > +}
>> > +
>> >  static WatchdogTimerModel model = {
>> >      .wdt_name = "i6300esb",
>> >      .wdt_description = "Intel 6300ESB",
>> > @@ -441,6 +449,7 @@ static void i6300esb_class_init(ObjectClass *klass,
>> void *data)
>> >      k->config_read = i6300esb_config_read;
>> >      k->config_write = i6300esb_config_write;
>> >      k->realize = i6300esb_realize;
>> > +    k->exit = i6300esb_exit;
>>
>> The wdt_diag288.c file seems to use k->unrealize for this purpose.
>> I don't know which is correct however.
>>
>>
>>
> wdt_diag288.c uses dc->unrealize for DeviceClass, while k->exit if for
> PCIDeviceClass.

Method exit() is deprecated, please use unrealize() in new code.

[...]

Re: [Qemu-devel] [PATCH] watchdog: 6300esb: add exit function

[Li Qiang]

2016-11-29 18:49 GMT+08:00 Markus Armbruster <armbru@redhat.com>:

> Li Qiang <liq3ea@gmail.com> writes:
>
> > Hi
> >
> > 2016-11-29 16:39 GMT+08:00 Richard W.M. Jones <rjones@redhat.com>:
> >
> >> On Mon, Nov 28, 2016 at 05:49:04PM -0800, Li Qiang wrote:
> >> > From: Li Qiang <liqiang6-s@360.cn>
> >> >
> >> > When the Intel 6300ESB watchdog is hot unplug. The timer allocated
> >> > in realize isn't freed thus leaking memory leak. This patch avoid
> >> > this through adding the exit function.
> >>
> >> I will just note that the real hardware is not hot-pluggable.  However
> >> we don't need to stick to the real hardware capabilities, so that's OK.
> >>
> >>
> >
> > If the hardware is not hot-pluggable, we can set dc->hotpluggable =
> false.
> >
> >
> >> Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> >> > ---
> >> >  hw/watchdog/wdt_i6300esb.c | 9 +++++++++
> >> >  1 file changed, 9 insertions(+)
> >> >
> >> > diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
> >> > index a83d951..49b3cd1 100644
> >> > --- a/hw/watchdog/wdt_i6300esb.c
> >> > +++ b/hw/watchdog/wdt_i6300esb.c
> >> > @@ -428,6 +428,14 @@ static void i6300esb_realize(PCIDevice *dev,
> Error
> >> **errp)
> >> >      /* qemu_register_coalesced_mmio (addr, 0x10); ? */
> >> >  }
> >> >
> >> > +static void i6300esb_exit(PCIDevice *dev)
> >> > +{
> >> > +    I6300State *d = WATCHDOG_I6300ESB_DEVICE(dev);
> >> > +
> >> > +    timer_del(d->timer);
> >> > +    timer_free(d->timer);
> >> > +}
> >> > +
> >> >  static WatchdogTimerModel model = {
> >> >      .wdt_name = "i6300esb",
> >> >      .wdt_description = "Intel 6300ESB",
> >> > @@ -441,6 +449,7 @@ static void i6300esb_class_init(ObjectClass
> *klass,
> >> void *data)
> >> >      k->config_read = i6300esb_config_read;
> >> >      k->config_write = i6300esb_config_write;
> >> >      k->realize = i6300esb_realize;
> >> > +    k->exit = i6300esb_exit;
> >>
> >> The wdt_diag288.c file seems to use k->unrealize for this purpose.
> >> I don't know which is correct however.
> >>
> >>
> >>
> > wdt_diag288.c uses dc->unrealize for DeviceClass, while k->exit if for
> > PCIDeviceClass.
>
> Method exit() is deprecated, please use unrealize() in new code.
>
> [...]
>

Hello,

IIUC in the PCIDeviceClass definition, there is just an exit member not a
unrealize.
The DeviceClass has an unrealize member.

Thanks.

Re: [Qemu-devel] [PATCH] watchdog: 6300esb: add exit function

[Markus Armbruster]

Li Qiang <liq3ea@gmail.com> writes:

> 2016-11-29 18:49 GMT+08:00 Markus Armbruster <armbru@redhat.com>:
[...]
>> Method exit() is deprecated, please use unrealize() in new code.
>>
>> [...]
>>
>
> Hello,
>
> IIUC in the PCIDeviceClass definition, there is just an exit member not a
> unrealize.
> The DeviceClass has an unrealize member.

You're right.

A less confusing PCIDeviceClass would be nice.