Re: [PATCH] tty add compat_ioctl
Arnd Bergmann wrote: - The return value of the new compat_ioctl methods should probably 'int', not 'long'. We've had the discussion before and then decided not to change the existing compat_ioctl and unlocked_ioctl functions -- even though int is more appropriate, but having the same prototype has the advantage that a driver can use the same function for both ->ioctl and ->compat_ioctl if all calls are compatible. Any comments on this or should I assume that compat_ioctl() will stay with ulong return type? -- 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] tty add compat_ioctl
Arnd Bergmann wrote: - The return value of the new compat_ioctl methods should probably 'int', not 'long'. We've had the discussion before and then decided not to change the existing compat_ioctl and unlocked_ioctl functions -- even though int is more appropriate, but having the same prototype has the advantage that a driver can use the same function for both -ioctl and -compat_ioctl if all calls are compatible. Any comments on this or should I assume that compat_ioctl() will stay with ulong return type? -- 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] tty add compat_ioctl
Paul Fulghum wrote: Arnd Bergmann wrote: - In your driver you don't get the big kernel lock in the compat_ioctl function. I assume that this is correct for the particular driver, but it may be nice if you could consequently also add an unlocked_ioctl function that can be used without the BKL for native ioctls. It would be good to hear an opinon on this from someone who has an insight in tty locking issues though, so I'm Cc:ing some people who have touched that recently. I don't count on higher level locking for synchronization issues specific to the driver. I thought the current compat_ioctl() was already meant to *not* have the BKL just like unlocked_ioctl. My thought was that any driver getting a recent update like compat_ioctl() would need to be reviewed for BKL safety and take the lock manually if necessary. Nevermind. I misread what you wrote (I'm tired). Yes, adding an unlocked_ioctl() makes sense. -- 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] tty add compat_ioctl
Arnd Bergmann wrote: - The return value of the new compat_ioctl methods should probably 'int', not 'long'. We've had the discussion before and then decided not to change the existing compat_ioctl and unlocked_ioctl functions -- even though int is more appropriate, but having the same prototype has the advantage that a driver can use the same function for both ->ioctl and ->compat_ioctl if all calls are compatible. I noticed that but thought the change in return value type had some higher purpose I had not perceived. If it can be int that would be the way to go. - In your driver you don't get the big kernel lock in the compat_ioctl function. I assume that this is correct for the particular driver, but it may be nice if you could consequently also add an unlocked_ioctl function that can be used without the BKL for native ioctls. It would be good to hear an opinon on this from someone who has an insight in tty locking issues though, so I'm Cc:ing some people who have touched that recently. I don't count on higher level locking for synchronization issues specific to the driver. I thought the current compat_ioctl() was already meant to *not* have the BKL just like unlocked_ioctl. My thought was that any driver getting a recent update like compat_ioctl() would need to be reviewed for BKL safety and take the lock manually if necessary. Drivers that are falling behind wont have a compat_ioctl defined at all. -- 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] tty add compat_ioctl
Arnd Bergmann wrote: - The return value of the new compat_ioctl methods should probably 'int', not 'long'. We've had the discussion before and then decided not to change the existing compat_ioctl and unlocked_ioctl functions -- even though int is more appropriate, but having the same prototype has the advantage that a driver can use the same function for both -ioctl and -compat_ioctl if all calls are compatible. I noticed that but thought the change in return value type had some higher purpose I had not perceived. If it can be int that would be the way to go. - In your driver you don't get the big kernel lock in the compat_ioctl function. I assume that this is correct for the particular driver, but it may be nice if you could consequently also add an unlocked_ioctl function that can be used without the BKL for native ioctls. It would be good to hear an opinon on this from someone who has an insight in tty locking issues though, so I'm Cc:ing some people who have touched that recently. I don't count on higher level locking for synchronization issues specific to the driver. I thought the current compat_ioctl() was already meant to *not* have the BKL just like unlocked_ioctl. My thought was that any driver getting a recent update like compat_ioctl() would need to be reviewed for BKL safety and take the lock manually if necessary. Drivers that are falling behind wont have a compat_ioctl defined at all. -- 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] tty add compat_ioctl
Paul Fulghum wrote: Arnd Bergmann wrote: - In your driver you don't get the big kernel lock in the compat_ioctl function. I assume that this is correct for the particular driver, but it may be nice if you could consequently also add an unlocked_ioctl function that can be used without the BKL for native ioctls. It would be good to hear an opinon on this from someone who has an insight in tty locking issues though, so I'm Cc:ing some people who have touched that recently. I don't count on higher level locking for synchronization issues specific to the driver. I thought the current compat_ioctl() was already meant to *not* have the BKL just like unlocked_ioctl. My thought was that any driver getting a recent update like compat_ioctl() would need to be reviewed for BKL safety and take the lock manually if necessary. Nevermind. I misread what you wrote (I'm tired). Yes, adding an unlocked_ioctl() makes sense. -- 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] tty add compat_ioctl method
On Thursday 03 May 2007, Paul Fulghum wrote: > > > declarations should never be hidden inside of an #ifdef. If you want to be > > extra clever here, you can do > > OK, I have no problem with that. > A declaration without implementation won't generate a warning? You only get a warning for static declarations without an implementation, not for globals. 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] tty add compat_ioctl method
Arnd Bergmann wrote: Looks ok mostly. Just some details: ... +#ifdef CONFIG_COMPAT + long (*compat_ioctl)(struct tty_struct *tty, struct file * file, +unsigned int cmd, unsigned long arg); +#endif I wouldn't hide this inside of an #ifdef. The structures are all static and therefore a single field per driver doesn't add much bload, but being able to always assign the .compat_ioctl pointer makes the code somewhat nicer OK --- a/drivers/char/tty_io.c 2006-11-29 15:57:37.0 -0600 +++ b/drivers/char/tty_io.c 2007-04-30 14:51:01.0 -0500 @@ -151,6 +151,9 @@ static int tty_open(struct inode *, stru static int tty_release(struct inode *, struct file *); int tty_ioctl(struct inode * inode, struct file * file, unsigned int cmd, unsigned long arg); +#ifdef CONFIG_COMPAT +long tty_compat_ioctl(struct file * file, unsigned int cmd, unsigned long arg); +#endif static int tty_fasync(int fd, struct file * filp, int on); static void release_mem(struct tty_struct *tty, int idx); declarations should never be hidden inside of an #ifdef. If you want to be extra clever here, you can do OK, I have no problem with that. A declaration without implementation won't generate a warning? #ifdef CONFIG_COMPAT long tty_compat_ioctl(struct file * file, unsigned int cmd, unsigned long arg); #else #define tty_compat_ioctl NULL #endif then you don't need an #ifdef in the code setting the function pointers. OK + tty = (struct tty_struct *)file->private_data; no need for the cast, ->private_data is void*. Yes, an easy fix. -- 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] tty add compat_ioctl method
On Wednesday 02 May 2007, Paul Fulghum wrote:
> Add compat_ioctl method for tty code to allow processing
> of 32 bit ioctl calls on 64 bit systems by tty core,
> tty drivers, and line disciplines.
Looks ok mostly. Just some details:
> --- a/include/linux/tty_driver.h 2006-11-29 15:57:37.0 -0600
> +++ b/include/linux/tty_driver.h 2007-04-30 14:05:02.0 -0500
> @@ -132,6 +132,10 @@ struct tty_operations {
> int (*chars_in_buffer)(struct tty_struct *tty);
> int (*ioctl)(struct tty_struct *tty, struct file * file,
> unsigned int cmd, unsigned long arg);
> +#ifdef CONFIG_COMPAT
> + long (*compat_ioctl)(struct tty_struct *tty, struct file * file,
> + unsigned int cmd, unsigned long arg);
> +#endif
> void (*set_termios)(struct tty_struct *tty, struct termios * old);
> void (*throttle)(struct tty_struct * tty);
> void (*unthrottle)(struct tty_struct * tty);
> @@ -193,6 +197,10 @@ struct tty_driver {
> int (*chars_in_buffer)(struct tty_struct *tty);
> int (*ioctl)(struct tty_struct *tty, struct file * file,
> unsigned int cmd, unsigned long arg);
> +#ifdef CONFIG_COMPAT
> + long (*compat_ioctl)(struct tty_struct *tty, struct file * file,
> + unsigned int cmd, unsigned long arg);
> +#endif
> void (*set_termios)(struct tty_struct *tty, struct termios * old);
> void (*throttle)(struct tty_struct * tty);
> void (*unthrottle)(struct tty_struct * tty);
I wouldn't hide this inside of an #ifdef. The structures are all static
and therefore a single field per driver doesn't add much bload, but
being able to always assign the .compat_ioctl pointer makes the code
somewhat nicer
> --- a/drivers/char/tty_io.c 2006-11-29 15:57:37.0 -0600
> +++ b/drivers/char/tty_io.c 2007-04-30 14:51:01.0 -0500
> @@ -151,6 +151,9 @@ static int tty_open(struct inode *, stru
> static int tty_release(struct inode *, struct file *);
> int tty_ioctl(struct inode * inode, struct file * file,
> unsigned int cmd, unsigned long arg);
> +#ifdef CONFIG_COMPAT
> +long tty_compat_ioctl(struct file * file, unsigned int cmd, unsigned long
> arg);
> +#endif
> static int tty_fasync(int fd, struct file * filp, int on);
> static void release_mem(struct tty_struct *tty, int idx);
declarations should never be hidden inside of an #ifdef. If you want to be
extra clever here, you can do
#ifdef CONFIG_COMPAT
long tty_compat_ioctl(struct file * file, unsigned int cmd, unsigned long arg);
#else
#define tty_compat_ioctl NULL
#endif
then you don't need an #ifdef in the code setting the function pointers.
> @@ -3284,6 +3299,32 @@ int tty_ioctl(struct inode * inode, stru
> return retval;
> }
>
> +#ifdef CONFIG_COMPAT
> +long tty_compat_ioctl(struct file * file, unsigned int cmd, unsigned long
> arg)
> +{
> + struct inode *inode = file->f_dentry->d_inode;
> + struct tty_struct *tty;
> + struct tty_ldisc *ld;
> + int retval = -ENOIOCTLCMD;
> +
> + tty = (struct tty_struct *)file->private_data;
no need for the cast, ->private_data is void*.
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] tty add compat_ioctl method
On Wednesday 02 May 2007, Paul Fulghum wrote:
Add compat_ioctl method for tty code to allow processing
of 32 bit ioctl calls on 64 bit systems by tty core,
tty drivers, and line disciplines.
Looks ok mostly. Just some details:
--- a/include/linux/tty_driver.h 2006-11-29 15:57:37.0 -0600
+++ b/include/linux/tty_driver.h 2007-04-30 14:05:02.0 -0500
@@ -132,6 +132,10 @@ struct tty_operations {
int (*chars_in_buffer)(struct tty_struct *tty);
int (*ioctl)(struct tty_struct *tty, struct file * file,
unsigned int cmd, unsigned long arg);
+#ifdef CONFIG_COMPAT
+ long (*compat_ioctl)(struct tty_struct *tty, struct file * file,
+ unsigned int cmd, unsigned long arg);
+#endif
void (*set_termios)(struct tty_struct *tty, struct termios * old);
void (*throttle)(struct tty_struct * tty);
void (*unthrottle)(struct tty_struct * tty);
@@ -193,6 +197,10 @@ struct tty_driver {
int (*chars_in_buffer)(struct tty_struct *tty);
int (*ioctl)(struct tty_struct *tty, struct file * file,
unsigned int cmd, unsigned long arg);
+#ifdef CONFIG_COMPAT
+ long (*compat_ioctl)(struct tty_struct *tty, struct file * file,
+ unsigned int cmd, unsigned long arg);
+#endif
void (*set_termios)(struct tty_struct *tty, struct termios * old);
void (*throttle)(struct tty_struct * tty);
void (*unthrottle)(struct tty_struct * tty);
I wouldn't hide this inside of an #ifdef. The structures are all static
and therefore a single field per driver doesn't add much bload, but
being able to always assign the .compat_ioctl pointer makes the code
somewhat nicer
--- a/drivers/char/tty_io.c 2006-11-29 15:57:37.0 -0600
+++ b/drivers/char/tty_io.c 2007-04-30 14:51:01.0 -0500
@@ -151,6 +151,9 @@ static int tty_open(struct inode *, stru
static int tty_release(struct inode *, struct file *);
int tty_ioctl(struct inode * inode, struct file * file,
unsigned int cmd, unsigned long arg);
+#ifdef CONFIG_COMPAT
+long tty_compat_ioctl(struct file * file, unsigned int cmd, unsigned long
arg);
+#endif
static int tty_fasync(int fd, struct file * filp, int on);
static void release_mem(struct tty_struct *tty, int idx);
declarations should never be hidden inside of an #ifdef. If you want to be
extra clever here, you can do
#ifdef CONFIG_COMPAT
long tty_compat_ioctl(struct file * file, unsigned int cmd, unsigned long arg);
#else
#define tty_compat_ioctl NULL
#endif
then you don't need an #ifdef in the code setting the function pointers.
@@ -3284,6 +3299,32 @@ int tty_ioctl(struct inode * inode, stru
return retval;
}
+#ifdef CONFIG_COMPAT
+long tty_compat_ioctl(struct file * file, unsigned int cmd, unsigned long
arg)
+{
+ struct inode *inode = file-f_dentry-d_inode;
+ struct tty_struct *tty;
+ struct tty_ldisc *ld;
+ int retval = -ENOIOCTLCMD;
+
+ tty = (struct tty_struct *)file-private_data;
no need for the cast, -private_data is void*.
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] tty add compat_ioctl method
Arnd Bergmann wrote: Looks ok mostly. Just some details: ... +#ifdef CONFIG_COMPAT + long (*compat_ioctl)(struct tty_struct *tty, struct file * file, +unsigned int cmd, unsigned long arg); +#endif I wouldn't hide this inside of an #ifdef. The structures are all static and therefore a single field per driver doesn't add much bload, but being able to always assign the .compat_ioctl pointer makes the code somewhat nicer OK --- a/drivers/char/tty_io.c 2006-11-29 15:57:37.0 -0600 +++ b/drivers/char/tty_io.c 2007-04-30 14:51:01.0 -0500 @@ -151,6 +151,9 @@ static int tty_open(struct inode *, stru static int tty_release(struct inode *, struct file *); int tty_ioctl(struct inode * inode, struct file * file, unsigned int cmd, unsigned long arg); +#ifdef CONFIG_COMPAT +long tty_compat_ioctl(struct file * file, unsigned int cmd, unsigned long arg); +#endif static int tty_fasync(int fd, struct file * filp, int on); static void release_mem(struct tty_struct *tty, int idx); declarations should never be hidden inside of an #ifdef. If you want to be extra clever here, you can do OK, I have no problem with that. A declaration without implementation won't generate a warning? #ifdef CONFIG_COMPAT long tty_compat_ioctl(struct file * file, unsigned int cmd, unsigned long arg); #else #define tty_compat_ioctl NULL #endif then you don't need an #ifdef in the code setting the function pointers. OK + tty = (struct tty_struct *)file-private_data; no need for the cast, -private_data is void*. Yes, an easy fix. -- 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] tty add compat_ioctl method
On Thursday 03 May 2007, Paul Fulghum wrote: declarations should never be hidden inside of an #ifdef. If you want to be extra clever here, you can do OK, I have no problem with that. A declaration without implementation won't generate a warning? You only get a warning for static declarations without an implementation, not for globals. 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/
