| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6e9d2094-0bf9-b2ac-29f3-99115b456fdb@linux.intel.com>
Date: Thu, 28 Sep 2023 16:31:52 +0300
From: Mathias Nyman <mathias.nyman@...ux.intel.com>
To: Wesley Cheng <quic_wcheng@...cinc.com>, mathias.nyman@...el.com,
gregkh@...uxfoundation.org, lgirdwood@...il.com,
broonie@...nel.org, perex@...ex.cz, tiwai@...e.com,
agross@...nel.org, andersson@...nel.org, konrad.dybcio@...aro.org,
robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
conor+dt@...nel.org, srinivas.kandagatla@...aro.org,
bgoswami@...cinc.com, Thinh.Nguyen@...opsys.com
Cc: linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
alsa-devel@...a-project.org, linux-arm-msm@...r.kernel.org,
devicetree@...r.kernel.org
Subject: Re: [PATCH v7 02/33] xhci: add helper to stop endpoint and wait for
completion
On 22.9.2023 0.48, Wesley Cheng wrote:
> From: Mathias Nyman <mathias.nyman@...ux.intel.com>
>
> Expose xhci_stop_endpoint_sync() which is a synchronous variant of
> xhci_queue_stop_endpoint(). This is useful for client drivers that are
> using the secondary interrupters, and need to stop/clean up the current
> session. The stop endpoint command handler will also take care of cleaning
> up the ring.
>
> Modifications to repurpose the new API into existing stop endpoint
> sequences was implemented by Wesley Cheng.
>
> Signed-off-by: Mathias Nyman <mathias.nyman@...ux.intel.com>
> Co-developed-by: Wesley Cheng <quic_wcheng@...cinc.com>
> Signed-off-by: Wesley Cheng <quic_wcheng@...cinc.com>
> ---
> drivers/usb/host/xhci-hub.c | 29 +++---------------
> drivers/usb/host/xhci.c | 60 +++++++++++++++++++++++++++----------
> drivers/usb/host/xhci.h | 2 ++
> 3 files changed, 50 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index 0054d02239e2..2f7309bdc922 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -489,7 +489,6 @@ EXPORT_SYMBOL_GPL(xhci_find_slot_id_by_port);
> static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
> {
> struct xhci_virt_device *virt_dev;
> - struct xhci_command *cmd;
> unsigned long flags;
> int ret;
> int i;
> @@ -501,10 +500,6 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
>
> trace_xhci_stop_device(virt_dev);
>
> - cmd = xhci_alloc_command(xhci, true, GFP_NOIO);
> - if (!cmd)
> - return -ENOMEM;
> -
> spin_lock_irqsave(&xhci->lock, flags);
> for (i = LAST_EP_INDEX; i > 0; i--) {
> if (virt_dev->eps[i].ring && virt_dev->eps[i].ring->dequeue) {
> @@ -521,7 +516,7 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
> if (!command) {
> spin_unlock_irqrestore(&xhci->lock, flags);
> ret = -ENOMEM;
> - goto cmd_cleanup;
> + goto out;
> }
>
> ret = xhci_queue_stop_endpoint(xhci, command, slot_id,
> @@ -529,30 +524,14 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
> if (ret) {
> spin_unlock_irqrestore(&xhci->lock, flags);
> xhci_free_command(xhci, command);
> - goto cmd_cleanup;
> + goto out;
> }
> }
> }
> - ret = xhci_queue_stop_endpoint(xhci, cmd, slot_id, 0, suspend);
> - if (ret) {
> - spin_unlock_irqrestore(&xhci->lock, flags);
> - goto cmd_cleanup;
> - }
> -
> - xhci_ring_cmd_db(xhci);
> spin_unlock_irqrestore(&xhci->lock, flags);
> + ret = xhci_stop_endpoint_sync(xhci, &virt_dev->eps[0], suspend);
I didn't take this new xhci_stop_endpoint_sync() helper into use as it causes an extra
xhci spinlock release and reacquire here.
Also the memory allocation flags differ, GFP_NOIO is turned into GFP_KERNEL after this change.
>
> - /* Wait for last stop endpoint command to finish */
> - wait_for_completion(cmd->completion);
> -
> - if (cmd->status == COMP_COMMAND_ABORTED ||
> - cmd->status == COMP_COMMAND_RING_STOPPED) {
> - xhci_warn(xhci, "Timeout while waiting for stop endpoint command\n");
> - ret = -ETIME;
> - }
> -
> -cmd_cleanup:
> - xhci_free_command(xhci, cmd);
> +out:
> return ret;
> }
>
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 3fd2b58ee1d3..163d533d6200 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -2758,6 +2758,46 @@ static int xhci_reserve_bandwidth(struct xhci_hcd *xhci,
> return -ENOMEM;
> }
>
> +/*
> + * Synchronous XHCI stop endpoint helper. Issues the stop endpoint command and
> + * waits for the command completion before returning.
> + */
> +int xhci_stop_endpoint_sync(struct xhci_hcd *xhci, struct xhci_virt_ep *ep, int suspend)
> +{
> + struct xhci_command *command;
> + unsigned long flags;
> + int ret;
> +
> + command = xhci_alloc_command(xhci, true, GFP_KERNEL);
> + if (!command)
> + return -ENOMEM;
> +
> + spin_lock_irqsave(&xhci->lock, flags);
> + ret = xhci_queue_stop_endpoint(xhci, command, ep->vdev->slot_id,
> + ep->ep_index, suspend);
> + if (ret < 0) {
> + spin_unlock_irqrestore(&xhci->lock, flags);
> + goto out;
> + }
> +
> + xhci_ring_cmd_db(xhci);
> + spin_unlock_irqrestore(&xhci->lock, flags);
> +
> + ret = wait_for_completion_timeout(command->completion, msecs_to_jiffies(3000));
> + if (!ret)
> + xhci_warn(xhci, "%s: Unable to stop endpoint.\n",
> + __func__);
> +
> + if (command->status == COMP_COMMAND_ABORTED ||
> + command->status == COMP_COMMAND_RING_STOPPED) {
> + xhci_warn(xhci, "Timeout while waiting for stop endpoint command\n");
> + ret = -ETIME;
> + }
> +out:
> + xhci_free_command(xhci, command);
> +
> + return ret;
> +}
>
> /* Issue a configure endpoint command or evaluate context command
> * and wait for it to finish.
> @@ -3078,7 +3118,7 @@ static void xhci_endpoint_reset(struct usb_hcd *hcd,
> struct xhci_virt_device *vdev;
> struct xhci_virt_ep *ep;
> struct xhci_input_control_ctx *ctrl_ctx;
> - struct xhci_command *stop_cmd, *cfg_cmd;
> + struct xhci_command *cfg_cmd;
> unsigned int ep_index;
> unsigned long flags;
> u32 ep_flag;
> @@ -3118,10 +3158,6 @@ static void xhci_endpoint_reset(struct usb_hcd *hcd,
> if (ep_flag == SLOT_FLAG || ep_flag == EP0_FLAG)
> return;
>
> - stop_cmd = xhci_alloc_command(xhci, true, GFP_NOWAIT);
> - if (!stop_cmd)
> - return;
> -
> cfg_cmd = xhci_alloc_command_with_ctx(xhci, true, GFP_NOWAIT);
> if (!cfg_cmd)
> goto cleanup;
> @@ -3144,23 +3180,16 @@ static void xhci_endpoint_reset(struct usb_hcd *hcd,
> goto cleanup;
> }
>
> - err = xhci_queue_stop_endpoint(xhci, stop_cmd, udev->slot_id,
> - ep_index, 0);
> + spin_unlock_irqrestore(&xhci->lock, flags);
> +
Same here, extra unlock -> lock, and GFP flags differ.
> + err = xhci_stop_endpoint_sync(xhci, ep, 0);
Thanks
Mathias
Powered by blists - more mailing lists