[RFC]mooring API

[RFC]mooring API

[Oliver Neukum]

Hi,

why this new API? Eli found a race with the existing API. Many
drivers are acribing to it semantics it never had. Now we have
sort of a fix, but it is not really elegant.
The anchors have always been about making it easier to write drivers.
Hence if driver authors are assumuning that they have a power, we
better provide that facility. What users need is a facility
to group URBs together and get rid of them no questions asked.
It would be best if we can do that with minimal changes.

Here is a V2 taking into account Alan's remarks, and using a separate
flag.

	Regards
		Oliver

From 79df4240287b712bbe08404af7f900c3bccfca40 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
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.

Signed-off-by: Oliver Neukum <oneukum@suse.de>
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/usb/core/hcd.c |  4 +++-
 drivers/usb/core/urb.c | 27 ++++++++++++++++++++++++++-
 include/linux/usb.h    |  3 +++
 3 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index a33b849e8beb..e1d26cb595c3 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1640,7 +1640,9 @@ 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);
+	smp_rmb(); /* against usb_(un)moor_urb() */
+	if (!urb->moored)
+		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..ee3c6c7c2630 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 usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor)
+{
+	__usb_anchor_urb(urb, anchor);
+}
 EXPORT_SYMBOL_GPL(usb_anchor_urb);
 
+void usb_moor_urb(struct urb *urb, struct usb_anchor *anchor)
+{
+	urb->moored = true;
+	__usb_anchor_urb(urb, anchor);
+}
+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;
+
+	urb->moored = false;
+	anchor = urb->anchor;
+	if (!anchor)
+		return;
+
+	__usb_unanchor_urb(urb, anchor);
+}
+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..b9e1464a2552 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1565,6 +1565,7 @@ struct urb {
 	void *hcpriv;			/* private data for host controller */
 	atomic_t use_count;		/* concurrent submissions counter */
 	atomic_t reject;		/* submissions will fail */
+	bool moored;			/* the URB is moored not anchored */
 
 	/* public: documented fields in the urb that can be used by drivers */
 	struct list_head urb_list;	/* list head for use by the urb's
@@ -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

Re: [RFC]mooring API

[Alan Stern]

On Thu, Aug 06, 2020 at 04:07:07PM +0200, Oliver Neukum wrote:
> Hi,
> 
> why this new API? Eli found a race with the existing API. Many
> drivers are acribing to it semantics it never had. Now we have
> sort of a fix, but it is not really elegant.
> The anchors have always been about making it easier to write drivers.
> Hence if driver authors are assumuning that they have a power, we
> better provide that facility. What users need is a facility
> to group URBs together and get rid of them no questions asked.
> It would be best if we can do that with minimal changes.
> 
> Here is a V2 taking into account Alan's remarks, and using a separate
> flag.
> 
> 	Regards
> 		Oliver
> 
> From 79df4240287b712bbe08404af7f900c3bccfca40 Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oneukum@suse.com>
> 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.
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.de>
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>  drivers/usb/core/hcd.c |  4 +++-
>  drivers/usb/core/urb.c | 27 ++++++++++++++++++++++++++-
>  include/linux/usb.h    |  3 +++
>  3 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index a33b849e8beb..e1d26cb595c3 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1640,7 +1640,9 @@ 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);
> +	smp_rmb(); /* against usb_(un)moor_urb() */

What is the race you want to protect against?

Wouldn't it be better to require drivers that want to moor an URB to do 
so before submitting it?  And to unmoor an URB only in the completion 
handler?  Then there wouldn't be any race.

Alan Stern

> +	if (!urb->moored)
> +		usb_unanchor_urb(urb);
>  	if (likely(status == 0))
>  		usb_led_activity(USB_LED_EVENT_HOST);

Re: [RFC]mooring API

[Eli Billauer]

Hello,

I feel I got more credit that I deserve. Hans de Goede discovered this 
issue and solved a specific problem that was related to the race back in 
6ec4147. I was just lucky (or unlucky) enough to get a kernel panic on 
my machine due to another problem, for which I submitted a patch.

To me the anchor API is great. If there is unclearances about its API, I 
suppose docs would help. The fact that the URB is unanchored prior to 
calling the completer is intuitive, so there's a clear benefit in that.

This requires some ungraceful code where almost nobody's looking, but if 
that makes the common programmer's life easier, I think it's a good deal.

As I see it, the question is if there are other situations where this 
race condition could cause bugs. It's all about looking for situations 
where it's harmful to consider the anchor idle because its list is empty 
(i.e. not take into account that one URB might be completing).

Regards,
    Eli

On 06/08/20 17:07, Oliver Neukum wrote:
> Hi,
>
> why this new API? Eli found a race with the existing API. Many
> drivers are acribing to it semantics it never had. Now we have
> sort of a fix, but it is not really elegant.
> The anchors have always been about making it easier to write drivers.
> Hence if driver authors are assumuning that they have a power, we
> better provide that facility. What users need is a facility
> to group URBs together and get rid of them no questions asked.
> It would be best if we can do that with minimal changes.
>
> Here is a V2 taking into account Alan's remarks, and using a separate
> flag.
>
> 	Regards
> 		Oliver
>
> > From 79df4240287b712bbe08404af7f900c3bccfca40 Mon Sep 17 00:00:00 2001
> From: Oliver Neukum<oneukum@suse.com>
> 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.
>
> Signed-off-by: Oliver Neukum<oneukum@suse.de>
> Signed-off-by: Oliver Neukum<oneukum@suse.com>
> ---
>   drivers/usb/core/hcd.c |  4 +++-
>   drivers/usb/core/urb.c | 27 ++++++++++++++++++++++++++-
>   include/linux/usb.h    |  3 +++
>   3 files changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index a33b849e8beb..e1d26cb595c3 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1640,7 +1640,9 @@ 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);
> +	smp_rmb(); /* against usb_(un)moor_urb() */
> +	if (!urb->moored)
> +		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..ee3c6c7c2630 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 usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor)
> +{
> +	__usb_anchor_urb(urb, anchor);
> +}
>   EXPORT_SYMBOL_GPL(usb_anchor_urb);
>
> +void usb_moor_urb(struct urb *urb, struct usb_anchor *anchor)
> +{
> +	urb->moored = true;
> +	__usb_anchor_urb(urb, anchor);
> +}
> +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;
> +
> +	urb->moored = false;
> +	anchor = urb->anchor;
> +	if (!anchor)
> +		return;
> +
> +	__usb_unanchor_urb(urb, anchor);
> +}
> +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..b9e1464a2552 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -1565,6 +1565,7 @@ struct urb {
>   	void *hcpriv;			/* private data for host controller */
>   	atomic_t use_count;		/* concurrent submissions counter */
>   	atomic_t reject;		/* submissions will fail */
> +	bool moored;			/* the URB is moored not anchored */
>
>   	/* public: documented fields in the urb that can be used by drivers */
>   	struct list_head urb_list;	/* list head for use by the urb's
> @@ -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);
>    

Re: [RFC]mooring API

[Oliver Neukum]

Am Donnerstag, den 06.08.2020, 11:20 -0400 schrieb Alan Stern:
> On Thu, Aug 06, 2020 at 04:07:07PM +0200, Oliver Neukum wrote:
> > Hi,
> > 
> > why this new API? Eli found a race with the existing API. Many
> > drivers are acribing to it semantics it never had. Now we have
> > sort of a fix, but it is not really elegant.
> > The anchors have always been about making it easier to write drivers.
> > Hence if driver authors are assumuning that they have a power, we
> > better provide that facility. What users need is a facility
> > to group URBs together and get rid of them no questions asked.
> > It would be best if we can do that with minimal changes.
> > 
> > Here is a V2 taking into account Alan's remarks, and using a separate
> > flag.
> > 
> > 	Regards
> > 		Oliver
> > 
> > From 79df4240287b712bbe08404af7f900c3bccfca40 Mon Sep 17 00:00:00 2001
> > From: Oliver Neukum <oneukum@suse.com>
> > 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.
> > 
> > Signed-off-by: Oliver Neukum <oneukum@suse.de>
> > Signed-off-by: Oliver Neukum <oneukum@suse.com>
> > ---
> >  drivers/usb/core/hcd.c |  4 +++-
> >  drivers/usb/core/urb.c | 27 ++++++++++++++++++++++++++-
> >  include/linux/usb.h    |  3 +++
> >  3 files changed, 32 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> > index a33b849e8beb..e1d26cb595c3 100644
> > --- a/drivers/usb/core/hcd.c
> > +++ b/drivers/usb/core/hcd.c
> > @@ -1640,7 +1640,9 @@ 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);
> > +	smp_rmb(); /* against usb_(un)moor_urb() */
> 
> What is the race you want to protect against?

It looks to me like I need to be sure that the URB could have
been moored on another CPU.

> Wouldn't it be better to require drivers that want to moor an URB to do 
> so before submitting it?  And to unmoor an URB only in the completion 
> handler?  Then there wouldn't be any race.

I am afraid we cannot do that, as it must be possible to unmoor an
URb that needs to be unmoored before it is submitted, even on
another CPU.

What do you think of the API itself?

	Regards
		Oliver

Re: [RFC]mooring API

[Oliver Neukum]

Am Donnerstag, den 06.08.2020, 19:14 +0300 schrieb Eli Billauer:
> Hello,
> 
> I feel I got more credit that I deserve. Hans de Goede discovered this 
> issue and solved a specific problem that was related to the race back in 
> 6ec4147. I was just lucky (or unlucky) enough to get a kernel panic on 
> my machine due to another problem, for which I submitted a patch.
> 
> To me the anchor API is great. If there is unclearances about its API, I 
> suppose docs would help. The fact that the URB is unanchored prior to 
> calling the completer is intuitive, so there's a clear benefit in that.

But is it necessary? DO you ever move URBs between anchors?

> This requires some ungraceful code where almost nobody's looking, but if 
> that makes the common programmer's life easier, I think it's a good deal.

It would be, if the deal is necessary. In hindsight it still looks
to me like completion should unanchor an URB.

	Regards
		Oliver

Re: [RFC]mooring API

[Alan Stern]

On Mon, Aug 10, 2020 at 04:35:18PM +0200, Oliver Neukum wrote:
> Am Donnerstag, den 06.08.2020, 11:20 -0400 schrieb Alan Stern:

> > > --- a/drivers/usb/core/hcd.c
> > > +++ b/drivers/usb/core/hcd.c
> > > @@ -1640,7 +1640,9 @@ 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);
> > > +	smp_rmb(); /* against usb_(un)moor_urb() */
> > 
> > What is the race you want to protect against?
> 
> It looks to me like I need to be sure that the URB could have
> been moored on another CPU.

You mean, the URB could have been anchored already, but another CPU 
could moor it just as this CPU is unachoring it?

For one thing, I doubt that a single smp_rmb() will provide any real 
protection.  For another, it would be best to just avoid races in the 
first place.  Can you think of any use case where it makes sense to moor 
an URB just as it is completing (or indeed at any time while it is 
active)?

> > Wouldn't it be better to require drivers that want to moor an URB to do 
> > so before submitting it?  And to unmoor an URB only in the completion 
> > handler?  Then there wouldn't be any race.
> 
> I am afraid we cannot do that, as it must be possible to unmoor an
> URb that needs to be unmoored before it is submitted, even on
> another CPU.

I should have said: require drivers that want to unmoor an URB to do so 
either before it is submitted or in (or after) the completion handler.  
In other words, don't moor or unmoor an URB while it is active.  How 
about that?

> What do you think of the API itself?

It looks fine as far as I can tell.  But I haven't worked on any drivers 
that use anchors.

Alan Stern

Re: [RFC]mooring API

[Eli Billauer]

On 10/08/20 17:39, Oliver Neukum wrote:
>> To me the anchor API is great. If there is unclearances about its API, I
>> >  suppose docs would help. The fact that the URB is unanchored prior to
>> >  calling the completer is intuitive, so there's a clear benefit in that.
>>      
> But is it necessary? DO you ever move URBs between anchors?
>    
I don't think it's relevant what I would or wouldn't do. The question is 
if someone out there would do that. Without any guidelines saying it's 
explicitly disallowed, some will do it. Actually, even if there are such 
guidelines.
>    
>> >  This requires some ungraceful code where almost nobody's looking, but if
>> >  that makes the common programmer's life easier, I think it's a good deal.
>>      
> It would be, if the deal is necessary. In hindsight it still looks
> to me like completion should unanchor an URB.
>    
Once again, I'm not sure that matters so much anymore. The anchor API 
has been around for a while, and there are drivers using it. Adding 
another API will undoubtedly create some confusion. That's why I think 
that the question should be what is functionally flawed with the current 
API, and if that can be fixed.

Regards,
    Eli

Re: [RFC]mooring API

[Oliver Neukum]

Am Montag, den 10.08.2020, 11:17 -0400 schrieb Alan Stern:
> On Mon, Aug 10, 2020 at 04:35:18PM +0200, Oliver Neukum wrote:

> 
> You mean, the URB could have been anchored already, but another CPU 
> could moor it just as this CPU is unachoring it?

I was worrying about accidental unanchoring of a moored URB, actually.

> For one thing, I doubt that a single smp_rmb() will provide any real 
> protection.  For another, it would be best to just avoid races in the 
> first place.  Can you think of any use case where it makes sense to moor 
> an URB just as it is completing (or indeed at any time while it is 
> active)?

No, I am removing it.


> I should have said: require drivers that want to unmoor an URB to do so 
> either before it is submitted or in (or after) the completion handler.  
> In other words, don't moor or unmoor an URB while it is active.  How 
> about that?

Entirely reasonable.

> > What do you think of the API itself?
> 
> It looks fine as far as I can tell.  But I haven't worked on any drivers 
> that use anchors.

Writing decent documentation for that is actually hard. Any sugestions?

	Regards
		Oliver

From 3caa2aa250808a86529302c6553dfcce93bf2927 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
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.

Signed-off-by: Oliver Neukum <oneukum@suse.de>
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 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..5cebbc2b3600 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->moored)
+		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..ee3c6c7c2630 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 usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor)
+{
+	__usb_anchor_urb(urb, anchor);
+}
 EXPORT_SYMBOL_GPL(usb_anchor_urb);
 
+void usb_moor_urb(struct urb *urb, struct usb_anchor *anchor)
+{
+	urb->moored = true;
+	__usb_anchor_urb(urb, anchor);
+}
+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;
+
+	urb->moored = false;
+	anchor = urb->anchor;
+	if (!anchor)
+		return;
+
+	__usb_unanchor_urb(urb, anchor);
+}
+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..b9e1464a2552 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1565,6 +1565,7 @@ struct urb {
 	void *hcpriv;			/* private data for host controller */
 	atomic_t use_count;		/* concurrent submissions counter */
 	atomic_t reject;		/* submissions will fail */
+	bool moored;			/* the URB is moored not anchored */
 
 	/* public: documented fields in the urb that can be used by drivers */
 	struct list_head urb_list;	/* list head for use by the urb's
@@ -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

Re: [RFC]mooring API

[Alan Stern]

On Tue, Aug 11, 2020 at 07:39:12PM +0200, Oliver Neukum wrote:
> Am Montag, den 10.08.2020, 11:17 -0400 schrieb Alan Stern:
> > On Mon, Aug 10, 2020 at 04:35:18PM +0200, Oliver Neukum wrote:
> 
> > 
> > You mean, the URB could have been anchored already, but another CPU 
> > could moor it just as this CPU is unachoring it?
> 
> I was worrying about accidental unanchoring of a moored URB, actually.
> 
> > For one thing, I doubt that a single smp_rmb() will provide any real 
> > protection.  For another, it would be best to just avoid races in the 
> > first place.  Can you think of any use case where it makes sense to moor 
> > an URB just as it is completing (or indeed at any time while it is 
> > active)?
> 
> No, I am removing it.
> 
> 
> > I should have said: require drivers that want to unmoor an URB to do so 
> > either before it is submitted or in (or after) the completion handler.  
> > In other words, don't moor or unmoor an URB while it is active.  How 
> > about that?
> 
> Entirely reasonable.
> 
> > > What do you think of the API itself?
> > 
> > It looks fine as far as I can tell.  But I haven't worked on any drivers 
> > that use anchors.
> 
> Writing decent documentation for that is actually hard. Any sugestions?

In the kerneldoc for usb_moor_urb and usb_unmoor_urb, mention that to 
avoid races, URBs should be moored or unmoored only when they are not 
active: before they are submitted, in the completion handler routine, or 
after the completion handler has run.

You'll have to figure out a good way of explaining the advantages of 
forcing drivers to do their own unmooring.  Maybe urb_destroy() should 
check for URBs that are still moored.

Alan Stern