Re: [RFC PATCH] usb/core: add current frame_number sysfs attr to hcd
On Tue, 19 Feb 2013, Stefan Tauner wrote: > On Tue, 19 Feb 2013 13:27:49 -0500 (EST) > Alan Stern wrote: > > > > Adding a completely new API that returns a timestamp associated with > > > the start/end/fixed offset of a frame number sounds like a perfect > > > solution for my problem. I am not sure what you mean with completion > > > (interrupt). AFAICS that term is used mainly/only in relation to > > > (signaling) completed URBs. How does that fit together? What can I do > > > to make it happen? > > > > Yes, completion interrupts are used for signalling when an URB > > completes. That's most of the signalling the CPU gets from a host > > controller; other things like port connect and disconnect are > > comparatively rare. > > Hm but there are also other potential interrupt sources at least for > some HCD types that could be used, e.g. OHCI_INTR_SF at the start of > frames in OHCI or the Frame List Rollover interrupt in EHCI? Yes. (Although the Frame List Rollover interrupt fires at a rather low frequency.) Don't forget -- if this ever does manage to get into the kernel, it will be necessary to modify more than just the EHCI and OHCI drivers. The kernel contains a whole bunch of host controller drivers, and every one would need to be updated. > The drawback of using other interrupt sources like OHCI_INTR_SF would > be of course that they... interrupt, a lot :) > Maybe the gathering through normally unused interrupts could be > enabled/disabled with ioctls (and while that is disabled the value is > only updated on completed URBs)? Sure, if you want to make things even more complicated... > What kind of interface do you propose? I would say a single file in > sysfs with one line frame counter and one line timestamp, but the sysfs > docs clearly don't encourage mixing types like this, if I understand > it right. Of course we need to make sure that the corresponding values > can be retrieved safely in sync... In fact, I think this interface is so unlikely to be used by more than a few people that I propose not adding it at all. :-) For your own purposes, of course, you can do whatever you want. > Do you know about a comparable API I could look at? Nothing other than the ones you're already aware of. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] usb/core: add current frame_number sysfs attr to hcd
On Tue, 19 Feb 2013 13:27:49 -0500 (EST) Alan Stern wrote: > > Adding a completely new API that returns a timestamp associated with > > the start/end/fixed offset of a frame number sounds like a perfect > > solution for my problem. I am not sure what you mean with completion > > (interrupt). AFAICS that term is used mainly/only in relation to > > (signaling) completed URBs. How does that fit together? What can I do > > to make it happen? > > Yes, completion interrupts are used for signalling when an URB > completes. That's most of the signalling the CPU gets from a host > controller; other things like port connect and disconnect are > comparatively rare. Hm but there are also other potential interrupt sources at least for some HCD types that could be used, e.g. OHCI_INTR_SF at the start of frames in OHCI or the Frame List Rollover interrupt in EHCI? The drawback of using other interrupt sources like OHCI_INTR_SF would be of course that they... interrupt, a lot :) Maybe the gathering through normally unused interrupts could be enabled/disabled with ioctls (and while that is disabled the value is only updated on completed URBs)? > The way they fit together is that completion IRQs are raised at > microframe boundaries. Hence, if the interrupt handler stores a > realtime clock value together with the new microframe number, there's > no need for polling. > > The drawback, of course, is that there won't _be_ any completion > interrupts until an active URB completes. Presumably that won't be a > problem for you, but it is a weakness for a general-purpose API. Yes, that is not very nice in general, but it's certainly an improvement over now. What kind of interface do you propose? I would say a single file in sysfs with one line frame counter and one line timestamp, but the sysfs docs clearly don't encourage mixing types like this, if I understand it right. Of course we need to make sure that the corresponding values can be retrieved safely in sync... Do you know about a comparable API I could look at? -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] usb/core: add current frame_number sysfs attr to hcd
On Tue, 19 Feb 2013, Stefan Tauner wrote: > > Using frame boundaries for time synchronization at the user level is > > possibly a defensible reason for exporting frame numbers. But there > > are better ways to accomplish this goal. For example, there could be > > an API that returns both a realtime value and a microframe number as of > > the next time a completion interrupt occurs. > > Adding a completely new API that returns a timestamp associated with > the start/end/fixed offset of a frame number sounds like a perfect > solution for my problem. I am not sure what you mean with completion > (interrupt). AFAICS that term is used mainly/only in relation to > (signaling) completed URBs. How does that fit together? What can I do > to make it happen? Yes, completion interrupts are used for signalling when an URB completes. That's most of the signalling the CPU gets from a host controller; other things like port connect and disconnect are comparatively rare. The way they fit together is that completion IRQs are raised at microframe boundaries. Hence, if the interrupt handler stores a realtime clock value together with the new microframe number, there's no need for polling. The drawback, of course, is that there won't _be_ any completion interrupts until an active URB completes. Presumably that won't be a problem for you, but it is a weakness for a general-purpose API. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] usb/core: add current frame_number sysfs attr to hcd
On Mon, 18 Feb 2013 22:10:45 -0500 (EST) Alan Stern wrote: > On Tue, 19 Feb 2013, Stefan Tauner wrote: > > > Therefore I am currently playing around with a > > usb_hcd_get_real_frame_number() which uses a ehci_get_real_frame() > > function hacked into ehci-hcd.c which returns the actual SOF counter > > without the artificial horizon limit, but this is of course not > > committable. I wonder though why the limitation is there in the first > > place. Most users of usb_get_current_frame_number() seem rather unhappy > > with the limitation (e.g. drivers/media/usb/uvc/uvc_video.c) or don't > > care because they cap the returned value to the (locally defined(!)) > > minimum of the schedule horizon (0xFF everywhere AFAICS). Only very few > > use it directly to determine a possible scheduling (e.g. > > drivers/isdn/hisax/st5481_d.c which looks rather dubious to my naive > > eyes BTW). > > I wasn't aware that anyone was trying to use this API. In my opinion > doing so is probably a mistake. Most of what I have seen is for debugging prints only, but not all. No idea if that is of any use under the discussed circumstances (of inaccuracies). > > So from the user's perspective I don't see a reason why the artificially > > limited reply should be returned instead of the complete value. I have > > not checked if the raw value is available on all HCDs, but I assume it > > is. Wouldn't it make sense to change usb_hcd_get_frame_number() to > > return the raw value and add another one that returns the actual limit > > of the schedule horizon e.g. periodic_size in the case of EHCI(?) for > > those users that want to scheduler packets? > > Users should never want to schedule packets. Fair enough, I just mentioned it because I don't want to break any existing users. > > If you think refactoring get_frame_number should be done I would be > > glad to work on it. A few pointers to what the result should look like > > and any obvious pitfalls would be appreciated in that case. > > Using frame boundaries for time synchronization at the user level is > possibly a defensible reason for exporting frame numbers. But there > are better ways to accomplish this goal. For example, there could be > an API that returns both a realtime value and a microframe number as of > the next time a completion interrupt occurs. Adding a completely new API that returns a timestamp associated with the start/end/fixed offset of a frame number sounds like a perfect solution for my problem. I am not sure what you mean with completion (interrupt). AFAICS that term is used mainly/only in relation to (signaling) completed URBs. How does that fit together? What can I do to make it happen? -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] usb/core: add current frame_number sysfs attr to hcd
On Tue, 19 Feb 2013, Stefan Tauner wrote: > Well, I am not entirely sure if even I want this in (as is) yet, > because it is just a part of a complete solution for my problem and I > can't imagine a reason why user space would want to know this odd > value as is. > So I can not give a committable rationale yet, but I can sum up what > I am trying to do eventually: I try to map SOF counters to > CLOCK_REALTIME timestamps in user space. Later I want to use that to > establish a global time base on microcontrollers connected via USB > without the hassle of running NTP or PTP over USB. In user space I am > waiting for changes in the frame_counter sysfs file by reading it and > comparing its contents to values previously read in a busy loop, which > is of course not very elegant but it works - apart from the fact that > the returned values are not the raw values (and as Alan noted, > might also be off a bit)... :) > > Therefore I am currently playing around with a > usb_hcd_get_real_frame_number() which uses a ehci_get_real_frame() > function hacked into ehci-hcd.c which returns the actual SOF counter > without the artificial horizon limit, but this is of course not > committable. I wonder though why the limitation is there in the first > place. Most users of usb_get_current_frame_number() seem rather unhappy > with the limitation (e.g. drivers/media/usb/uvc/uvc_video.c) or don't > care because they cap the returned value to the (locally defined(!)) > minimum of the schedule horizon (0xFF everywhere AFAICS). Only very few > use it directly to determine a possible scheduling (e.g. > drivers/isdn/hisax/st5481_d.c which looks rather dubious to my naive > eyes BTW). I wasn't aware that anyone was trying to use this API. In my opinion doing so is probably a mistake. In the future, all scheduling of periodic transfers will be done by the host controller driver; the upper-layer drivers won't have any choice in the matter. Hence they shouldn't care about the current frame number. (Note that drivers can still determine in which frames individual isochronous packets were sent. That information is available from urb->start_frame.) > So from the user's perspective I don't see a reason why the artificially > limited reply should be returned instead of the complete value. I have This was probably the result of some early design decisions which have not turned out well. > not checked if the raw value is available on all HCDs, but I assume it > is. Wouldn't it make sense to change usb_hcd_get_frame_number() to > return the raw value and add another one that returns the actual limit > of the schedule horizon e.g. periodic_size in the case of EHCI(?) for > those users that want to scheduler packets? Users should never want to schedule packets. > If not, I would really like to see the documentation of > usb_get_current_frame_number() be changed to make it more clear that it > is not the raw SOF value and *why*. I have not figured out what > *exactly* it is yet/how the iso scheduling works... grasping the kernel > - even a subsystem - is quite an effort :) If/when I do I will send a > documentation patch. It's going to get more complicated -- in one sense, that is: more complicated for the controller driver, but less complicated for everybody else. As far as I'm concerned, the usb_get_current_frame_number() API can be removed (unless somebody suggests a good use for it). > If you think refactoring get_frame_number should be done I would be > glad to work on it. A few pointers to what the result should look like > and any obvious pitfalls would be appreciated in that case. Using frame boundaries for time synchronization at the user level is possibly a defensible reason for exporting frame numbers. But there are better ways to accomplish this goal. For example, there could be an API that returns both a realtime value and a microframe number as of the next time a completion interrupt occurs. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] usb/core: add current frame_number sysfs attr to hcd
On Mon, 18 Feb 2013 07:20:56 -0800 Greg KH wrote: > On Mon, Feb 18, 2013 at 03:34:07PM +0100, Stefan Tauner wrote: > > This allows user space to retrieve the current frame number on a USB > > as returned by usb_get_current_frame_number(...) via sysfs. > > > > Signed-off-by: Stefan Tauner > > --- > > That's sadly not necessarily the raw value seen on the bus because > > the individual driver functions called by usb_get_current_frame_number(...) > > seem to limit the possible range to the "schedule horizon" of isochronous > > packets. IMHO the function name is a misnomer and I would like to hear your > > opinion on this, a possible new name for it and a better way to retrieve the > > real/raw value. > > > > The rationale for this patch can be found in the thread with the subject > > "Correlating SOF with host system time" from last december > > (201212042020.qb4kkt0n008...@mail2.student.tuwien.ac.at). > > My whole use case/idea was kinda frowned upon then, but there was little > > discussion about how it should/could be implemented in the kernel code in > > case one really wants to try it. > > For those of us with bad memories, care to expand on why you want this > in the changelog body of the patch so that the world will remember it as > well? Well, I am not entirely sure if even I want this in (as is) yet, because it is just a part of a complete solution for my problem and I can't imagine a reason why user space would want to know this odd value as is. So I can not give a committable rationale yet, but I can sum up what I am trying to do eventually: I try to map SOF counters to CLOCK_REALTIME timestamps in user space. Later I want to use that to establish a global time base on microcontrollers connected via USB without the hassle of running NTP or PTP over USB. In user space I am waiting for changes in the frame_counter sysfs file by reading it and comparing its contents to values previously read in a busy loop, which is of course not very elegant but it works - apart from the fact that the returned values are not the raw values (and as Alan noted, might also be off a bit)... :) Therefore I am currently playing around with a usb_hcd_get_real_frame_number() which uses a ehci_get_real_frame() function hacked into ehci-hcd.c which returns the actual SOF counter without the artificial horizon limit, but this is of course not committable. I wonder though why the limitation is there in the first place. Most users of usb_get_current_frame_number() seem rather unhappy with the limitation (e.g. drivers/media/usb/uvc/uvc_video.c) or don't care because they cap the returned value to the (locally defined(!)) minimum of the schedule horizon (0xFF everywhere AFAICS). Only very few use it directly to determine a possible scheduling (e.g. drivers/isdn/hisax/st5481_d.c which looks rather dubious to my naive eyes BTW). So from the user's perspective I don't see a reason why the artificially limited reply should be returned instead of the complete value. I have not checked if the raw value is available on all HCDs, but I assume it is. Wouldn't it make sense to change usb_hcd_get_frame_number() to return the raw value and add another one that returns the actual limit of the schedule horizon e.g. periodic_size in the case of EHCI(?) for those users that want to scheduler packets? If not, I would really like to see the documentation of usb_get_current_frame_number() be changed to make it more clear that it is not the raw SOF value and *why*. I have not figured out what *exactly* it is yet/how the iso scheduling works... grasping the kernel - even a subsystem - is quite an effort :) If/when I do I will send a documentation patch. If you think refactoring get_frame_number should be done I would be glad to work on it. A few pointers to what the result should look like and any obvious pitfalls would be appreciated in that case. > Whenever you add/modify/remove a sysfs attribute, you also need to > document it in Documentation/ABI/. Care to do that and resend this > patch? Sure, if you think it makes sense to add the code as is, I can do that any time. Thanks for the amazingly fast reply BTW. -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] usb/core: add current frame_number sysfs attr to hcd
On Mon, 18 Feb 2013, Stefan Tauner wrote: > This allows user space to retrieve the current frame number on a USB > as returned by usb_get_current_frame_number(...) via sysfs. > > Signed-off-by: Stefan Tauner > --- > That's sadly not necessarily the raw value seen on the bus because > the individual driver functions called by usb_get_current_frame_number(...) > seem to limit the possible range to the "schedule horizon" of isochronous > packets. IMHO the function name is a misnomer and I would like to hear your > opinion on this, a possible new name for it and a better way to retrieve the > real/raw value. A problem, as you point out, is that the individual host controller drivers are somewhat careless about how they implement this. For example, the value reported by ehci-hcd increments 125 us before the value seen on the bus (in addition to being limited to the "schedule horizon" for no good reason). Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] usb/core: add current frame_number sysfs attr to hcd
On Mon, Feb 18, 2013 at 03:34:07PM +0100, Stefan Tauner wrote: > This allows user space to retrieve the current frame number on a USB > as returned by usb_get_current_frame_number(...) via sysfs. > > Signed-off-by: Stefan Tauner > --- > That's sadly not necessarily the raw value seen on the bus because > the individual driver functions called by usb_get_current_frame_number(...) > seem to limit the possible range to the "schedule horizon" of isochronous > packets. IMHO the function name is a misnomer and I would like to hear your > opinion on this, a possible new name for it and a better way to retrieve the > real/raw value. > > The rationale for this patch can be found in the thread with the subject > "Correlating SOF with host system time" from last december > (201212042020.qb4kkt0n008...@mail2.student.tuwien.ac.at). > My whole use case/idea was kinda frowned upon then, but there was little > discussion about how it should/could be implemented in the kernel code in > case one really wants to try it. For those of us with bad memories, care to expand on why you want this in the changelog body of the patch so that the world will remember it as well? > Adding the sysfs attribute was the easiest way for me to communicate > this to user space, but at least for my use case a non-polling approach > would have been better. Any suggestions on how this could be done by a > newbie like me are very welcome. Whenever you add/modify/remove a sysfs attribute, you also need to document it in Documentation/ABI/. Care to do that and resend this patch? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH] usb/core: add current frame_number sysfs attr to hcd
This allows user space to retrieve the current frame number on a USB
as returned by usb_get_current_frame_number(...) via sysfs.
Signed-off-by: Stefan Tauner
---
That's sadly not necessarily the raw value seen on the bus because
the individual driver functions called by usb_get_current_frame_number(...)
seem to limit the possible range to the "schedule horizon" of isochronous
packets. IMHO the function name is a misnomer and I would like to hear your
opinion on this, a possible new name for it and a better way to retrieve the
real/raw value.
The rationale for this patch can be found in the thread with the subject
"Correlating SOF with host system time" from last december
(201212042020.qb4kkt0n008...@mail2.student.tuwien.ac.at).
My whole use case/idea was kinda frowned upon then, but there was little
discussion about how it should/could be implemented in the kernel code in
case one really wants to try it.
Adding the sysfs attribute was the easiest way for me to communicate
this to user space, but at least for my use case a non-polling approach
would have been better. Any suggestions on how this could be done by a
newbie like me are very welcome.
The patch is against version 3.8.0-6.13 of the Ubuntu 13.04 development
kernel, but I really hope it is simple enough to apply upstream, but
I don't expect it to get merged as is anyway, so please don't flame too
much if it does not cleanly. :)
I have tested it on my Ubuntu 12.04 system, lspci -nn | grep USB
00:1a.0 USB controller [0c03]: Intel Corporation 5 Series/3400 Series Chipset
USB2 Enhanced Host Controller [8086:3b3c] (rev 06)
00:1d.0 USB controller [0c03]: Intel Corporation 5 Series/3400 Series Chipset
USB2 Enhanced Host Controller [8086:3b34] (rev 06)
It creates two files:
/sys/devices/pci\:00/\:00\:1a.0/usb1/frame_number
and
/sys/devices/pci\:00/\:00\:1d.0/usb2/frame_number
which contain sane, distinct, looping values between 0 and about 2^10 - 1
---
drivers/usb/core/hcd.c | 16
1 file changed, 16 insertions(+)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 8e64adf..9c8e9ab 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -862,10 +862,26 @@ static DEVICE_ATTR(authorized_default, 0644,
usb_host_authorized_default_show,
usb_host_authorized_default_store);
+/*
+ * Show the current frame number as returned by usb_get_current_frame_number()
+ * (or a negative value in the case of errors)
+ */
+static ssize_t usb_bus_frame_number_show(struct device *dev,
+struct device_attribute *attr,
+char *buf)
+{
+ struct usb_device *rh_usb_dev = to_usb_device(dev);
+ int fn = usb_get_current_frame_number(rh_usb_dev);
+ return snprintf(buf, PAGE_SIZE, "%d\n", fn);
+}
+
+static DEVICE_ATTR(frame_number, S_IRUGO, usb_bus_frame_number_show, NULL);
+
/* Group all the USB bus attributes */
static struct attribute *usb_bus_attrs[] = {
&dev_attr_authorized_default.attr,
+ &dev_attr_frame_number.attr,
NULL,
};
--
Kind regards, Stefan Tauner
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
