| Message ID | 20201020091024.320381-1-luc@lmichel.fr |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] hw/core/qdev-clock: add a reference on aliased clocks | expand |
Patchew URL: https://patchew.org/QEMU/20201020091024.320381-1-luc@lmichel.fr/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20201020091024.320381-1-luc@lmichel.fr
Subject: [PATCH v2] hw/core/qdev-clock: add a reference on aliased clocks
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
error: RPC failed; result=7, HTTP code = 0
fatal: The remote end hung up unexpectedly
error: Could not fetch 3c8cf5a9c21ff8782164d1def7f44bd888713384
Traceback (most recent call last):
File "patchew-tester/src/patchew-cli", line 521, in test_one
git_clone_repo(clone, r["repo"], r["head"], logf, True)
File "patchew-tester/src/patchew-cli", line 48, in git_clone_repo
stdout=logf, stderr=logf)
File "/opt/rh/rh-python36/root/usr/lib64/python3.6/subprocess.py", line 291, in check_call
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['git', 'remote', 'add', '-f', '--mirror=fetch', '3c8cf5a9c21ff8782164d1def7f44bd888713384', 'https://github.com/patchew-project/qemu']' returned non-zero exit status 1.
The full log is available at
http://patchew.org/logs/20201020091024.320381-1-luc@lmichel.fr/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Hi On Tue, Oct 20, 2020 at 1:11 PM Luc Michel <luc@lmichel.fr> wrote: > When aliasing a clock with the qdev_alias_clock() function, a new link > property is created on the device aliasing the clock. The link points > to the aliased clock and use the OBJ_PROP_LINK_STRONG flag. This > property is read only since it does not provide a check callback for > modifications. > > The object_property_add_link() documentation stats that with > OBJ_PROP_LINK_STRONG properties, the linked object reference count get > decremented when the property is deleted. But it is _not_ incremented on > creation (object_property_add_link() does not actually know the link). > > This commit increments the reference count on the aliased clock to > ensure the aliased clock stays alive during the property lifetime, and > to avoid a double-free memory error when the property gets deleted. > > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > Signed-off-by: Luc Michel <luc@lmichel.fr> > --- > In principle, that makes sense. But I don't see any users of that API yet. It would have been nice to have some unit tests for qdev-clock.h.. hw/core/qdev-clock.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c > index 6a9a340d0f..eb05f2a13c 100644 > --- a/hw/core/qdev-clock.c > +++ b/hw/core/qdev-clock.c > @@ -59,10 +59,18 @@ static NamedClockList *qdev_init_clocklist(DeviceState > *dev, const char *name, > } else { > object_property_add_link(OBJECT(dev), name, > object_get_typename(OBJECT(clk)), > (Object **) &ncl->clock, > NULL, OBJ_PROP_LINK_STRONG); > + /* > + * Since the link property has the OBJ_PROP_LINK_STRONG flag, the > clk > + * object reference count gets decremented on property deletion. > + * However object_property_add_link does not increment it since it > + * doesn't know the linked object. Increment it here to ensure the > + * aliased clock stays alive during this device life-time. > + */ > + object_ref(OBJECT(clk)); > } > > ncl->clock = clk; > > QLIST_INSERT_HEAD(&dev->clocks, ncl, node); > -- > 2.28.0 > > > -- Marc-André Lureau <div dir="ltr"><div dir="ltr">Hi<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Oct 20, 2020 at 1:11 PM Luc Michel <<a href="mailto:luc@lmichel.fr">luc@lmichel.fr</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">When aliasing a clock with the qdev_alias_clock() function, a new link<br> property is created on the device aliasing the clock. The link points<br> to the aliased clock and use the OBJ_PROP_LINK_STRONG flag. This<br> property is read only since it does not provide a check callback for<br> modifications.<br> <br> The object_property_add_link() documentation stats that with<br> OBJ_PROP_LINK_STRONG properties, the linked object reference count get<br> decremented when the property is deleted. But it is _not_ incremented on<br> creation (object_property_add_link() does not actually know the link).<br> <br> This commit increments the reference count on the aliased clock to<br> ensure the aliased clock stays alive during the property lifetime, and<br> to avoid a double-free memory error when the property gets deleted.<br> <br> Reviewed-by: Philippe Mathieu-Daudé <<a href="mailto:f4bug@amsat.org" target="_blank">f4bug@amsat.org</a>><br> Signed-off-by: Luc Michel <<a href="mailto:luc@lmichel.fr" target="_blank">luc@lmichel.fr</a>><br> ---<br></blockquote><div><br></div><div>In principle, that makes sense. But I don't see any users of that API yet.</div><div><br></div><div>It would have been nice to have some unit tests for qdev-clock.h..</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> hw/core/qdev-clock.c | 8 ++++++++<br> 1 file changed, 8 insertions(+)<br> <br> diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c<br> index 6a9a340d0f..eb05f2a13c 100644<br> --- a/hw/core/qdev-clock.c<br> +++ b/hw/core/qdev-clock.c<br> @@ -59,10 +59,18 @@ static NamedClockList *qdev_init_clocklist(DeviceState *dev, const char *name,<br> } else {<br> object_property_add_link(OBJECT(dev), name,<br> object_get_typename(OBJECT(clk)),<br> (Object **) &ncl->clock,<br> NULL, OBJ_PROP_LINK_STRONG);<br> + /*<br> + * Since the link property has the OBJ_PROP_LINK_STRONG flag, the clk<br> + * object reference count gets decremented on property deletion.<br> + * However object_property_add_link does not increment it since it<br> + * doesn't know the linked object. Increment it here to ensure the<br> + * aliased clock stays alive during this device life-time.<br> + */<br> + object_ref(OBJECT(clk));<br> }<br> <br> ncl->clock = clk;<br> <br> QLIST_INSERT_HEAD(&dev->clocks, ncl, node);<br> -- <br> 2.28.0<br> <br> <br> </blockquote></div><br clear="all"><br>-- <br><div dir="ltr" class="gmail_signature">Marc-André Lureau<br></div></div>
On 15:23 Fri 23 Oct , Marc-André Lureau wrote: > Hi > > On Tue, Oct 20, 2020 at 1:11 PM Luc Michel <luc@lmichel.fr> wrote: > > > When aliasing a clock with the qdev_alias_clock() function, a new link > > property is created on the device aliasing the clock. The link points > > to the aliased clock and use the OBJ_PROP_LINK_STRONG flag. This > > property is read only since it does not provide a check callback for > > modifications. > > > > The object_property_add_link() documentation stats that with > > OBJ_PROP_LINK_STRONG properties, the linked object reference count get > > decremented when the property is deleted. But it is _not_ incremented on > > creation (object_property_add_link() does not actually know the link). > > > > This commit increments the reference count on the aliased clock to > > ensure the aliased clock stays alive during the property lifetime, and > > to avoid a double-free memory error when the property gets deleted. > > > > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > Signed-off-by: Luc Michel <luc@lmichel.fr> > > --- > > > > In principle, that makes sense. But I don't see any users of that API yet. Yes, Peter encountered a double-free error because of this missing object_ref when he applied my Raspberry PI CPRMAN series, which makes use of qdev_alias_clock. Peter: do you consider taking this patch to be able to pick up the CPRMAN series again? > > It would have been nice to have some unit tests for qdev-clock.h.. Yes I agree. -- Luc > > hw/core/qdev-clock.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c > > index 6a9a340d0f..eb05f2a13c 100644 > > --- a/hw/core/qdev-clock.c > > +++ b/hw/core/qdev-clock.c > > @@ -59,10 +59,18 @@ static NamedClockList *qdev_init_clocklist(DeviceState > > *dev, const char *name, > > } else { > > object_property_add_link(OBJECT(dev), name, > > object_get_typename(OBJECT(clk)), > > (Object **) &ncl->clock, > > NULL, OBJ_PROP_LINK_STRONG); > > + /* > > + * Since the link property has the OBJ_PROP_LINK_STRONG flag, the > > clk > > + * object reference count gets decremented on property deletion. > > + * However object_property_add_link does not increment it since it > > + * doesn't know the linked object. Increment it here to ensure the > > + * aliased clock stays alive during this device life-time. > > + */ > > + object_ref(OBJECT(clk)); > > } > > > > ncl->clock = clk; > > > > QLIST_INSERT_HEAD(&dev->clocks, ncl, node); > > -- > > 2.28.0 > > > > > > > > -- > Marc-André Lureau --
diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c index 6a9a340d0f..eb05f2a13c 100644 --- a/hw/core/qdev-clock.c +++ b/hw/core/qdev-clock.c @@ -59,10 +59,18 @@ static NamedClockList *qdev_init_clocklist(DeviceState *dev, const char *name, } else { object_property_add_link(OBJECT(dev), name, object_get_typename(OBJECT(clk)), (Object **) &ncl->clock, NULL, OBJ_PROP_LINK_STRONG); + /* + * Since the link property has the OBJ_PROP_LINK_STRONG flag, the clk + * object reference count gets decremented on property deletion. + * However object_property_add_link does not increment it since it + * doesn't know the linked object. Increment it here to ensure the + * aliased clock stays alive during this device life-time. + */ + object_ref(OBJECT(clk)); } ncl->clock = clk; QLIST_INSERT_HEAD(&dev->clocks, ncl, node);