Re: [RFCv2 01/10] xhci: Use command structures when calling xhci_configure_endpoint
On 02/05/2014 04:21 AM, Dan Williams wrote: Hi Mathias, comments below: s/xhci_check_bandwith/xhci_check_bandwidth/ s/strucure/structure/ s/strucure/structure/ s/requre/require/ s/strucure/structure/ Thanks I guess I need to start using a spell checker for commit messages. One cleanup we may want to consider in this series is making xhci_alloc_command() more readable. My brain hurts when I see "false, false" as I wonder what that means. I took a look and of the 4 possible ways to call xhci_alloc_command, we only use 2: $ git grep xhci_alloc_command\(.*\) | grep -o xhci_alloc_command\(xhci,.*,.*, | sort -u xhci_alloc_command(xhci, false, true, xhci_alloc_command(xhci, true, true, So a first take is to just have a xhci_alloc_command() for "true, true" and a xhci_alloc_command_no_ctx() for "false, true". ...uh oh, this series adds a usage of: xhci_alloc_command(xhci, false, false, ...any reason we can't just use something like xhci_alloc_command_no_ctx() instead? Actually just make xhci_alloc_command() take an option in_ctx parameter, when it is NULL xhci_alloc_command will allocate one, otherwise it will use the passed in one. This would make the code more readable. The same thing needs to be done for the completion parameter as well then. Do you think this change would fit in this patch series, or maybe as a separate fix? xhci_dbg_trace(xhci, trace_xhci_dbg_quirks, "Queueing configure endpoint command"); xhci_queue_configure_endpoint(xhci, xhci->devs[slot_id]->in_ctx->dma, slot_id, false); xhci_ring_cmd_db(xhci); + kfree(command); It's not really acceptable to add dead code in a patch. Consider the case where some of the patches are reverted due to a regression. If, for example we revert patch 2, the unused infrastructure in patch1 does not get deleted. Patch size minimization is good, but not when it separates new infrastructure from its first user. This was a tradeoff I wasn't sure how to do. The first six patches make sure there exists a command structure every time a command is submitted. I added the kfrees because I didn't want to leak memory up to the patch where the command can be freed in its right place (patch 9). Actually, now looking at it, the command is still not properly freed between patches 7 and 9. Any suggestions? squashing first most of the commits together, or just ignoring that memory is leaked mid-series? -bandwidth_change: - xhci_dbg_trace(xhci, trace_xhci_dbg_context_change, - "Completed config ep cmd"); - virt_dev->cmd_status = cmd_comp_code; - complete(&virt_dev->cmd_completion); return; This change has no description in the change log. What's the reason for deleting the goto? Previously xhci_configure_endpoint() could also be called without a command parameter. In this case the completion was _not_ added to device's own "command wait list". xhci_configure_endpoint() would wait for completion on xhci->devs[udev->slot_id]->cmd_completion, and the code after the bandwith_change goto was run. Now this patch forces all xhci_configure_endpoint() callers to have a command structure parameter, and now in all cases we're waiting for a configure endpoint completion, the completion is added to the device's own "command wait list". These completions are called in the beginning of handle_cmd_completion_ep by handle_cmd_in_cmd_wait_list(). I probably should add some description about this in the changelog as well. Given that we are waiting for the command to finish within xhci_configure_endpoint() shouldn't we free the completion in xhci_configure_endpoint as well? In other words in what cases do we need an xhci_command to have a longer lifetime than the scope of the execution routine (xhci_stop_device, xhci_configure_endpoint, xhci_discover_or_reset_device, xhci_alloc_dev, xhci_setup_device). Many of the functions that call xhci_configure_endpoint() handle their command strucure and completion allocation/freeing in their own little way. I didn't want to mess with these. For example xhci_free_streams() uses some pre-allocated command strucure command = vdev->eps[ep_index].stream_info->free_streams_command; while xhci_update_hub_device() allocates a new command with completion before calling xhci_configure_endpoint(), and frees them both afterwards Taking things a step further it seems that all the locations where we asynchronously queue commands are in the completion handlers for other commands. I'm wondering if this would be cleaner if we simply queued all command submissions and completion events to a single threaded workqueue. I'll go through the rest of the series to see if that impression makes sense, but something to consider... Handling the command completions in a workqueue could make sense, then a
Re: [RFCv2 01/10] xhci: Use command structures when calling xhci_configure_endpoint
Hi Mathias, comments below:
On Thu, Jan 30, 2014 at 6:10 AM, Mathias Nyman
wrote:
> To create a global command queue we need to fill a command structure
> for each entry on the command ring.
>
> We start by requiring xhci_configure_endpoint() to be called with
> a proper command structure. Functions xhci_check_maxpacket and
> xhci_check_bandwith
s/xhci_check_bandwith/xhci_check_bandwidth/
> that called xhci_configure_endpoint without a command strucure are fixed.
s/strucure/structure/
>
> A special case where an endpoint needs to be configured after reset, done in
> interrupt
> context is also changed to create a command strucure. (this command
s/strucure/structure/
> structure is not used yet, but will be later when we requre all queued
> commands
s/requre/require/
> to have a command strucure)
s/strucure/structure/
>
> Signed-off-by: Mathias Nyman
> ---
> drivers/usb/host/xhci-ring.c | 10 ++---
> drivers/usb/host/xhci.c | 97
>
> 2 files changed, 56 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 1e2f3f4..da83a844 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1180,12 +1180,15 @@ static void xhci_handle_cmd_reset_ep(struct xhci_hcd
> *xhci, int slot_id,
> * because the HW can't handle two commands being queued in a row.
> */
> if (xhci->quirks & XHCI_RESET_EP_QUIRK) {
> + struct xhci_command *command;
> + command = xhci_alloc_command(xhci, false, false, GFP_ATOMIC);
One cleanup we may want to consider in this series is making
xhci_alloc_command() more readable. My brain hurts when I see "false,
false" as I wonder what that means. I took a look and of the 4
possible ways to call xhci_alloc_command, we only use 2:
$ git grep xhci_alloc_command\(.*\) | grep -o
xhci_alloc_command\(xhci,.*,.*, | sort -u
xhci_alloc_command(xhci, false, true,
xhci_alloc_command(xhci, true, true,
So a first take is to just have a xhci_alloc_command() for "true,
true" and a xhci_alloc_command_no_ctx() for "false, true".
...uh oh, this series adds a usage of:
xhci_alloc_command(xhci, false, false,
...any reason we can't just use something like
xhci_alloc_command_no_ctx() instead?
Actually just make xhci_alloc_command() take an option in_ctx
parameter, when it is NULL xhci_alloc_command will allocate one,
otherwise it will use the passed in one.
> xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
> "Queueing configure endpoint command");
> xhci_queue_configure_endpoint(xhci,
> xhci->devs[slot_id]->in_ctx->dma, slot_id,
> false);
> xhci_ring_cmd_db(xhci);
> + kfree(command);
It's not really acceptable to add dead code in a patch. Consider the
case where some of the patches are reverted due to a regression. If,
for example we revert patch 2, the unused infrastructure in patch1
does not get deleted. Patch size minimization is good, but not when
it separates new infrastructure from its first user.
> } else {
> /* Clear our internal halted state and restart the ring(s) */
> xhci->devs[slot_id]->eps[ep_index].ep_state &= ~EP_HALTED;
> @@ -1439,7 +1442,7 @@ static void xhci_handle_cmd_config_ep(struct xhci_hcd
> *xhci, int slot_id,
> add_flags - SLOT_FLAG == drop_flags) {
> ep_state = virt_dev->eps[ep_index].ep_state;
> if (!(ep_state & EP_HALTED))
> - goto bandwidth_change;
> + return;
> xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
> "Completed config ep cmd - "
> "last ep index = %d, state = %d",
> @@ -1449,11 +1452,6 @@ static void xhci_handle_cmd_config_ep(struct xhci_hcd
> *xhci, int slot_id,
> ring_doorbell_for_active_rings(xhci, slot_id, ep_index);
> return;
> }
> -bandwidth_change:
> - xhci_dbg_trace(xhci, trace_xhci_dbg_context_change,
> - "Completed config ep cmd");
> - virt_dev->cmd_status = cmd_comp_code;
> - complete(&virt_dev->cmd_completion);
> return;
This change has no description in the change log. What's the reason
for deleting the goto?
> }
>
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 4265b48..a40485e 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1179,10 +1179,10 @@ static int xhci_configure_endpoint(struct xhci_hcd
> *xhci,
> static int xhci_check_maxpacket(struct xhci_hcd *xhci, unsigned int slot_id,
> unsigned int ep_index, struct urb *urb)
> {
> - struct xhci_container_ctx *in_ctx;
> struct xhci_container_ctx *out_ctx;
>
[RFCv2 01/10] xhci: Use command structures when calling xhci_configure_endpoint
To create a global command queue we need to fill a command structure
for each entry on the command ring.
We start by requiring xhci_configure_endpoint() to be called with
a proper command structure. Functions xhci_check_maxpacket and
xhci_check_bandwith
that called xhci_configure_endpoint without a command strucure are fixed.
A special case where an endpoint needs to be configured after reset, done in
interrupt
context is also changed to create a command strucure. (this command
structure is not used yet, but will be later when we requre all queued commands
to have a command strucure)
Signed-off-by: Mathias Nyman
---
drivers/usb/host/xhci-ring.c | 10 ++---
drivers/usb/host/xhci.c | 97
2 files changed, 56 insertions(+), 51 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 1e2f3f4..da83a844 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1180,12 +1180,15 @@ static void xhci_handle_cmd_reset_ep(struct xhci_hcd
*xhci, int slot_id,
* because the HW can't handle two commands being queued in a row.
*/
if (xhci->quirks & XHCI_RESET_EP_QUIRK) {
+ struct xhci_command *command;
+ command = xhci_alloc_command(xhci, false, false, GFP_ATOMIC);
xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
"Queueing configure endpoint command");
xhci_queue_configure_endpoint(xhci,
xhci->devs[slot_id]->in_ctx->dma, slot_id,
false);
xhci_ring_cmd_db(xhci);
+ kfree(command);
} else {
/* Clear our internal halted state and restart the ring(s) */
xhci->devs[slot_id]->eps[ep_index].ep_state &= ~EP_HALTED;
@@ -1439,7 +1442,7 @@ static void xhci_handle_cmd_config_ep(struct xhci_hcd
*xhci, int slot_id,
add_flags - SLOT_FLAG == drop_flags) {
ep_state = virt_dev->eps[ep_index].ep_state;
if (!(ep_state & EP_HALTED))
- goto bandwidth_change;
+ return;
xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
"Completed config ep cmd - "
"last ep index = %d, state = %d",
@@ -1449,11 +1452,6 @@ static void xhci_handle_cmd_config_ep(struct xhci_hcd
*xhci, int slot_id,
ring_doorbell_for_active_rings(xhci, slot_id, ep_index);
return;
}
-bandwidth_change:
- xhci_dbg_trace(xhci, trace_xhci_dbg_context_change,
- "Completed config ep cmd");
- virt_dev->cmd_status = cmd_comp_code;
- complete(&virt_dev->cmd_completion);
return;
}
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 4265b48..a40485e 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1179,10 +1179,10 @@ static int xhci_configure_endpoint(struct xhci_hcd
*xhci,
static int xhci_check_maxpacket(struct xhci_hcd *xhci, unsigned int slot_id,
unsigned int ep_index, struct urb *urb)
{
- struct xhci_container_ctx *in_ctx;
struct xhci_container_ctx *out_ctx;
struct xhci_input_control_ctx *ctrl_ctx;
struct xhci_ep_ctx *ep_ctx;
+ struct xhci_command *command;
int max_packet_size;
int hw_max_packet_size;
int ret = 0;
@@ -1207,18 +1207,24 @@ static int xhci_check_maxpacket(struct xhci_hcd *xhci,
unsigned int slot_id,
/* FIXME: This won't work if a non-default control endpoint
* changes max packet sizes.
*/
- in_ctx = xhci->devs[slot_id]->in_ctx;
- ctrl_ctx = xhci_get_input_control_ctx(xhci, in_ctx);
+
+ command = xhci_alloc_command(xhci, false, true, GFP_KERNEL);
+ if (!command)
+ return -ENOMEM;
+
+ command->in_ctx = xhci->devs[slot_id]->in_ctx;
+ ctrl_ctx = xhci_get_input_control_ctx(xhci, command->in_ctx);
if (!ctrl_ctx) {
xhci_warn(xhci, "%s: Could not get input context, bad
type.\n",
__func__);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto command_cleanup;
}
/* Set up the modified control endpoint 0 */
xhci_endpoint_copy(xhci, xhci->devs[slot_id]->in_ctx,
xhci->devs[slot_id]->out_ctx, ep_index);
- ep_ctx = xhci_get_ep_ctx(xhci, in_ctx, ep_index);
+ ep_ctx = xhci_get_ep_ctx(xhci, command->in_ctx, ep_index);
ep_ctx->ep_info2 &= cpu_to_le32(~MAX_PACKET_MASK);
ep_ctx->ep_info2 |= cpu_to_le32(MAX_PACKET(max_packet_size));
@@ -122
