Re: usbmon, usb core, ARM
On Sunday 23 January 2005 5:17 pm, Pete Zaitcev wrote: > On Sun, 23 Jan 2005 15:34:23 -0800, David Brownell <[EMAIL PROTECTED]> wrote: > > ... > > > and this is what usbmon intercepts. > > > For one thing, dev is down-counted in usb_unlink_urb(). ^^ > > > > I don't see any refcounting calls in that routine. It couldn't change > > the refcounts, in any case ... the HCD owns the URB until giveback(), > > and all unlinking does is accelerate getting to that giveback(). > > I am talking about this code (in 2.6.11-rc2): > > static void urb_unlink (struct urb *urb) ^^ See what I mean? That's not usb_unlink_urb()! And if you look at that function, you'll see I was correct. If you're concerned about that little helper function then just call the URB tracing hooks earlier, or move the call to urb_unlink() later. That sequencing in giveback() obviously doesn't matter ... the sketches for hcd_monitor_hook() were probably correct when added, but that was a very LONG time ago, before things like urb_unlink() was added. > ... > > void usb_hcd_giveback_urb (struct usb_hcd *hcd, struct urb *urb, struct > pt_regs *regs) > ... > > Even if urb->dev was possible to dereference in the completion callback, > the hcd was not available. I'm not following your logic still. I see it as a parameter right there; how could it suddenly become unavailable And remember the hooking is done BEFORE the callback, not IN it. > > For example, how does userspace provide a filter to say what URBs > > it's interested in, and what level of information to report? > > I do not have plans to do that. In the network space, it's the difference > between BPF (and its spawn LPF), and, say DLPI tap or /dev/nit on SunOS. So you plan to spit EVERY urb up into userspace? Copy EVERY byte of data? I could understand that as an initial prototype, but as a long term plan it seems troublesome. It would certainly affect the protocol timings, and we know that USB peripherals can be sensitive to such things. And for applications like watching video captures, copying a 24 Mbyte/sec stream does make a noticeable performance hit. (Especially when it's done during IRQ processing...) Note that I didn't say any particular networking filter model should be applied, that was your implication. If one of those models works well for USB, so be it. But I wouldn't necessarily expect any of them to do that, even though one can argue that USB is a little network based on parallels of addressing. > However, nothing prevents you from adding another reader type to usbmon, > the one which does filtering. Heck, an ability to replace whole usbmon > is a design parameter. Indeed, that's exactly why I'd like to split out discussion of the hooks (currently living only as comments) from what's done with them (such as usbmon). The comments about an hcd_monitor_hook() were done with expectation that someone would someday want to plug in such components ... and have the ability to experiment with them. If we could agree on reusable monitor hooks, sufficient for your current work, that'd be a good step. - Dave - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: usbmon, usb core, ARM
On Sun, 23 Jan 2005 15:34:23 -0800, David Brownell <[EMAIL PROTECTED]> wrote:
> > so let me restate it. Simply put, neither dev nor hcd are available
> > at the time urb->complete is called,
>
> Completely untrue. They are at a minimum provided through the URB itself,
> and giveback (which is the only place that call is made!) is passed the HCD
> as a parameter. Or aren't you talking about 2.6.11 code?
> > and this is what usbmon intercepts.
> > For one thing, dev is down-counted in usb_unlink_urb().
>
> I don't see any refcounting calls in that routine. It couldn't change
> the refcounts, in any case ... the HCD owns the URB until giveback(),
> and all unlinking does is accelerate getting to that giveback().
I am talking about this code (in 2.6.11-rc2):
static void urb_unlink (struct urb *urb)
{
..
/* clear all state linking urb to this dev (and hcd) */
spin_lock_irqsave (_data_lock, flags);
list_del_init (>urb_list);
spin_unlock_irqrestore (_data_lock, flags);
usb_put_dev (urb->dev);<= What is this, chopped liver?
}
void usb_hcd_giveback_urb (struct usb_hcd *hcd, struct urb *urb, struct pt_regs
*regs)
{
urb_unlink (urb);
// NOTE: a generic device/urb monitoring hook would go here.
/* pass ownership to the completion handler */
urb->complete (urb, regs);
atomic_dec (>use_count);
if (unlikely (urb->reject))
wake_up (_kill_urb_queue);
usb_put_urb (urb);
}
Even if urb->dev was possible to dereference in the completion callback,
the hcd was not available.
But like I said, it's a moot point with explicit hooks.
> For example, how does userspace provide a filter to say what URBs
> it's interested in, and what level of information to report?
I do not have plans to do that. In the network space, it's the difference
between BPF (and its spawn LPF), and, say DLPI tap or /dev/nit on SunOS.
By loading rules into kernel BSD programmers hoped to save a few cycles
by reducing the number of packets delivered to tcpdump. As far as I can
tell, nobody has a practical use for it, savings are miniscule...
I think some distros do not even configure LPF into kernels anymore.
However, nothing prevents you from adding another reader type to usbmon,
the one which does filtering. Heck, an ability to replace whole usbmon
is a design parameter. Rusty rewrote netfilter from scractch two times
(ipfwadm, ip-something-which-I-forgot, iptables). I don't think I can do
everything right with usbmon at first pass either. So I expect other
developers to try out ideas and experiment. Build your filter and prove
me wrong.
-- Pete
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-usb-devel] Re: usbmon, usb core, ARM
On Saturday 22 January 2005 12:12 am, Pete Zaitcev wrote: > On Thu, 20 Jan 2005 22:28:31 -0800, David Brownell <[EMAIL PROTECTED]> wrote: > > > By the way ... on the topic of usbmon rather than changing > > usbcore, is there a brief writeup of what you want this > > new version to be doing -- and how? Like, why put the > > spy hooks in that location, rather than any of the other > > choices. (Many of them would be less surprising to me!) > > The main idea was to make usbmon invisible if it's not actively monitoring. > But after thinking about your message I see that it was a misguided approach. > I'm going to add "if (usbmon_is_running(bus)) usb_mon_hook(bus, urb);" into > hot paths instead, approximately where current suggestion comments are. OK ... > I cannot figure out if you understand the nature of the problem with current > usbmon, Where "current" == "research project from a couple years ago, which doesn't even compile any more"?!? I don't think many people would, which is why I wanted to see more info ... :) > so let me restate it. Simply put, neither dev nor hcd are available > at the time urb->complete is called, Completely untrue. They are at a minimum provided through the URB itself, and giveback (which is the only place that call is made!) is passed the HCD as a parameter. Or aren't you talking about 2.6.11 code? > and this is what usbmon intercepts. > For one thing, dev is down-counted in usb_unlink_urb(). I don't see any refcounting calls in that routine. It couldn't change the refcounts, in any case ... the HCD owns the URB until giveback(), and all unlinking does is accelerate getting to that giveback(). > Adding hooks explicitly has its disadvantages. Although the giveback > path is much better, in the submit path such hook cannot inform usbmon > if the submission fails, and an additional hook is needed. Anyhow, you'll > see it all in the patch, please give me a couple of days. OK. Though I'd still like to see the usbcore hooks as a separate patch (which can be easily reviewed), with docs (and maybe an example) to help (idiots like) me see how it's supposed to work. For example, how does userspace provide a filter to say what URBs it's interested in, and what level of information to report? (Like, "only the first N bytes of data", timestamping, etc.) That is, how would a USB analogue of tcpdump be able to say "spy on what happens with that device", or "spy on all enumeration". - Dave > Greetings, > -- Pete > - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-usb-devel] Re: usbmon, usb core, ARM
On Saturday 22 January 2005 12:12 am, Pete Zaitcev wrote: On Thu, 20 Jan 2005 22:28:31 -0800, David Brownell [EMAIL PROTECTED] wrote: By the way ... on the topic of usbmon rather than changing usbcore, is there a brief writeup of what you want this new version to be doing -- and how? Like, why put the spy hooks in that location, rather than any of the other choices. (Many of them would be less surprising to me!) The main idea was to make usbmon invisible if it's not actively monitoring. But after thinking about your message I see that it was a misguided approach. I'm going to add if (usbmon_is_running(bus)) usb_mon_hook(bus, urb); into hot paths instead, approximately where current suggestion comments are. OK ... I cannot figure out if you understand the nature of the problem with current usbmon, Where current == research project from a couple years ago, which doesn't even compile any more?!? I don't think many people would, which is why I wanted to see more info ... :) so let me restate it. Simply put, neither dev nor hcd are available at the time urb-complete is called, Completely untrue. They are at a minimum provided through the URB itself, and giveback (which is the only place that call is made!) is passed the HCD as a parameter. Or aren't you talking about 2.6.11 code? and this is what usbmon intercepts. For one thing, dev is down-counted in usb_unlink_urb(). I don't see any refcounting calls in that routine. It couldn't change the refcounts, in any case ... the HCD owns the URB until giveback(), and all unlinking does is accelerate getting to that giveback(). Adding hooks explicitly has its disadvantages. Although the giveback path is much better, in the submit path such hook cannot inform usbmon if the submission fails, and an additional hook is needed. Anyhow, you'll see it all in the patch, please give me a couple of days. OK. Though I'd still like to see the usbcore hooks as a separate patch (which can be easily reviewed), with docs (and maybe an example) to help (idiots like) me see how it's supposed to work. For example, how does userspace provide a filter to say what URBs it's interested in, and what level of information to report? (Like, only the first N bytes of data, timestamping, etc.) That is, how would a USB analogue of tcpdump be able to say spy on what happens with that device, or spy on all enumeration. - Dave Greetings, -- Pete - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: usbmon, usb core, ARM
On Sun, 23 Jan 2005 15:34:23 -0800, David Brownell [EMAIL PROTECTED] wrote:
so let me restate it. Simply put, neither dev nor hcd are available
at the time urb-complete is called,
Completely untrue. They are at a minimum provided through the URB itself,
and giveback (which is the only place that call is made!) is passed the HCD
as a parameter. Or aren't you talking about 2.6.11 code?
and this is what usbmon intercepts.
For one thing, dev is down-counted in usb_unlink_urb().
I don't see any refcounting calls in that routine. It couldn't change
the refcounts, in any case ... the HCD owns the URB until giveback(),
and all unlinking does is accelerate getting to that giveback().
I am talking about this code (in 2.6.11-rc2):
static void urb_unlink (struct urb *urb)
{
..
/* clear all state linking urb to this dev (and hcd) */
spin_lock_irqsave (hcd_data_lock, flags);
list_del_init (urb-urb_list);
spin_unlock_irqrestore (hcd_data_lock, flags);
usb_put_dev (urb-dev);= What is this, chopped liver?
}
void usb_hcd_giveback_urb (struct usb_hcd *hcd, struct urb *urb, struct pt_regs
*regs)
{
urb_unlink (urb);
// NOTE: a generic device/urb monitoring hook would go here.
/* pass ownership to the completion handler */
urb-complete (urb, regs);
atomic_dec (urb-use_count);
if (unlikely (urb-reject))
wake_up (usb_kill_urb_queue);
usb_put_urb (urb);
}
Even if urb-dev was possible to dereference in the completion callback,
the hcd was not available.
But like I said, it's a moot point with explicit hooks.
For example, how does userspace provide a filter to say what URBs
it's interested in, and what level of information to report?
I do not have plans to do that. In the network space, it's the difference
between BPF (and its spawn LPF), and, say DLPI tap or /dev/nit on SunOS.
By loading rules into kernel BSD programmers hoped to save a few cycles
by reducing the number of packets delivered to tcpdump. As far as I can
tell, nobody has a practical use for it, savings are miniscule...
I think some distros do not even configure LPF into kernels anymore.
However, nothing prevents you from adding another reader type to usbmon,
the one which does filtering. Heck, an ability to replace whole usbmon
is a design parameter. Rusty rewrote netfilter from scractch two times
(ipfwadm, ip-something-which-I-forgot, iptables). I don't think I can do
everything right with usbmon at first pass either. So I expect other
developers to try out ideas and experiment. Build your filter and prove
me wrong.
-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: usbmon, usb core, ARM
On Sunday 23 January 2005 5:17 pm, Pete Zaitcev wrote: On Sun, 23 Jan 2005 15:34:23 -0800, David Brownell [EMAIL PROTECTED] wrote: ... and this is what usbmon intercepts. For one thing, dev is down-counted in usb_unlink_urb(). ^^ I don't see any refcounting calls in that routine. It couldn't change the refcounts, in any case ... the HCD owns the URB until giveback(), and all unlinking does is accelerate getting to that giveback(). I am talking about this code (in 2.6.11-rc2): static void urb_unlink (struct urb *urb) ^^ See what I mean? That's not usb_unlink_urb()! And if you look at that function, you'll see I was correct. If you're concerned about that little helper function then just call the URB tracing hooks earlier, or move the call to urb_unlink() later. That sequencing in giveback() obviously doesn't matter ... the sketches for hcd_monitor_hook() were probably correct when added, but that was a very LONG time ago, before things like urb_unlink() was added. ... void usb_hcd_giveback_urb (struct usb_hcd *hcd, struct urb *urb, struct pt_regs *regs) ... Even if urb-dev was possible to dereference in the completion callback, the hcd was not available. I'm not following your logic still. I see it as a parameter right there; how could it suddenly become unavailable And remember the hooking is done BEFORE the callback, not IN it. For example, how does userspace provide a filter to say what URBs it's interested in, and what level of information to report? I do not have plans to do that. In the network space, it's the difference between BPF (and its spawn LPF), and, say DLPI tap or /dev/nit on SunOS. So you plan to spit EVERY urb up into userspace? Copy EVERY byte of data? I could understand that as an initial prototype, but as a long term plan it seems troublesome. It would certainly affect the protocol timings, and we know that USB peripherals can be sensitive to such things. And for applications like watching video captures, copying a 24 Mbyte/sec stream does make a noticeable performance hit. (Especially when it's done during IRQ processing...) Note that I didn't say any particular networking filter model should be applied, that was your implication. If one of those models works well for USB, so be it. But I wouldn't necessarily expect any of them to do that, even though one can argue that USB is a little network based on parallels of addressing. However, nothing prevents you from adding another reader type to usbmon, the one which does filtering. Heck, an ability to replace whole usbmon is a design parameter. Indeed, that's exactly why I'd like to split out discussion of the hooks (currently living only as comments) from what's done with them (such as usbmon). The comments about an hcd_monitor_hook() were done with expectation that someone would someday want to plug in such components ... and have the ability to experiment with them. If we could agree on reusable monitor hooks, sufficient for your current work, that'd be a good step. - Dave - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: usbmon, usb core, ARM
On Thu, 20 Jan 2005 22:28:31 -0800, David Brownell <[EMAIL PROTECTED]> wrote: > By the way ... on the topic of usbmon rather than changing > usbcore, is there a brief writeup of what you want this > new version to be doing -- and how? Like, why put the > spy hooks in that location, rather than any of the other > choices. (Many of them would be less surprising to me!) The main idea was to make usbmon invisible if it's not actively monitoring. But after thinking about your message I see that it was a misguided approach. I'm going to add "if (usbmon_is_running(bus)) usb_mon_hook(bus, urb);" into hot paths instead, approximately where current suggestion comments are. I cannot figure out if you understand the nature of the problem with current usbmon, so let me restate it. Simply put, neither dev nor hcd are available at the time urb->complete is called, and this is what usbmon intercepts. For one thing, dev is down-counted in usb_unlink_urb(). This is why I tried to find a way to avoid using them. Your "dev == hcd->self.root_hub" is entirely out of question. But it's probably a moot point now anyway. Adding hooks explicitly has its disadvantages. Although the giveback path is much better, in the submit path such hook cannot inform usbmon if the submission fails, and an additional hook is needed. Anyhow, you'll see it all in the patch, please give me a couple of days. Greetings, -- Pete - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: usbmon, usb core, ARM
On Thu, 20 Jan 2005 22:28:31 -0800, David Brownell [EMAIL PROTECTED] wrote: By the way ... on the topic of usbmon rather than changing usbcore, is there a brief writeup of what you want this new version to be doing -- and how? Like, why put the spy hooks in that location, rather than any of the other choices. (Many of them would be less surprising to me!) The main idea was to make usbmon invisible if it's not actively monitoring. But after thinking about your message I see that it was a misguided approach. I'm going to add if (usbmon_is_running(bus)) usb_mon_hook(bus, urb); into hot paths instead, approximately where current suggestion comments are. I cannot figure out if you understand the nature of the problem with current usbmon, so let me restate it. Simply put, neither dev nor hcd are available at the time urb-complete is called, and this is what usbmon intercepts. For one thing, dev is down-counted in usb_unlink_urb(). This is why I tried to find a way to avoid using them. Your dev == hcd-self.root_hub is entirely out of question. But it's probably a moot point now anyway. Adding hooks explicitly has its disadvantages. Although the giveback path is much better, in the submit path such hook cannot inform usbmon if the submission fails, and an additional hook is needed. Anyhow, you'll see it all in the patch, please give me a couple of days. Greetings, -- Pete - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: usbmon, usb core, ARM
On Thursday 20 January 2005 11:35 am, Pete Zaitcev wrote:
> On Wed, 19 Jan 2005 09:08:34 -0800, David Brownell <[EMAIL PROTECTED]> wrote:
> I do not like to refer to a dev because I do not quite understand where
> the necessary usb_dev_get/_put are now. But if you guarantee that the
> urb->dev is refcounted properly while urb is processed by
> usb_hcd_giveback_urb,
> I do not mind an extra indirection.
We have no reason to suspect bugs there; if there were any,
lots of things would have been breaking for a long time now.
> What would be the right test in usb_hcd_giveback_urb, then?
> It looks to me that you want me to use this:
>
> urb_is_for_root_hub(urb) {
Actually it'd be more like dev_is_root_hub(dev, bus), since
both values are readily at hand -- you're basically just
wanting to wrap "dev == hcd->self.root_hub" in most cases.
Though I'm still not clear why you'd want to change that
working code; nothing's broken now, after all.
By the way ... on the topic of usbmon rather than changing
usbcore, is there a brief writeup of what you want this
new version to be doing -- and how? Like, why put the
spy hooks in that location, rather than any of the other
choices. (Many of them would be less surprising to me!)
- Dave
> return urb->dev == urb->dev->bus->hcpriv->self.root_hub;
> }
>
> This is just ... ew. Can we use pipe for now or do you have
> a better idea?
>
> -- Pete
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: usbmon, usb core, ARM
On Wed, 19 Jan 2005 09:08:34 -0800, David Brownell <[EMAIL PROTECTED]> wrote:
> On Wednesday 19 January 2005 7:42 am, Pete Zaitcev wrote:
> > Relying on pipe makes
> > tests dependant on URB only. No references to bus or HCD, therefore no
> > extra refcounts or worries about oopses. Also, HC drivers zero out the
> > urb->dev in giveback sequence which is a royal pain when trying to identify
> > a root hub.
>
> That was a 2.4-ism, it should now be gone. So an inlined function to
> test whether urb->dev is the root hub should suffice; I know there's
> code that does that already.
I do not like to refer to a dev because I do not quite understand where
the necessary usb_dev_get/_put are now. But if you guarantee that the
urb->dev is refcounted properly while urb is processed by usb_hcd_giveback_urb,
I do not mind an extra indirection.
What would be the right test in usb_hcd_giveback_urb, then?
It looks to me that you want me to use this:
urb_is_for_root_hub(urb) {
return urb->dev == urb->dev->bus->hcpriv->self.root_hub;
}
This is just ... ew. Can we use pipe for now or do you have
a better idea?
-- Pete
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: usbmon, usb core, ARM
On Wed, 19 Jan 2005 09:08:34 -0800, David Brownell [EMAIL PROTECTED] wrote:
On Wednesday 19 January 2005 7:42 am, Pete Zaitcev wrote:
Relying on pipe makes
tests dependant on URB only. No references to bus or HCD, therefore no
extra refcounts or worries about oopses. Also, HC drivers zero out the
urb-dev in giveback sequence which is a royal pain when trying to identify
a root hub.
That was a 2.4-ism, it should now be gone. So an inlined function to
test whether urb-dev is the root hub should suffice; I know there's
code that does that already.
I do not like to refer to a dev because I do not quite understand where
the necessary usb_dev_get/_put are now. But if you guarantee that the
urb-dev is refcounted properly while urb is processed by usb_hcd_giveback_urb,
I do not mind an extra indirection.
What would be the right test in usb_hcd_giveback_urb, then?
It looks to me that you want me to use this:
urb_is_for_root_hub(urb) {
return urb-dev == urb-dev-bus-hcpriv-self.root_hub;
}
This is just ... ew. Can we use pipe for now or do you have
a better idea?
-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: usbmon, usb core, ARM
On Thursday 20 January 2005 11:35 am, Pete Zaitcev wrote:
On Wed, 19 Jan 2005 09:08:34 -0800, David Brownell [EMAIL PROTECTED] wrote:
I do not like to refer to a dev because I do not quite understand where
the necessary usb_dev_get/_put are now. But if you guarantee that the
urb-dev is refcounted properly while urb is processed by
usb_hcd_giveback_urb,
I do not mind an extra indirection.
We have no reason to suspect bugs there; if there were any,
lots of things would have been breaking for a long time now.
What would be the right test in usb_hcd_giveback_urb, then?
It looks to me that you want me to use this:
urb_is_for_root_hub(urb) {
Actually it'd be more like dev_is_root_hub(dev, bus), since
both values are readily at hand -- you're basically just
wanting to wrap dev == hcd-self.root_hub in most cases.
Though I'm still not clear why you'd want to change that
working code; nothing's broken now, after all.
By the way ... on the topic of usbmon rather than changing
usbcore, is there a brief writeup of what you want this
new version to be doing -- and how? Like, why put the
spy hooks in that location, rather than any of the other
choices. (Many of them would be less surprising to me!)
- Dave
return urb-dev == urb-dev-bus-hcpriv-self.root_hub;
}
This is just ... ew. Can we use pipe for now or do you have
a better idea?
-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: usbmon, usb core, ARM
On Wednesday 19 January 2005 7:42 am, Pete Zaitcev wrote: > Relying on pipe makes > tests dependant on URB only. No references to bus or HCD, therefore no > extra refcounts or worries about oopses. Also, HC drivers zero out the > urb->dev in giveback sequence which is a royal pain when trying to identify > a root hub. That was a 2.4-ism, it should now be gone. So an inlined function to test whether urb->dev is the root hub should suffice; I know there's code that does that already. - Dave - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: usbmon, usb core, ARM
Am Mittwoch, 19. Januar 2005 16:42 schrieb Pete Zaitcev: > On Tue, 18 Jan 2005 22:14:24 -0800, David Brownell <[EMAIL PROTECTED]> wrote: > > > > > Also, I don't like the idea of scattering knowledge all over the place > > > > that the root hub is always given address 1 ... > > > > which you didn't address yet. > > Yes, I have to look why you do not like using the pipe. Relying on pipe makes > tests dependant on URB only. No references to bus or HCD, therefore no > extra refcounts or worries about oopses. Also, HC drivers zero out the > urb->dev in giveback sequence which is a royal pain when trying to identify > a root hub. I thought about adding an extra flag like URB_ROOT_HUB to split That idea was good. It is simple and will simplify the code cleanly. > this use from the abuse of URB_NO_TRANSFER_DMA_MAP, but pipe looks better > all around. If you look at it from the angle I did, it stands to reason > that excessive encapsulation only masks _why_ it was safer, e.g. if one sees > something like urb_is_root_hub(urb), one must look up the implementation > to know if it uses urb->dev or not. Relying on address 1 without any symbolic > constant is obviously a bad idea though, I'll fix that. True, but pipe must die. It has no real basis. Regards Oliver - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: usbmon, usb core, ARM
On Tue, 18 Jan 2005 22:14:24 -0800, David Brownell <[EMAIL PROTECTED]> wrote: > > > Also, I don't like the idea of scattering knowledge all over the place > > > that the root hub is always given address 1 ... > > which you didn't address yet. Yes, I have to look why you do not like using the pipe. Relying on pipe makes tests dependant on URB only. No references to bus or HCD, therefore no extra refcounts or worries about oopses. Also, HC drivers zero out the urb->dev in giveback sequence which is a royal pain when trying to identify a root hub. I thought about adding an extra flag like URB_ROOT_HUB to split this use from the abuse of URB_NO_TRANSFER_DMA_MAP, but pipe looks better all around. If you look at it from the angle I did, it stands to reason that excessive encapsulation only masks _why_ it was safer, e.g. if one sees something like urb_is_root_hub(urb), one must look up the implementation to know if it uses urb->dev or not. Relying on address 1 without any symbolic constant is obviously a bad idea though, I'll fix that. -- Pete - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: usbmon, usb core, ARM
On Tue, 18 Jan 2005 22:14:24 -0800, David Brownell [EMAIL PROTECTED] wrote: Also, I don't like the idea of scattering knowledge all over the place that the root hub is always given address 1 ... which you didn't address yet. Yes, I have to look why you do not like using the pipe. Relying on pipe makes tests dependant on URB only. No references to bus or HCD, therefore no extra refcounts or worries about oopses. Also, HC drivers zero out the urb-dev in giveback sequence which is a royal pain when trying to identify a root hub. I thought about adding an extra flag like URB_ROOT_HUB to split this use from the abuse of URB_NO_TRANSFER_DMA_MAP, but pipe looks better all around. If you look at it from the angle I did, it stands to reason that excessive encapsulation only masks _why_ it was safer, e.g. if one sees something like urb_is_root_hub(urb), one must look up the implementation to know if it uses urb-dev or not. Relying on address 1 without any symbolic constant is obviously a bad idea though, I'll fix that. -- Pete - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: usbmon, usb core, ARM
Am Mittwoch, 19. Januar 2005 16:42 schrieb Pete Zaitcev: On Tue, 18 Jan 2005 22:14:24 -0800, David Brownell [EMAIL PROTECTED] wrote: Also, I don't like the idea of scattering knowledge all over the place that the root hub is always given address 1 ... which you didn't address yet. Yes, I have to look why you do not like using the pipe. Relying on pipe makes tests dependant on URB only. No references to bus or HCD, therefore no extra refcounts or worries about oopses. Also, HC drivers zero out the urb-dev in giveback sequence which is a royal pain when trying to identify a root hub. I thought about adding an extra flag like URB_ROOT_HUB to split That idea was good. It is simple and will simplify the code cleanly. this use from the abuse of URB_NO_TRANSFER_DMA_MAP, but pipe looks better all around. If you look at it from the angle I did, it stands to reason that excessive encapsulation only masks _why_ it was safer, e.g. if one sees something like urb_is_root_hub(urb), one must look up the implementation to know if it uses urb-dev or not. Relying on address 1 without any symbolic constant is obviously a bad idea though, I'll fix that. True, but pipe must die. It has no real basis. Regards Oliver - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: usbmon, usb core, ARM
On Wednesday 19 January 2005 7:42 am, Pete Zaitcev wrote: Relying on pipe makes tests dependant on URB only. No references to bus or HCD, therefore no extra refcounts or worries about oopses. Also, HC drivers zero out the urb-dev in giveback sequence which is a royal pain when trying to identify a root hub. That was a 2.4-ism, it should now be gone. So an inlined function to test whether urb-dev is the root hub should suffice; I know there's code that does that already. - Dave - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: usbmon, usb core, ARM
On Tuesday 18 January 2005 9:20 pm, Pete Zaitcev wrote: > > However, David objects to the patch on the grounds that it can damage ARM. Actually what I said was: > > Those patches were added for important reasons. (Or did you add some > > other solution to the issue described in that comment?) which on closer examination (of just this patch, split out from all the usbmon stuff) may well have been your cue to say something like "my solution was to add a special case for root hubs into every urb's giveback() path ... even though I left in the comment specifying that this must be handled in the original way". As well as: > > Also, I don't like the idea of scattering knowledge all over the place > > that the root hub is always given address 1 ... which you didn't address yet. - Dave - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
usbmon, usb core, ARM
Dear Russell:
I have a favour to ask of you. I need the following patch to be applied
to the USB core:
diff -urpN -X dontdiff linux-2.6.11-rc1-bk4/drivers/usb/core/hcd.c
linux-2.6.11-rc1-bk4-lem/drivers/usb/core/hcd.c
--- linux-2.6.11-rc1-bk4/drivers/usb/core/hcd.c 2005-01-12 16:35:53.0
-0800
+++ linux-2.6.11-rc1-bk4-lem/drivers/usb/core/hcd.c 2005-01-17
21:38:51.0 -0800
@@ -1099,14 +1104,12 @@ static int hcd_submit_urb (struct urb *u
urb = usb_get_urb (urb);
atomic_inc (>use_count);
- if (urb->dev == hcd->self.root_hub) {
+ if (usb_pipedevice(urb->pipe) == 1) {
/* NOTE: requirement on hub callers (usbfs and the hub
* driver, for now) that URBs' urb->transfer_buffer be
* valid and usb_buffer_{sync,unmap}() not be needed, since
* they could clobber root hub response data.
*/
- urb->transfer_flags |= (URB_NO_TRANSFER_DMA_MAP
- | URB_NO_SETUP_DMA_MAP);
status = rh_urb_enqueue (hcd, urb);
goto done;
}
@@ -1168,7 +1171,7 @@ unlink1 (struct usb_hcd *hcd, struct urb
{
int value;
- if (urb->dev == hcd->self.root_hub)
+ if (usb_pipedevice(urb->pipe) == 1)
value = usb_rh_urb_dequeue (hcd, urb);
else {
@@ -1258,7 +1261,7 @@ static int hcd_unlink_urb (struct urb *u
* finish unlinking the initial failed usb_set_address()
* or device descriptor fetch.
*/
- if (!hcd->saw_irq && hcd->self.root_hub != urb->dev) {
+ if (!hcd->saw_irq && usb_pipedevice(urb->pipe) != 1) {
dev_warn (hcd->self.controller, "Unlink after no-IRQ? "
"Controller is probably using the wrong IRQ."
"\n");
@@ -1465,12 +1468,8 @@ void usb_hcd_giveback_urb (struct usb_hc
{
urb_unlink (urb);
- // NOTE: a generic device/urb monitoring hook would go here.
- // hcd_monitor_hook(MONITOR_URB_FINISH, urb, dev)
- // It would catch exit/unlink paths for all urbs.
-
/* lower level hcd code should use *_dma exclusively */
- if (hcd->self.controller->dma_mask) {
+ if (hcd->self.controller->dma_mask && usb_pipedevice(urb->pipe) != 1) {
if (usb_pipecontrol (urb->pipe)
&& !(urb->transfer_flags & URB_NO_SETUP_DMA_MAP))
dma_unmap_single (hcd->self.controller, urb->setup_dma,
However, David objects to the patch on the grounds that it can damage ARM.
I am sure that what I do matches perfectly what ARM needs, based on this:
http://www.kernel.org/pub/linux/kernel/people/gregkh/usb/2.5/usb-core-2-2.5.33.patch
#This was first noticed on ARM (no surprises here); the root hub
#code, rh_call_control(), placed data into the buffer and then
#called usb_hcd_giveback_urb(). This function called
#pci_unmap_single() on this region which promptly destroyed the
#data that rh_call_control() had placed there. This lead to a
#corrupted device descriptor and the "too many configurations"
#message.
So, it would help me a lot if you tested the patch on a system with SA-
against a regression and thus buried this silly ARM canard decisively.
Please let me know if you have time to help me out.
Thank you,
-- Pete
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
usbmon, usb core, ARM
Dear Russell:
I have a favour to ask of you. I need the following patch to be applied
to the USB core:
diff -urpN -X dontdiff linux-2.6.11-rc1-bk4/drivers/usb/core/hcd.c
linux-2.6.11-rc1-bk4-lem/drivers/usb/core/hcd.c
--- linux-2.6.11-rc1-bk4/drivers/usb/core/hcd.c 2005-01-12 16:35:53.0
-0800
+++ linux-2.6.11-rc1-bk4-lem/drivers/usb/core/hcd.c 2005-01-17
21:38:51.0 -0800
@@ -1099,14 +1104,12 @@ static int hcd_submit_urb (struct urb *u
urb = usb_get_urb (urb);
atomic_inc (urb-use_count);
- if (urb-dev == hcd-self.root_hub) {
+ if (usb_pipedevice(urb-pipe) == 1) {
/* NOTE: requirement on hub callers (usbfs and the hub
* driver, for now) that URBs' urb-transfer_buffer be
* valid and usb_buffer_{sync,unmap}() not be needed, since
* they could clobber root hub response data.
*/
- urb-transfer_flags |= (URB_NO_TRANSFER_DMA_MAP
- | URB_NO_SETUP_DMA_MAP);
status = rh_urb_enqueue (hcd, urb);
goto done;
}
@@ -1168,7 +1171,7 @@ unlink1 (struct usb_hcd *hcd, struct urb
{
int value;
- if (urb-dev == hcd-self.root_hub)
+ if (usb_pipedevice(urb-pipe) == 1)
value = usb_rh_urb_dequeue (hcd, urb);
else {
@@ -1258,7 +1261,7 @@ static int hcd_unlink_urb (struct urb *u
* finish unlinking the initial failed usb_set_address()
* or device descriptor fetch.
*/
- if (!hcd-saw_irq hcd-self.root_hub != urb-dev) {
+ if (!hcd-saw_irq usb_pipedevice(urb-pipe) != 1) {
dev_warn (hcd-self.controller, Unlink after no-IRQ?
Controller is probably using the wrong IRQ.
\n);
@@ -1465,12 +1468,8 @@ void usb_hcd_giveback_urb (struct usb_hc
{
urb_unlink (urb);
- // NOTE: a generic device/urb monitoring hook would go here.
- // hcd_monitor_hook(MONITOR_URB_FINISH, urb, dev)
- // It would catch exit/unlink paths for all urbs.
-
/* lower level hcd code should use *_dma exclusively */
- if (hcd-self.controller-dma_mask) {
+ if (hcd-self.controller-dma_mask usb_pipedevice(urb-pipe) != 1) {
if (usb_pipecontrol (urb-pipe)
!(urb-transfer_flags URB_NO_SETUP_DMA_MAP))
dma_unmap_single (hcd-self.controller, urb-setup_dma,
However, David objects to the patch on the grounds that it can damage ARM.
I am sure that what I do matches perfectly what ARM needs, based on this:
http://www.kernel.org/pub/linux/kernel/people/gregkh/usb/2.5/usb-core-2-2.5.33.patch
#This was first noticed on ARM (no surprises here); the root hub
#code, rh_call_control(), placed data into the buffer and then
#called usb_hcd_giveback_urb(). This function called
#pci_unmap_single() on this region which promptly destroyed the
#data that rh_call_control() had placed there. This lead to a
#corrupted device descriptor and the too many configurations
#message.
So, it would help me a lot if you tested the patch on a system with SA-
against a regression and thus buried this silly ARM canard decisively.
Please let me know if you have time to help me out.
Thank you,
-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: usbmon, usb core, ARM
On Tuesday 18 January 2005 9:20 pm, Pete Zaitcev wrote: However, David objects to the patch on the grounds that it can damage ARM. Actually what I said was: Those patches were added for important reasons. (Or did you add some other solution to the issue described in that comment?) which on closer examination (of just this patch, split out from all the usbmon stuff) may well have been your cue to say something like my solution was to add a special case for root hubs into every urb's giveback() path ... even though I left in the comment specifying that this must be handled in the original way. As well as: Also, I don't like the idea of scattering knowledge all over the place that the root hub is always given address 1 ... which you didn't address yet. - Dave - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
