Re: [PATCH] xhci: remove endpoint ring cache

[Mathias Nyman]

On 05.06.2017 11:46, Anurag Kumar Vulisha wrote:

Hi Mathias,

I have tested this patch with my platform and it works fine

Thanks,
Anurag Kumar Vulisha


Thanks, I'll add your tested-by tag to it as well, and add it to my tree

-Mathias

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] xhci: remove endpoint ring cache

[Anurag Kumar Vulisha]

Hi Mathias,

I have tested this patch with my platform and it works fine

Thanks,
Anurag Kumar Vulisha

>-Original Message-
>From: Mathias Nyman [mailto:mathias.ny...@linux.intel.com]
>Sent: Friday, June 02, 2017 4:51 PM
>To: Anurag Kumar Vulisha 
>Cc: gre...@linuxfoundation.org; Anirudha Sarangi ;
>Anurag Kumar Vulisha ; linux-usb@vger.kernel.org;
>Mathias Nyman 
>Subject: [PATCH] xhci: remove endpoint ring cache
>
>Having a properly working ring cache could ease a bit the memory
>reallocation, but this current implemetation isn't the correct way.
>It's faulty and hogs a lot of memory.
>
>A pool of cached rings that any device could use would be more useful,
>but xhci driver isn't there yet, just keeping the basic functionality
>working is already a handful.
>
>How about your case, does removing the cache work instead with your setup?
>
>8<---
>
>Anurag Kumar Vulisha reported several issues with xhci endpoint
>ring caching.
>
>31 Rings are cached per device before a ring is freed.
>These cached rings are not used as default if a new ring is needed.
>They are only used if the drive fails to allocate memory for a ring.
>
>The current ring cache is more a reason to why we run out memry than a
>help when we actually do so.
>
>Anurag Kumar Vulisha tried to use cached rings as a first option and
>found new issues with cached ring initialization.
>Cached rings were first zeroed and then manually reinitialized with link
>rbs etc, but forgetting to set some important bits like cycle toggle bit.
>
>Remove the ring cache completely as it's a faulty premature optimization
>eating memory
>
>Reported-by: Anurag Kumar Vulisha 
>Signed-off-by: Mathias Nyman 
>---
> drivers/usb/host/xhci-mem.c | 81 -
> drivers/usb/host/xhci.c | 17 +-
> drivers/usb/host/xhci.h |  6 +---
> 3 files changed, 15 insertions(+), 89 deletions(-)
>
>diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>index 1f1687e..7c2501a 100644
>--- a/drivers/usb/host/xhci-mem.c
>+++ b/drivers/usb/host/xhci-mem.c
>@@ -407,64 +407,17 @@ static struct xhci_ring *xhci_ring_alloc(struct xhci_hcd
>*xhci,
>   return NULL;
> }
>
>-void xhci_free_or_cache_endpoint_ring(struct xhci_hcd *xhci,
>+void xhci_free_endpoint_ring(struct xhci_hcd *xhci,
>   struct xhci_virt_device *virt_dev,
>   unsigned int ep_index)
> {
>-  int rings_cached;
>-
>-  rings_cached = virt_dev->num_rings_cached;
>-  if (rings_cached < XHCI_MAX_RINGS_CACHED) {
>-  virt_dev->ring_cache[rings_cached] =
>-  virt_dev->eps[ep_index].ring;
>-  virt_dev->num_rings_cached++;
>-  xhci_dbg(xhci, "Cached old ring, "
>-  "%d ring%s cached\n",
>-  virt_dev->num_rings_cached,
>-  (virt_dev->num_rings_cached > 1) ? "s" : "");
>-  } else {
>-  xhci_ring_free(xhci, virt_dev->eps[ep_index].ring);
>-  xhci_dbg(xhci, "Ring cache full (%d rings), "
>-  "freeing ring\n",
>-  virt_dev->num_rings_cached);
>-  }
>+  xhci_ring_free(xhci, virt_dev->eps[ep_index].ring);
>   virt_dev->eps[ep_index].ring = NULL;
> }
>
>-/* Zero an endpoint ring (except for link TRBs) and move the enqueue and
>dequeue
>- * pointers to the beginning of the ring.
>- */
>-static void xhci_reinit_cached_ring(struct xhci_hcd *xhci,
>-  struct xhci_ring *ring, unsigned int cycle_state,
>-  enum xhci_ring_type type)
>-{
>-  struct xhci_segment *seg = ring->first_seg;
>-  int i;
>-
>-  do {
>-  memset(seg->trbs, 0,
>-  sizeof(union xhci_trb)*TRBS_PER_SEGMENT);
>-  if (cycle_state == 0) {
>-  for (i = 0; i < TRBS_PER_SEGMENT; i++)
>-  seg->trbs[i].link.control |=
>-  cpu_to_le32(TRB_CYCLE);
>-  }
>-  /* All endpoint rings have link TRBs */
>-  xhci_link_segments(xhci, seg, seg->next, type);
>-  seg = seg->next;
>-  } while (seg != ring->first_seg);
>-  ring->type = type;
>-  xhci_initialize_ring_info(ring, cycle_state);
>-  /* td list should be empty since all URBs have been cancelled,
>-   * but just in case...
>-   */
>-  INIT_LIST_HEAD(>td_list);
>-}
>-
> /*
>  * Expand an existing ring.
>- * Look for a cached ring or allocate a new ring which has same segment
>numbers
>- * and link the two rings.
>+ * Allocate a new ring which has same segment numbers and link the two rings.
>  */
> int xhci_ring_expansion(struct xhci_hcd *xhci, struct xhci_ring *ring,
>