Patchwork [12/12] usb: use IRQ watching

login
register
mail settings
Submitter Tejun Heo
Date June 13, 2010, 3:31 p.m.
Message ID <1276443098-20653-13-git-send-email-tj@kernel.org>
Download mbox | patch
Permalink /patch/55447/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Tejun Heo - June 13, 2010, 3:31 p.m.
Ask IRQ subsystem to watch HCD IRQ line after initialization.  This at
least keeps USB ports which are occupied on initialization working and
eases bug reporting and debugging.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/usb/core/hcd.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
gregkh@suse.de - June 14, 2010, 9:41 p.m.
On Sun, Jun 13, 2010 at 05:31:38PM +0200, Tejun Heo wrote:
> Ask IRQ subsystem to watch HCD IRQ line after initialization.  This at
> least keeps USB ports which are occupied on initialization working and
> eases bug reporting and debugging.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
>  drivers/usb/core/hcd.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 12742f1..383875f 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2270,6 +2270,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  					"request interrupt %d failed\n", irqnum);
>  			goto err_request_irq;
>  		}
> +		watch_irq(irqnum, hcd);

So if there's a routing problem, it turns into a polled interrupt?  Do
we really want that?  I wonder how long people will run without
realizing that there are problems with their system if their devices
still work.

Other than that minor comment, it all looks good to me.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - June 14, 2010, 9:52 p.m.
Hello,

On 06/14/2010 11:41 PM, Greg KH wrote:
> So if there's a routing problem, it turns into a polled interrupt?  Do
> we really want that?

Oh yeah, I really want that for libata.  Routing is only part of the
problem and flaky IRQ is something we have to learn to cope with.

> I wonder how long people will run without realizing that there are
> problems with their system if their devices still work.

I think things would be better this way.  If the drives (both cd and
hard) / input devices are not accessible, most people would simply
give up rather than reporting, and many cases are transient problems
which happen only once in the blue moon.

It would be great if some kind of automatic reporting can be used
(similar to kerneloops?).  Hmm... maybe make the warnings scarier?

Thanks.
gregkh@suse.de - June 14, 2010, 10:11 p.m.
On Mon, Jun 14, 2010 at 11:52:10PM +0200, Tejun Heo wrote:
> Hello,
> 
> On 06/14/2010 11:41 PM, Greg KH wrote:
> > So if there's a routing problem, it turns into a polled interrupt?  Do
> > we really want that?
> 
> Oh yeah, I really want that for libata.  Routing is only part of the
> problem and flaky IRQ is something we have to learn to cope with.
> 
> > I wonder how long people will run without realizing that there are
> > problems with their system if their devices still work.
> 
> I think things would be better this way.  If the drives (both cd and
> hard) / input devices are not accessible, most people would simply
> give up rather than reporting, and many cases are transient problems
> which happen only once in the blue moon.
> 
> It would be great if some kind of automatic reporting can be used
> (similar to kerneloops?).  Hmm... maybe make the warnings scarier?

If you throw a WARN(), then kerneloops will pick it up, so you should be
fine there.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - June 14, 2010, 10:19 p.m.
cc'ing Kay, hi.

On 06/14/2010 11:52 PM, Tejun Heo wrote:
> Hello,
> 
> On 06/14/2010 11:41 PM, Greg KH wrote:
>> So if there's a routing problem, it turns into a polled interrupt?  Do
>> we really want that?
> 
> Oh yeah, I really want that for libata.  Routing is only part of the
> problem and flaky IRQ is something we have to learn to cope with.
> 
>> I wonder how long people will run without realizing that there are
>> problems with their system if their devices still work.
> 
> I think things would be better this way.  If the drives (both cd and
> hard) / input devices are not accessible, most people would simply
> give up rather than reporting, and many cases are transient problems
> which happen only once in the blue moon.
> 
> It would be great if some kind of automatic reporting can be used
> (similar to kerneloops?).  Hmm... maybe make the warnings scarier?

Hmm... maybe what we can do is generating an uevent when an IRQ is
confirmed to be bad and then let udev notify the user.  That way we'll
probably have better chance of getting bug reports and users have
whiny but working system.

Thanks.
Kay Sievers - June 15, 2010, 10:30 a.m.
On Tue, Jun 15, 2010 at 00:19, Tejun Heo <tj@kernel.org> wrote:
> On 06/14/2010 11:52 PM, Tejun Heo wrote:
>> On 06/14/2010 11:41 PM, Greg KH wrote:
>>> So if there's a routing problem, it turns into a polled interrupt?  Do
>>> we really want that?
>>
>> Oh yeah, I really want that for libata.  Routing is only part of the
>> problem and flaky IRQ is something we have to learn to cope with.
>>
>>> I wonder how long people will run without realizing that there are
>>> problems with their system if their devices still work.
>>
>> I think things would be better this way.  If the drives (both cd and
>> hard) / input devices are not accessible, most people would simply
>> give up rather than reporting, and many cases are transient problems
>> which happen only once in the blue moon.
>>
>> It would be great if some kind of automatic reporting can be used
>> (similar to kerneloops?).  Hmm... maybe make the warnings scarier?
>
> Hmm... maybe what we can do is generating an uevent when an IRQ is
> confirmed to be bad and then let udev notify the user.  That way we'll
> probably have better chance of getting bug reports and users have
> whiny but working system.

Not really, uevents are not picked up by anything that could report an
error to userspace, they are just seen by udev. Also uevents are
usually not the proper passing method. They are not meant to ever
transport higher frequency events, or structured data. They cause to
run the entire udev rule matching machine, and update symlinks and
permissions with every event.

We will need some better error reporting facility. On Linux you don't
even get notified when the kernel mounts your filesystem read-only
because of an error. It will only end up in 'dmesg' as a pretty much
undefined bunch of words. :)

We will need some generic error reporting facility, with structured
data exported, and where userspace stuff can subscribe to.
Uevents/udev can not really properly provide such infrastructure.
Maybe that can be extended somehow, but using kobject_uevent() and
trigger the usual udev rule engine is not what we are looking for, for
sane error reporting.

Kay
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jean Delvare - June 15, 2010, 11:05 a.m.
On Tue, 15 Jun 2010 12:30:00 +0200, Kay Sievers wrote:
> On Tue, Jun 15, 2010 at 00:19, Tejun Heo <tj@kernel.org> wrote:
> > Hmm... maybe what we can do is generating an uevent when an IRQ is
> > confirmed to be bad and then let udev notify the user.  That way we'll
> > probably have better chance of getting bug reports and users have
> > whiny but working system.
> 
> Not really, uevents are not picked up by anything that could report an
> error to userspace, they are just seen by udev. Also uevents are
> usually not the proper passing method. They are not meant to ever
> transport higher frequency events, or structured data. They cause to
> run the entire udev rule matching machine, and update symlinks and
> permissions with every event.
> 
> We will need some better error reporting facility. On Linux you don't
> even get notified when the kernel mounts your filesystem read-only
> because of an error. It will only end up in 'dmesg' as a pretty much
> undefined bunch of words. :)
> 
> We will need some generic error reporting facility, with structured
> data exported, and where userspace stuff can subscribe to.
> Uevents/udev can not really properly provide such infrastructure.
> Maybe that can be extended somehow, but using kobject_uevent() and
> trigger the usual udev rule engine is not what we are looking for, for
> sane error reporting.

Random idea of the day (I don't know anything about it all): let the
kernel connect to D-Bus and use it somehow?
Tejun Heo - June 15, 2010, 11:20 a.m.
Hello, Kay.

On 06/15/2010 12:30 PM, Kay Sievers wrote:
>> Hmm... maybe what we can do is generating an uevent when an IRQ is
>> confirmed to be bad and then let udev notify the user.  That way we'll
>> probably have better chance of getting bug reports and users have
>> whiny but working system.
> 
> Not really, uevents are not picked up by anything that could report an
> error to userspace, they are just seen by udev. Also uevents are
> usually not the proper passing method. They are not meant to ever
> transport higher frequency events, or structured data. They cause to
> run the entire udev rule matching machine, and update symlinks and
> permissions with every event.

Oh, these will be very low frequency event.  At most once per
irq_expect or irqaction.  e.g. on a machine with four hard drives, it
can only happen four times after boot unless the driver is unloaded
and reloaded, so from frequency standpoint it should be okay.

> We will need some better error reporting facility. On Linux you don't
> even get notified when the kernel mounts your filesystem read-only
> because of an error. It will only end up in 'dmesg' as a pretty much
> undefined bunch of words. :)

That one is a very low frequency too.

> We will need some generic error reporting facility, with structured
> data exported, and where userspace stuff can subscribe to.
> Uevents/udev can not really properly provide such infrastructure.
> Maybe that can be extended somehow, but using kobject_uevent() and
> trigger the usual udev rule engine is not what we are looking for, for
> sane error reporting.

It's true that there are higher frequency events which will be
horrible to report via kobject_uevent().  Hmmm... but the thing is
that events which should annoy the user by userland notification can't
be definition high freq.  There's only so many users can recognize and
respond, so the frequency limitation of uevent might actually fit here
although it would be nice to have some kind of safety mechanism.
Still no go for uevent?

Thanks.
Kay Sievers - June 15, 2010, 1:30 p.m.
On Tue, Jun 15, 2010 at 13:05, Jean Delvare <khali@linux-fr.org> wrote:
> On Tue, 15 Jun 2010 12:30:00 +0200, Kay Sievers wrote:
>> On Tue, Jun 15, 2010 at 00:19, Tejun Heo <tj@kernel.org> wrote:
>> > Hmm... maybe what we can do is generating an uevent when an IRQ is
>> > confirmed to be bad and then let udev notify the user.  That way we'll
>> > probably have better chance of getting bug reports and users have
>> > whiny but working system.
>>
>> Not really, uevents are not picked up by anything that could report an
>> error to userspace, they are just seen by udev. Also uevents are
>> usually not the proper passing method. They are not meant to ever
>> transport higher frequency events, or structured data. They cause to
>> run the entire udev rule matching machine, and update symlinks and
>> permissions with every event.
>>
>> We will need some better error reporting facility. On Linux you don't
>> even get notified when the kernel mounts your filesystem read-only
>> because of an error. It will only end up in 'dmesg' as a pretty much
>> undefined bunch of words. :)
>>
>> We will need some generic error reporting facility, with structured
>> data exported, and where userspace stuff can subscribe to.
>> Uevents/udev can not really properly provide such infrastructure.
>> Maybe that can be extended somehow, but using kobject_uevent() and
>> trigger the usual udev rule engine is not what we are looking for, for
>> sane error reporting.
>
> Random idea of the day (I don't know anything about it all): let the
> kernel connect to D-Bus and use it somehow?

Yeah, D-Bus is an peer-to-peer IPC mechanism/protocol. The D-Bus
daemon can filter and multiplex/distibute messages.

It's very similar to what we can do with netlink. The netlink
multicast stuff can even provide lots of the functionality the D-Bus
daemon provides.

I think we should avoid the D-Bus complexity for the very low-level
stuff. Very much like udev is not using it, but has efficient
in-kernel message filtering based on Berkeley Packet Filters, and
multiple listeners event subscription/distribution based on netlink
multicast functionality.

Not sure if netlink is the right answer here, but it's surely easier
to handle than D-Bus, and would provide a very similar functionality.

Kay
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kay Sievers - June 15, 2010, 1:36 p.m.
On Tue, Jun 15, 2010 at 13:20, Tejun Heo <tj@kernel.org> wrote:
> On 06/15/2010 12:30 PM, Kay Sievers wrote:
>>> Hmm... maybe what we can do is generating an uevent when an IRQ is
>>> confirmed to be bad and then let udev notify the user.  That way we'll
>>> probably have better chance of getting bug reports and users have
>>> whiny but working system.
>>
>> Not really, uevents are not picked up by anything that could report an
>> error to userspace, they are just seen by udev. Also uevents are
>> usually not the proper passing method. They are not meant to ever
>> transport higher frequency events, or structured data. They cause to
>> run the entire udev rule matching machine, and update symlinks and
>> permissions with every event.
>
> Oh, these will be very low frequency event.  At most once per
> irq_expect or irqaction.  e.g. on a machine with four hard drives, it
> can only happen four times after boot unless the driver is unloaded
> and reloaded, so from frequency standpoint it should be okay.
>
>> We will need some better error reporting facility. On Linux you don't
>> even get notified when the kernel mounts your filesystem read-only
>> because of an error. It will only end up in 'dmesg' as a pretty much
>> undefined bunch of words. :)
>
> That one is a very low frequency too.
>
>> We will need some generic error reporting facility, with structured
>> data exported, and where userspace stuff can subscribe to.
>> Uevents/udev can not really properly provide such infrastructure.
>> Maybe that can be extended somehow, but using kobject_uevent() and
>> trigger the usual udev rule engine is not what we are looking for, for
>> sane error reporting.
>
> It's true that there are higher frequency events which will be
> horrible to report via kobject_uevent().  Hmmm... but the thing is
> that events which should annoy the user by userland notification can't
> be definition high freq.  There's only so many users can recognize and
> respond, so the frequency limitation of uevent might actually fit here
> although it would be nice to have some kind of safety mechanism.
> Still no go for uevent?

Yeah, I'm pretty sure that's not what we want. We want structured
data, and a generic channel to pass all sort of errors through, and a
userspace part to handle it in a sane way. Many error sources may also
not have a device path in /sys, and therefore no uevent to send.
Uevents/udev just seem so convinient because it's already there, but I
think, has too many limitations to provide the needed functionality.
Besides the fact that nothing listens to these events in userspace
today -- it's a lot more to think through and work on than passing
things through uevents, especially some generic classification and
structured data passing, which is needed, instead of the current
free-text 'dmsg' or the property-based stuff in uevents. I'm very sure
it's the wrong facility to use.

Kay
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - June 15, 2010, 5:36 p.m.
Hello,

On 06/15/2010 03:36 PM, Kay Sievers wrote:
> Yeah, I'm pretty sure that's not what we want. We want structured
> data, and a generic channel to pass all sort of errors through, and a
> userspace part to handle it in a sane way. Many error sources may also
> not have a device path in /sys, and therefore no uevent to send.
> Uevents/udev just seem so convinient because it's already there, but I
> think, has too many limitations to provide the needed functionality.
> Besides the fact that nothing listens to these events in userspace
> today -- it's a lot more to think through and work on than passing
> things through uevents, especially some generic classification and
> structured data passing, which is needed, instead of the current
> free-text 'dmsg' or the property-based stuff in uevents. I'm very sure
> it's the wrong facility to use.

Yeah, well, if you say so.  It would be very nice to have report this
type of critical events in somewhat formatted way so that they can be
processed automatically and presented to the user in more accessible
manner.  I doubt requiring strict structure would work in the long
run.  It'll likely end up being able to cover only portion of what's
originally designed and and stagnate in time.  I think control
information including identification and severity + free form string
would be much more manageable.

It would really be great to have something like that.  I can easily
think of several libata events the user should be notified of from the
top of my head but currently are buried in dmesg.

Thanks.
gregkh@suse.de - June 15, 2010, 5:47 p.m.
On Tue, Jun 15, 2010 at 07:36:12PM +0200, Tejun Heo wrote:
> Hello,
> 
> On 06/15/2010 03:36 PM, Kay Sievers wrote:
> > Yeah, I'm pretty sure that's not what we want. We want structured
> > data, and a generic channel to pass all sort of errors through, and a
> > userspace part to handle it in a sane way. Many error sources may also
> > not have a device path in /sys, and therefore no uevent to send.
> > Uevents/udev just seem so convinient because it's already there, but I
> > think, has too many limitations to provide the needed functionality.
> > Besides the fact that nothing listens to these events in userspace
> > today -- it's a lot more to think through and work on than passing
> > things through uevents, especially some generic classification and
> > structured data passing, which is needed, instead of the current
> > free-text 'dmsg' or the property-based stuff in uevents. I'm very sure
> > it's the wrong facility to use.
> 
> Yeah, well, if you say so.  It would be very nice to have report this
> type of critical events in somewhat formatted way so that they can be
> processed automatically and presented to the user in more accessible
> manner.  I doubt requiring strict structure would work in the long
> run.  It'll likely end up being able to cover only portion of what's
> originally designed and and stagnate in time.  I think control
> information including identification and severity + free form string
> would be much more manageable.
> 
> It would really be great to have something like that.  I can easily
> think of several libata events the user should be notified of from the
> top of my head but currently are buried in dmesg.

That is what Andi Kleen and others are working on at the moment.  I
think he's posted to lkml about it, and there are going to be more talks
about it at the Plumbers conference this year.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - June 15, 2010, 5:52 p.m.
On 06/15/2010 07:47 PM, Greg KH wrote:
>> It would really be great to have something like that.  I can easily
>> think of several libata events the user should be notified of from the
>> top of my head but currently are buried in dmesg.
> 
> That is what Andi Kleen and others are working on at the moment.  I
> think he's posted to lkml about it, and there are going to be more talks
> about it at the Plumbers conference this year.

Ah, cool.  Yeah, I'll be happy to be a beta user for such feature.

thanks.
Tejun Heo - June 21, 2010, 1:51 p.m.
On 06/13/2010 05:31 PM, Tejun Heo wrote:
> Ask IRQ subsystem to watch HCD IRQ line after initialization.  This at
> least keeps USB ports which are occupied on initialization working and
> eases bug reporting and debugging.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>

Greg, if you think the change is okay, can you please ack it?  Also,
would it be okay to route this through irq tree?

Thanks.
gregkh@suse.de - June 21, 2010, 8:27 p.m.
On Mon, Jun 21, 2010 at 03:51:25PM +0200, Tejun Heo wrote:
> On 06/13/2010 05:31 PM, Tejun Heo wrote:
> > Ask IRQ subsystem to watch HCD IRQ line after initialization.  This at
> > least keeps USB ports which are occupied on initialization working and
> > eases bug reporting and debugging.
> > 
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> 
> Greg, if you think the change is okay, can you please ack it?  Also,
> would it be okay to route this through irq tree?

Sorry, I thought I did earlier.  Yes, feel free to add:
	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
and take it through what ever tree you want to :)

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - June 22, 2010, 7:32 a.m.
On 06/21/2010 10:27 PM, Greg KH wrote:
> On Mon, Jun 21, 2010 at 03:51:25PM +0200, Tejun Heo wrote:
>> On 06/13/2010 05:31 PM, Tejun Heo wrote:
>>> Ask IRQ subsystem to watch HCD IRQ line after initialization.  This at
>>> least keeps USB ports which are occupied on initialization working and
>>> eases bug reporting and debugging.
>>>
>>> Signed-off-by: Tejun Heo <tj@kernel.org>
>>
>> Greg, if you think the change is okay, can you please ack it?  Also,
>> would it be okay to route this through irq tree?
> 
> Sorry, I thought I did earlier.  Yes, feel free to add:
> 	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
> and take it through what ever tree you want to :)

Great, thanks. :-)

Patch

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 12742f1..383875f 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2270,6 +2270,7 @@  int usb_add_hcd(struct usb_hcd *hcd,
 					"request interrupt %d failed\n", irqnum);
 			goto err_request_irq;
 		}
+		watch_irq(irqnum, hcd);
 		hcd->irq = irqnum;
 		dev_info(hcd->self.controller, "irq %d, %s 0x%08llx\n", irqnum,
 				(hcd->driver->flags & HCD_MEMORY) ?