Changes since v1:
- dropped RFC status as "this seems like the sort of feature somebody might
reasonably want to use -- if they know exactly what they're doing";
- new preparatory patch (1/2) which moves already-in-there sysfs code into
ehci-sysfs.c;
- moved uframe_periodic_max parameter from module option to sysfs attribute,
so that it can be set per controller and at runtime, added validity checks;
- clarified a bit bandwith analysis for 96% max periodic setup as noticed by
Alan Stern;
- clarified patch description saying that set in stone 80% max periodic is
specified by USB 2.0;
Kirill Smelkov (2):
USB: EHCI: Move sysfs related bits into ehci-sysfs.c
USB: EHCI: Allow users to override 80% max periodic bandwidth
drivers/usb/host/ehci-hcd.c | 11 ++-
drivers/usb/host/ehci-hub.c | 75 -----------------
drivers/usb/host/ehci-sched.c | 17 ++--
drivers/usb/host/ehci-sysfs.c | 184 +++++++++++++++++++++++++++++++++++++++++
drivers/usb/host/ehci.h | 2 +
5 files changed, 202 insertions(+), 87 deletions(-)
create mode 100644 drivers/usb/host/ehci-sysfs.c
--
1.7.6.rc3
The only sysfs attr implemented so far is "companion" from ehci-hub.c,
but in the next patch we are going to add another sysfs file, so prior
to that let's structure things and move already-in-there sysfs code to
separate file.
Signed-off-by: Kirill Smelkov <[email protected]>
---
drivers/usb/host/ehci-hcd.c | 5 +-
drivers/usb/host/ehci-hub.c | 75 --------------------------------
drivers/usb/host/ehci-sysfs.c | 94 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 97 insertions(+), 77 deletions(-)
create mode 100644 drivers/usb/host/ehci-sysfs.c
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index e18862c..8306155 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -336,6 +336,7 @@ static void ehci_work(struct ehci_hcd *ehci);
#include "ehci-mem.c"
#include "ehci-q.c"
#include "ehci-sched.c"
+#include "ehci-sysfs.c"
/*-------------------------------------------------------------------------*/
@@ -520,7 +521,7 @@ static void ehci_stop (struct usb_hcd *hcd)
ehci_reset (ehci);
spin_unlock_irq(&ehci->lock);
- remove_companion_file(ehci);
+ remove_sysfs_files(ehci);
remove_debug_files (ehci);
/* root hub is shut down separately (first, when possible) */
@@ -754,7 +755,7 @@ static int ehci_run (struct usb_hcd *hcd)
* since the class device isn't created that early.
*/
create_debug_files(ehci);
- create_companion_file(ehci);
+ create_sysfs_files(ehci);
return 0;
}
diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index ea6184b..d9e8d71 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -471,29 +471,6 @@ static int ehci_bus_resume (struct usb_hcd *hcd)
/*-------------------------------------------------------------------------*/
-/* Display the ports dedicated to the companion controller */
-static ssize_t show_companion(struct device *dev,
- struct device_attribute *attr,
- char *buf)
-{
- struct ehci_hcd *ehci;
- int nports, index, n;
- int count = PAGE_SIZE;
- char *ptr = buf;
-
- ehci = hcd_to_ehci(bus_to_hcd(dev_get_drvdata(dev)));
- nports = HCS_N_PORTS(ehci->hcs_params);
-
- for (index = 0; index < nports; ++index) {
- if (test_bit(index, &ehci->companion_ports)) {
- n = scnprintf(ptr, count, "%d\n", index + 1);
- ptr += n;
- count -= n;
- }
- }
- return ptr - buf;
-}
-
/*
* Sets the owner of a port
*/
@@ -528,58 +505,6 @@ static void set_owner(struct ehci_hcd *ehci, int portnum, int new_owner)
}
}
-/*
- * Dedicate or undedicate a port to the companion controller.
- * Syntax is "[-]portnum", where a leading '-' sign means
- * return control of the port to the EHCI controller.
- */
-static ssize_t store_companion(struct device *dev,
- struct device_attribute *attr,
- const char *buf, size_t count)
-{
- struct ehci_hcd *ehci;
- int portnum, new_owner;
-
- ehci = hcd_to_ehci(bus_to_hcd(dev_get_drvdata(dev)));
- new_owner = PORT_OWNER; /* Owned by companion */
- if (sscanf(buf, "%d", &portnum) != 1)
- return -EINVAL;
- if (portnum < 0) {
- portnum = - portnum;
- new_owner = 0; /* Owned by EHCI */
- }
- if (portnum <= 0 || portnum > HCS_N_PORTS(ehci->hcs_params))
- return -ENOENT;
- portnum--;
- if (new_owner)
- set_bit(portnum, &ehci->companion_ports);
- else
- clear_bit(portnum, &ehci->companion_ports);
- set_owner(ehci, portnum, new_owner);
- return count;
-}
-static DEVICE_ATTR(companion, 0644, show_companion, store_companion);
-
-static inline int create_companion_file(struct ehci_hcd *ehci)
-{
- int i = 0;
-
- /* with integrated TT there is no companion! */
- if (!ehci_is_TDI(ehci))
- i = device_create_file(ehci_to_hcd(ehci)->self.controller,
- &dev_attr_companion);
- return i;
-}
-
-static inline void remove_companion_file(struct ehci_hcd *ehci)
-{
- /* with integrated TT there is no companion! */
- if (!ehci_is_TDI(ehci))
- device_remove_file(ehci_to_hcd(ehci)->self.controller,
- &dev_attr_companion);
-}
-
-
/*-------------------------------------------------------------------------*/
static int check_reset_complete (
diff --git a/drivers/usb/host/ehci-sysfs.c b/drivers/usb/host/ehci-sysfs.c
new file mode 100644
index 0000000..347c8cb
--- /dev/null
+++ b/drivers/usb/host/ehci-sysfs.c
@@ -0,0 +1,94 @@
+/*
+ * Copyright (C) 2001-2004 by David Brownell
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+/* this file is part of ehci-hcd.c */
+
+
+/* Display the ports dedicated to the companion controller */
+static ssize_t show_companion(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct ehci_hcd *ehci;
+ int nports, index, n;
+ int count = PAGE_SIZE;
+ char *ptr = buf;
+
+ ehci = hcd_to_ehci(bus_to_hcd(dev_get_drvdata(dev)));
+ nports = HCS_N_PORTS(ehci->hcs_params);
+
+ for (index = 0; index < nports; ++index) {
+ if (test_bit(index, &ehci->companion_ports)) {
+ n = scnprintf(ptr, count, "%d\n", index + 1);
+ ptr += n;
+ count -= n;
+ }
+ }
+ return ptr - buf;
+}
+
+/*
+ * Dedicate or undedicate a port to the companion controller.
+ * Syntax is "[-]portnum", where a leading '-' sign means
+ * return control of the port to the EHCI controller.
+ */
+static ssize_t store_companion(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct ehci_hcd *ehci;
+ int portnum, new_owner;
+
+ ehci = hcd_to_ehci(bus_to_hcd(dev_get_drvdata(dev)));
+ new_owner = PORT_OWNER; /* Owned by companion */
+ if (sscanf(buf, "%d", &portnum) != 1)
+ return -EINVAL;
+ if (portnum < 0) {
+ portnum = - portnum;
+ new_owner = 0; /* Owned by EHCI */
+ }
+ if (portnum <= 0 || portnum > HCS_N_PORTS(ehci->hcs_params))
+ return -ENOENT;
+ portnum--;
+ if (new_owner)
+ set_bit(portnum, &ehci->companion_ports);
+ else
+ clear_bit(portnum, &ehci->companion_ports);
+ set_owner(ehci, portnum, new_owner);
+ return count;
+}
+static DEVICE_ATTR(companion, 0644, show_companion, store_companion);
+
+static inline int create_sysfs_files(struct ehci_hcd *ehci)
+{
+ int i = 0;
+
+ /* with integrated TT there is no companion! */
+ if (!ehci_is_TDI(ehci))
+ i = device_create_file(ehci_to_hcd(ehci)->self.controller,
+ &dev_attr_companion);
+ return i;
+}
+
+static inline void remove_sysfs_files(struct ehci_hcd *ehci)
+{
+ /* with integrated TT there is no companion! */
+ if (!ehci_is_TDI(ehci))
+ device_remove_file(ehci_to_hcd(ehci)->self.controller,
+ &dev_attr_companion);
+}
--
1.7.6.rc3
There are cases, when 80% max isochronous bandwidth is too limiting.
For example I have two USB video capture cards which stream uncompressed
video, and to stream full NTSC + PAL videos we'd need
NTSC 640x480 YUV422 @30fps ~17.6 MB/s
PAL 720x576 YUV422 @25fps ~19.7 MB/s
isoc bandwidth.
Now, due to limited alt settings in capture devices NTSC one ends up
streaming with max_pkt_size=2688 and PAL with max_pkt_size=2892, both
with interval=1. In terms of microframe time allocation this gives
NTSC ~53us
PAL ~57us
and together
~110us > 100us == 80% of 125us uframe time.
So those two devices can't work together simultaneously because the'd
over allocate isochronous bandwidth.
80% seemed a bit arbitrary to me, and I've tried to raise it to 90% and
both devices started to work together, so I though sometimes it would be
a good idea for users to override hardcoded default of max 80% isoc
bandwidth.
After all, isn't it a user who should decide how to load the bus? If I
can live with 10% or even 5% bulk bandwidth that should be ok. I'm a USB
newcomer, but that 80% set in stone by USB 2.0 specification seems to be
chosen pretty arbitrary to me, just to serve as a reasonable default.
NOTE: for two streams with max_pkt_size=3072 (worst case) both time
allocation would be 60us+60us=120us which is 96% periodic bandwidth
leaving 4% for bulk and control. Alan Stern suggested that bulk then
would be problematic (less than 300*8 bittimes left per microframe), but
I think that is still enough for control traffic.
Signed-off-by: Kirill Smelkov <[email protected]>
---
drivers/usb/host/ehci-hcd.c | 6 +++
drivers/usb/host/ehci-sched.c | 17 +++----
drivers/usb/host/ehci-sysfs.c | 98 +++++++++++++++++++++++++++++++++++++++--
drivers/usb/host/ehci.h | 2 +
4 files changed, 109 insertions(+), 14 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 8306155..4ee62be 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -572,6 +572,12 @@ static int ehci_init(struct usb_hcd *hcd)
hcc_params = ehci_readl(ehci, &ehci->caps->hcc_params);
/*
+ * by default set standard 80% (== 100 usec/uframe) max periodic
+ * bandwidth as required by USB 2.0
+ */
+ ehci->uframe_periodic_max = 100;
+
+ /*
* hw default: 1K periodic list heads, one per frame.
* periodic_size can shrink by USBCMD update if hcc_params allows.
*/
diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
index 6c9fbe3..2abf854 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -172,7 +172,7 @@ periodic_usecs (struct ehci_hcd *ehci, unsigned frame, unsigned uframe)
}
}
#ifdef DEBUG
- if (usecs > 100)
+ if (usecs > ehci->uframe_periodic_max)
ehci_err (ehci, "uframe %d sched overrun: %d usecs\n",
frame * 8 + uframe, usecs);
#endif
@@ -709,11 +709,8 @@ static int check_period (
if (uframe >= 8)
return 0;
- /*
- * 80% periodic == 100 usec/uframe available
- * convert "usecs we need" to "max already claimed"
- */
- usecs = 100 - usecs;
+ /* convert "usecs we need" to "max already claimed" */
+ usecs = ehci->uframe_periodic_max - usecs;
/* we "know" 2 and 4 uframe intervals were rejected; so
* for period 0, check _every_ microframe in the schedule.
@@ -1286,9 +1283,9 @@ itd_slot_ok (
{
uframe %= period;
do {
- /* can't commit more than 80% periodic == 100 usec */
+ /* can't commit more than uframe_periodic_max usec */
if (periodic_usecs (ehci, uframe >> 3, uframe & 0x7)
- > (100 - usecs))
+ > (ehci->uframe_periodic_max - usecs))
return 0;
/* we know urb->interval is 2^N uframes */
@@ -1345,7 +1342,7 @@ sitd_slot_ok (
#endif
/* check starts (OUT uses more than one) */
- max_used = 100 - stream->usecs;
+ max_used = ehci->uframe_periodic_max - stream->usecs;
for (tmp = stream->raw_mask & 0xff; tmp; tmp >>= 1, uf++) {
if (periodic_usecs (ehci, frame, uf) > max_used)
return 0;
@@ -1354,7 +1351,7 @@ sitd_slot_ok (
/* for IN, check CSPLIT */
if (stream->c_usecs) {
uf = uframe & 7;
- max_used = 100 - stream->c_usecs;
+ max_used = ehci->uframe_periodic_max - stream->c_usecs;
do {
tmp = 1 << uf;
tmp <<= 8;
diff --git a/drivers/usb/host/ehci-sysfs.c b/drivers/usb/host/ehci-sysfs.c
index 347c8cb..fe212ef 100644
--- a/drivers/usb/host/ehci-sysfs.c
+++ b/drivers/usb/host/ehci-sysfs.c
@@ -74,21 +74,111 @@ static ssize_t store_companion(struct device *dev,
}
static DEVICE_ATTR(companion, 0644, show_companion, store_companion);
+
+/*
+ * Display / Set uframe_periodic_max
+ */
+static ssize_t show_uframe_periodic_max(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct ehci_hcd *ehci;
+ int n;
+
+ ehci = hcd_to_ehci(bus_to_hcd(dev_get_drvdata(dev)));
+ n = scnprintf(buf, PAGE_SIZE, "%d\n", ehci->uframe_periodic_max);
+ return n;
+}
+
+
+static ssize_t store_uframe_periodic_max(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct ehci_hcd *ehci;
+ unsigned uframe_periodic_max;
+ unsigned frame, uframe, allocated;
+ unsigned long flags;
+ ssize_t ret;
+
+ ehci = hcd_to_ehci(bus_to_hcd(dev_get_drvdata(dev)));
+ if (sscanf(buf, "%u", &uframe_periodic_max) != 1)
+ return -EINVAL;
+
+ if (uframe_periodic_max < 100 || uframe_periodic_max >= 125) {
+ ehci_info(ehci, "rejecting invalid request for "
+ "uframe_periodic_max=%u\n", uframe_periodic_max);
+ return -EINVAL;
+ }
+
+ ret = -EINVAL;
+
+ /*
+ * lock, so that our checking does not race with possible periodic
+ * bandwidth allocation through submitting new urbs.
+ */
+ spin_lock_irqsave (&ehci->lock, flags);
+
+ /*
+ * for request to decrease max periodic bandwidth, we have to check
+ * every microframe in the schedule to see whether the decrease is
+ * possible.
+ */
+ if (uframe_periodic_max < ehci->uframe_periodic_max)
+ for (frame = 0; frame < ehci->periodic_size; ++frame)
+ for (uframe = 0; uframe < 7; ++uframe) {
+ allocated = periodic_usecs (ehci, frame, uframe);
+ if (allocated > uframe_periodic_max) {
+ ehci_info(ehci,
+ "cannot decrease uframe_periodic_max becase periodic bandwidth "
+ "is already allocated (%u > %u)\n",
+ allocated, uframe_periodic_max);
+ goto out_unlock;
+ }
+ }
+
+ /* increasing is always ok */
+
+ ehci_info(ehci, "setting max periodic bandwidth to %u%% "
+ "(== %u usec/uframe)\n",
+ 100*uframe_periodic_max/125, uframe_periodic_max);
+
+ if (uframe_periodic_max != 100)
+ ehci_warn(ehci, "max periodic bandwidth set is non-standard\n");
+
+ ehci->uframe_periodic_max = uframe_periodic_max;
+ ret = count;
+
+out_unlock:
+ spin_unlock_irqrestore (&ehci->lock, flags);
+ return ret;
+}
+static DEVICE_ATTR(uframe_periodic_max, 0644, show_uframe_periodic_max, store_uframe_periodic_max);
+
+
static inline int create_sysfs_files(struct ehci_hcd *ehci)
{
+ struct device *controller = ehci_to_hcd(ehci)->self.controller;
int i = 0;
/* with integrated TT there is no companion! */
if (!ehci_is_TDI(ehci))
- i = device_create_file(ehci_to_hcd(ehci)->self.controller,
- &dev_attr_companion);
+ i = device_create_file(controller, &dev_attr_companion);
+ if (i)
+ goto out;
+
+ i = device_create_file(controller, &dev_attr_uframe_periodic_max);
+out:
return i;
}
static inline void remove_sysfs_files(struct ehci_hcd *ehci)
{
+ struct device *controller = ehci_to_hcd(ehci)->self.controller;
+
/* with integrated TT there is no companion! */
if (!ehci_is_TDI(ehci))
- device_remove_file(ehci_to_hcd(ehci)->self.controller,
- &dev_attr_companion);
+ device_remove_file(controller, &dev_attr_companion);
+
+ device_remove_file(controller, &dev_attr_uframe_periodic_max);
}
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index bd6ff48..fa3129f 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -87,6 +87,8 @@ struct ehci_hcd { /* one per controller */
union ehci_shadow *pshadow; /* mirror hw periodic table */
int next_uframe; /* scan periodic, start here */
unsigned periodic_sched; /* periodic activity count */
+ unsigned uframe_periodic_max; /* max periodic time per uframe */
+
/* list of itds & sitds completed while clock_frame was still active */
struct list_head cached_itd_list;
--
1.7.6.rc3
On Fri, 24 Jun 2011, Kirill Smelkov wrote:
> The only sysfs attr implemented so far is "companion" from ehci-hub.c,
> but in the next patch we are going to add another sysfs file, so prior
> to that let's structure things and move already-in-there sysfs code to
> separate file.
>
> Signed-off-by: Kirill Smelkov <[email protected]>
> diff --git a/drivers/usb/host/ehci-sysfs.c b/drivers/usb/host/ehci-sysfs.c
> new file mode 100644
> index 0000000..347c8cb
> --- /dev/null
> +++ b/drivers/usb/host/ehci-sysfs.c
> @@ -0,0 +1,94 @@
> +/*
> + * Copyright (C) 2001-2004 by David Brownell
One little note... It's understandable that you put this line here,
but it isn't really appropriate. All the code you're moving into this
new file was written by me.
Apart from that one issue,
Acked-off-by: Alan Stern <[email protected]>
On Sat, Jun 25, 2011 at 08:10:52AM -0400, Alan Stern wrote:
> On Fri, 24 Jun 2011, Kirill Smelkov wrote:
>
> > The only sysfs attr implemented so far is "companion" from ehci-hub.c,
> > but in the next patch we are going to add another sysfs file, so prior
> > to that let's structure things and move already-in-there sysfs code to
> > separate file.
> >
> > Signed-off-by: Kirill Smelkov <[email protected]>
> > diff --git a/drivers/usb/host/ehci-sysfs.c b/drivers/usb/host/ehci-sysfs.c
> > new file mode 100644
> > index 0000000..347c8cb
> > --- /dev/null
> > +++ b/drivers/usb/host/ehci-sysfs.c
> > @@ -0,0 +1,94 @@
> > +/*
> > + * Copyright (C) 2001-2004 by David Brownell
>
> One little note... It's understandable that you put this line here,
> but it isn't really appropriate. All the code you're moving into this
> new file was written by me.
Now I see, yes, in 57e06c11 (EHCI: force high-speed devices to run at
full speed; Jan 16 2007).
> Apart from that one issue,
>
> Acked-off-by: Alan Stern <[email protected]>
Thanks.
What should we do with this patch now? Should I put
Copyright (C) 2007 by Alan Stern
there? Or something else?
Or maybe drop that copyright notice altogether becase usually it gets
outdated very quickly, and who made what is visible through git
log/blame?
I'm ok with any case, please just tell me how to proceed.
And what about main "[PATCH v2 2/2] USB: EHCI: Allow users to override
80% max periodic bandwidth"?
Was it Acked together with this one, or not and review is pending?
Curious because I'm new here...
Thanks,
Kirill
On Wed, 29 Jun 2011, Kirill Smelkov wrote:
> > Apart from that one issue,
> >
> > Acked-off-by: Alan Stern <[email protected]>
>
> Thanks.
>
> What should we do with this patch now? Should I put
>
>
> Copyright (C) 2007 by Alan Stern
>
> there? Or something else?
Yes, put that in.
> Or maybe drop that copyright notice altogether becase usually it gets
> outdated very quickly, and who made what is visible through git
> log/blame?
Copyright information is important, and it must be present in the
actual file -- not somewhere else (such as a changelog).
> I'm ok with any case, please just tell me how to proceed.
>
>
> And what about main "[PATCH v2 2/2] USB: EHCI: Allow users to override
> 80% max periodic bandwidth"?
>
> Was it Acked together with this one, or not and review is pending?
> Curious because I'm new here...
I haven't taken the time to review it yet, sorry...
Mostly it looks okay. In store_uframe_periodic_max(), you can use
kstrtouint() instead of sscanf() -- that seems to be the trend these
days.
Also, when decreasing the schedule limit, do you think it is really
necessary to check that the current allocation doesn't exceed the new
limit? I think it would be sufficient to apply the new limit just to
new bandwidth allocation requests. After all, this API is meant for
experts only.
Alan Stern
On Wed, Jun 29, 2011 at 11:23:11AM -0400, Alan Stern wrote:
> On Wed, 29 Jun 2011, Kirill Smelkov wrote:
>
> > > Apart from that one issue,
> > >
> > > Acked-off-by: Alan Stern <[email protected]>
> >
> > Thanks.
> >
> > What should we do with this patch now? Should I put
> >
> >
> > Copyright (C) 2007 by Alan Stern
> >
> > there? Or something else?
>
> Yes, put that in.
>
> > Or maybe drop that copyright notice altogether becase usually it gets
> > outdated very quickly, and who made what is visible through git
> > log/blame?
>
> Copyright information is important, and it must be present in the
> actual file -- not somewhere else (such as a changelog).
Ok, I've put that in.
> > I'm ok with any case, please just tell me how to proceed.
> >
> >
> > And what about main "[PATCH v2 2/2] USB: EHCI: Allow users to override
> > 80% max periodic bandwidth"?
> >
> > Was it Acked together with this one, or not and review is pending?
> > Curious because I'm new here...
>
> I haven't taken the time to review it yet, sorry...
Nevermind, I've just pinged.
> Mostly it looks okay. In store_uframe_periodic_max(), you can use
> kstrtouint() instead of sscanf() -- that seems to be the trend these
> days.
Thanks for the advice - changed to kstrtouint (which as it turned out,
with base=0 can read oct/hex/dec data as specified by user).
> Also, when decreasing the schedule limit, do you think it is really
> necessary to check that the current allocation doesn't exceed the new
> limit? I think it would be sufficient to apply the new limit just to
> new bandwidth allocation requests. After all, this API is meant for
> experts only.
I think yes, it is needed. E.g. because there is this check in
periodic_usecs():
#ifdef DEBUG
if (usecs > ehci->uframe_periodic_max)
ehci_err (ehci, "uframe %d sched overrun: %d usecs\n",
frame * 8 + uframe, usecs);
#endif
return usecs;
}
and periodic_usecs() is called in e.g. this chain:
itd_submit
iso_stream_schedule
itd_slot_ok
periodic_usecs
and others.
I'd leave this check as is - to me it would be useful in debug mode to
verify that we've not overallocated a period.
Also, even if this knob would be useful only to experts, it would be
better to put feedback onto the knob so that people could know whether
thir request could be served or not.
What do you think?
Kirill
On Wed, 29 Jun 2011, Kirill Smelkov wrote:
> > Also, when decreasing the schedule limit, do you think it is really
> > necessary to check that the current allocation doesn't exceed the new
> > limit? I think it would be sufficient to apply the new limit just to
> > new bandwidth allocation requests. After all, this API is meant for
> > experts only.
>
> I think yes, it is needed. E.g. because there is this check in
> periodic_usecs():
>
> #ifdef DEBUG
> if (usecs > ehci->uframe_periodic_max)
> ehci_err (ehci, "uframe %d sched overrun: %d usecs\n",
> frame * 8 + uframe, usecs);
> #endif
> return usecs;
> }
>
> and periodic_usecs() is called in e.g. this chain:
>
> itd_submit
> iso_stream_schedule
> itd_slot_ok
> periodic_usecs
>
> and others.
That won't matter unless DEBUG is defined.
> I'd leave this check as is - to me it would be useful in debug mode to
> verify that we've not overallocated a period.
>
> Also, even if this knob would be useful only to experts, it would be
> better to put feedback onto the knob so that people could know whether
> thir request could be served or not.
>
> What do you think?
Can you make that check conditional on DEBUG being set?
Alan Stern
On Wed, Jun 29, 2011 at 01:35:04PM -0400, Alan Stern wrote:
> On Wed, 29 Jun 2011, Kirill Smelkov wrote:
>
> > > Also, when decreasing the schedule limit, do you think it is really
> > > necessary to check that the current allocation doesn't exceed the new
> > > limit? I think it would be sufficient to apply the new limit just to
> > > new bandwidth allocation requests. After all, this API is meant for
> > > experts only.
> >
> > I think yes, it is needed. E.g. because there is this check in
> > periodic_usecs():
> >
> > #ifdef DEBUG
> > if (usecs > ehci->uframe_periodic_max)
> > ehci_err (ehci, "uframe %d sched overrun: %d usecs\n",
> > frame * 8 + uframe, usecs);
> > #endif
> > return usecs;
> > }
> >
> > and periodic_usecs() is called in e.g. this chain:
> >
> > itd_submit
> > iso_stream_schedule
> > itd_slot_ok
> > periodic_usecs
> >
> > and others.
>
> That won't matter unless DEBUG is defined.
Yes, but still it would be good to always keep the invariant
allocated <= uframe_periodic_max
and that debug is there to catch when this breaks.
> > I'd leave this check as is - to me it would be useful in debug mode to
> > verify that we've not overallocated a period.
> >
> > Also, even if this knob would be useful only to experts, it would be
> > better to put feedback onto the knob so that people could know whether
> > thir request could be served or not.
> >
> > What do you think?
>
> Can you make that check conditional on DEBUG being set?
Yes I can, but it seems to me we are starting to complicate the code.
What's the problem with returning error on setting uframe_periodic_max <
already allocated usb bandwith?
Kirill
P.S.
The checking is not a priority for me here, so if you think it's better not
to check or do it under #ifdef - let's do it. Though of course we all
have our preferences :)
On Wed, 29 Jun 2011, Kirill Smelkov wrote:
> Yes, but still it would be good to always keep the invariant
>
> allocated <= uframe_periodic_max
>
> and that debug is there to catch when this breaks.
Then perhaps it should print out the maximum number of microseconds
already allocated for any uframe, instead of stopping as soon as it
finds something above the new limit.
> > Can you make that check conditional on DEBUG being set?
>
> Yes I can, but it seems to me we are starting to complicate the code.
>
> What's the problem with returning error on setting uframe_periodic_max <
> already allocated usb bandwith?
No problem, really.
> The checking is not a priority for me here, so if you think it's better not
> to check or do it under #ifdef - let's do it. Though of course we all
> have our preferences :)
Yes, it's just a matter of taste. I prefer to add as little code as
possible for a feature that won't be used much.
Alan Stern
On Fri, Jun 24, 2011 at 08:48:06PM +0400, Kirill Smelkov wrote:
>
> Changes since v1:
>
>
> - dropped RFC status as "this seems like the sort of feature somebody might
> reasonably want to use -- if they know exactly what they're doing";
>
> - new preparatory patch (1/2) which moves already-in-there sysfs code into
> ehci-sysfs.c;
>
> - moved uframe_periodic_max parameter from module option to sysfs attribute,
> so that it can be set per controller and at runtime, added validity checks;
>
> - clarified a bit bandwith analysis for 96% max periodic setup as noticed by
> Alan Stern;
>
> - clarified patch description saying that set in stone 80% max periodic is
> specified by USB 2.0;
Have you tested this patch by maxing out this bandwidth on various
types of host controllers? It's entirely possible that you'll run into
vendor-specific bugs if you try to pack the schedule with isochronous
transfers. I don't think any hardware designer would seriously test or
validate their hardware with a schedule that is basically a violation of
the USB bus spec (more than 80% for periodic transfers).
But if Alan is fine with giving users a way to shoot themselves in the
foot, and it's disabled by default, then I don't particularly mind this
patch.
Sarah Sharp