Re: [PATCH v2] xhci: Allocate the td array and urb_priv together.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Dec 18, 2013 at 11:24:47AM -0000, David Laight wrote:
> This saves a kzalloc() call on every transfer and some memory
> indirections.
> 
> The only possible downside is for isochronous tranfers with 64 td
> when the allocate is 8+4096 bytes (on 64bit systems) so requires
> an additional page.

If you take this to embedded world there could be a bad side effect too.
The free memory isn't abundant and the fragmentation may make a bigger
kmalloc() to cost more than 2 smaller ones.

Br, David Cohen

> 
> Signed-off-by: David Laight <david.laight@xxxxxxxxxx>
> ---
> 
> v2: Added signed-off-by line
> 
>  drivers/usb/host/xhci-mem.c  |  4 +---
>  drivers/usb/host/xhci-ring.c | 22 ++++++++++------------
>  drivers/usb/host/xhci.c      | 24 ++++++------------------
>  drivers/usb/host/xhci.h      |  2 +-
>  4 files changed, 18 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 79cdcc2..9b5d1c3 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -1675,10 +1675,8 @@ struct xhci_command *xhci_alloc_command(struct xhci_hcd *xhci,
>  
>  void xhci_urb_free_priv(struct xhci_hcd *xhci, struct urb_priv *urb_priv)
>  {
> -	if (urb_priv) {
> -		kfree(urb_priv->td[0]);
> +	if (urb_priv)
>  		kfree(urb_priv);
> -	}
>  }
>  
>  void xhci_free_command(struct xhci_hcd *xhci,
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index e38abc2..d3f4a9a 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -3007,7 +3007,7 @@ static int prepare_transfer(struct xhci_hcd *xhci,
>  		return ret;
>  
>  	urb_priv = urb->hcpriv;
> -	td = urb_priv->td[td_index];
> +	td = &urb_priv->td[td_index];
>  
>  	INIT_LIST_HEAD(&td->td_list);
>  	INIT_LIST_HEAD(&td->cancelled_td_list);
> @@ -3024,8 +3024,6 @@ static int prepare_transfer(struct xhci_hcd *xhci,
>  	td->start_seg = ep_ring->enq_seg;
>  	td->first_trb = ep_ring->enqueue;
>  
> -	urb_priv->td[td_index] = td;
> -
>  	return 0;
>  }
>  
> @@ -3216,7 +3214,7 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
>  		return trb_buff_len;
>  
>  	urb_priv = urb->hcpriv;
> -	td = urb_priv->td[0];
> +	td = &urb_priv->td[0];
>  
>  	/*
>  	 * Don't give the first TRB to the hardware (by toggling the cycle bit)
> @@ -3387,7 +3385,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
>  		return ret;
>  
>  	urb_priv = urb->hcpriv;
> -	td = urb_priv->td[0];
> +	td = &urb_priv->td[0];
>  
>  	/*
>  	 * Don't give the first TRB to the hardware (by toggling the cycle bit)
> @@ -3517,7 +3515,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
>  		return ret;
>  
>  	urb_priv = urb->hcpriv;
> -	td = urb_priv->td[0];
> +	td = &urb_priv->td[0];
>  
>  	/*
>  	 * Don't give the first TRB to the hardware (by toggling the cycle bit)
> @@ -3729,7 +3727,7 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
>  			goto cleanup;
>  		}
>  
> -		td = urb_priv->td[i];
> +		td = &urb_priv->td[i];
>  		for (j = 0; j < trbs_per_td; j++) {
>  			u32 remainder = 0;
>  			field = 0;
> @@ -3829,20 +3827,20 @@ cleanup:
>  	/* Clean up a partially enqueued isoc transfer. */
>  
>  	for (i--; i >= 0; i--)
> -		list_del_init(&urb_priv->td[i]->td_list);
> +		list_del_init(&urb_priv->td[i].td_list);
>  
>  	/* Use the first TD as a temporary variable to turn the TDs we've queued
>  	 * into No-ops with a software-owned cycle bit. That way the hardware
>  	 * won't accidentally start executing bogus TDs when we partially
>  	 * overwrite them.  td->first_trb and td->start_seg are already set.
>  	 */
> -	urb_priv->td[0]->last_trb = ep_ring->enqueue;
> +	urb_priv->td[0].last_trb = ep_ring->enqueue;
>  	/* Every TRB except the first & last will have its cycle bit flipped. */
> -	td_to_noop(xhci, ep_ring, urb_priv->td[0], true);
> +	td_to_noop(xhci, ep_ring, &urb_priv->td[0], true);
>  
>  	/* Reset the ring enqueue back to the first TRB and its cycle bit. */
> -	ep_ring->enqueue = urb_priv->td[0]->first_trb;
> -	ep_ring->enq_seg = urb_priv->td[0]->start_seg;
> +	ep_ring->enqueue = urb_priv->td[0].first_trb;
> +	ep_ring->enq_seg = urb_priv->td[0].start_seg;
>  	ep_ring->cycle_state = start_cycle;
>  	ep_ring->num_trbs_free = ep_ring->num_trbs_free_temp;
>  	usb_hcd_unlink_urb_from_ep(bus_to_hcd(urb->dev->bus), urb);
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 6e0d886..0969f74 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1248,12 +1248,11 @@ static int xhci_check_maxpacket(struct xhci_hcd *xhci, unsigned int slot_id,
>  int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flags)
>  {
>  	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> -	struct xhci_td *buffer;
>  	unsigned long flags;
>  	int ret = 0;
>  	unsigned int slot_id, ep_index;
>  	struct urb_priv	*urb_priv;
> -	int size, i;
> +	int size;
>  
>  	if (!urb || xhci_check_args(hcd, urb->dev, urb->ep,
>  					true, true, __func__) <= 0)
> @@ -1275,21 +1274,10 @@ int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flags)
>  		size = 1;
>  
>  	urb_priv = kzalloc(sizeof(struct urb_priv) +
> -				  size * sizeof(struct xhci_td *), mem_flags);
> +			   size * sizeof(struct xhci_td), mem_flags);
>  	if (!urb_priv)
>  		return -ENOMEM;
>  
> -	buffer = kzalloc(size * sizeof(struct xhci_td), mem_flags);
> -	if (!buffer) {
> -		kfree(urb_priv);
> -		return -ENOMEM;
> -	}
> -
> -	for (i = 0; i < size; i++) {
> -		urb_priv->td[i] = buffer;
> -		buffer++;
> -	}
> -
>  	urb_priv->length = size;
>  	urb_priv->td_cnt = 0;
>  	urb->hcpriv = urb_priv;
> @@ -1470,7 +1458,7 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
>  				"HW died, freeing TD.");
>  		urb_priv = urb->hcpriv;
>  		for (i = urb_priv->td_cnt; i < urb_priv->length; i++) {
> -			td = urb_priv->td[i];
> +			td = &urb_priv->td[i];
>  			if (!list_empty(&td->td_list))
>  				list_del_init(&td->td_list);
>  			if (!list_empty(&td->cancelled_td_list))
> @@ -1514,11 +1502,11 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
>  				urb, urb->dev->devpath,
>  				urb->ep->desc.bEndpointAddress,
>  				(unsigned long long) xhci_trb_virt_to_dma(
> -					urb_priv->td[i]->start_seg,
> -					urb_priv->td[i]->first_trb));
> +					urb_priv->td[i].start_seg,
> +					urb_priv->td[i].first_trb));
>  
>  	for (; i < urb_priv->length; i++) {
> -		td = urb_priv->td[i];
> +		td = &urb_priv->td[i];
>  		list_add_tail(&td->cancelled_td_list, &ep->cancelled_td_list);
>  	}
>  
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 72ad988..a7d8fdd 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1363,7 +1363,7 @@ struct xhci_scratchpad {
>  struct urb_priv {
>  	int	length;
>  	int	td_cnt;
> -	struct	xhci_td	*td[0];
> +	struct	xhci_td	td[0];
>  };
>  
>  /*
> -- 
> 1.8.1.2
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux