[PATCH] hpet header sanitization

[PATCH] hpet header sanitization

[Randy.Dunlap]

From: Randy Dunlap <rdunlap@xenotime.net>

Add __KERNEL__ block.
Use __KERNEL__ to allow ioctl interface to be usable.

Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>
---
 include/linux/hpet.h |   36 ++++++++++++++++++++----------------
 1 files changed, 20 insertions(+), 16 deletions(-)

--- linux-2616-work.orig/include/linux/hpet.h
+++ linux-2616-work/include/linux/hpet.h
@@ -3,6 +3,8 @@
 
 #include <linux/compiler.h>
 
+#ifdef __KERNEL__
+
 /*
  * Offsets into HPET Registers
  */
@@ -85,22 +87,6 @@ struct hpet {
 #define	Tn_FSB_INT_ADDR_SHIFT		(32UL)
 #define	Tn_FSB_INT_VAL_MASK		(0x00000000ffffffffULL)
 
-struct hpet_info {
-	unsigned long hi_ireqfreq;	/* Hz */
-	unsigned long hi_flags;	/* information */
-	unsigned short hi_hpet;
-	unsigned short hi_timer;
-};
-
-#define	HPET_INFO_PERIODIC	0x0001	/* timer is periodic */
-
-#define	HPET_IE_ON	_IO('h', 0x01)	/* interrupt on */
-#define	HPET_IE_OFF	_IO('h', 0x02)	/* interrupt off */
-#define	HPET_INFO	_IOR('h', 0x03, struct hpet_info)
-#define	HPET_EPI	_IO('h', 0x04)	/* enable periodic */
-#define	HPET_DPI	_IO('h', 0x05)	/* disable periodic */
-#define	HPET_IRQFREQ	_IOW('h', 0x6, unsigned long)	/* IRQFREQ usec */
-
 /*
  * exported interfaces
  */
@@ -133,4 +119,22 @@ int hpet_register(struct hpet_task *, in
 int hpet_unregister(struct hpet_task *);
 int hpet_control(struct hpet_task *, unsigned int, unsigned long);
 
+#endif /* __KERNEL__ */
+
+struct hpet_info {
+	unsigned long hi_ireqfreq;	/* Hz */
+	unsigned long hi_flags;	/* information */
+	unsigned short hi_hpet;
+	unsigned short hi_timer;
+};
+
+#define	HPET_INFO_PERIODIC	0x0001	/* timer is periodic */
+
+#define	HPET_IE_ON	_IO('h', 0x01)	/* interrupt on */
+#define	HPET_IE_OFF	_IO('h', 0x02)	/* interrupt off */
+#define	HPET_INFO	_IOR('h', 0x03, struct hpet_info)
+#define	HPET_EPI	_IO('h', 0x04)	/* enable periodic */
+#define	HPET_DPI	_IO('h', 0x05)	/* disable periodic */
+#define	HPET_IRQFREQ	_IOW('h', 0x6, unsigned long)	/* IRQFREQ usec */
+
 #endif				/* !__HPET__ */


---

Re: [PATCH] hpet header sanitization

[Andrew Morton]

"Randy.Dunlap" <rdunlap@xenotime.net> wrote:
>
> From: Randy Dunlap <rdunlap@xenotime.net>
> 
> Add __KERNEL__ block.
> Use __KERNEL__ to allow ioctl interface to be usable.

hm, why?

My general approach to __KERNEL__ fixes is to not support new includers of
kernel headers but to accept patches which fix up existing applications of
__KERNEL__.

It's basically a compromise between the
dont-include-kernel-headers-from-userspace fundamentalists and the
but-i-want-my-stuff-to-work pragmatists ;)

But hpet.h never had __KERNEL__, so there's no regression here.

That being said, it looks like a sensible change - let's see if we can
sneak it in without the fundies noticing.

oops.

Re: [PATCH] hpet header sanitization

[Randy.Dunlap]

On Tue, 21 Mar 2006 16:13:03 -0800 Andrew Morton wrote:

> "Randy.Dunlap" <rdunlap@xenotime.net> wrote:
> >
> > From: Randy Dunlap <rdunlap@xenotime.net>
> > 
> > Add __KERNEL__ block.
> > Use __KERNEL__ to allow ioctl interface to be usable.
> 
> hm, why?

because there is a test/example source file in (inside)
Documentation/hpet.txt that won't build otherwise.
And because hpet.h contains _userspace_ ioctl interface struct
and macros...


> My general approach to __KERNEL__ fixes is to not support new includers of
> kernel headers but to accept patches which fix up existing applications of
> __KERNEL__.
> 
> It's basically a compromise between the
> dont-include-kernel-headers-from-userspace fundamentalists and the
> but-i-want-my-stuff-to-work pragmatists ;)
> 
> But hpet.h never had __KERNEL__, so there's no regression here.
> 
> That being said, it looks like a sensible change - let's see if we can
> sneak it in without the fundies noticing.

---
~Randy

Re: [PATCH] hpet header sanitization

[Arjan van de Ven]

On Tue, 2006-03-21 at 16:26 -0800, Randy.Dunlap wrote:
> On Tue, 21 Mar 2006 16:13:03 -0800 Andrew Morton wrote:
> 
> > "Randy.Dunlap" <rdunlap@xenotime.net> wrote:
> > >
> > > From: Randy Dunlap <rdunlap@xenotime.net>
> > > 
> > > Add __KERNEL__ block.
> > > Use __KERNEL__ to allow ioctl interface to be usable.
> > 
> > hm, why?
> 
> because there is a test/example source file in (inside)
> Documentation/hpet.txt that won't build otherwise.
> And because hpet.h contains _userspace_ ioctl interface struct
> and macros...


then please split the header in 2 parts; one for the kernel
and one for userspace


either put both here, or move the kernel one to the directory where the
source code is

Re: [PATCH] hpet header sanitization

[Arnd Bergmann]

On Tuesday 21 March 2006 23:46, Randy.Dunlap wrote:
> +#define        HPET_IE_ON      _IO('h', 0x01)  /* interrupt on */
> +#define        HPET_IE_OFF     _IO('h', 0x02)  /* interrupt off */
> +#define        HPET_INFO       _IOR('h', 0x03, struct hpet_info)
> +#define        HPET_EPI        _IO('h', 0x04)  /* enable periodic */
> +#define        HPET_DPI        _IO('h', 0x05)  /* disable periodic */
> +#define        HPET_IRQFREQ    _IOW('h', 0x6, unsigned long)   /* IRQFREQ usec */

By the way, HPET_INFO and HPET_IRQFREQ don't work for 32 bit user space,
the driver needs a compat_ioctl() method to handle those.

	Arnd <><

Re: [PATCH] hpet header sanitization

[Clemens Ladisch]

Arnd Bergmann wrote:
> On Tuesday 21 March 2006 23:46, Randy.Dunlap wrote:
> > +#define        HPET_IE_ON      _IO('h', 0x01)  /* interrupt on */
> > +#define        HPET_IE_OFF     _IO('h', 0x02)  /* interrupt off */
> > +#define        HPET_INFO       _IOR('h', 0x03, struct hpet_info)
> > +#define        HPET_EPI        _IO('h', 0x04)  /* enable periodic */
> > +#define        HPET_DPI        _IO('h', 0x05)  /* disable periodic */
> > +#define        HPET_IRQFREQ    _IOW('h', 0x6, unsigned long)   /* IRQFREQ usec */
> 
> By the way, HPET_INFO and HPET_IRQFREQ don't work for 32 bit user space,
> the driver needs a compat_ioctl() method to handle those.

There isn't any program (except the example in the docs) that uses any
of these ioctls, and I'm writing patches to make this device available
through portable timer APIs like hrtimer/POSIX clocks/ALSA that are much
easier to use besides, so I think it would be a good idea to just
schedule these ioctls for removal.


Clemens

Re: [PATCH] hpet header sanitization

[Randy.Dunlap]

On Wed, 22 Mar 2006 12:14:46 +0100 Clemens Ladisch wrote:

> Arnd Bergmann wrote:
> > On Tuesday 21 March 2006 23:46, Randy.Dunlap wrote:
> > > +#define        HPET_IE_ON      _IO('h', 0x01)  /* interrupt on */
> > > +#define        HPET_IE_OFF     _IO('h', 0x02)  /* interrupt off */
> > > +#define        HPET_INFO       _IOR('h', 0x03, struct hpet_info)
> > > +#define        HPET_EPI        _IO('h', 0x04)  /* enable periodic */
> > > +#define        HPET_DPI        _IO('h', 0x05)  /* disable periodic */
> > > +#define        HPET_IRQFREQ    _IOW('h', 0x6, unsigned long)   /* IRQFREQ usec */
> > 
> > By the way, HPET_INFO and HPET_IRQFREQ don't work for 32 bit user space,
> > the driver needs a compat_ioctl() method to handle those.
> 
> There isn't any program (except the example in the docs) that uses any
> of these ioctls, and I'm writing patches to make this device available
> through portable timer APIs like hrtimer/POSIX clocks/ALSA that are much
> easier to use besides, so I think it would be a good idea to just
> schedule these ioctls for removal.

How do you (or can you) know that there are no programs that use
that ioctl?  Yes, scheduling for removal would be OK.

---
~Randy

Re: [PATCH] hpet header sanitization

[Randy.Dunlap]

On Wed, 22 Mar 2006 10:02:19 +0100 Arjan van de Ven wrote:

> On Tue, 2006-03-21 at 16:26 -0800, Randy.Dunlap wrote:
> > On Tue, 21 Mar 2006 16:13:03 -0800 Andrew Morton wrote:
> > 
> > > "Randy.Dunlap" <rdunlap@xenotime.net> wrote:
> > > >
> > > > From: Randy Dunlap <rdunlap@xenotime.net>
> > > > 
> > > > Add __KERNEL__ block.
> > > > Use __KERNEL__ to allow ioctl interface to be usable.
> > > 
> > > hm, why?
> > 
> > because there is a test/example source file in (inside)
> > Documentation/hpet.txt that won't build otherwise.
> > And because hpet.h contains _userspace_ ioctl interface struct
> > and macros...
> 
> 
> then please split the header in 2 parts; one for the kernel
> and one for userspace

so would you tell me what the purpose (use) of __KERNEL__
is meant to be, please?

Fortunately there are only about 165 header files in include/
that use both __KERNEL__ and _IO() macros (out of 5425 header
files).


> either put both here, or move the kernel one to the directory where the
> source code is


---
~Randy

Re: [PATCH] hpet header sanitization

[Arjan van de Ven]

On Wed, 2006-03-22 at 09:26 -0800, Randy.Dunlap wrote:
> On Wed, 22 Mar 2006 10:02:19 +0100 Arjan van de Ven wrote:
> 
> > On Tue, 2006-03-21 at 16:26 -0800, Randy.Dunlap wrote:
> > > On Tue, 21 Mar 2006 16:13:03 -0800 Andrew Morton wrote:
> > > 
> > > > "Randy.Dunlap" <rdunlap@xenotime.net> wrote:
> > > > >
> > > > > From: Randy Dunlap <rdunlap@xenotime.net>
> > > > > 
> > > > > Add __KERNEL__ block.
> > > > > Use __KERNEL__ to allow ioctl interface to be usable.
> > > > 
> > > > hm, why?
> > > 
> > > because there is a test/example source file in (inside)
> > > Documentation/hpet.txt that won't build otherwise.
> > > And because hpet.h contains _userspace_ ioctl interface struct
> > > and macros...
> > 
> > 
> > then please split the header in 2 parts; one for the kernel
> > and one for userspace
> 
> so would you tell me what the purpose (use) of __KERNEL__
> is meant to be, please?

for legacy headers.. the same ;)
Thats no reason to fix up new cases... things should get better not just
get a small rubber bandaid...

Re: [PATCH] hpet header sanitization

[Randy.Dunlap]

On Wed, 22 Mar 2006 21:08:02 +0100 Arjan van de Ven wrote:

> On Wed, 2006-03-22 at 09:26 -0800, Randy.Dunlap wrote:
> > On Wed, 22 Mar 2006 10:02:19 +0100 Arjan van de Ven wrote:
> > 
> > > On Tue, 2006-03-21 at 16:26 -0800, Randy.Dunlap wrote:
> > > > On Tue, 21 Mar 2006 16:13:03 -0800 Andrew Morton wrote:
> > > > 
> > > > > "Randy.Dunlap" <rdunlap@xenotime.net> wrote:
> > > > > >
> > > > > > From: Randy Dunlap <rdunlap@xenotime.net>
> > > > > > 
> > > > > > Add __KERNEL__ block.
> > > > > > Use __KERNEL__ to allow ioctl interface to be usable.
> > > > > 
> > > > > hm, why?
> > > > 
> > > > because there is a test/example source file in (inside)
> > > > Documentation/hpet.txt that won't build otherwise.
> > > > And because hpet.h contains _userspace_ ioctl interface struct
> > > > and macros...
> > > 
> > > 
> > > then please split the header in 2 parts; one for the kernel
> > > and one for userspace
> > 
> > so would you tell me what the purpose (use) of __KERNEL__
> > is meant to be, please?
> 
> for legacy headers.. the same ;)

"the same" as what?  I'm not understanding these partial sentences.

> Thats no reason to fix up new cases... things should get better not just
> get a small rubber bandaid...

Yes, I'm not disagreeing with the no-bandaids part.  I just haven't
read that __KERNEL__ is legacy only.


---
~Randy

Re: [PATCH] hpet header sanitization

[Arnd Bergmann]

On Wednesday 22 March 2006 12:14, Clemens Ladisch wrote:
> There isn't any program (except the example in the docs) that uses any
> of these ioctls, and I'm writing patches to make this device available
> through portable timer APIs like hrtimer/POSIX clocks/ALSA that are much
> easier to use besides, so I think it would be a good idea to just
> schedule these ioctls for removal.

Ok, in that case I guess all of the header file should be wrapped inside
#ifdef __KERNEL__. Until now it was not possible to include that header
file in order to get the ioctl definition. It would be somewhat
counterproductive to schedule the user interface for removal while at
the same time making it easier to use it.

Also, I don't see any user of the ioctl function in the kernel, although
it's exported. Are there any out-of-tree users?

	Arnd <><

Re: [PATCH] hpet header sanitization

[Arjan van de Ven]

On Wed, 2006-03-22 at 23:45 +0100, Arnd Bergmann wrote:
> On Wednesday 22 March 2006 12:14, Clemens Ladisch wrote:
> > There isn't any program (except the example in the docs) that uses any
> > of these ioctls, and I'm writing patches to make this device available
> > through portable timer APIs like hrtimer/POSIX clocks/ALSA that are much
> > easier to use besides, so I think it would be a good idea to just
> > schedule these ioctls for removal.
> 
> Ok, in that case I guess all of the header file should be wrapped inside
> #ifdef __KERNEL__. Until now it was not possible to include that header
> file in order to get the ioctl definition. It would be somewhat
> counterproductive to schedule the user interface for removal while at
> the same time making it easier to use it.
> 
> Also, I don't see any user of the ioctl function in the kernel, although
> it's exported. Are there any out-of-tree users?


hmm there used to be (in s390 iirc ;) but now it's a good opportunity
for Adrian to do a cleanup patch for it 

Re: [PATCH] hpet header sanitization

[Clemens Ladisch]

Randy.Dunlap wrote:
> On Wed, 22 Mar 2006 12:14:46 +0100 Clemens Ladisch wrote:
> > There isn't any program (except the example in the docs) that
> > uses any of these ioctls, and I'm writing patches to make this
> > device available through portable timer APIs like hrtimer/POSIX
> > clocks/ALSA that are much easier to use besides, so I think it
> > would be a good idea to just schedule these ioctls for removal.
>
> How do you (or can you) know that there are no programs that use
> that ioctl?

Because most parts of hpet.c were orignially written on an emulator
and were so buggy as to be effectively unusable until recently.

Additionally, there aren't many BIOSes out there that manage to
initialize the third HPET timer (the one used by these ioctls)
correctly, or that bother to enable the HPET device at all.  (This
seems to be changing due to the Windows Vista logo requirements.)


Regards,
Clemens