Re: [PATCH] synclink_gt add compat_ioctl
Arnd Bergmann wrote: To solve this, you can to change include/linux/Kbuild to list synclink.h as unifdef-y instead of header-y, and put the parts that you don't want to be in user space inside of #ifdef __KERNEL__. Alternatively, you can put these kernel-internal definitions into a private header file in drivers/char that does not get installed in the first place. That would be particularly useful if you can also move other parts of linux/synclink.h into the private header, when they are not part of the external ABI. Understood. That is the last piece to the puzzle. I think the first approach would be better as synclink.h is all interface definitions. The kernel specific parts are in the individual synclink drivers (such as register definitions, etc). The only exception to this is now the ioctl32 structures and I would hate to break out a whole new header just for that. Thanks again for your targeted and informative help. (Thanks to Andrew for your patience in dealing with a patch that never should have been submitted.) -- Paul - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
On Tuesday 08 May 2007, Paul Fulghum wrote: > make[3]: *** No rule to make target > `/usr/src/devel/usr/include/linux/.check.synclink.h', needed by > `__headerscheck'. Stop. > > linux/kexec.h includes linux/compat.h without a similar error, > though that is inside of a #ifdef CONFIG_KEXEC > > Moving linux/compat.h from synclink.h to synclink_gt.c > removes the error. > > This is the last error standing in my way and I'm trying > to figure out the rules for when and where you are allowed > to use compat.h, I'm not familiar with the headerscheck > facility so I'm not sure what it is looking for and the > error is not very helpful. There is nothing in Documentation > covering it. The warning is about the situation that linux/synclink.h gets installed by make headers_install, but linux/compat.h does not get installed, so any user program including linux/synclink.h will fail to build. To solve this, you can to change include/linux/Kbuild to list synclink.h as unifdef-y instead of header-y, and put the parts that you don't want to be in user space inside of #ifdef __KERNEL__. Alternatively, you can put these kernel-internal definitions into a private header file in drivers/char that does not get installed in the first place. That would be particularly useful if you can also move other parts of linux/synclink.h into the private header, when they are not part of the external ABI. Arnd <>< - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
On Sun, 2007-05-06 at 02:27 +0200, Arnd Bergmann wrote: > Now that you mention the duplication, this sounds wrong as well. The easiest > solution is probably to just put the definition of your data structure > inside of #ifdef CONFIG_COMPAT in the header file. The structure definition was already inside of #ifdef CONFIG_COMPAT The problem is including linux/compat.h inside of synclink.h causes an error on i386. The headerscheck facility is spitting out an error even if the #include is inside of #ifdef CONFIG_COMPAT make[3]: *** No rule to make target `/usr/src/devel/usr/include/linux/.check.synclink.h', needed by `__headerscheck'. Stop. linux/kexec.h includes linux/compat.h without a similar error, though that is inside of a #ifdef CONFIG_KEXEC Moving linux/compat.h from synclink.h to synclink_gt.c removes the error. This is the last error standing in my way and I'm trying to figure out the rules for when and where you are allowed to use compat.h, I'm not familiar with the headerscheck facility so I'm not sure what it is looking for and the error is not very helpful. There is nothing in Documentation covering it. -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
On Saturday 05 May 2007, Paul Fulghum wrote:
> That declaration will need to be duplicated in each driver that
> uses it (4 drivers in my case). In that sense (a structure declaration
> used by multiple code modules) it does seem like an interface definition.
>
> If that is what is needed, I will do it.
Now that you mention the duplication, this sounds wrong as well. The easiest
solution is probably to just put the definition of your data structure
inside of #ifdef CONFIG_COMPAT in the header file.
Or you could go really fancy and write a new file that does the synclink
compat_ioctl handling in a generic way end in the end just calls the
fops->{unlocked_,}ioctl() function.
Which reminds me that I have been meaning to do a patch that creates
a new generic_compat_ioctl() [1] function for some time, and convert
drivers to this if their handlers are all compatible.
Arnd <><
[1]
/*
* Can be used as the ->compat_ioctl method in the file_operations
* for any driver that does not need any conversion in its ioctl
* handler
*/
long generic_file_compat_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
int ret;
arg = (unsigned long)compat_ptr(arg);
if (file->f_ops->unlocked_ioctl)
ret = file->f_ops->unlocked_ioctl(file, cmd, arg);
else {
lock_kernel();
ret = file->f_ops->ioctl(file, cmd, arg);
unlock_kernel();
} else
ret = -ENOIOCTLCMD;
return ret;
}
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
On Friday 04 May 2007, Paul Fulghum wrote:
>
> > - It is fishy that apart from one outlier in kexec.h, synclink.h is the
> > only header file which uses compat_ulong_t. Are we doing this right?
>
> Arnd, do you have any comment on this?
I think most others just define the compat data structures in the
same file that implements the headers, inside the same #ifdef that
hides the functions using them.
This makes sense, because the data structures here don't define
an interface, but rather describe what the interface looks like
in the 32 bit case.
> It seems like the compatible types should be available
> in something that is already commonly used like linux/types.h
>
> I'm fine with it either way. I'm not in
> a position to be making those kinds of decisions
> for widely used infrastructure, so I'll leave
> that for someone further up the food chain.
All common compat_* data structures are defined in
include/{linux,asm}/compat.h, which are empty if CONFIG_COMPAT=n.
It's against our normal conventions to hide declarations
inside an #ifdef, but I can see that it does keep the code
size down to make it impossible to compile code that is used
for compat on architectures that don't need it.
Arnd <><
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
Arnd Bergmann wrote: On Friday 04 May 2007, Paul Fulghum wrote: - It is fishy that apart from one outlier in kexec.h, synclink.h is the only header file which uses compat_ulong_t. Are we doing this right? Arnd, do you have any comment on this? I think most others just define the compat data structures in the same file that implements the headers, inside the same #ifdef that hides the functions using them. This makes sense, because the data structures here don't define an interface, but rather describe what the interface looks like in the 32 bit case. OK, moving the compatible structure declarations from the header to the individual source files will fix all the header mess and the odd compilation errors on i386 when using the compat.h header inside of another header. That declaration will need to be duplicated in each driver that uses it (4 drivers in my case). In that sense (a structure declaration used by multiple code modules) it does seem like an interface definition. If that is what is needed, I will do it. -- Paul - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
Andrew Morton wrote: On Thu, 03 May 2007 13:01:17 -0500 Paul Fulghum <[EMAIL PROTECTED]> wrote: Add compat_ioctl handler to synclink_gt driver. i386 allmodconfig: make[3]: *** No rule to make target `/usr/src/devel/usr/include/linux/.check.synclink.h', needed by `__headerscheck'. Stop. I got tired of this patch - I think I'll drop it. This all seems to be related to the use of compat_ulong_t Since my original patch worked fine using unsigned int, how about I go back to that? -- Paul - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
On Thu, 03 May 2007 13:01:17 -0500 Paul Fulghum <[EMAIL PROTECTED]> wrote: > Add compat_ioctl handler to synclink_gt driver. i386 allmodconfig: make[3]: *** No rule to make target `/usr/src/devel/usr/include/linux/.check.synclink.h', needed by `__headerscheck'. Stop. I got tired of this patch - I think I'll drop it. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
Andrew Morton wrote: In file included from drivers/char/synclink_gt.c:85: include/linux/synclink.h:175: error: expected specifier-qualifier-list before 'compat_ulong_t' - We might as well do the same ifdef-avoidery trick around compat_ioctl() too. That required that it be renamed. - It is fishy that apart from one outlier in kexec.h, synclink.h is the only header file which uses compat_ulong_t. Are we doing this right? Arnd, do you have any comment on this? It seems like the compatible types should be available in something that is already commonly used like linux/types.h I'm fine with it either way. I'm not in a position to be making those kinds of decisions for widely used infrastructure, so I'll leave that for someone further up the food chain. -- Paul Fulghum Microgate Systems, Ltd. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
On Thu, 03 May 2007 13:01:17 -0500
Paul Fulghum <[EMAIL PROTECTED]> wrote:
> Add compat_ioctl handler to synclink_gt driver.
>
> The one case requiring a separate 32 bit handler could be
> removed by redefining the associated structure in
> a way compatible with both 32 and 64 bit systems. But that
> approach would break existing native 64 bit user applications.
A made a few changes here...
From: Andrew Morton <[EMAIL PROTECTED]>
- Fix i386 build:
In file included from drivers/char/synclink_gt.c:85:
include/linux/synclink.h:175: error: expected specifier-qualifier-list before
'compat_ulong_t'
- We might as well do the same ifdef-avoidery trick around compat_ioctl()
too. That required that it be renamed.
- It is fishy that apart from one outlier in kexec.h, synclink.h is the
only header file which uses compat_ulong_t. Are we doing this right?
Cc: Alan Cox <[EMAIL PROTECTED]>
Cc: Arnd Bergmann <[EMAIL PROTECTED]>
Cc: Paul Fulghum <[EMAIL PROTECTED]>
Signed-off-by: Andrew Morton <[EMAIL PROTECTED]>
---
drivers/char/synclink_gt.c | 16 +---
include/linux/synclink.h |5 +++--
2 files changed, 12 insertions(+), 9 deletions(-)
diff -puN drivers/char/synclink_gt.c~synclink_gt-add-compat_ioctl-fix
drivers/char/synclink_gt.c
--- a/drivers/char/synclink_gt.c~synclink_gt-add-compat_ioctl-fix
+++ a/drivers/char/synclink_gt.c
@@ -1176,15 +1176,16 @@ static int ioctl(struct tty_struct *tty,
}
#ifdef CONFIG_COMPAT
-static long compat_ioctl(struct tty_struct *tty, struct file *file,
+static long synclink_compat_ioctl(struct tty_struct *tty, struct file *file,
unsigned int cmd, unsigned long arg)
{
struct slgt_info *info = tty->driver_data;
int rc = -ENOIOCTLCMD;
- if (sanity_check(info, tty->name, "compat_ioctl"))
+ if (sanity_check(info, tty->name, "synclink_compat_ioctl"))
return -ENODEV;
- DBGINFO(("%s compat_ioctl() cmd=%08X\n", info->device_name, cmd));
+ DBGINFO(("%s synclink_compat_ioctl() cmd=%08X\n",
+ info->device_name, cmd));
switch (cmd) {
@@ -1219,9 +1220,12 @@ static long compat_ioctl(struct tty_stru
break;
}
- DBGINFO(("%s compat_ioctl() cmd=%08X rc=%d\n", info->device_name, cmd,
rc));
+ DBGINFO(("%s synclink_compat_ioctl() cmd=%08X rc=%d\n",
+ info->device_name, cmd, rc));
return rc;
}
+#else
+#define synclink_compat_ioctl NULL
#endif
/*
@@ -3554,9 +3558,7 @@ static const struct tty_operations ops =
.chars_in_buffer = chars_in_buffer,
.flush_buffer = flush_buffer,
.ioctl = ioctl,
-#ifdef CONFIG_COMPAT
- .compat_ioctl = compat_ioctl,
-#endif
+ .compat_ioctl = synclink_compat_ioctl,
.throttle = throttle,
.unthrottle = unthrottle,
.send_xchar = send_xchar,
diff -puN include/linux/synclink.h~synclink_gt-add-compat_ioctl-fix
include/linux/synclink.h
--- a/include/linux/synclink.h~synclink_gt-add-compat_ioctl-fix
+++ a/include/linux/synclink.h
@@ -169,9 +169,9 @@ typedef struct _MGSL_PARAMS
} MGSL_PARAMS, *PMGSL_PARAMS;
+#ifdef CONFIG_COMPAT
/* provide 32 bit ioctl compatibility on 64 bit systems */
-struct MGSL_PARAMS32
-{
+struct MGSL_PARAMS32 {
compat_ulong_t mode;
unsigned char loopback;
unsigned short flags;
@@ -186,6 +186,7 @@ struct MGSL_PARAMS32
unsigned char stop_bits;
unsigned char parity;
};
+#endif
#define MICROGATE_VENDOR_ID 0x13c0
#define SYNCLINK_DEVICE_ID 0x0010
_
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
Arnd Bergmann wrote: The same function contains a copy_from_user(), which cannot be called with interrupts disabled, so yes, I am very certain it will not change. Good point. -- Paul - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
On Thursday 03 May 2007, Paul Fulghum wrote:
> >> +
> >> +spin_lock_irqsave(&info->lock, flags);
> >
> > no need for _irqsave, just use spin_{,un}lock_irq() when you know that
> > interrupts are enabled.
>
> That makes me a little uneasy. The locking
> mechanisms (and just about everything else) above the driver
> seem to change frequently. This involves not just the VFS but
> the tty core as well.
>
> If you are confident this will not change, I will
> switch to spin_lock(). I used spin_lock_irqsave() to be
> more robust against changes to behavior outside my driver.
The same function contains a copy_from_user(), which cannot
be called with interrupts disabled, so yes, I am very certain
it will not change.
Arnd <><
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
Arnd Bergmann wrote:
+ case MGSL_IOCGPARAMS32:
+ rc = get_params32(info, (struct MGSL_PARAMS32 __user
*)compat_ptr(arg));
+ break;
No need for the cast here.
OK, for some reason I remember getting a warning
error without it. I must have been mistaken.
+
+ spin_lock_irqsave(&info->lock, flags);
no need for _irqsave, just use spin_{,un}lock_irq() when you know that
interrupts are enabled.
That makes me a little uneasy. The locking
mechanisms (and just about everything else) above the driver
seem to change frequently. This involves not just the VFS but
the tty core as well.
If you are confident this will not change, I will
switch to spin_lock(). I used spin_lock_irqsave() to be
more robust against changes to behavior outside my driver.
+/* provide 32 bit ioctl compatibility on 64 bit systems */
+struct MGSL_PARAMS32
+{
+ unsigned intmode;
+ unsigned char loopback;
+ unsigned short flags;
+ unsigned char encoding;
+ unsigned intclock_speed;
+ unsigned char addr_filter;
+ unsigned short crc_type;
+ unsigned char preamble_length;
+ unsigned char preamble;
+ unsigned intdata_rate;
+ unsigned char data_bits;
+ unsigned char stop_bits;
+ unsigned char parity;
+};
The definition is correct, but by convention it would be better to use
compat_ulong_t instead of unsigned int for those fields that are an
unsigned long in native mode.
OK
--
Paul
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
On Wednesday 02 May 2007, Paul Fulghum wrote:
> + switch (cmd) {
> +
> + case MGSL_IOCSPARAMS32:
> + rc = set_params32(info, (struct MGSL_PARAMS32 __user
> *)compat_ptr(arg));
> + break;
> +
> + case MGSL_IOCGPARAMS32:
> + rc = get_params32(info, (struct MGSL_PARAMS32 __user
> *)compat_ptr(arg));
> + break;
No need for the cast here.
> +
> + spin_lock_irqsave(&info->lock, flags);
no need for _irqsave, just use spin_{,un}lock_irq() when you know that
interrupts are enabled.
> --- a/include/linux/synclink.h2007-04-25 22:08:32.0 -0500
> +++ b/include/linux/synclink.h2007-05-02 14:59:17.0 -0500
> @@ -167,6 +167,24 @@ typedef struct _MGSL_PARAMS
>
> } MGSL_PARAMS, *PMGSL_PARAMS;
>
> +/* provide 32 bit ioctl compatibility on 64 bit systems */
> +struct MGSL_PARAMS32
> +{
> + unsigned intmode;
> + unsigned char loopback;
> + unsigned short flags;
> + unsigned char encoding;
> + unsigned intclock_speed;
> + unsigned char addr_filter;
> + unsigned short crc_type;
> + unsigned char preamble_length;
> + unsigned char preamble;
> + unsigned intdata_rate;
> + unsigned char data_bits;
> + unsigned char stop_bits;
> + unsigned char parity;
> +};
The definition is correct, but by convention it would be better to use
compat_ulong_t instead of unsigned int for those fields that are an
unsigned long in native mode.
Arnd <><
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
