On Thu, Jul 03, 2008 at 09:10:19AM +0200, Uwe Kleine-König wrote:
BTW, the above wording "the user space driver is responsible for
acknowledging and re-enabling the interrupt" is misleading. The kernel
always has to acknowledge/disable/mask the interrupt. Userspace can only
reenable it, ideally by writing to a chip register. In some cornercases
for broken hardware we need the newly introduced write function.
I question the whole concept as such. The concept of having a generic
irq handler using disable_irq_nosync() makes no sense at all.
Reasons:
- We do not introduce new possibilities. Everything can be done without
that patch.
- We offer users an irq handler that shouldn't be used. It is seldom the
best solution to call disable_irq_nosync() to disable an interrupt. In
almost all cases you should use the irq mask register of the chip your
driver handles. What do you want to write into the docs? Here's an irq
handler, but please use it only if you're really desperate?
- The only argument in favor of that concept was that it saves a few
lines of irq handler code. Given the fact that all the handler has to
do is toggle one bit in a hardware register, this is really not much.
And you tempt people to delete 5 lines of good code and replace them
with a sub-optimal generic irq handler.
- You introduce the need for an irqcontrol function. This was not the
intention of that concept. Normal UIO devices don't need a write
function, this is only used for broken hardware. If you have normal
hardware, and implement a proper 5 lines irq handler, userspace can simply
reenable the irq by writing to a hardware register it has mapped
anyway. In your concept, it has to use write() on /dev/uioX, which
means you have to go all the way through libc, vfs, and the UIO core
to finally call your irqcontrol function, which in turn calls
enable_irq. As I said, there is broken hardware around where this is
the only way, but most chips allow you to do it properly.
OK, the version Magnus sent last is different, so some of my arguments
are superfluous now.
Please, to make things simpler, let's only talk about the concept as
such and not go into implementation details. I deliberately do not review
that code (although I believe it has more bugs than the one Alan found),
because as long as the concept doesn't make sense, I don't care how it
is implemented.
You both keep telling me how valuable that patch is but never answered my
question what the advantage would be. I cannot see it yet.
OK, that is one argument in favor. I always admitted that, but said that
this is not enough to compensate for the disadvantages mentioned above.
For me, this shows two things:
- I never ever had to use disable_irq_nosync() in any UIO driver yet,
otherwise I would have noticed.
- Magnus turned in a patch that he never tested.
It's not a good idea to add nonsense code and tell the users to ignore
it whenever they can...
Well, at least the whole stuff would have to be explained in the docs.
You add an element to struct uio_info, together with a new #define. And
a whole class of drivers using that stuff would have a write() function
without needing it.
Oh, no, stay cool ;-)
Thanks,
Hans
--
