| Message ID | 5b3c30d268ea2d13d303759ef3dfee8d72830084.camel@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] extension of the anchor API | expand |
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. > 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. > +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. > + > 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..." > +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. > +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. > + } > + 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. Alan Stern
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
On Thu, Mar 25, 2021 at 05:04:25PM +0100, Oliver Neukum wrote: > Am Donnerstag, den 25.03.2021, 11:06 -0400 schrieb Alan Stern: > > > +: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" ? Hmmm. What happens if you submit an anchor's worth of URBs, but then you kill them in the reverse order (which is how you would normally want to cancel a bunch of URBs)? Since each URB gets moved to the end of the anchor's list when it completes, after they are all killed the list will be reversed. So the next time you submit the anchor, the order of URBs will be backward. If some of the URBs completed before they were killed, the order will be mixed up. Of course, if you never use the URBs on an anchor after killing it, this doesn't matter. Alan Stern
Am Donnerstag, den 25.03.2021, 14:38 -0400 schrieb Alan Stern: > On Thu, Mar 25, 2021 at 05:04:25PM +0100, Oliver Neukum wrote: > > Am Donnerstag, den 25.03.2021, 11:06 -0400 schrieb Alan Stern: > > > > +: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" ? > > Hmmm. What happens if you submit an anchor's worth of URBs, but then > you kill them in the reverse order (which is how you would normally want > to cancel a bunch of URBs)? Since each URB gets moved to the end of the > anchor's list when it completes, after they are all killed the list will > be reversed. So the next time you submit the anchor, the order of URBs > will be backward. If some of the URBs completed before they were > killed, the order will be mixed up. Yes. If the URBs themselves, as opposed to their payloads, are different, this will happen. Yet I am afraid we are looking at a necessary race condition here. If you cancel a non-atomic operation, you will need to deal with all possible intermediate stages of completion. > Of course, if you never use the URBs on an anchor after killing it, this > doesn't matter. Yes, to partially solve this issue I wrote usb_transfer_anchors() which allows you to separate those URBs you kill (or submit) by shifting them to another anchor. This is incomplete, as obviously something you kill may do a transfer. Regards Oliver
On Thu, Apr 08, 2021 at 11:23:05AM +0200, Oliver Neukum wrote: > Am Donnerstag, den 25.03.2021, 14:38 -0400 schrieb Alan Stern: > > On Thu, Mar 25, 2021 at 05:04:25PM +0100, Oliver Neukum wrote: > > > Am Donnerstag, den 25.03.2021, 11:06 -0400 schrieb Alan Stern: > > > > > +: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" ? > > > > Hmmm. What happens if you submit an anchor's worth of URBs, but then > > you kill them in the reverse order (which is how you would normally want > > to cancel a bunch of URBs)? Since each URB gets moved to the end of the > > anchor's list when it completes, after they are all killed the list will > > be reversed. So the next time you submit the anchor, the order of URBs > > will be backward. If some of the URBs completed before they were > > killed, the order will be mixed up. > > Yes. If the URBs themselves, as opposed to their payloads, are > different, this will happen. Yet I am afraid we are looking at a > necessary race condition here. If you cancel a non-atomic operation, > you will need to deal with all possible intermediate stages of > completion. That's not the point. The point is that the description you wrote is incorrect. I can imagine someone who doesn't understand the details of the anchor/mooring API creating an array of pointers to URBs, then filling in those URBs in the array's order. That would mess things up if a previous kill caused the order of the anchor list to be different from the array order. How about instead of moving URBs to the end of the list when they complete, you have the anchor maintain a pointer to the most recently submitted URB? Alan Stern > > Of course, if you never use the URBs on an anchor after killing it, this > > doesn't matter. > > Yes, to partially solve this issue I wrote > usb_transfer_anchors() > which allows you to separate those URBs you kill (or submit) > by shifting them to another anchor. This is incomplete, > as obviously something you kill may do a transfer. > > Regards > Oliver
Am Donnerstag, den 08.04.2021, 11:07 -0400 schrieb Alan Stern: > On Thu, Apr 08, 2021 at 11:23:05AM +0200, Oliver Neukum wrote: > > Am Donnerstag, den 25.03.2021, 14:38 -0400 schrieb Alan Stern: > > Yes. If the URBs themselves, as opposed to their payloads, are > > different, this will happen. Yet I am afraid we are looking at a > > necessary race condition here. If you cancel a non-atomic operation, > > you will need to deal with all possible intermediate stages of > > completion. > > That's not the point. The point is that the description you wrote is > incorrect. > > I can imagine someone who doesn't understand the details of the > anchor/mooring API creating an array of pointers to URBs, then filling > in those URBs in the array's order. That would mess things up if a > previous kill caused the order of the anchor list to be different from > the array order. OK, I will fix the description. > How about instead of moving URBs to the end of the list when they > complete, you have the anchor maintain a pointer to the most recently > submitted URB? That presumes that the URBs will finish in order. I don't think such an assumption can be made. Regards Oliver
On Mon, Apr 12, 2021 at 11:58:16AM +0200, Oliver Neukum wrote: > Am Donnerstag, den 08.04.2021, 11:07 -0400 schrieb Alan Stern: > > On Thu, Apr 08, 2021 at 11:23:05AM +0200, Oliver Neukum wrote: > > > Am Donnerstag, den 25.03.2021, 14:38 -0400 schrieb Alan Stern: > > > > Yes. If the URBs themselves, as opposed to their payloads, are > > > different, this will happen. Yet I am afraid we are looking at a > > > necessary race condition here. If you cancel a non-atomic operation, > > > you will need to deal with all possible intermediate stages of > > > completion. > > > > That's not the point. The point is that the description you wrote is > > incorrect. > > > > I can imagine someone who doesn't understand the details of the > > anchor/mooring API creating an array of pointers to URBs, then filling > > in those URBs in the array's order. That would mess things up if a > > previous kill caused the order of the anchor list to be different from > > the array order. > > OK, I will fix the description. > > > How about instead of moving URBs to the end of the list when they > > complete, you have the anchor maintain a pointer to the most recently > > submitted URB? > > That presumes that the URBs will finish in order. I don't think such > an assumption can be made. I don't understand -- I can't detect any such presumption. As far as I can tell, the only reason for maintaining the URBs in any particular order on the anchor list is so that usb_kill_anchored_urbs and usb_poison_anchored_urbs can kill them in reverse order of submission. THat's why the current code moves completed URBs to the end of the list. If you keep a pointer to the most recently submitted URB, killing them easy enough to do. Start with that URB, then go backward through the list (wrapping to the end when you reach the beginning of the list). The order in which the URBs complete doesn't matter, because trying to unlink a completed URB won't cause any harm. The only assumption here is that URBs get submitted in the list's order (possibly circularly) -- this should always be true. Alan Stern
Am Montag, den 12.04.2021, 11:06 -0400 schrieb Alan Stern: > On Mon, Apr 12, 2021 at 11:58:16AM +0200, Oliver Neukum wrote: > > That presumes that the URBs will finish in order. I don't think such > > an assumption can be made. > > I don't understand -- I can't detect any such presumption. OK, this shows that I am bad at explaining. > > As far as I can tell, the only reason for maintaining the URBs in any > particular order on the anchor list is so that usb_kill_anchored_urbs > and usb_poison_anchored_urbs can kill them in reverse order of > submission. THat's why the current code moves completed URBs to the end > of the list. No longer strictly true, as the API has a call to submit everything on an anchor, but I think it boils down to the same thing. > If you keep a pointer to the most recently submitted URB, killing them > easy enough to do. Start with that URB, then go backward through the > list (wrapping to the end when you reach the beginning of the list). Yes, but that supposes that the next on the list has not been resubmitted _before_ the one after it. If you do not keep the list ordered, but in the initial order, we can have the situation that A (happens most recently submitted) is followed by B and C, but C was submitted before B. > > The order in which the URBs complete doesn't matter, because trying to > unlink a completed URB won't cause any harm. As long as it stays completed. > The only assumption here > is that URBs get submitted in the list's order (possibly circularly) -- > this should always be true. I am afraid we cannot guarantee that. It might intuitively seem so, but nothing guarantees that all URBs are going to the same endpoint. Regards Oliver
On Wed, Apr 14, 2021 at 10:12:01AM +0200, Oliver Neukum wrote: > Am Montag, den 12.04.2021, 11:06 -0400 schrieb Alan Stern: > > On Mon, Apr 12, 2021 at 11:58:16AM +0200, Oliver Neukum wrote: > > > > That presumes that the URBs will finish in order. I don't think such > > > an assumption can be made. > > > > I don't understand -- I can't detect any such presumption. > > OK, this shows that I am bad at explaining. > > > > As far as I can tell, the only reason for maintaining the URBs in any > > particular order on the anchor list is so that usb_kill_anchored_urbs > > and usb_poison_anchored_urbs can kill them in reverse order of > > submission. THat's why the current code moves completed URBs to the end > > of the list. > > No longer strictly true, as the API has a call to submit everything > on an anchor, but I think it boils down to the same thing. > > > If you keep a pointer to the most recently submitted URB, killing them > > easy enough to do. Start with that URB, then go backward through the > > list (wrapping to the end when you reach the beginning of the list). > > Yes, but that supposes that the next on the list has not been > resubmitted _before_ the one after it. > > If you do not keep the list ordered, but in the initial order, > we can have the situation that A (happens most recently submitted) > is followed by B and C, but C was submitted before B. I think the only reasonable alternative is to move an URB to the end of the list when it is submitted, rather than when it completes. Have you considered doing it that way? The real problem with usb_submit_anchored_urbs is that the core can't know in what order the caller wants the URBs to be submitted. If the URBs are all more or less identical (for example, all reading the same amount of data from the same endpoint) then this doesn't matter. But if they aren't (for example, if they write different buffers or use different endpoints) it does matter. In the kerneldoc you can explain that if the anchor has not been used since its URBs were added then the URBs will be submitted in the order they were added to the anchor, but otherwise they will be submitted in an unspecified order, which may not be suitable. > > The order in which the URBs complete doesn't matter, because trying to > > unlink a completed URB won't cause any harm. > > As long as it stays completed. Rather, as long as they complete in order of submission. > > The only assumption here > > is that URBs get submitted in the list's order (possibly circularly) -- > > this should always be true. > > I am afraid we cannot guarantee that. It might intuitively seem so, > but nothing guarantees that all URBs are going to the same endpoint. I hadn't thought of that. Do anchors get used that way anywhere? Alan Stern
Am Mittwoch, den 14.04.2021, 10:56 -0400 schrieb Alan Stern: > On Wed, Apr 14, 2021 at 10:12:01AM +0200, Oliver Neukum wrote: > > Am Montag, den 12.04.2021, 11:06 -0400 schrieb Alan Stern: > > > On Mon, Apr 12, 2021 at 11:58:16AM +0200, Oliver Neukum wrote: > > > > That presumes that the URBs will finish in order. I don't think such > > > > an assumption can be made. > > > > > > I don't understand -- I can't detect any such presumption. > > > > OK, this shows that I am bad at explaining. > > > As far as I can tell, the only reason for maintaining the URBs in any > > > particular order on the anchor list is so that usb_kill_anchored_urbs > > > and usb_poison_anchored_urbs can kill them in reverse order of > > > submission. THat's why the current code moves completed URBs to the end > > > of the list. > > > > No longer strictly true, as the API has a call to submit everything > > on an anchor, but I think it boils down to the same thing. > > > > > If you keep a pointer to the most recently submitted URB, killing them > > > easy enough to do. Start with that URB, then go backward through the > > > list (wrapping to the end when you reach the beginning of the list). > > > > Yes, but that supposes that the next on the list has not been > > resubmitted _before_ the one after it. > > > > If you do not keep the list ordered, but in the initial order, > > we can have the situation that A (happens most recently submitted) > > is followed by B and C, but C was submitted before B. > > I think the only reasonable alternative is to move an URB to the end of > the list when it is submitted, rather than when it completes. Have you > considered doing it that way? No, that did not occur to me. Back to the drawing board. Still I have to put it somewhere when I anchor an URB. Head or tail? > The real problem with usb_submit_anchored_urbs is that the core can't > know in what order the caller wants the URBs to be submitted. If the I think the reasonable assumption is that they need to be submitted in the order they were anchored. > In the kerneldoc you can explain that if the anchor has not been used > since its URBs were added then the URBs will be submitted in the order > they were added to the anchor, but otherwise they will be submitted in > an unspecified order, which may not be suitable. Yes. > > > The order in which the URBs complete doesn't matter, because trying to > > > unlink a completed URB won't cause any harm. > > > > As long as it stays completed. > > Rather, as long as they complete in order of submission. > > > > The only assumption here > > > is that URBs get submitted in the list's order (possibly circularly) -- > > > this should always be true. > > > > I am afraid we cannot guarantee that. It might intuitively seem so, > > but nothing guarantees that all URBs are going to the same endpoint. > > I hadn't thought of that. Do anchors get used that way anywhere? I haven't found an example, but I thought it could not be ruled out. So you think that that case should be discouraged in documentation and henceforth ignored? So we do agree that we need the following: a - submit in the order you anchored b - kill or poison in the reverse order c - unpoison does not really matter but better do it in the submit order? Does that mean that the list needs to be kept ordered by sequence of submission? I think so. Regards Oliver
On Thu, Apr 15, 2021 at 01:23:21PM +0200, Oliver Neukum wrote: > Am Mittwoch, den 14.04.2021, 10:56 -0400 schrieb Alan Stern: > > On Wed, Apr 14, 2021 at 10:12:01AM +0200, Oliver Neukum wrote: > > > Am Montag, den 12.04.2021, 11:06 -0400 schrieb Alan Stern: > > > > On Mon, Apr 12, 2021 at 11:58:16AM +0200, Oliver Neukum wrote: > > > > > That presumes that the URBs will finish in order. I don't think such > > > > > an assumption can be made. > > > > > > > > I don't understand -- I can't detect any such presumption. > > > > > > OK, this shows that I am bad at explaining. > > > > As far as I can tell, the only reason for maintaining the URBs in any > > > > particular order on the anchor list is so that usb_kill_anchored_urbs > > > > and usb_poison_anchored_urbs can kill them in reverse order of > > > > submission. THat's why the current code moves completed URBs to the end > > > > of the list. > > > > > > No longer strictly true, as the API has a call to submit everything > > > on an anchor, but I think it boils down to the same thing. > > > > > > > If you keep a pointer to the most recently submitted URB, killing them > > > > easy enough to do. Start with that URB, then go backward through the > > > > list (wrapping to the end when you reach the beginning of the list). > > > > > > Yes, but that supposes that the next on the list has not been > > > resubmitted _before_ the one after it. > > > > > > If you do not keep the list ordered, but in the initial order, > > > we can have the situation that A (happens most recently submitted) > > > is followed by B and C, but C was submitted before B. > > > > I think the only reasonable alternative is to move an URB to the end of > > the list when it is submitted, rather than when it completes. Have you > > considered doing it that way? > > No, that did not occur to me. Back to the drawing board. > Still I have to put it somewhere when I anchor an URB. Head or tail? Tail, so that the list's order will be the same as the order in which URBs were added. > > The real problem with usb_submit_anchored_urbs is that the core can't > > know in what order the caller wants the URBs to be submitted. If the > > I think the reasonable assumption is that they need to be submitted > in the order they were anchored. > > > In the kerneldoc you can explain that if the anchor has not been used > > since its URBs were added then the URBs will be submitted in the order > > they were added to the anchor, but otherwise they will be submitted in > > an unspecified order, which may not be suitable. > > Yes. > > > > > The order in which the URBs complete doesn't matter, because trying to > > > > unlink a completed URB won't cause any harm. > > > > > > As long as it stays completed. > > > > Rather, as long as they complete in order of submission. > > > > > > The only assumption here > > > > is that URBs get submitted in the list's order (possibly circularly) -- > > > > this should always be true. > > > > > > I am afraid we cannot guarantee that. It might intuitively seem so, > > > but nothing guarantees that all URBs are going to the same endpoint. > > > > I hadn't thought of that. Do anchors get used that way anywhere? > > I haven't found an example, but I thought it could not be ruled out. > So you think that that case should be discouraged in documentation > and henceforth ignored? It should work okay. Really I was just concerned about usb_submit_anchored_urbs using the wrong order. People need to be aware that the the order may be wrong. Put a big warning about it in the kerneldoc. As for reordering the URBs in the list... I suppose it's unavoidable if the URBs can be for different endpoints. I think it makes more sense to do it when the URBs are submitted; that way you know that the list order matches the submission order at all times. But it means you have to be careful when submitting all the URBs at once -- especially if a completion handler resubmits -- because you want to avoid submitting an URB twice. > So we do agree that we need the following: > > a - submit in the order you > anchored > b - kill or poison in the reverse order > c - unpoison does not really matter but better do it in the submit > order? Doesn't matter. Whatever is easiest. > Does that mean that the list needs to be kept ordered by sequence > of submission? I think so. Yes. Alan Stern
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 +URBs. The order of URBs will be maintained in such a way +that completing URBs go to the back of the chain. +The whole anchor can then be manipulated as a whole. + 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 +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 +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. + + diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c index 3f0381344221..99b599d0056d 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -1631,6 +1631,7 @@ static void __usb_hcd_giveback_urb(struct urb *urb) struct usb_hcd *hcd = bus_to_hcd(urb->dev->bus); struct usb_anchor *anchor = urb->anchor; int status = urb->unlinked; + unsigned long flags; urb->hcpriv = NULL; if (unlikely((urb->transfer_flags & URB_SHORT_NOT_OK) && @@ -1641,7 +1642,19 @@ 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); + } else { + /* + * we do not kick it off the list + * but it needs to go to the end + * this needs to be done atomically + */ + spin_lock_irqsave(&anchor->lock, flags); + list_del(&urb->anchor_list); + list_add_tail(&urb->anchor_list, &anchor->urb_list); + spin_unlock_irqrestore(&anchor->lock, flags); + } 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 357b149b20d3..cd351c2ffd38 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 && @@ -170,6 +182,7 @@ void usb_unanchor_urb(struct urb *urb) return; anchor = urb->anchor; + urb->moored = false; if (!anchor) return; @@ -185,6 +198,86 @@ void usb_unanchor_urb(struct urb *urb) } EXPORT_SYMBOL_GPL(usb_unanchor_urb); +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); + } + spin_unlock_irqrestore(&anchor->lock, flags); + +bail_out: + if (error) + *error = rv; + return count; +} +EXPORT_SYMBOL_GPL(usb_submit_anchored_urbs); + +int usb_transfer_anchors( + struct usb_anchor *from, + struct usb_anchor *to, + int *error, + int (*transform_fn)(struct urb *urb, gfp_t gfp), + gfp_t gfp) +{ + int rv = 0; + int count = 0; + unsigned long flags; + struct urb *cur; + bool moor; + + spin_lock_irqsave(&from->lock, flags); + list_for_each_entry(cur, &from->urb_list, urb_list) { + usb_get_urb(cur); + moor = cur->moored; + __usb_unanchor_urb(cur, from); + __usb_anchor_urb(cur, to); + spin_unlock_irqrestore(&from->lock, flags); + rv = (transform_fn)(cur, gfp); + spin_lock_irqsave(&from->lock, flags); + if (rv < 0) { + /* put it back */ + __usb_unanchor_urb(cur, to); + __usb_anchor_urb(cur, from); + cur->moored = moor; + spin_unlock_irqrestore(&from->lock, flags); + goto bail_out; + } else { + count++; + } + usb_put_urb(cur); + } + spin_unlock_irqrestore(&from->lock, flags); + +bail_out: + if (error) + *error = rv; + return count; + +} +EXPORT_SYMBOL_GPL(usb_transfer_anchors); + +void inline usb_unmoor_urb(struct urb *urb) +{ + urb->moored = false; +} +EXPORT_SYMBOL_GPL(usb_unmoor_urb); + /*-------------------------------------------------------------------*/ static const int pipetypes[4] = { @@ -978,6 +1071,7 @@ struct urb *usb_get_from_anchor(struct usb_anchor *anchor) anchor_list); usb_get_urb(victim); __usb_unanchor_urb(victim, anchor); + victim->moored = false; } else { victim = NULL; } @@ -1005,6 +1099,7 @@ void usb_scuttle_anchored_urbs(struct usb_anchor *anchor) while (!list_empty(&anchor->urb_list)) { victim = list_entry(anchor->urb_list.prev, struct urb, anchor_list); + victim->moored = false; __usb_unanchor_urb(victim, anchor); } surely_empty = usb_anchor_check_wakeup(anchor); diff --git a/include/linux/usb.h b/include/linux/usb.h index d6a41841b93e..07c431dfeaf5 100644 --- a/include/linux/usb.h +++ b/include/linux/usb.h @@ -1567,6 +1567,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 @@ -1726,6 +1727,13 @@ extern void usb_kill_urb(struct urb *urb); extern void usb_poison_urb(struct urb *urb); extern void usb_unpoison_urb(struct urb *urb); extern void usb_block_urb(struct urb *urb); +extern int usb_submit_anchored_urbs(struct usb_anchor *anchor, int *error, gfp_t gfp); +extern int usb_transfer_anchors( + struct usb_anchor *from, + struct usb_anchor *to, + int *error, + int (*transform_fn)(struct urb *urb, gfp_t gfp), + gfp_t gfp); extern void usb_kill_anchored_urbs(struct usb_anchor *anchor); extern void usb_poison_anchored_urbs(struct usb_anchor *anchor); extern void usb_unpoison_anchored_urbs(struct usb_anchor *anchor); @@ -1734,6 +1742,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 inline 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);