Re: [RFCv2 01/10] xhci: Use command structures when calling xhci_configure_endpoint

From: Mathias Nyman
Date: Wed Feb 05 2014 - 09:51:29 EST

On 02/05/2014 04:21 AM, Dan Williams wrote:
Hi Mathias, comments below:


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->devs[slot_id]->in_ctx->dma, slot_id,
+ 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?

- 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);

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 all the async-queued commands could be allocated outside interrupt context. Not sure if this would expose or create some new races.

I'm not completely sure on what you have in mind when you say you want to "queue all command submission and completion events to a single threaded workqueue"

Thanks for taking a look at this

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at