Did PCI/IRQ allocation change significantly after 4.2 kernel?
I'm investigating why our drivers no longer work correctly after the 4.2 kernel. I have verified that in the 4.4 kernel, interrupts no longer work for us as they did before. Here are the symptoms: The driver requests an IRQ, the one that is in the pci_dev structure that comes from pci_get_device(). In most cases, that IRQ is 7. The request_irq call is successful, and the driver reports that it has IRQ 7. However, interrupts never arrive after that, and eventually an exception is thrown indicating that interrupts WERE arriving at IRQ 16 and nobody responded to them. Doing this same test with a 4.2 kernel, I see that even though lspci initially reports that the board is on IRQ 7, after the driver is loaded it then reports that the board is on IRQ 16. The driver also says it is on IRQ 16, and everything works after that. Here is what I've found so far: both kernels report the board is on IRQ 7 according to lspci before the driver is loaded. After the driver is loaded, the 4.2 kernel lspci says the board is on IRQ 16 and the 4.4 kernel says it is still on IRQ 7. In both cases, the actual interrupts appear to be coming on IRQ 16 and the 4.4 kernel fails because that is not where the interrupt handler was put. I have also found that the IRQ associated with the pci_device changes during the pci_enable_device call (before the request_irq call). In the 4.2 kernel, it changes from 7 to 16, but in the 4.4 kernel it stays 7. This is the most obvious difference I've found so far between the two kernels. What I don't understand is why the request_irq call succeeds in the 4.4 kernel, yet interrupts don't arrive on IRQ 7...they go to IRQ 16 instead. I looked through the LinuxVersions on kernelnewbies.org for 4.3 and 4.4 but did not see anything that looked like it related to IRQs, pci devices, or any of that (at least, for my particular problem). Can someone direct me to the pertinent documentation about the change if it exists? I'm walking through the kernel code now to try and find why the 4.2 pci_enable changes the IRQ from 7 to 16, but the 4.4 does not. Thank you for any help. Rob Groner Senior Electronics Engineer Modules and Systems RTD Embedded Technologies, Inc. ISO 9001 and AS9100 Certified www.rtd.com<http://www.rtd.com/>
On Tue, Mar 29, 2016 at 02:15:23PM +0000, Rob Groner wrote:
I’m investigating why our drivers no longer work correctly after the 4.2 kernel. I have verified that in the 4.4 kernel, interrupts no longer work for us as they did before. Here are the symptoms:
What drivers are these? Do you have a pointer to the source for them? And what platform are you having these problems on, x86, arm64, something else? thanks, greg k-h
On Tue, Mar 29, 2016 at 07:42:44AM -0700, Greg KH wrote:
On Tue, Mar 29, 2016 at 02:15:23PM +0000, Rob Groner wrote:
I’m investigating why our drivers no longer work correctly after the 4.2 kernel. I have verified that in the 4.4 kernel, interrupts no longer work for us as they did before. Here are the symptoms:
What drivers are these? Do you have a pointer to the source for them?
And what platform are you having these problems on, x86, arm64, something else?
Also, can you use 'git bisect' to find the problem commit? That might be the easiest way forward. thanks, greg k-h
On Tue, 2016-03-29 at 07:43 -0700, Greg KH wrote:
On Tue, Mar 29, 2016 at 07:42:44AM -0700, Greg KH wrote:
On Tue, Mar 29, 2016 at 02:15:23PM +0000, Rob Groner wrote:
I’m investigating why our drivers no longer work correctly after the 4.2 kernel. I have verified that in the 4.4 kernel, interrupts no longer work for us as they did before. Here are the symptoms:
What drivers are these? Do you have a pointer to the source for them?
And what platform are you having these problems on, x86, arm64, something else?
x86 only (we don't support other platforms). An example of the code (this problem appears to be affecting all of our drivers so far) is on our website. http://www.rtd.com/software/DM/DM35x18/DM35418_Linux_v03.00.00.tar.gz If you end up looking at the driver code, I apologize in advance for all of the obvious errors you might find. It's a little intimidating having one of the authors of LDD look at your code... I feel like I didn't study hard enough for this.
Also, can you use 'git bisect' to find the problem commit? That might be the easiest way forward.
Umm...yes! As soon as I figure out what that means. I'm a git newbie (we use svn), but I shall learn as quickly as I can. I take it that it basically means "start with a kernel that worked and then start moving forward through commits until I find the one that broke", right? Rob
On Tue, Mar 29, 2016 at 11:27:49AM -0400, Rob Groner wrote:
On Tue, 2016-03-29 at 07:43 -0700, Greg KH wrote:
On Tue, Mar 29, 2016 at 07:42:44AM -0700, Greg KH wrote:
On Tue, Mar 29, 2016 at 02:15:23PM +0000, Rob Groner wrote:
I’m investigating why our drivers no longer work correctly after the 4.2 kernel. I have verified that in the 4.4 kernel, interrupts no longer work for us as they did before. Here are the symptoms:
What drivers are these? Do you have a pointer to the source for them?
And what platform are you having these problems on, x86, arm64, something else?
x86 only (we don't support other platforms). An example of the code (this problem appears to be affecting all of our drivers so far) is on our website.
http://www.rtd.com/software/DM/DM35x18/DM35418_Linux_v03.00.00.tar.gz
If you end up looking at the driver code, I apologize in advance for all of the obvious errors you might find. It's a little intimidating having one of the authors of LDD look at your code... I feel like I didn't study hard enough for this.
Heh, no worries. One question, why are you saving off the pci irq before you call request_irq()? You shouldn't care what the number is, and with MSI and other fun, you might get a "different" irq number than the main PCI device was because nothing was really assigned to it just yet. Also, why not submit this for inclusion in the main kernel tree? That will make your ongoing maintenance of the code much easier.
Also, can you use 'git bisect' to find the problem commit? That might be the easiest way forward.
Umm...yes! As soon as I figure out what that means. I'm a git newbie (we use svn), but I shall learn as quickly as I can. I take it that it basically means "start with a kernel that worked and then start moving forward through commits until I find the one that broke", right?
Yes, but it uses a tree search, making it faster than just "one commit after each other" Read 'man git-bisect' for all of the details. thanks, greg k-h
On Tue, 2016-03-29 at 08:43 -0700, Greg KH wrote:
On Tue, Mar 29, 2016 at 11:27:49AM -0400, Rob Groner wrote:
x86 only (we don't support other platforms). An example of the code (this problem appears to be affecting all of our drivers so far) is on our website.
http://www.rtd.com/software/DM/DM35x18/DM35418_Linux_v03.00.00.tar.gz
If you end up looking at the driver code, I apologize in advance for all of the obvious errors you might find. It's a little intimidating having one of the authors of LDD look at your code... I feel like I didn't study hard enough for this.
Heh, no worries. One question, why are you saving off the pci irq before you call request_irq()? You shouldn't care what the number is, and with MSI and other fun, you might get a "different" irq number than the main PCI device was because nothing was really assigned to it just yet.
Can I invoke my 5th amendment rights here? Actually, I don't really know. Most of that driver is inherited from other drivers we had before it so a lot of code is "legacy" for lack of a better term. The driver appears to care about what the IRQ number because it uses it in several other places in the driver: to compare to the incoming IRQ in the interrupt handler, and to use when the "free_irq" call is required. If we shouldn't care what the IRQ is then that means we don't need it for those things? Or are you saying we should just keep a pointer to the pci_dev and reference that IRQ value instead of saving our own?
Also, why not submit this for inclusion in the main kernel tree? That will make your ongoing maintenance of the code much easier.
I had considered that since this driver currently supports 5 of our boards (that's better than most of our drivers). It would be a nice thing to say it is supported in the kernel. But I'm not sure how that will make maintenance easier. I had a small view into the patch submitting process earlier this year, and it didn't seem easy...Is it different if I'm patching my own driver?
Also, can you use 'git bisect' to find the problem commit? That might be the easiest way forward.
Umm...yes! As soon as I figure out what that means. I'm a git newbie (we use svn), but I shall learn as quickly as I can. I take it that it basically means "start with a kernel that worked and then start moving forward through commits until I find the one that broke", right?
Yes, but it uses a tree search, making it faster than just "one commit after each other" Read 'man git-bisect' for all of the details.
Gotcha! I'll get to work on that. Thanks. Rob
On Tue, Mar 29, 2016 at 02:27:01PM -0400, Rob Groner wrote:
On Tue, 2016-03-29 at 08:43 -0700, Greg KH wrote:
On Tue, Mar 29, 2016 at 11:27:49AM -0400, Rob Groner wrote:
x86 only (we don't support other platforms). An example of the code (this problem appears to be affecting all of our drivers so far) is on our website.
http://www.rtd.com/software/DM/DM35x18/DM35418_Linux_v03.00.00.tar.gz
If you end up looking at the driver code, I apologize in advance for all of the obvious errors you might find. It's a little intimidating having one of the authors of LDD look at your code... I feel like I didn't study hard enough for this.
Heh, no worries. One question, why are you saving off the pci irq before you call request_irq()? You shouldn't care what the number is, and with MSI and other fun, you might get a "different" irq number than the main PCI device was because nothing was really assigned to it just yet.
Can I invoke my 5th amendment rights here?
Actually, I don't really know. Most of that driver is inherited from other drivers we had before it so a lot of code is "legacy" for lack of a better term.
The driver appears to care about what the IRQ number because it uses it in several other places in the driver: to compare to the incoming IRQ in the interrupt handler, and to use when the "free_irq" call is required. If we shouldn't care what the IRQ is then that means we don't need it for those things? Or are you saying we should just keep a pointer to the pci_dev and reference that IRQ value instead of saving our own?
Hm, maybe this is ok, it just seems odd that you check the irq number in the handler, that shouldn't be needed at all as the core will not call you unless the irq you have signed up for has been triggered.
Also, why not submit this for inclusion in the main kernel tree? That will make your ongoing maintenance of the code much easier.
I had considered that since this driver currently supports 5 of our boards (that's better than most of our drivers). It would be a nice thing to say it is supported in the kernel. But I'm not sure how that will make maintenance easier. I had a small view into the patch submitting process earlier this year, and it didn't seem easy...Is it different if I'm patching my own driver?
You will get other people fixing your bugs and for any api changes, they will be made automatically. And, odds are, your driver will get a lot smaller, there seems to be things in there that aren't needed. And less code means less bugs and easier to maintain overtime. It shouldn't be hard to merge patches for a driver you maintain, if so then the development process is at fault, and let me know what's going on and I'll work to help fix it. thanks, greg k-h
On Tue, 2016-03-29 at 11:38 -0700, Greg KH wrote:
On Tue, Mar 29, 2016 at 02:27:01PM -0400, Rob Groner wrote:
On Tue, 2016-03-29 at 08:43 -0700, Greg KH wrote:
On Tue, Mar 29, 2016 at 11:27:49AM -0400, Rob Groner wrote:
The driver appears to care about what the IRQ number because it uses it in several other places in the driver: to compare to the incoming IRQ in the interrupt handler, and to use when the "free_irq" call is required. If we shouldn't care what the IRQ is then that means we don't need it for those things? Or are you saying we should just keep a pointer to the pci_dev and reference that IRQ value instead of saving our own?
Hm, maybe this is ok, it just seems odd that you check the irq number in the handler, that shouldn't be needed at all as the core will not call you unless the irq you have signed up for has been triggered.
Has that always been true? I'm pretty sure that code came from a driver that was around the 2.6.35 era, or earlier. I will happily remove that IRQ check, though. I'm always interested in less code to maintain. Don't I still need to keep ahold of the IRQ number to call free_irq() when the resources are released, though?
Also, why not submit this for inclusion in the main kernel tree? That will make your ongoing maintenance of the code much easier.
I had considered that since this driver currently supports 5 of our boards (that's better than most of our drivers). It would be a nice thing to say it is supported in the kernel. But I'm not sure how that will make maintenance easier. I had a small view into the patch submitting process earlier this year, and it didn't seem easy...Is it different if I'm patching my own driver?
You will get other people fixing your bugs and for any api changes, they will be made automatically. And, odds are, your driver will get a lot smaller, there seems to be things in there that aren't needed. And less code means less bugs and easier to maintain overtime.
It shouldn't be hard to merge patches for a driver you maintain, if so then the development process is at fault, and let me know what's going on and I'll work to help fix it.
That is encouraging and persuasive. I will make submitting the driver one of my pet projects. I need to put a new coat of paint on it before I submit it for consideration, though. Do I post to this mailing list when I'm ready? Or is there a more pertinent one? Thanks Rob
participants (3)
-
Greg KH -
Rob Groner -
Rob Groner