On Tue, Jul 28, 2020 at 11:44:48AM +0200, Oliver Neukum wrote:
> From c37e910758b0a05d4c6b8d058974b1264f9d0aef Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oneukum@xxxxxxxx>
> Date: Tue, 28 Jul 2020 11:38:23 +0200
> Subject: [PATCH] USB: add mooring API
>
> This is a simplified and thereby better version of the anchor API.
> Anchors have the problem that they unanchor an URB upon giveback,
> which creates a window during which an URB is unanchored but not
> yet returned, leading to operations on anchors not having the
> semantics many driver errornously assume them to have.
> The new API keeps an URB on an anchor until it is explicitly
> unmoored.
So you will require the completion handler to unmoor its URBs. The idea
seems sound enough, although the patch itself contains a couple of small
errors (see below). The real problem is that any drivers relying on
usb_kill_anchored_urbs will still have to be fixed up by hand.
Also, it's not a good idea to store the new flag in urb->transfer_flags.
Accessing bitflags without proper locking isn't SMP-safe. Maybe you can
use some of the bits in urb->reject instead.
> Signed-off-by: Oliver Neukum <oneukum@xxxxxxx>
> ---
> drivers/usb/core/hcd.c | 3 ++-
> drivers/usb/core/urb.c | 27 ++++++++++++++++++++++++++-
> include/linux/usb.h | 3 +++
> 3 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index a33b849e8beb..861d30180709 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1640,7 +1640,8 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
> unmap_urb_for_dma(hcd, urb);
> usbmon_urb_complete(&hcd->self, urb, status);
> usb_anchor_suspend_wakeups(anchor);
> - usb_unanchor_urb(urb);
> + if (!urb->transfer_flags && URB_ANCHOR_PERMANENT)
Missing parens, and && instead of &.
> + usb_unanchor_urb(urb);
> if (likely(status == 0))
> usb_led_activity(USB_LED_EVENT_HOST);
>
> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index 7bc23469f4e4..1acfbd4e6323 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -123,7 +123,7 @@ EXPORT_SYMBOL_GPL(usb_get_urb);
> * This can be called to have access to URBs which are to be executed
> * without bothering to track them
> */
> -void usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor)
> +static void __usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor)
> {
> unsigned long flags;
>
> @@ -137,8 +137,20 @@ void usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor)
>
> spin_unlock_irqrestore(&anchor->lock, flags);
> }
> +
> +void inline usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor)
Shouldn't be marked inline.
> +{
> + __usb_anchor_urb(urb, anchor);
> +}
> EXPORT_SYMBOL_GPL(usb_anchor_urb);
>
> +void usb_moor_urb(struct urb *urb, struct usb_anchor *anchor)
> +{
> + urb->transfer_flags |= URB_ANCHOR_PERMANENT;
> + __usb_anchor_urb( urb, anchor);
Extra space character.
> +}
> +EXPORT_SYMBOL_GPL(usb_moor_urb);
> +
> static int usb_anchor_check_wakeup(struct usb_anchor *anchor)
> {
> return atomic_read(&anchor->suspend_wakeups) == 0 &&
> @@ -185,6 +197,19 @@ void usb_unanchor_urb(struct urb *urb)
> }
> EXPORT_SYMBOL_GPL(usb_unanchor_urb);
>
> +void usb_unmoor_urb(struct urb *urb)
> +{
> + struct usb_anchor *anchor;
> +
> + anchor = urb->anchor;
> + if (!anchor)
> + return;
> +
> + __usb_unanchor_urb(urb, anchor);
> + urb->transfer_flags &= ~URB_ANCHOR_PERMANENT;
Maybe move this line up before the check for !anchor.
Alan Stern
> +}
> +EXPORT_SYMBOL_GPL(usb_unmoor_urb);
> +
> /*-------------------------------------------------------------------*/
>
> static const int pipetypes[4] = {
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 20c555db4621..cacebdb01bfd 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -1347,6 +1347,7 @@ extern int usb_disabled(void);
> #define URB_SETUP_MAP_LOCAL 0x00200000 /* HCD-local setup packet */
> #define URB_DMA_SG_COMBINED 0x00400000 /* S-G entries were combined */
> #define URB_ALIGNED_TEMP_BUFFER 0x00800000 /* Temp buffer was alloc'd */
> +#define URB_ANCHOR_PERMANENT 0x01000000 /* Keep anchored across callback */
>
> struct usb_iso_packet_descriptor {
> unsigned int offset;
> @@ -1732,6 +1733,8 @@ extern void usb_anchor_suspend_wakeups(struct usb_anchor *anchor);
> extern void usb_anchor_resume_wakeups(struct usb_anchor *anchor);
> extern void usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor);
> extern void usb_unanchor_urb(struct urb *urb);
> +extern void usb_moor_urb(struct urb *urb, struct usb_anchor *anchor);
> +extern void usb_unmoor_urb(struct urb *urb);
> extern int usb_wait_anchor_empty_timeout(struct usb_anchor *anchor,
> unsigned int timeout);
> extern struct urb *usb_get_from_anchor(struct usb_anchor *anchor);
> --
> 2.16.4
>
