Re: [PATCH 1/1] UIO: Add a write() function to enable/disable interrupts

[Leon Woestenberg]

Hello,

On Sat, May 24, 2008 at 12:43 AM, Hans J. Koch <hjk@xxxxxxxxxxxxx> wrote:
> On Fri, May 23, 2008 at 10:44:42PM +0200, Leon Woestenberg wrote:
>>
>> Shouldn't this be more future-proof, what if we need to abuse write()
>> for something else in the future?
>
> We don't. I'm thinking about letting the function fail if irq_on is not
> 0 or 1, just to stop any ideas of abusing write().
>
We don't want to be future-proof?

With kernel UIO and userspace driver in seperate source repositories,
expect serious API drift in the longer term. I.e. the UIO interface
must be backwards and forwards proof IMHO.

> read() and write() only deal with irq handling, all data exchange with the
> device is done through mapped memory.
>
*Currently*, read() and write() only deal with irq handling.

In the future you might want to add a second control. I cannot think
of what that should be now, much like it was not foreseen a write()
call was needed.

>> I would suggest a check for ppos to be 0 (zero) as well, just to be
>> sure and future-proof and backwards-safe.
>
> write() is only for enabling/disabling irqs, there's only one possible
> value of count, and we don't have a seek function. So why check ppos?
>
So that *if* we have a second write()able location (again, for
something I cannot foresee now), you at least check that the userspace
proper wants to enable/disable the interrupt.

AFAIK, POSIX pwrite() does not require a seek() implementation in the
driver, but will come in with a different ppos.

Idea and patch looks fine, I just wanted to bring this up so that it
is considered.

Regards,
-- 
Leon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/