[Qemu-devel] [PULL 0/9] Usb 20190220 patches

[Qemu-devel] [PULL 0/9] Usb 20190220 patches

[Gerd Hoffmann]

The following changes since commit 2e68b8620637a4ee8c79b5724144b726af1e261b:

  Merge remote-tracking branch 'remotes/dgibson/tags/ppc-for-4.0-20190219' into staging (2019-02-18 16:20:13 +0000)

are available in the git repository at:

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

for you to fetch changes up to 7011baece29d0e197c54c4e57326ba88e67a4949:

  usb: remove unnecessary NULL device check from usb_ep_get() (2019-02-20 09:41:23 +0100)

----------------------------------------------------------------
usb: usb_ep_get() fixes

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

Liam Merwick (9):
  usb: rearrange usb_ep_get()
  xhci: add asserts to help with static code analysis
  xhci: check device is not NULL before calling usb_ep_get()
  ehci: check device is not NULL before calling usb_ep_get()
  ohci: check device is not NULL before calling usb_ep_get()
  uhci: check device is not NULL before calling usb_ep_get()
  usb: check device is not NULL before calling usb_ep_get()
  usb: add device checks before redirector calls to usb_ep_get()
  usb: remove unnecessary NULL device check from usb_ep_get()

 hw/usb/core.c     | 6 ++----
 hw/usb/hcd-ehci.c | 7 +++++--
 hw/usb/hcd-musb.c | 8 ++++----
 hw/usb/hcd-ohci.c | 8 ++++++++
 hw/usb/hcd-uhci.c | 8 +++++---
 hw/usb/hcd-xhci.c | 8 +++++---
 hw/usb/redirect.c | 3 ++-
 7 files changed, 31 insertions(+), 17 deletions(-)

-- 
2.9.3

[Qemu-devel] [PULL 1/9] usb: rearrange usb_ep_get()

[Gerd Hoffmann]

From: Liam Merwick <liam.merwick@oracle.com>

There is no need to calculate the 'eps' variable in usb_ep_get()
if 'ep' is the control endpoint.  Instead the calculation should
be done after validating the input before returning an entry
indexed by the endpoint 'ep'.

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
Reviewed-by: Darren Kenny <Darren.Kenny@oracle.com>
Reviewed-by: Mark Kanda <Mark.Kanda@oracle.com>
Reviewed-by: Ameya More <ameya.more@oracle.com>
Message-id: 1549460216-25808-2-git-send-email-liam.merwick@oracle.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/core.c b/hw/usb/core.c
index 241ae66b15..bfb7ae67bb 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -720,12 +720,12 @@ struct USBEndpoint *usb_ep_get(USBDevice *dev, int pid, int ep)
     if (dev == NULL) {
         return NULL;
     }
-    eps = (pid == USB_TOKEN_IN) ? dev->ep_in : dev->ep_out;
     if (ep == 0) {
         return &dev->ep_ctl;
     }
     assert(pid == USB_TOKEN_IN || pid == USB_TOKEN_OUT);
     assert(ep > 0 && ep <= USB_MAX_ENDPOINTS);
+    eps = (pid == USB_TOKEN_IN) ? dev->ep_in : dev->ep_out;
     return eps + ep - 1;
 }
 
-- 
2.9.3

[Qemu-devel] [PULL 2/9] xhci: add asserts to help with static code analysis

[Gerd Hoffmann]

From: Liam Merwick <liam.merwick@oracle.com>

Most callers of xhci_port_update() and xhci_wakeup() pass in a pointer
to an array entry and can never be NULL but add two defensive asserts
to protect against future changes (e.g. adding a new port speed, etc.)
adding a path through xhci_lookup_port() that could result in the
return of a NULL XHCIPort.

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
Message-id: 1549460216-25808-3-git-send-email-liam.merwick@oracle.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-xhci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 19c64f7ff4..99b83aaa9e 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -2607,6 +2607,7 @@ static void xhci_port_update(XHCIPort *port, int is_detach)
 {
     uint32_t pls = PLS_RX_DETECT;
 
+    assert(port);
     port->portsc = PORTSC_PP;
     if (!is_detach && xhci_port_have_device(port)) {
         port->portsc |= PORTSC_CCS;
@@ -3215,6 +3216,7 @@ static void xhci_wakeup(USBPort *usbport)
     XHCIState *xhci = usbport->opaque;
     XHCIPort *port = xhci_lookup_port(xhci, usbport);
 
+    assert(port);
     if (get_field(port->portsc, PORTSC_PLS) != PLS_U3) {
         return;
     }
-- 
2.9.3

[Qemu-devel] [PULL 3/9] xhci: check device is not NULL before calling usb_ep_get()

[Gerd Hoffmann]

From: Liam Merwick <liam.merwick@oracle.com>

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
Message-id: 1549460216-25808-4-git-send-email-liam.merwick@oracle.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-xhci.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 99b83aaa9e..ec28bee319 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3276,10 +3276,10 @@ static USBEndpoint *xhci_epid_to_usbep(XHCIEPContext *epctx)
         return NULL;
     }
     uport = epctx->xhci->slots[epctx->slotid - 1].uport;
+    if (!uport || !uport->dev) {
+        return NULL;
+    }
     token = (epctx->epid & 1) ? USB_TOKEN_IN : USB_TOKEN_OUT;
-    if (!uport) {
-        return NULL;
-    }
     return usb_ep_get(uport->dev, token, epctx->epid >> 1);
 }
 
-- 
2.9.3

[Qemu-devel] [PULL 4/9] ehci: check device is not NULL before calling usb_ep_get()

[Gerd Hoffmann]

From: Liam Merwick <liam.merwick@oracle.com>

In ehci_process_itd(), the call to ehci_find_device() can return NULL
if it doesn't find a device matching 'devaddr' so explicitly check
the return value before passing it to usb_ep_get().

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
Message-id: 1549460216-25808-5-git-send-email-liam.merwick@oracle.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-ehci.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 9b132cb0d3..62dab0592f 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1439,9 +1439,12 @@ static int ehci_process_itd(EHCIState *ehci,
                 qemu_sglist_add(&ehci->isgl, ptr1 + off, len);
             }
 
-            pid = dir ? USB_TOKEN_IN : USB_TOKEN_OUT;
-
             dev = ehci_find_device(ehci, devaddr);
+            if (dev == NULL) {
+                ehci_trace_guest_bug(ehci, "no device found");
+                return -1;
+            }
+            pid = dir ? USB_TOKEN_IN : USB_TOKEN_OUT;
             ep = usb_ep_get(dev, pid, endp);
             if (ep && ep->type == USB_ENDPOINT_XFER_ISOC) {
                 usb_packet_setup(&ehci->ipacket, pid, ep, 0, addr, false,
-- 
2.9.3

[Qemu-devel] [PULL 5/9] ohci: check device is not NULL before calling usb_ep_get()

[Gerd Hoffmann]

From: Liam Merwick <liam.merwick@oracle.com>

A call to ohci_find_device() can return NULL if it doesn't find a
device matching 'addr' so for the two callers, explicitly check
the return value before passing it to usb_ep_get().

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
Message-id: 1549460216-25808-6-git-send-email-liam.merwick@oracle.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-ohci.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index c34cf5b73a..196a9f7200 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -848,6 +848,10 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
         bool int_req = relative_frame_number == frame_count &&
                        OHCI_BM(iso_td.flags, TD_DI) == 0;
         dev = ohci_find_device(ohci, OHCI_BM(ed->flags, ED_FA));
+        if (dev == NULL) {
+            trace_usb_ohci_td_dev_error();
+            return 1;
+        }
         ep = usb_ep_get(dev, pid, OHCI_BM(ed->flags, ED_EN));
         usb_packet_setup(&ohci->usb_packet, pid, ep, 0, addr, false, int_req);
         usb_packet_addbuf(&ohci->usb_packet, ohci->usb_buf, len);
@@ -1071,6 +1075,10 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
             return 1;
         }
         dev = ohci_find_device(ohci, OHCI_BM(ed->flags, ED_FA));
+        if (dev == NULL) {
+            trace_usb_ohci_td_dev_error();
+            return 1;
+        }
         ep = usb_ep_get(dev, pid, OHCI_BM(ed->flags, ED_EN));
         usb_packet_setup(&ohci->usb_packet, pid, ep, 0, addr, !flag_r,
                          OHCI_BM(td.flags, TD_DI) == 0);
-- 
2.9.3

[Qemu-devel] [PULL 6/9] uhci: check device is not NULL before calling usb_ep_get()

[Gerd Hoffmann]

From: Liam Merwick <liam.merwick@oracle.com>

In uhci_handle_td(), the call to ehci_find_device() can return NULL
if it doesn't find a device matching 'addr' so explicitly check
the return value before passing it to usb_ep_get().

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
Message-id: 1549460216-25808-7-git-send-email-liam.merwick@oracle.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-uhci.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index e694b62086..09df29ff9c 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -858,13 +858,15 @@ static int uhci_handle_td(UHCIState *s, UHCIQueue *q, uint32_t qh_addr,
 
     /* Allocate new packet */
     if (q == NULL) {
-        USBDevice *dev = uhci_find_device(s, (td->token >> 8) & 0x7f);
-        USBEndpoint *ep = usb_ep_get(dev, pid, (td->token >> 15) & 0xf);
+        USBDevice *dev;
+        USBEndpoint *ep;
 
-        if (ep == NULL) {
+        dev = uhci_find_device(s, (td->token >> 8) & 0x7f);
+        if (dev == NULL) {
             return uhci_handle_td_error(s, td, td_addr, USB_RET_NODEV,
                                         int_mask);
         }
+        ep = usb_ep_get(dev, pid, (td->token >> 15) & 0xf);
         q = uhci_queue_new(s, qh_addr, td, ep);
     }
     async = uhci_async_alloc(q, td_addr);
-- 
2.9.3

[Qemu-devel] [PULL 7/9] usb: check device is not NULL before calling usb_ep_get()

[Gerd Hoffmann]

From: Liam Merwick <liam.merwick@oracle.com>

In musb_packet(), the call to usb_find_device() can return NULL
if it doesn't find a device matching 'addr' so explicitly check
the return value before passing it to usb_ep_get().  This then
allows the subsequent calculation of 'id' to be streamlined.

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
Message-id: 1549460216-25808-8-git-send-email-liam.merwick@oracle.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-musb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/usb/hcd-musb.c b/hw/usb/hcd-musb.c
index d70a91a58c..85d7796554 100644
--- a/hw/usb/hcd-musb.c
+++ b/hw/usb/hcd-musb.c
@@ -628,11 +628,11 @@ static void musb_packet(MUSBState *s, MUSBEndPoint *ep,
 
     /* A wild guess on the FADDR semantics... */
     dev = usb_find_device(&s->port, ep->faddr[idx]);
+    if (dev == NULL) {
+        return;
+    }
     uep = usb_ep_get(dev, pid, ep->type[idx] & 0xf);
-    id = pid;
-    if (uep) {
-        id |= (dev->addr << 16) | (uep->nr << 8);
-    }
+    id = pid | (dev->addr << 16) | (uep->nr << 8);
     usb_packet_setup(&ep->packey[dir].p, pid, uep, 0, id, false, true);
     usb_packet_addbuf(&ep->packey[dir].p, ep->buf[idx], len);
     ep->packey[dir].ep = ep;
-- 
2.9.3

[Qemu-devel] [PULL 8/9] usb: add device checks before redirector calls to usb_ep_get()

[Gerd Hoffmann]

From: Liam Merwick <liam.merwick@oracle.com>

Add an assert and an explicit check before the two callers to
usb_ep_get() in the USB redirector code to ensure the device
passed in is not NULL.

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
Message-id: 1549460216-25808-9-git-send-email-liam.merwick@oracle.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/redirect.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 18a42d1938..7cb6b120d4 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1728,6 +1728,7 @@ static void usbredir_ep_info(void *priv,
     USBRedirDevice *dev = priv;
     int i;
 
+    assert(dev != NULL);
     for (i = 0; i < MAX_ENDPOINTS; i++) {
         dev->endpoint[i].type = ep_info->type[i];
         dev->endpoint[i].interval = ep_info->interval[i];
@@ -2125,7 +2126,7 @@ static int usbredir_post_load(void *priv, int version_id)
 {
     USBRedirDevice *dev = priv;
 
-    if (dev->parser == NULL) {
+    if (dev == NULL || dev->parser == NULL) {
         return 0;
     }
 
-- 
2.9.3

[Qemu-devel] [PULL 9/9] usb: remove unnecessary NULL device check from usb_ep_get()

[Gerd Hoffmann]

From: Liam Merwick <liam.merwick@oracle.com>

No caller of usb_ep_get() calls it with a NULL device (previous commits
have addressed the few remaining cases which didn't explicitly check).
Replace check for 'dev == NULL' with an assert instead.

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
Message-id: 1549460216-25808-10-git-send-email-liam.merwick@oracle.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/usb/core.c b/hw/usb/core.c
index bfb7ae67bb..8fbd9c7d57 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -717,9 +717,7 @@ struct USBEndpoint *usb_ep_get(USBDevice *dev, int pid, int ep)
 {
     struct USBEndpoint *eps;
 
-    if (dev == NULL) {
-        return NULL;
-    }
+    assert(dev != NULL);
     if (ep == 0) {
         return &dev->ep_ctl;
     }
-- 
2.9.3

Re: [Qemu-devel] [PULL 8/9] usb: add device checks before redirector calls to usb_ep_get()

[Yuval Shaia]

On Wed, Feb 20, 2019 at 12:13:45PM +0100, Gerd Hoffmann wrote:
> From: Liam Merwick <liam.merwick@oracle.com>
> 
> Add an assert and an explicit check before the two callers to
> usb_ep_get() in the USB redirector code to ensure the device
> passed in is not NULL.
> 
> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
> Message-id: 1549460216-25808-9-git-send-email-liam.merwick@oracle.com
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/usb/redirect.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index 18a42d1938..7cb6b120d4 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -1728,6 +1728,7 @@ static void usbredir_ep_info(void *priv,
>      USBRedirDevice *dev = priv;
>      int i;
>  
> +    assert(dev != NULL);

Suggesting:
    assert(dev)

>      for (i = 0; i < MAX_ENDPOINTS; i++) {
>          dev->endpoint[i].type = ep_info->type[i];
>          dev->endpoint[i].interval = ep_info->interval[i];
> @@ -2125,7 +2126,7 @@ static int usbredir_post_load(void *priv, int version_id)
>  {
>      USBRedirDevice *dev = priv;
>  
> -    if (dev->parser == NULL) {
> +    if (dev == NULL || dev->parser == NULL) {

Suggesting
    if (!dev || !dev->parser)

>          return 0;
>      }
>  
> -- 
> 2.9.3
> 
> 

Re: [Qemu-devel] [PULL 0/9] Usb 20190220 patches

[Peter Maydell]

On Wed, 20 Feb 2019 at 11:19, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> The following changes since commit 2e68b8620637a4ee8c79b5724144b726af1e261b:
>
>   Merge remote-tracking branch 'remotes/dgibson/tags/ppc-for-4.0-20190219' into staging (2019-02-18 16:20:13 +0000)
>
> are available in the git repository at:
>
>   git://git.kraxel.org/qemu tags/usb-20190220-pull-request
>
> for you to fetch changes up to 7011baece29d0e197c54c4e57326ba88e67a4949:
>
>   usb: remove unnecessary NULL device check from usb_ep_get() (2019-02-20 09:41:23 +0100)
>
> ----------------------------------------------------------------
> usb: usb_ep_get() fixes
>

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.0
for any user-visible changes.

-- PMM

Re: [Qemu-devel] [PULL 8/9] usb: add device checks before redirector calls to usb_ep_get()

[Liam Merwick]

On 20/02/2019 11:24, Yuval Shaia wrote:
> On Wed, Feb 20, 2019 at 12:13:45PM +0100, Gerd Hoffmann wrote:
>> From: Liam Merwick <liam.merwick@oracle.com>
>>
>> Add an assert and an explicit check before the two callers to
>> usb_ep_get() in the USB redirector code to ensure the device
>> passed in is not NULL.
>>
>> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
>> Message-id: 1549460216-25808-9-git-send-email-liam.merwick@oracle.com
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>>   hw/usb/redirect.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
>> index 18a42d1938..7cb6b120d4 100644
>> --- a/hw/usb/redirect.c
>> +++ b/hw/usb/redirect.c
>> @@ -1728,6 +1728,7 @@ static void usbredir_ep_info(void *priv,
>>       USBRedirDevice *dev = priv;
>>       int i;
>>   
>> +    assert(dev != NULL);
> 
> Suggesting:
>      assert(dev)
> 
>>       for (i = 0; i < MAX_ENDPOINTS; i++) {
>>           dev->endpoint[i].type = ep_info->type[i];
>>           dev->endpoint[i].interval = ep_info->interval[i];
>> @@ -2125,7 +2126,7 @@ static int usbredir_post_load(void *priv, int version_id)
>>   {
>>       USBRedirDevice *dev = priv;
>>   
>> -    if (dev->parser == NULL) {
>> +    if (dev == NULL || dev->parser == NULL) {
> 
> Suggesting
>      if (!dev || !dev->parser)
> 

The rest of the file tests for '== NULL' so I used that for consistency 
(but in any case, my personal preferences is to just use that boolean 
style check with boolean variables).  As it happens, the commits have 
already been pulled.

Regards,
Liam