Re: Did PCI/IRQ allocation change significantly after 4.2 kernel?
On Tue, Mar 29, 2016 at 03:11:46PM -0400, Rob Groner wrote:
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?
Yes you do.
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?
I can take it "as-is" through the staging tree (drivers/staging/) which lets the community start to clean it up now, if you don't have the time to do the work now. Then you can work within the kernel development process to polish up the remaining issues before we merge it out of the staging tree into the "real" portion of the kernel. If you are interested in this, just let me know. thanks, greg k-h
On Tue, Mar 29, 2016 at 12:18:40PM -0700, Greg KH wrote:
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?
I can take it "as-is" through the staging tree (drivers/staging/) which lets the community start to clean it up now, if you don't have the time to do the work now. Then you can work within the kernel development process to polish up the remaining issues before we merge it out of the staging tree into the "real" portion of the kernel.
If you are interested in this, just let me know.
In looking at your driver closer, what exactly is it doing? At first glance, it seems to be just a bunch of ioctls to directly access the memory in the card, is that correct? If so, why not just use the UIO layer instead? If not, what is the driver trying to do? thanks, greg k-h
On Tue, 2016-03-29 at 12:22 -0700, Greg KH wrote:
On Tue, Mar 29, 2016 at 12:18:40PM -0700, Greg KH wrote:
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?
I can take it "as-is" through the staging tree (drivers/staging/) which lets the community start to clean it up now, if you don't have the time to do the work now. Then you can work within the kernel development process to polish up the remaining issues before we merge it out of the staging tree into the "real" portion of the kernel.
If you are interested in this, just let me know.
In looking at your driver closer, what exactly is it doing? At first glance, it seems to be just a bunch of ioctls to directly access the memory in the card, is that correct? If so, why not just use the UIO layer instead? If not, what is the driver trying to do?
thanks,
greg k-h
Your first glance is probably correct. The driver handles reads and writes to registers via IOCTLs from the user library, as well as interrupts and DMA. There are probably two main reasons the driver is structured like that: 1) All "new" drivers here are essentially tailored versions of previous drivers that have been around for a while, so this wasn't built-from-scratch new. While it means that any poor design decisions made early on are perpetuated, it also means to us that we aren't re-inventing the wheel and we're mostly using a driver that has proven to work for quite a while. 2) The library code for the board is shared (internally) with our Windows driver package, and in some cases DOS too. Since Windows uses IOCTLs, we can essentially share the exact same library files, and only the IOCTL call itself is OS-specific. I know #1 is not a good reason and I'd be happy to work towards re-writing the driver in a more correct way, but probably not if it would cause us to have to split into a Linux and Windows library versions. Keeping the library common at this point has saved us a lot of development time. I absolutely appreciate the feedback on driver design...I've never really received any and all I know about Linux drivers is what I read online, read in LDD, and what was in the drivers that existed when I came to work here. Is the current form of the driver just "bad" or "intolerably bad"? Thanks! Rob
On Tue, Mar 29, 2016 at 04:11:22PM -0400, Rob Groner wrote:
Your first glance is probably correct. The driver handles reads and writes to registers via IOCTLs from the user library, as well as interrupts and DMA. There are probably two main reasons the driver is structured like that: 1) All "new" drivers here are essentially tailored versions of previous drivers that have been around for a while, so this wasn't built-from-scratch new. While it means that any poor design decisions made early on are perpetuated, it also means to us that we aren't re-inventing the wheel and we're mostly using a driver that has proven to work for quite a while. 2) The library code for the board is shared (internally) with our Windows driver package, and in some cases DOS too. Since Windows uses IOCTLs, we can essentially share the exact same library files, and only the IOCTL call itself is OS-specific.
I know #1 is not a good reason and I'd be happy to work towards re-writing the driver in a more correct way, but probably not if it would cause us to have to split into a Linux and Windows library versions. Keeping the library common at this point has saved us a lot of development time.
I understand the need for userspace libraries to be "unified", but you might be able to get away with no kernel driver at all, just use the UIO driver for your device and then read/write the memory mapped area of your card directly from your library/application. That will have the benifit of making your Linux implementation faster as well :)
I absolutely appreciate the feedback on driver design...I've never really received any and all I know about Linux drivers is what I read online, read in LDD, and what was in the drivers that existed when I came to work here. Is the current form of the driver just "bad" or "intolerably bad"?
It's not "bad", but there is room for improvement to make it smaller and more "flexible" (i.e. no static list of devices, use the pci driver model better, etc.) You should also make sure you are properly checking your ioctl calls to ensure you don't have any security issues hiding here, that wouldn't be good for your users. I think you are doing this correctly in dm35418_validate_pci_access(), but it would be good to verify this again to be sure, your ioctl structures are a bit "odd" in that they try to be generic so it's hard to really tell what you are passing around. thanks, greg k-h
On Tue, 2016-03-29 at 17:57 -0700, Greg KH wrote:
On Tue, Mar 29, 2016 at 04:11:22PM -0400, Rob Groner wrote:
Your first glance is probably correct. The driver handles reads and writes to registers via IOCTLs from the user library, as well as interrupts and DMA. There are probably two main reasons the driver is structured like that: 1) All "new" drivers here are essentially tailored versions of previous drivers that have been around for a while, so this wasn't built-from-scratch new. While it means that any poor design decisions made early on are perpetuated, it also means to us that we aren't re-inventing the wheel and we're mostly using a driver that has proven to work for quite a while. 2) The library code for the board is shared (internally) with our Windows driver package, and in some cases DOS too. Since Windows uses IOCTLs, we can essentially share the exact same library files, and only the IOCTL call itself is OS-specific.
I know #1 is not a good reason and I'd be happy to work towards re-writing the driver in a more correct way, but probably not if it would cause us to have to split into a Linux and Windows library versions. Keeping the library common at this point has saved us a lot of development time.
I understand the need for userspace libraries to be "unified", but you might be able to get away with no kernel driver at all, just use the UIO driver for your device and then read/write the memory mapped area of your card directly from your library/application. That will have the benifit of making your Linux implementation faster as well :)
I'm looking at the UIO API for the first time, and I'm beginning to understand it, and I *do* see the benefits of it. Instead of working to get this driver accepted in the kernel, I think I am going to instead make a UIO implementation my pet project.
From what I've learned so far, I'll still need a kernel driver for the interrupt handler, just much smaller. One thing I haven't been able to find out for sure, however, is if DMA is possible through a UIO implementation. Not seeing any mention of it on kernel.org isn't encouraging.
I absolutely appreciate the feedback on driver design...I've never really received any and all I know about Linux drivers is what I read online, read in LDD, and what was in the drivers that existed when I came to work here. Is the current form of the driver just "bad" or "intolerably bad"?
It's not "bad", but there is room for improvement to make it smaller and more "flexible" (i.e. no static list of devices, use the pci driver model better, etc.) You should also make sure you are properly checking your ioctl calls to ensure you don't have any security issues hiding here, that wouldn't be good for your users. I think you are doing this correctly in dm35418_validate_pci_access(), but it would be good to verify this again to be sure, your ioctl structures are a bit "odd" in that they try to be generic so it's hard to really tell what you are passing around.
That is good input. I'll take another look at how I scrutinize what is being sent in IOCTLs. I'm guessing that it would probably be better if I used read() and write() instead of a READ IOCTL and WRITE IOCTL, correct? BTW, git bisect continues. It says about 10 more tries.... Rob
On Wed, Mar 30, 2016 at 11:51:50AM -0400, Rob Groner wrote:
On Tue, 2016-03-29 at 17:57 -0700, Greg KH wrote:
On Tue, Mar 29, 2016 at 04:11:22PM -0400, Rob Groner wrote:
Your first glance is probably correct. The driver handles reads and writes to registers via IOCTLs from the user library, as well as interrupts and DMA. There are probably two main reasons the driver is structured like that: 1) All "new" drivers here are essentially tailored versions of previous drivers that have been around for a while, so this wasn't built-from-scratch new. While it means that any poor design decisions made early on are perpetuated, it also means to us that we aren't re-inventing the wheel and we're mostly using a driver that has proven to work for quite a while. 2) The library code for the board is shared (internally) with our Windows driver package, and in some cases DOS too. Since Windows uses IOCTLs, we can essentially share the exact same library files, and only the IOCTL call itself is OS-specific.
I know #1 is not a good reason and I'd be happy to work towards re-writing the driver in a more correct way, but probably not if it would cause us to have to split into a Linux and Windows library versions. Keeping the library common at this point has saved us a lot of development time.
I understand the need for userspace libraries to be "unified", but you might be able to get away with no kernel driver at all, just use the UIO driver for your device and then read/write the memory mapped area of your card directly from your library/application. That will have the benifit of making your Linux implementation faster as well :)
I'm looking at the UIO API for the first time, and I'm beginning to understand it, and I *do* see the benefits of it. Instead of working to get this driver accepted in the kernel, I think I am going to instead make a UIO implementation my pet project.
From what I've learned so far, I'll still need a kernel driver for the interrupt handler, just much smaller.
That is correct.
One thing I haven't been able to find out for sure, however, is if DMA is possible through a UIO implementation. Not seeing any mention of it on kernel.org isn't encouraging.
It keeps coming up in dicussion on how to do this in a "generic" way, look at the lkml archives for the details. But you can do this in a specific-way for your own device, there are 2 drivers in the tree that handle this in some manner, try looking at those for inspiration. thanks, greg k-h
And, we have a winner! ----------------------------------------------------------------------- rtd@kernel-dev:~/git/kernels/linux$ git bisect good 991de2e59090e55c65a7f59a049142e3c480f7bd is the first bad commit commit 991de2e59090e55c65a7f59a049142e3c480f7bd Author: Jiang Liu <jiang.liu@linux.intel.com> Date: Wed Jun 10 16:54:59 2015 +0800 PCI, x86: Implement pcibios_alloc_irq() and pcibios_free_irq() To support IOAPIC hotplug, we need to allocate PCI IRQ resources on demand and free them when not used anymore. Implement pcibios_alloc_irq() and pcibios_free_irq() to dynamically allocate and free PCI IRQs. Remove mp_should_keep_irq(), which is no longer used. [bhelgaas: changelog] Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Acked-by: Thomas Gleixner <tglx@linutronix.de> :040000 040000 765e2d5232d53247ec260b34b51589c3bccb36ae f680234a27685e94b1a35ae2a7218f8eafa9071a M arch :040000 040000 d55a682bcde72682e883365e88ad1df6186fd54d f82c470a04a6845fcf5e0aa934512c75628f798d M drivers ---------------------------------------------------------------- I'll sort out what all of that means tomorrow... Rob
On Wed, Mar 30, 2016 at 05:24:40PM -0400, Rob Groner wrote:
And, we have a winner!
----------------------------------------------------------------------- rtd@kernel-dev:~/git/kernels/linux$ git bisect good 991de2e59090e55c65a7f59a049142e3c480f7bd is the first bad commit commit 991de2e59090e55c65a7f59a049142e3c480f7bd Author: Jiang Liu <jiang.liu@linux.intel.com> Date: Wed Jun 10 16:54:59 2015 +0800
PCI, x86: Implement pcibios_alloc_irq() and pcibios_free_irq()
To support IOAPIC hotplug, we need to allocate PCI IRQ resources on demand and free them when not used anymore.
Implement pcibios_alloc_irq() and pcibios_free_irq() to dynamically allocate and free PCI IRQs.
Remove mp_should_keep_irq(), which is no longer used.
[bhelgaas: changelog] Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Acked-by: Thomas Gleixner <tglx@linutronix.de>
:040000 040000 765e2d5232d53247ec260b34b51589c3bccb36ae f680234a27685e94b1a35ae2a7218f8eafa9071a M arch :040000 040000 d55a682bcde72682e883365e88ad1df6186fd54d f82c470a04a6845fcf5e0aa934512c75628f798d M drivers ----------------------------------------------------------------
I'll sort out what all of that means tomorrow...
Great job, please email these developers, and the pci mailing list with the information and they will help you out. greg k-h
participants (2)
-
Greg KH -
Rob Groner