[Qemu-devel] [PULL 0/3] Usb 20170929 patches

[Qemu-devel] [PULL 0/3] Usb 20170929 patches

[Gerd Hoffmann]

The following changes since commit ab161529261928ae7f3556e3220829c34b2686ec:

  Merge remote-tracking branch 'remotes/dgilbert/tags/pull-migration-20170927a' into staging (2017-09-27 22:44:51 +0100)

are available in the git repository at:

  git://git.kraxel.org/qemu tags/usb-20170929-pull-request

for you to fetch changes up to 13787d59cf86bac230c4d3eec3580110f53ac113:

  usb: Use angle brackets for cacard include directive (2017-09-29 12:28:26 +0200)

----------------------------------------------------------------
usb fixes.

----------------------------------------------------------------

Fam Zheng (1):
  usb: Use angle brackets for cacard include directive

Gerd Hoffmann (1):
  usb: fix libusb config variable name.

Thomas Huth (1):
  hw/usb/bus: Remove bad object_unparent() from usb_try_create_simple()

 hw/usb/bus.c                | 4 +---
 hw/usb/ccid-card-passthru.c | 2 +-
 hw/usb/Makefile.objs        | 2 +-
 3 files changed, 3 insertions(+), 5 deletions(-)

-- 
2.9.3

[Qemu-devel] [PULL 1/3] hw/usb/bus: Remove bad object_unparent() from usb_try_create_simple()

[Gerd Hoffmann]

From: Thomas Huth <thuth@redhat.com>

Valgrind detects an invalid read operation when hot-plugging of an
USB device fails:

$ valgrind x86_64-softmmu/qemu-system-x86_64 -device usb-ehci -nographic -S
==30598== Memcheck, a memory error detector
==30598== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==30598== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==30598== Command: x86_64-softmmu/qemu-system-x86_64 -device usb-ehci -nographic -S
==30598==
QEMU 2.10.50 monitor - type 'help' for more information
(qemu) device_add usb-tablet
(qemu) device_add usb-tablet
(qemu) device_add usb-tablet
(qemu) device_add usb-tablet
(qemu) device_add usb-tablet
(qemu) device_add usb-tablet
==30598== Invalid read of size 8
==30598==    at 0x60EF50: object_unparent (object.c:445)
==30598==    by 0x580F0D: usb_try_create_simple (bus.c:346)
==30598==    by 0x581BEB: usb_claim_port (bus.c:451)
==30598==    by 0x582310: usb_qdev_realize (bus.c:257)
==30598==    by 0x4CB399: device_set_realized (qdev.c:914)
==30598==    by 0x60E26D: property_set_bool (object.c:1886)
==30598==    by 0x61235E: object_property_set_qobject (qom-qobject.c:27)
==30598==    by 0x61000F: object_property_set_bool (object.c:1162)
==30598==    by 0x4567C3: qdev_device_add (qdev-monitor.c:630)
==30598==    by 0x456D52: qmp_device_add (qdev-monitor.c:807)
==30598==    by 0x470A99: hmp_device_add (hmp.c:1933)
==30598==    by 0x3679C3: handle_hmp_command (monitor.c:3123)

The object_unparent() here is not necessary anymore since commit
69382d8b3e8600b3 ("qdev: Fix object reference leak in case device.realize()
fails"), so let's remove it now.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
Message-id: 1506526106-30971-1-git-send-email-thuth@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/bus.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index d910f849e7..e56dc3348a 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -341,9 +341,7 @@ static USBDevice *usb_try_create_simple(USBBus *bus, const char *name,
     object_property_set_bool(OBJECT(dev), true, "realized", &err);
     if (err) {
         error_propagate(errp, err);
-        error_prepend(errp, "Failed to initialize USB device '%s': ",
-                      name);
-        object_unparent(OBJECT(dev));
+        error_prepend(errp, "Failed to initialize USB device '%s': ", name);
         return NULL;
     }
     return dev;
-- 
2.9.3

[Qemu-devel] [PULL 2/3] usb: fix libusb config variable name.

[Gerd Hoffmann]

Cc: Jan Kiszka <jan.kiszka@siemens.com>
Fixes: 4e5ee5b21c84fe3023a64b5cc2e12a52ab0597c1
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Tested-by: Jan Kiszka <jan.kiszka@siemens.com>
Message-id: 20170926063820.30773-1-kraxel@redhat.com
---
 hw/usb/Makefile.objs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
index 9255234c63..0e6d54b21f 100644
--- a/hw/usb/Makefile.objs
+++ b/hw/usb/Makefile.objs
@@ -42,7 +42,7 @@ redirect.o-cflags = $(USB_REDIR_CFLAGS)
 redirect.o-libs = $(USB_REDIR_LIBS)
 
 # usb pass-through
-ifeq ($(CONFIG_LIBUSB)$(CONFIG_USB),yy)
+ifeq ($(CONFIG_USB_LIBUSB)$(CONFIG_USB),yy)
 common-obj-y += host-libusb.o host-legacy.o
 else
 common-obj-y += host-stub.o
-- 
2.9.3

[Qemu-devel] [PULL 3/3] usb: Use angle brackets for cacard include directive

[Gerd Hoffmann]

From: Fam Zheng <famz@redhat.com>

This is a library header, so angle brackets are more appropriate; also
move the line to before QEMU headers, as is recommended in HACKING.

Signed-off-by: Fam Zheng <famz@redhat.com>
Message-id: 20170920085952.3872-1-famz@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/ccid-card-passthru.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
index 45d96b03c6..117711862e 100644
--- a/hw/usb/ccid-card-passthru.c
+++ b/hw/usb/ccid-card-passthru.c
@@ -9,11 +9,11 @@
  */
 
 #include "qemu/osdep.h"
+#include <cacard/vscard_common.h>
 #include "chardev/char-fe.h"
 #include "qemu/error-report.h"
 #include "qemu/sockets.h"
 #include "ccid.h"
-#include "cacard/vscard_common.h"
 
 #define DPRINTF(card, lvl, fmt, ...)                    \
 do {                                                    \
-- 
2.9.3

Re: [Qemu-devel] [PULL 0/3] Usb 20170929 patches

[Peter Maydell]

On 29 September 2017 at 12:00, Gerd Hoffmann <kraxel@redhat.com> wrote:
> The following changes since commit ab161529261928ae7f3556e3220829c34b2686ec:
>
>   Merge remote-tracking branch 'remotes/dgilbert/tags/pull-migration-20170927a' into staging (2017-09-27 22:44:51 +0100)
>
> are available in the git repository at:
>
>   git://git.kraxel.org/qemu tags/usb-20170929-pull-request
>
> for you to fetch changes up to 13787d59cf86bac230c4d3eec3580110f53ac113:
>
>   usb: Use angle brackets for cacard include directive (2017-09-29 12:28:26 +0200)
>
> ----------------------------------------------------------------
> usb fixes.
>
> ----------------------------------------------------------------

This fails to build from clean (x86-64 Linux host):

make[1]: *** No rule to make target '../hw/usb/host-stub.o', needed by
'qemu-system-microblaze'. Stop.

(and similar for other targets).

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/3] Usb 20170929 patches

[Gerd Hoffmann]

  Hi,

> This fails to build from clean (x86-64 Linux host):
> 
> make[1]: *** No rule to make target '../hw/usb/host-stub.o', needed
> by
> 'qemu-system-microblaze'. Stop.
> 
> (and similar for other targets).

/me looks puzzled.

There is a "hw/usb/host-stub.c" file in the repo, so I fail so see why
make thinks it can't build the object file ...

Also it works fine in my testing, including a full build of all targets
(some with and some without usb support).

cheers,
  Gerd

Re: [Qemu-devel] [PULL 0/3] Usb 20170929 patches

[Thomas Huth]

On 04.10.2017 10:43, Gerd Hoffmann wrote:
>   Hi,
> 
>> This fails to build from clean (x86-64 Linux host):
>>
>> make[1]: *** No rule to make target '../hw/usb/host-stub.o', needed
>> by
>> 'qemu-system-microblaze'. Stop.
>>
>> (and similar for other targets).
> 
> /me looks puzzled.
> 
> There is a "hw/usb/host-stub.c" file in the repo, so I fail so see why
> make thinks it can't build the object file ...
> 
> Also it works fine in my testing, including a full build of all targets
> (some with and some without usb support).

Ah, deja vu. I think you've now run into the problem that I had with my
patch, too - see:

 https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01875.html
 https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg02082.html

I think this is a race condition when building with "make -j" in
parallel. Just add a line like this and you should be fine:

common-obj-$(CONFIG_ALL) += host-stub.o

 Thomas

Re: [Qemu-devel] [PULL 0/3] Usb 20170929 patches

[Gerd Hoffmann]

  Hi,
> 
> Ah, deja vu. I think you've now run into the problem that I had with
> my
> patch, too - see:
> 
>  https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01875.html
>  https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg02082.html
> 
> I think this is a race condition when building with "make -j" in
> parallel. Just add a line like this and you should be fine:
> 
> common-obj-$(CONFIG_ALL) += host-stub.o

I'd guess when compiling this unconditionally we better move it over to
stubs.

cheers,
  Gerd

diff --git a/hw/usb/host-stub.c b/stubs/usb-host.c
similarity index 100%
rename from hw/usb/host-stub.c
rename to stubs/usb-host.c
diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
index 0e6d54b21f..c2f46ac540 100644
--- a/hw/usb/Makefile.objs
+++ b/hw/usb/Makefile.objs
@@ -44,8 +44,6 @@ redirect.o-libs = $(USB_REDIR_LIBS)
 # usb pass-through
 ifeq ($(CONFIG_USB_LIBUSB)$(CONFIG_USB),yy)
 common-obj-y += host-libusb.o host-legacy.o
-else
-common-obj-y += host-stub.o
 endif
 
 host-libusb.o-cflags := $(LIBUSB_CFLAGS)
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index c7594796c3..edccdc5f1e 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -41,3 +41,4 @@ stub-obj-y += vmgenid.o
 stub-obj-y += xen-common.o
 stub-obj-y += xen-hvm.o
 stub-obj-y += pci-host-piix.o
+stub-obj-y += usb-host.o

Re: [Qemu-devel] [PULL 0/3] Usb 20170929 patches

[Paolo Bonzini]

On 04/10/2017 11:50, Gerd Hoffmann wrote:
>   Hi,
>>
>> Ah, deja vu. I think you've now run into the problem that I had with
>> my
>> patch, too - see:
>>
>>  https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01875.html
>>  https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg02082.html
>>
>> I think this is a race condition when building with "make -j" in
>> parallel. Just add a line like this and you should be fine:
>>
>> common-obj-$(CONFIG_ALL) += host-stub.o
> 
> I'd guess when compiling this unconditionally we better move it over to
> stubs.

No, just do what Thomas said.  See hw/pci/Makefile.objs for an example:

common-obj-$(call lnot,$(CONFIG_PCI)) += pci-stub.o
common-obj-$(CONFIG_ALL) += pci-stub.o

So just add the line outside the "if".

Paolo

> cheers,
>   Gerd
> 
> diff --git a/hw/usb/host-stub.c b/stubs/usb-host.c
> similarity index 100%
> rename from hw/usb/host-stub.c
> rename to stubs/usb-host.c
> diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
> index 0e6d54b21f..c2f46ac540 100644
> --- a/hw/usb/Makefile.objs
> +++ b/hw/usb/Makefile.objs
> @@ -44,8 +44,6 @@ redirect.o-libs = $(USB_REDIR_LIBS)
>  # usb pass-through
>  ifeq ($(CONFIG_USB_LIBUSB)$(CONFIG_USB),yy)
>  common-obj-y += host-libusb.o host-legacy.o
> -else
> -common-obj-y += host-stub.o
>  endif
>  
>  host-libusb.o-cflags := $(LIBUSB_CFLAGS)
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index c7594796c3..edccdc5f1e 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -41,3 +41,4 @@ stub-obj-y += vmgenid.o
>  stub-obj-y += xen-common.o
>  stub-obj-y += xen-hvm.o
>  stub-obj-y += pci-host-piix.o
> +stub-obj-y += usb-host.o
> 

Re: [Qemu-devel] [PULL 0/3] Usb 20170929 patches

[Paolo Bonzini]

On 04/10/2017 11:37, Thomas Huth wrote:
> Ah, deja vu. I think you've now run into the problem that I had with my
> patch, too - see:
> 
>  https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01875.html
>  https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg02082.html
> 
> I think this is a race condition when building with "make -j" in
> parallel. Just add a line like this and you should be fine:
> 
> common-obj-$(CONFIG_ALL) += host-stub.o

That's correct!  Maybe it should be documented in build-system.txt.

It's basically an optimization, CONFIG_ALL is used when some stubs are
not used in all subtargets but you still want to build them once-only.

Then the toplevel Makefile will typically see the configuration symbol
as defined (because it operates on the union of all targets'
configuration symbols), and will skip builting the "stubs" file.  Using
CONFIG_ALL for the stubs file ensures that the object file is available
before recursing.

Paolo