Did PCI/IRQ allocation change significantly after 4.2 kernel?

Did PCI/IRQ allocation change significantly after 4.2 kernel?

[Rob Groner]

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/>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20160329/41852846/attachment.html 

Did PCI/IRQ allocation change significantly after 4.2 kernel?

[Greg KH]

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

Did PCI/IRQ allocation change significantly after 4.2 kernel?

[Greg KH]

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

Did PCI/IRQ allocation change significantly after 4.2 kernel?

[Rob Groner]

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

Did PCI/IRQ allocation change significantly after 4.2 kernel?

[Greg KH]

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

Did PCI/IRQ allocation change significantly after 4.2 kernel?

[Rob Groner]

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

Did PCI/IRQ allocation change significantly after 4.2 kernel?

[Greg KH]

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

Did PCI/IRQ allocation change significantly after 4.2 kernel?

[Rob Groner]

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

Did PCI/IRQ allocation change significantly after 4.2 kernel?

[Greg KH]

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

Did PCI/IRQ allocation change significantly after 4.2 kernel?

[Greg KH]

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

Did PCI/IRQ allocation change significantly after 4.2 kernel?

[Rob Groner]

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

Did PCI/IRQ allocation change significantly after 4.2 kernel?

[Greg KH]

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

Did PCI/IRQ allocation change significantly after 4.2 kernel?

[Rob Groner]

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

Did PCI/IRQ allocation change significantly after 4.2 kernel?

[Rob Groner]

And, we have a winner!

-----------------------------------------------------------------------
rtd at 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

Did PCI/IRQ allocation change significantly after 4.2 kernel?

[Greg KH]

On Wed, Mar 30, 2016 at 05:24:40PM -0400, Rob Groner wrote:
> And, we have a winner!
> 
> -----------------------------------------------------------------------
> rtd at 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

Did PCI/IRQ allocation change significantly after 4.2 kernel?

[Greg KH]

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