RE: [PATCH 36/37] xhci: simplify how we store TDs in urb private data
From: David Laight > Sent: 23 January 2017 11:58 > From: Mathias Nyman > > Sent: 20 January 2017 14:47 > > > Instead of storing a zero length array of td pointers, and then > > allocate memory both for the td pointer array and the td's, just > > use a zero length array of actual td's in urb private data. > > This reminds me of an old patch that got reverted because things > broke because the lifetimes/accesses of the data wasn't the obvious one. Mathias checked (off list) and the handling of stalled endpoints has apparently changed - which is what caused the problem with the old patch. David -- 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 36/37] xhci: simplify how we store TDs in urb private data
On 23.01.2017 14:15, Mathias Nyman wrote: On 23.01.2017 13:57, David Laight wrote: From: Mathias Nyman Sent: 20 January 2017 14:47 Instead of storing a zero length array of td pointers, and then allocate memory both for the td pointer array and the td's, just use a zero length array of actual td's in urb private data. This reminds me of an old patch that got reverted because things broke because the lifetimes/accesses of the data wasn't the obvious one. Can you recall more details about that case? I'd hate to revert this later. Thanks for the (off list) info. You submitted a similar patch in 2013, but it caused a regression and was not applied: https://www.spinics.net/lists/linux-usb/msg99748.html Reason it regressed back then was because we handled stalled endpoints a bit differently. We saved a pointer to the current TD of a stalled endpoint, then gave back the URB, and freed urb_priv, but _not_ the TD that urb_priv->td[x] pointed to. This TD was later used in the endpoint reset callback to figure out where we should set the dequeue pointer of that ring. Handling stalled endpoints has changed since then. We don't store the TD pointer in STALL cases, and we free the urb_priv completely when giving back the URB see: commit 8e71a322fdb127814bcba423a512914ca5bc6cf5 USB: xhci: Reset a halted endpoint immediately when we encounter a stall. So now it should work -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 36/37] xhci: simplify how we store TDs in urb private data
From: Mathias Nyman > Sent: 20 January 2017 14:47 > Instead of storing a zero length array of td pointers, and then > allocate memory both for the td pointer array and the td's, just > use a zero length array of actual td's in urb private data. This reminds me of an old patch that got reverted because things broke because the lifetimes/accesses of the data wasn't the obvious one. That might have been a different structure. FWIW it is a shame that things like USB3 ethernet can't transmit and receive without all these intermediate and dynamically allocated structures. David -- 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 36/37] xhci: simplify how we store TDs in urb private data
On 23.01.2017 13:57, David Laight wrote: From: Mathias Nyman Sent: 20 January 2017 14:47 Instead of storing a zero length array of td pointers, and then allocate memory both for the td pointer array and the td's, just use a zero length array of actual td's in urb private data. This reminds me of an old patch that got reverted because things broke because the lifetimes/accesses of the data wasn't the obvious one. Can you recall more details about that case? I'd hate to revert this later. I looked at the history how we ended up with this type of allocation for urb priv. I couldn't find any reverts or reasons why it's the way it is. That might have been a different structure. FWIW it is a shame that things like USB3 ethernet can't transmit and receive without all these intermediate and dynamically allocated structures. Well, there's one less allocation after this patch :) -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
[PATCH 36/37] xhci: simplify how we store TDs in urb private data
Instead of storing a zero length array of td pointers, and then
allocate memory both for the td pointer array and the td's, just
use a zero length array of actual td's in urb private data.
old:
struct urb_priv {
struct xhci_td *td[0]
}
new:
struct urb_priv {
struct xhci_td td[0]
}
Signed-off-by: Mathias Nyman
---
drivers/usb/host/xhci-mem.c | 5 +
drivers/usb/host/xhci-ring.c | 20 ++--
drivers/usb/host/xhci.c | 24 ++--
drivers/usb/host/xhci.h | 2 +-
4 files changed, 18 insertions(+), 33 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index e452492..ba1853f4 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1817,10 +1817,7 @@ struct xhci_command *xhci_alloc_command(struct xhci_hcd
*xhci,
void xhci_urb_free_priv(struct urb_priv *urb_priv)
{
- if (urb_priv) {
- kfree(urb_priv->td[0]);
- kfree(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 dbbf8c2..5e51109 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2841,7 +2841,7 @@ static int prepare_transfer(struct xhci_hcd *xhci,
return ret;
urb_priv = urb->hcpriv;
- td = urb_priv->td[td_index];
+ td = _priv->td[td_index];
INIT_LIST_HEAD(>td_list);
INIT_LIST_HEAD(>cancelled_td_list);
@@ -3137,7 +3137,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t
mem_flags,
if (urb->transfer_flags & URB_ZERO_PACKET && urb_priv->num_tds > 1)
need_zero_pkt = true;
- td = urb_priv->td[0];
+ td = _priv->td[0];
/*
* Don't give the first TRB to the hardware (by toggling the cycle bit)
@@ -3230,7 +3230,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t
mem_flags,
ret = prepare_transfer(xhci, xhci->devs[slot_id],
ep_index, urb->stream_id,
1, urb, 1, mem_flags);
- urb_priv->td[1]->last_trb = ring->enqueue;
+ urb_priv->td[1].last_trb = ring->enqueue;
field = TRB_TYPE(TRB_NORMAL) | ring->cycle_state | TRB_IOC;
queue_trb(xhci, ring, 0, 0, 0, TRB_INTR_TARGET(0), field);
}
@@ -3282,7 +3282,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 = _priv->td[0];
/*
* Don't give the first TRB to the hardware (by toggling the cycle bit)
@@ -3570,7 +3570,7 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci,
gfp_t mem_flags,
return ret;
goto cleanup;
}
- td = urb_priv->td[i];
+ td = _priv->td[i];
/* use SIA as default, if frame id is used overwrite it */
sia_frame_id = TRB_SIA;
@@ -3677,20 +3677,20 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci,
gfp_t mem_flags,
/* Clean up a partially enqueued isoc transfer. */
for (i--; i >= 0; i--)
- list_del_init(_priv->td[i]->td_list);
+ list_del_init(_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, _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 bee6272..dde5c2d 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1332,12 +1332,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;
