Re: [RFC]extension of the anchor API

[Oliver Neukum <oneukum@suse.com>]

Am Donnerstag, den 25.03.2021, 11:06 -0400 schrieb Alan Stern:
> On Thu, Mar 25, 2021 at 12:03:33PM +0100, Oliver Neukum wrote:
> > Hi,
> > 
> > looking at the anchor API it seems to me that it is
> > weak due to removing an URB from its anchor upon completion.
> > That is not always what drivers want. If you throw away
> > the URB after usage, then this is good, as you typically do on a write
> > path. If, however, you are recieving, you typically reuse the URB
> > and you reanchor it right in the completion handler.
> > 
> > To make this easier I am proposing a feature called a "mooring"
> > which makes the association between anchor and URB permanent
> > and a few helper functions.
> > 
> > What do you think?
> 
> Have you considered just changing the anchor API instead?  It would 
> require auditing all uses of anchors (get rid of manual re-anchorings 
> and add manual un-anchorings), but it might end up being simpler.

Probably not. I looked at drivers and fire and forget is done a lot.
Transmitting and recieving have different needs. I tried to unify
code paths as much as possible.
> 
> > 	Regards
> > 		Oliver
> > 
> > From 577795900c90d7a40b082935747086be94d7f8be Mon Sep 17 00:00:00 2001
> > From: Oliver Neukum <oneukum@suse.com>
> > Date: Tue, 28 Jul 2020 11:38:23 +0200
> > Subject: [PATCH 1/2] 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.com>
> > ---
> >  Documentation/driver-api/usb/anchors.rst | 39 +++++++++-
> >  drivers/usb/core/hcd.c                   | 15 +++-
> >  drivers/usb/core/urb.c                   | 97 +++++++++++++++++++++++-
> >  include/linux/usb.h                      | 10 +++
> >  4 files changed, 157 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/driver-api/usb/anchors.rst b/Documentation/driver-api/usb/anchors.rst
> > index 4b248e691bd6..501f46aea75f 100644
> > --- a/Documentation/driver-api/usb/anchors.rst
> > +++ b/Documentation/driver-api/usb/anchors.rst
> > @@ -1,8 +1,8 @@
> >  USB Anchors
> >  ~~~~~~~~~~~
> >  
> > -What is anchor?
> > -===============
> > +What is an anchor?
> > +==================
> >  
> >  A USB driver needs to support some callbacks requiring
> >  a driver to cease all IO to an interface. To do so, a
> > @@ -12,6 +12,19 @@ for them. The anchor is a data structure takes care of
> >  keeping track of URBs and provides methods to deal with
> >  multiple URBs.
> >  
> > +What is a mooring?
> > +==================
> > +
> > +A mooring is a permanent anchoring that persist across
> > +the completion of an URB.
> > +The drawback of anchors is that there is an unavoidable
> > +window between taking an URB off an anchor for completion
> > +and the completion itself.
> > +A mooring acts as a permanent anchor to which you can add
> 
> This says that the anchor is permanent, but what you really mean is
> that the URB is permanently added to the anchor.

Noted

> 
> > +URBs. The order of URBs will be maintained in such a way
> > +that completing URBs go to the back of the chain.
> 
> Is that really helpful to anybody?  Why does the order of URBs on the
> anchor (or mooring) matter?  Rather than assuming the URBs are in some
> particular order, drivers could iterate over the URBs attached to an
> anchor, given the appropriate API.
> 
> > +The whole anchor can then be manipulated as a whole.
> 
> Too many "whole"s.

Noted

> > +
> >  Allocation and Initialisation
> >  =============================
> >  
> > @@ -35,6 +48,13 @@ is automatic. A function is provided to forcibly finish (kill)
> >  all URBs associated with an anchor.
> >  Furthermore, disassociation can be made with :c:func:`usb_unanchor_urb`
> >  
> > +Association and disassociation of URBs with moorings
> > +====================================================
> > +
> > +An association of URBs to an anchor is made by an explicit
> 
> This should be clearer.  "An URB can be moored to an anchor by 
> calling..."

Noted

> > +call to :c:func:`usb_moor_urb`. A moored URB can be turned
> > +into an anchored URB by :c:func:`usb_unmoor_urb`
> > +
> >  Operations on multitudes of URBs
> >  ================================
> >  
> > @@ -81,3 +101,18 @@ Returns the oldest anchored URB of an anchor. The URB is unanchored
> >  and returned with a reference. As you may mix URBs to several
> >  destinations in one anchor you have no guarantee the chronologically
> >  first submitted URB is returned.
> > +
> > +:c:func:`usb_submit_anchored_urbs`
> > +---------------------------------
> > +
> > +The URBs contained in anchor are chronologically submitted until
> 
> "chronologically" is the wrong word.  They are submitted in the order
> of the anchor's list, which is the same as the order that an iterator
> would use.

OK. "In the same sequence as they were anchored" ?
> 
> > +they are all submitted or an error happens during submission.
> > +
> > +:c:func:`usb_transfer_anchors`
> > +------------------------------
> > +
> > +Transfers URBs from an anchor to another anchor by means of a
> > +transform function you pass to the method. It proceeds until
> > +all URBs are transfered or an error is encountered during transfer.
> > +
> > +
> > +int usb_submit_anchored_urbs(struct usb_anchor *anchor, int *error, gfp_t gfp)
> > +{
> > +	int rv = 0;
> > +	int count = 0;
> > +	unsigned long flags;
> > +	struct urb *cur;
> > +
> > +	spin_lock_irqsave(&anchor->lock, flags);
> > +	list_for_each_entry(cur, &anchor->urb_list, urb_list) {
> > +		usb_get_urb(cur);
> > +		spin_unlock_irqrestore(&anchor->lock, flags);
> > +		rv = usb_submit_urb(cur, gfp);
> > +		if (!rv) {
> > +			count++;
> > +		} else {
> > +			usb_put_urb(cur);
> > +			goto bail_out;
> > +		}
> > +		spin_lock_irqsave(&anchor->lock, flags);
> > +		usb_put_urb(cur);
> 
> When an URB is successfully submitted, the core takes a reference to
> it.  So you can do the usb_put_urb immediately after usb_submit_urb.

OK.


> > +	}
> > +	spin_unlock_irqrestore(&anchor->lock, flags);
> > +
> > +bail_out:
> > +	if (error)
> > +		*error = rv;
> > +	return count;
> > +}
> > +EXPORT_SYMBOL_GPL(usb_submit_anchored_urbs);
> 
> I didn't carefully review the rest, but it seems okay.

Very good. I am preparing a V2 and then look for additional testers.

	Regards
		Oliver