Re: [SeaBIOS] [PATCH 3/4] usb: add usb_update_pipe()

[Kevin O'Connor]

On Wed, Aug 21, 2013 at 03:20:22PM +0200, Gerd Hoffmann wrote:
> Preparation for better xhci support: allows to notify host controllers
> instead of going through a free+alloc cycle.
> Signed-off-by: Gerd Hoffmann
>
> src/usb.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/src/usb.c b/src/usb.c
> index 808f426..903dd78 100644
> --- a/src/usb.c
> +++ b/src/usb.c
> @@ -40,6 +40,18 @@ usb_alloc_pipe(struct usbdevice_s *usbdev
>      }
>  }
>
> +// Update an pipe (used for control only)
> +struct usb_pipe *
> +usb_update_pipe(struct usbdevice_s *usbdev, struct usb_pipe *pipe
>            , struct usb_endpoint_descriptor *epdesc)
>
> +{
> switch (usbdev->hub->cntl->type) {
> default:
>    free_pipe(pipe);
>    return usb_alloc_pipe(usbdev, epdesc);
> }
>
> +}
>
> // Send a message on a control pipe using the default control descriptor.
>  static int
>  send_control(struct usb_pipe *pipe, int dir, const void *cmd, int cmdsize
> @@ -258,7 +270,6 @@ usb_set_address(struct usbdevice_s *usbdev)
>      req.wIndex = 0;
>      req.wLength = 0;
>      int ret = send_default_control(usbdev->defpipe, &req, NULL);
> free_pipe(usbdev->defpipe);
> if (ret)
>     return -1;

This leaks a pipe in the error case.  Leaking a pipe would be very bad
as it would leave memory that the controller can DMA into that an OS
may use.  (Pipes can be allocated with "tmphigh" ram which is not
reserved from the OS after POST.)

[...]
> @@ -372,9 +382,10 @@ usb_hub_port_setup(void *data)
>  // Configure the device
>  int count = configure_usb_device(usbdev);
> free_pipe(usbdev->defpipe);
> if (!count)
> if (!count) {
>    free_pipe(usbdev->defpipe);
>    hub->op->disconnect(hub, port);
> }
> hub->devcount += count;
>
> done:
>      hub->threads--;

This leaks the hub pipe if any of its ports are used.

-Kevin