2009-06-12 10:28:09

by Mike Frysinger

[permalink] [raw]
Subject: [PATCH] tty: fix unused warning when TCGETX is not defined

If TCGETX is not defined, we end up with this warning:
drivers/char/tty_ioctl.c: In function ‘tty_mode_ioctl’:
drivers/char/tty_ioctl.c:950: warning: unused variable ‘ktermx’

Since the variable is only used in one case statement, push it down to
the local case scope.

Signed-off-by: Mike Frysinger <[email protected]>
---
drivers/char/tty_ioctl.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tty_ioctl.c b/drivers/char/tty_ioctl.c
index 8116bb1..b24f6c6 100644
--- a/drivers/char/tty_ioctl.c
+++ b/drivers/char/tty_ioctl.c
@@ -947,7 +947,6 @@ int tty_mode_ioctl(struct tty_struct *tty, struct file *file,
void __user *p = (void __user *)arg;
int ret = 0;
struct ktermios kterm;
- struct termiox ktermx;

if (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
tty->driver->subtype == PTY_TYPE_MASTER)
@@ -1049,7 +1048,8 @@ int tty_mode_ioctl(struct tty_struct *tty, struct file *file,
return ret;
#endif
#ifdef TCGETX
- case TCGETX:
+ case TCGETX: {
+ struct termiox ktermx;
if (real_tty->termiox == NULL)
return -EINVAL;
mutex_lock(&real_tty->termios_mutex);
@@ -1058,6 +1058,7 @@ int tty_mode_ioctl(struct tty_struct *tty, struct file *file,
if (copy_to_user(p, &ktermx, sizeof(struct termiox)))
ret = -EFAULT;
return ret;
+ }
case TCSETX:
return set_termiox(real_tty, p, 0);
case TCSETXW:
--
1.6.3.1


2009-06-12 10:32:31

by Alan

[permalink] [raw]
Subject: Re: [PATCH] tty: fix unused warning when TCGETX is not defined

On Fri, 12 Jun 2009 06:27:48 -0400
Mike Frysinger <[email protected]> wrote:

> If TCGETX is not defined, we end up with this warning:
> drivers/char/tty_ioctl.c: In function ‘tty_mode_ioctl’:
> drivers/char/tty_ioctl.c:950: warning: unused variable ‘ktermx’
>
> Since the variable is only used in one case statement, push it down to
> the local case scope.

If I wasn't so nice I'd just make it and the lack of BOTHER definitions
on platforms error. Really there shouldn't be anyone without the features
defined ;)

Alan

2009-06-12 10:34:57

by Mike Frysinger

[permalink] [raw]
Subject: Re: [PATCH] tty: fix unused warning when TCGETX is not defined

On Fri, Jun 12, 2009 at 06:33, Alan Cox wrote:
> On Fri, 12 Jun 2009 06:27:48 -0400 Mike Frysinger wrote:
>> If TCGETX is not defined, we end up with this warning:
>> drivers/char/tty_ioctl.c: In function ‘tty_mode_ioctl’:
>> drivers/char/tty_ioctl.c:950: warning: unused variable ‘ktermx’
>>
>> Since the variable is only used in one case statement, push it down to
>> the local case scope.
>
> If I wasn't so nice I'd just make it and the lack of BOTHER definitions
> on platforms error. Really there shouldn't be anyone without the features
> defined ;)

if i knew a lick about these extended tty pieces, i'd look at hooking them up

are these really arch specific ?
-mike

2009-06-12 10:37:22

by Alan

[permalink] [raw]
Subject: Re: [PATCH] tty: fix unused warning when TCGETX is not defined

> > If I wasn't so nice I'd just make it and the lack of BOTHER definitions
> > on platforms error. Really there shouldn't be anyone without the features
> > defined ;)
>
> if i knew a lick about these extended tty pieces, i'd look at hooking them up
>
> are these really arch specific ?

The ioctl numbers have to be (although most platforms use the same
values), and the "BOTHER" definition for arbitary baud rates depends on
the format of struct termios - which again varies by architecture. Its
usually the case that CBAUDEX|0 isn't used for anything so we use that
for BOTHER.

Other than the numbering they are not arch specific, so just pick the
constants for the platform.

Alan

2009-06-12 10:45:44

by Mike Frysinger

[permalink] [raw]
Subject: Re: [PATCH] tty: fix unused warning when TCGETX is not defined

On Fri, Jun 12, 2009 at 06:38, Alan Cox wrote:
>> > If I wasn't so nice I'd just make it and the lack of BOTHER definitions
>> > on platforms error. Really there shouldn't be anyone without the features
>> > defined ;)
>>
>> if i knew a lick about these extended tty pieces, i'd look at hooking them up
>>
>> are these really arch specific ?
>
> The ioctl numbers have to be (although most platforms use the same
> values), and the "BOTHER" definition for arbitary baud rates depends on
> the format of struct termios - which again varies by architecture. Its
> usually the case that CBAUDEX|0 isn't used for anything so we use that
> for BOTHER.

mmm BOTHER is used to get arbitrary speeds right ? i recall testing
that on Blackfin already so i'm pretty sure that works ...

> Other than the numbering they are not arch specific, so just pick the
> constants for the platform.

the guys who did the original Blackfin arch port simply copied the x86
termios stuff (which actually kind of sucks because it means they
copied the termios2 wart)

wonder if people would get annoyed if i changed the Blackfin headers
to do #include <asm/../../../x86/include/asm/foo.h> ;)

doing a diff between x86 and Blackfin headers shows that
termbits/termios are exact copies (ignoring the #ifdef header
protection) and that the Blackfin ioctls.h is missing:
TIOCGRS485
TIOCSRS485
TCGETX
TCSETX
TCSETXF
TCSETXW
TIOCGHAYESESP
TIOCSHAYESESP
-mike

2009-06-12 10:48:13

by Arnd Bergmann

[permalink] [raw]
Subject: [PATCH] blackfin: update ioctls.h

The blackfin version of ioctls.h is a copy of the x86 version, just missing
some fixes that were not included in all copies. This change brings it back
in line with the latest version. Once my asm-generic tree gets merged, the
file can be replaced with an '#include <asm-generic/ioctls.h>'.

Signed-off-by: Arnd Bergmann <[email protected]>

---
On Friday 12 June 2009, Mike Frysinger wrote:
> On Fri, Jun 12, 2009 at 06:33, Alan Cox wrote:
> > On Fri, 12 Jun 2009 06:27:48 -0400 Mike Frysinger wrote:
> >> If TCGETX is not defined, we end up with this warning:
> >> drivers/char/tty_ioctl.c: In function ‘tty_mode_ioctl’:
> >> drivers/char/tty_ioctl.c:950: warning: unused variable ‘ktermx’
> >>
> >> Since the variable is only used in one case statement, push it down to
> >> the local case scope.
> >
> > If I wasn't so nice I'd just make it and the lack of BOTHER definitions
> > on platforms error. Really there shouldn't be anyone without the features
> > defined ;)
>
> if i knew a lick about these extended tty pieces, i'd look at hooking them up
>
> are these really arch specific ?

diff --git a/arch/blackfin/include/asm/ioctls.h b/arch/blackfin/include/asm/ioctls.h
index 895e317..3bcc071 100644
--- a/arch/blackfin/include/asm/ioctls.h
+++ b/arch/blackfin/include/asm/ioctls.h
@@ -43,7 +43,7 @@
#define TIOCSETD 0x5423
#define TIOCGETD 0x5424
#define TCSBRKP 0x5425 /* Needed for POSIX tcsendbreak() */
-#define TIOCTTYGSTRUCT 0x5426 /* For debugging only */
+/* #define TIOCTTYGSTRUCT 0x5426 For debugging only */
#define TIOCSBRK 0x5427 /* BSD compatibility */
#define TIOCCBRK 0x5428 /* BSD compatibility */
#define TIOCGSID 0x5429 /* Return the session ID of FD */
@@ -51,11 +51,17 @@
#define TCSETS2 _IOW('T', 0x2B, struct termios2)
#define TCSETSW2 _IOW('T', 0x2C, struct termios2)
#define TCSETSF2 _IOW('T', 0x2D, struct termios2)
-/* Get Pty Number (of pty-mux device) */
-#define TIOCGPTN _IOR('T', 0x30, unsigned int)
+#define TIOCGRS485 0x542E
+#define TIOCSRS485 0x542F
+#define TIOCGPTN _IOR('T', 0x30, unsigned int) /* Get Pty Number (of pty-mux device) */
#define TIOCSPTLCK _IOW('T', 0x31, int) /* Lock/unlock Pty */
+#define TCGETX 0x5432 /* SYS5 TCGETX compatibility */
+#define TCSETX 0x5433
+#define TCSETXF 0x5434
+#define TCSETXW 0x5435

-#define FIONCLEX 0x5450 /* these numbers need to be adjusted. */
+
+#define FIONCLEX 0x5450
#define FIOCLEX 0x5451
#define FIOASYNC 0x5452
#define TIOCSERCONFIG 0x5453

2009-06-12 10:53:47

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH] tty: fix unused warning when TCGETX is not defined

On Friday 12 June 2009, Mike Frysinger wrote:
> TIOCGRS485
> TIOCSRS485
> TCGETX
> TCSETX
> TCSETXF
> TCSETXW

These can easily be added, as my patch does.

> TIOCGHAYESESP
> TIOCSHAYESESP

These two are better left out IMHO. TIOCGHAYESESP on half the architectures
has the same number as FIOQSIZE on the other half (including blackfin).
The only driver using it is ESPSERIAL, which is marked as broken and depending
on ISA, so you really don't care.

Arnd <><

2009-06-12 10:57:06

by Mike Frysinger

[permalink] [raw]
Subject: [PATCH] Blackfin: sync termios header changes with x86

The Blackfin port copied the x86 termios interface, so pull updates from
it to get the latest termios ioctls and such.

Signed-off-by: Mike Frysinger <[email protected]>
---
Arnd: if you don't mind, i'd rather merge this ... i can put it into my
next batch of Blackfin changes for 2.6.31

arch/blackfin/include/asm/ioctls.h | 35 ++++++++------
arch/blackfin/include/asm/termbits.h | 54 +++++++++++-----------
arch/blackfin/include/asm/termios.h | 86 +++++++++++++++++++++-------------
3 files changed, 101 insertions(+), 74 deletions(-)

diff --git a/arch/blackfin/include/asm/ioctls.h b/arch/blackfin/include/asm/ioctls.h
index 895e317..e22c400 100644
--- a/arch/blackfin/include/asm/ioctls.h
+++ b/arch/blackfin/include/asm/ioctls.h
@@ -6,7 +6,7 @@
/* 0x54 is just a magic number to make these relatively unique ('T') */

#define TCGETS 0x5401
-#define TCSETS 0x5402
+#define TCSETS 0x5402 /* Clashes with SNDCTL_TMR_START sound ioctl */
#define TCSETSW 0x5403
#define TCSETSF 0x5404
#define TCGETA 0x5405
@@ -43,19 +43,25 @@
#define TIOCSETD 0x5423
#define TIOCGETD 0x5424
#define TCSBRKP 0x5425 /* Needed for POSIX tcsendbreak() */
-#define TIOCTTYGSTRUCT 0x5426 /* For debugging only */
-#define TIOCSBRK 0x5427 /* BSD compatibility */
-#define TIOCCBRK 0x5428 /* BSD compatibility */
-#define TIOCGSID 0x5429 /* Return the session ID of FD */
+/* #define TIOCTTYGSTRUCT 0x5426 - Former debugging-only ioctl */
+#define TIOCSBRK 0x5427 /* BSD compatibility */
+#define TIOCCBRK 0x5428 /* BSD compatibility */
+#define TIOCGSID 0x5429 /* Return the session ID of FD */
#define TCGETS2 _IOR('T', 0x2A, struct termios2)
#define TCSETS2 _IOW('T', 0x2B, struct termios2)
#define TCSETSW2 _IOW('T', 0x2C, struct termios2)
#define TCSETSF2 _IOW('T', 0x2D, struct termios2)
-/* Get Pty Number (of pty-mux device) */
+#define TIOCGRS485 0x542E
+#define TIOCSRS485 0x542F
#define TIOCGPTN _IOR('T', 0x30, unsigned int)
-#define TIOCSPTLCK _IOW('T', 0x31, int) /* Lock/unlock Pty */
+ /* Get Pty Number (of pty-mux device) */
+#define TIOCSPTLCK _IOW('T', 0x31, int) /* Lock/unlock Pty */
+#define TCGETX 0x5432 /* SYS5 TCGETX compatibility */
+#define TCSETX 0x5433
+#define TCSETXF 0x5434
+#define TCSETXW 0x5435

-#define FIONCLEX 0x5450 /* these numbers need to be adjusted. */
+#define FIONCLEX 0x5450
#define FIOCLEX 0x5451
#define FIOASYNC 0x5452
#define TIOCSERCONFIG 0x5453
@@ -63,15 +69,16 @@
#define TIOCSERSWILD 0x5455
#define TIOCGLCKTRMIOS 0x5456
#define TIOCSLCKTRMIOS 0x5457
-#define TIOCSERGSTRUCT 0x5458 /* For debugging only */
-#define TIOCSERGETLSR 0x5459 /* Get line status register */
-#define TIOCSERGETMULTI 0x545A /* Get multiport config */
-#define TIOCSERSETMULTI 0x545B /* Set multiport config */
+#define TIOCSERGSTRUCT 0x5458 /* For debugging only */
+#define TIOCSERGETLSR 0x5459 /* Get line status register */
+#define TIOCSERGETMULTI 0x545A /* Get multiport config */
+#define TIOCSERSETMULTI 0x545B /* Set multiport config */

#define TIOCMIWAIT 0x545C /* wait for a change on serial input line(s) */
#define TIOCGICOUNT 0x545D /* read serial port inline interrupt counts */
-
-#define FIOQSIZE 0x545E
+#define TIOCGHAYESESP 0x545E /* Get Hayes ESP configuration */
+#define TIOCSHAYESESP 0x545F /* Set Hayes ESP configuration */
+#define FIOQSIZE 0x5460

/* Used for packet mode */
#define TIOCPKT_DATA 0
diff --git a/arch/blackfin/include/asm/termbits.h b/arch/blackfin/include/asm/termbits.h
index f37feb7..faa569f 100644
--- a/arch/blackfin/include/asm/termbits.h
+++ b/arch/blackfin/include/asm/termbits.h
@@ -3,40 +3,40 @@

#include <linux/posix_types.h>

-typedef unsigned char cc_t;
-typedef unsigned int speed_t;
-typedef unsigned int tcflag_t;
+typedef unsigned char cc_t;
+typedef unsigned int speed_t;
+typedef unsigned int tcflag_t;

#define NCCS 19
struct termios {
- tcflag_t c_iflag; /* input mode flags */
- tcflag_t c_oflag; /* output mode flags */
- tcflag_t c_cflag; /* control mode flags */
- tcflag_t c_lflag; /* local mode flags */
- cc_t c_line; /* line discipline */
- cc_t c_cc[NCCS]; /* control characters */
+ tcflag_t c_iflag; /* input mode flags */
+ tcflag_t c_oflag; /* output mode flags */
+ tcflag_t c_cflag; /* control mode flags */
+ tcflag_t c_lflag; /* local mode flags */
+ cc_t c_line; /* line discipline */
+ cc_t c_cc[NCCS]; /* control characters */
};

struct termios2 {
- tcflag_t c_iflag; /* input mode flags */
- tcflag_t c_oflag; /* output mode flags */
- tcflag_t c_cflag; /* control mode flags */
- tcflag_t c_lflag; /* local mode flags */
- cc_t c_line; /* line discipline */
- cc_t c_cc[NCCS]; /* control characters */
- speed_t c_ispeed; /* input speed */
- speed_t c_ospeed; /* output speed */
+ tcflag_t c_iflag; /* input mode flags */
+ tcflag_t c_oflag; /* output mode flags */
+ tcflag_t c_cflag; /* control mode flags */
+ tcflag_t c_lflag; /* local mode flags */
+ cc_t c_line; /* line discipline */
+ cc_t c_cc[NCCS]; /* control characters */
+ speed_t c_ispeed; /* input speed */
+ speed_t c_ospeed; /* output speed */
};

struct ktermios {
- tcflag_t c_iflag; /* input mode flags */
- tcflag_t c_oflag; /* output mode flags */
- tcflag_t c_cflag; /* control mode flags */
- tcflag_t c_lflag; /* local mode flags */
- cc_t c_line; /* line discipline */
- cc_t c_cc[NCCS]; /* control characters */
- speed_t c_ispeed; /* input speed */
- speed_t c_ospeed; /* output speed */
+ tcflag_t c_iflag; /* input mode flags */
+ tcflag_t c_oflag; /* output mode flags */
+ tcflag_t c_cflag; /* control mode flags */
+ tcflag_t c_lflag; /* local mode flags */
+ cc_t c_line; /* line discipline */
+ cc_t c_cc[NCCS]; /* control characters */
+ speed_t c_ispeed; /* input speed */
+ speed_t c_ospeed; /* output speed */
};

/* c_cc characters */
@@ -140,7 +140,7 @@ struct ktermios {
#define HUPCL 0002000
#define CLOCAL 0004000
#define CBAUDEX 0010000
-#define BOTHER 0010000
+#define BOTHER 0010000 /* non standard rate */
#define B57600 0010001
#define B115200 0010002
#define B230400 0010003
@@ -160,7 +160,7 @@ struct ktermios {
#define CMSPAR 010000000000 /* mark or space (stick) parity */
#define CRTSCTS 020000000000 /* flow control */

-#define IBSHIFT 16 /* Shift from CBAUD to CIBAUD */
+#define IBSHIFT 16 /* Shift from CBAUD to CIBAUD */

/* c_lflag bits */
#define ISIG 0000001
diff --git a/arch/blackfin/include/asm/termios.h b/arch/blackfin/include/asm/termios.h
index d50d063..a5f2f13 100644
--- a/arch/blackfin/include/asm/termios.h
+++ b/arch/blackfin/include/asm/termios.h
@@ -13,11 +13,11 @@ struct winsize {

#define NCC 8
struct termio {
- unsigned short c_iflag; /* input mode flags */
- unsigned short c_oflag; /* output mode flags */
- unsigned short c_cflag; /* control mode flags */
- unsigned short c_lflag; /* local mode flags */
- unsigned char c_line; /* line discipline */
+ unsigned short c_iflag; /* input mode flags */
+ unsigned short c_oflag; /* output mode flags */
+ unsigned short c_cflag; /* control mode flags */
+ unsigned short c_lflag; /* local mode flags */
+ unsigned char c_line; /* line discipline */
unsigned char c_cc[NCC]; /* control characters */
};

@@ -41,6 +41,8 @@ struct termio {

#ifdef __KERNEL__

+#include <asm/uaccess.h>
+
/* intr=^C quit=^\ erase=del kill=^U
eof=^D vtime=\0 vmin=\1 sxtc=\0
start=^Q stop=^S susp=^Z eol=\0
@@ -58,37 +60,55 @@ struct termio {
*(unsigned short *) &(termios)->x = __tmp; \
}

-#define user_termio_to_kernel_termios(termios, termio) \
-({ \
- SET_LOW_TERMIOS_BITS(termios, termio, c_iflag); \
- SET_LOW_TERMIOS_BITS(termios, termio, c_oflag); \
- SET_LOW_TERMIOS_BITS(termios, termio, c_cflag); \
- SET_LOW_TERMIOS_BITS(termios, termio, c_lflag); \
- copy_from_user((termios)->c_cc, (termio)->c_cc, NCC); \
-})
+static inline int user_termio_to_kernel_termios(struct ktermios *termios,
+ struct termio __user *termio)
+{
+ SET_LOW_TERMIOS_BITS(termios, termio, c_iflag);
+ SET_LOW_TERMIOS_BITS(termios, termio, c_oflag);
+ SET_LOW_TERMIOS_BITS(termios, termio, c_cflag);
+ SET_LOW_TERMIOS_BITS(termios, termio, c_lflag);
+ get_user(termios->c_line, &termio->c_line);
+ return copy_from_user(termios->c_cc, termio->c_cc, NCC);
+}

/*
* Translate a "termios" structure into a "termio". Ugh.
*/
-#define kernel_termios_to_user_termio(termio, termios) \
-({ \
- put_user((termios)->c_iflag, &(termio)->c_iflag); \
- put_user((termios)->c_oflag, &(termio)->c_oflag); \
- put_user((termios)->c_cflag, &(termio)->c_cflag); \
- put_user((termios)->c_lflag, &(termio)->c_lflag); \
- put_user((termios)->c_line, &(termio)->c_line); \
- copy_to_user((termio)->c_cc, (termios)->c_cc, NCC); \
-})
-
-#define user_termios_to_kernel_termios(k, u) \
- copy_from_user(k, u, sizeof(struct termios2))
-#define kernel_termios_to_user_termios(u, k) \
- copy_to_user(u, k, sizeof(struct termios2))
-#define user_termios_to_kernel_termios_1(k, u) \
- copy_from_user(k, u, sizeof(struct termios))
-#define kernel_termios_to_user_termios_1(u, k) \
- copy_to_user(u, k, sizeof(struct termios))
-
-#endif /* __KERNEL__ */
+static inline int kernel_termios_to_user_termio(struct termio __user *termio,
+ struct ktermios *termios)
+{
+ put_user((termios)->c_iflag, &(termio)->c_iflag);
+ put_user((termios)->c_oflag, &(termio)->c_oflag);
+ put_user((termios)->c_cflag, &(termio)->c_cflag);
+ put_user((termios)->c_lflag, &(termio)->c_lflag);
+ put_user((termios)->c_line, &(termio)->c_line);
+ return copy_to_user((termio)->c_cc, (termios)->c_cc, NCC);
+}
+
+static inline int user_termios_to_kernel_termios(struct ktermios *k,
+ struct termios2 __user *u)
+{
+ return copy_from_user(k, u, sizeof(struct termios2));
+}
+
+static inline int kernel_termios_to_user_termios(struct termios2 __user *u,
+ struct ktermios *k)
+{
+ return copy_to_user(u, k, sizeof(struct termios2));
+}
+
+static inline int user_termios_to_kernel_termios_1(struct ktermios *k,
+ struct termios __user *u)
+{
+ return copy_from_user(k, u, sizeof(struct termios));
+}
+
+static inline int kernel_termios_to_user_termios_1(struct termios __user *u,
+ struct ktermios *k)
+{
+ return copy_to_user(u, k, sizeof(struct termios));
+}
+
+#endif /* __KERNEL__ */

#endif /* __BFIN_TERMIOS_H__ */
--
1.6.3.1

2009-06-12 11:30:28

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH] Blackfin: sync termios header changes with x86

On Friday 12 June 2009, Mike Frysinger wrote:

> Arnd: if you don't mind, i'd rather merge this ... i can put it into my
> next batch of Blackfin changes for 2.6.31

I don't see how that helps, because:

> #define TIOCMIWAIT 0x545C /* wait for a change on serial input line(s) */
> #define TIOCGICOUNT 0x545D /* read serial port inline interrupt counts */
> -
> -#define FIOQSIZE 0x545E
> +#define TIOCGHAYESESP 0x545E /* Get Hayes ESP configuration */
> +#define TIOCSHAYESESP 0x545F /* Set Hayes ESP configuration */
> +#define FIOQSIZE 0x5460

This breaks existing applications using FIOQSIZE. You really shouldn't do that.

> diff --git a/arch/blackfin/include/asm/termbits.h b/arch/blackfin/include/asm/termbits.h
> index f37feb7..faa569f 100644
> --- a/arch/blackfin/include/asm/termbits.h
> +++ b/arch/blackfin/include/asm/termbits.h

This one only changes whitespace, what is the point?

> --- a/arch/blackfin/include/asm/termios.h
> +++ b/arch/blackfin/include/asm/termios.h

This change fixes one bug but not another:

> @@ -58,37 +60,55 @@ struct termio {
> *(unsigned short *) &(termios)->x = __tmp; \
> }
>
> -#define user_termio_to_kernel_termios(termios, termio) \
> -({ \
> - SET_LOW_TERMIOS_BITS(termios, termio, c_iflag); \
> - SET_LOW_TERMIOS_BITS(termios, termio, c_oflag); \
> - SET_LOW_TERMIOS_BITS(termios, termio, c_cflag); \
> - SET_LOW_TERMIOS_BITS(termios, termio, c_lflag); \
> - copy_from_user((termios)->c_cc, (termio)->c_cc, NCC); \
> -})
> +static inline int user_termio_to_kernel_termios(struct ktermios *termios,
> + struct termio __user *termio)
> +{
> + SET_LOW_TERMIOS_BITS(termios, termio, c_iflag);
> + SET_LOW_TERMIOS_BITS(termios, termio, c_oflag);
> + SET_LOW_TERMIOS_BITS(termios, termio, c_cflag);
> + SET_LOW_TERMIOS_BITS(termios, termio, c_lflag);
> + get_user(termios->c_line, &termio->c_line);
> + return copy_from_user(termios->c_cc, termio->c_cc, NCC);
> +}

You correctly read termios->c_line now, which was missing previously,
but you still don't check the return value of the get_user and
copy_from_user functions. The code also has a very ugly property
of working only on little-endian architectures (which includes
blackfin AFAICT) but should not serve as an example.

If you want a working version of this file, best copy it from avr32
or frv. Or just wait for the asm-generic version.

Arnd <><

2009-06-12 11:36:17

by Mike Frysinger

[permalink] [raw]
Subject: Re: [PATCH] Blackfin: sync termios header changes with x86

On Fri, Jun 12, 2009 at 07:29, Arnd Bergmann wrote:
> On Friday 12 June 2009, Mike Frysinger wrote:
>>  #define TIOCMIWAIT   0x545C  /* wait for a change on serial input line(s) */
>>  #define TIOCGICOUNT  0x545D  /* read serial port inline interrupt counts */
>> -
>> -#define FIOQSIZE     0x545E
>> +#define TIOCGHAYESESP   0x545E  /* Get Hayes ESP configuration */
>> +#define TIOCSHAYESESP   0x545F  /* Set Hayes ESP configuration */
>> +#define FIOQSIZE     0x5460
>
> This breaks existing applications using FIOQSIZE. You really shouldn't do that.

meh, no one uses that anyways ;)

>> diff --git a/arch/blackfin/include/asm/termbits.h b/arch/blackfin/include/asm/termbits.h
>> index f37feb7..faa569f 100644
>> --- a/arch/blackfin/include/asm/termbits.h
>> +++ b/arch/blackfin/include/asm/termbits.h
>
> This one only changes whitespace, what is the point?

makes diffing easier

>> --- a/arch/blackfin/include/asm/termios.h
>> +++ b/arch/blackfin/include/asm/termios.h
>
> This change fixes one bug but not another:

better than none at all !

>> @@ -58,37 +60,55 @@ struct termio {
>>       *(unsigned short *) &(termios)->x = __tmp; \
>>  }
>>
>> -#define user_termio_to_kernel_termios(termios, termio) \
>> -({ \
>> -     SET_LOW_TERMIOS_BITS(termios, termio, c_iflag); \
>> -     SET_LOW_TERMIOS_BITS(termios, termio, c_oflag); \
>> -     SET_LOW_TERMIOS_BITS(termios, termio, c_cflag); \
>> -     SET_LOW_TERMIOS_BITS(termios, termio, c_lflag); \
>> -     copy_from_user((termios)->c_cc, (termio)->c_cc, NCC); \
>> -})
>> +static inline int user_termio_to_kernel_termios(struct ktermios *termios,
>> +                                             struct termio __user *termio)
>> +{
>> +     SET_LOW_TERMIOS_BITS(termios, termio, c_iflag);
>> +     SET_LOW_TERMIOS_BITS(termios, termio, c_oflag);
>> +     SET_LOW_TERMIOS_BITS(termios, termio, c_cflag);
>> +     SET_LOW_TERMIOS_BITS(termios, termio, c_lflag);
>> +     get_user(termios->c_line, &termio->c_line);
>> +     return copy_from_user(termios->c_cc, termio->c_cc, NCC);
>> +}
>
> You correctly read termios->c_line now, which was missing previously,
> but you still don't check the return value of the get_user and
> copy_from_user functions. The code also has a very ugly property
> of working only on little-endian architectures (which includes
> blackfin AFAICT) but should not serve as an example.

Blackfin is LE which means the assumption is fine by me

> If you want a working version of this file, best copy it from avr32
> or frv. Or just wait for the asm-generic version.

is asm-generic going to be in 2.6.31 ? otherwise i'd prefer to have
Blackfin fixed now rather than 2.6.32+
-mike

2009-06-12 11:56:58

by Alan

[permalink] [raw]
Subject: Re: [PATCH] tty: fix unused warning when TCGETX is not defined

> mmm BOTHER is used to get arbitrary speeds right ? i recall testing
> that on Blackfin already so i'm pretty sure that works ...

Blackfin isn't an offender on that one, but a few other people are ;)

Alan

2009-06-12 11:57:26

by Alan

[permalink] [raw]
Subject: Re: [PATCH] Blackfin: sync termios header changes with x86

> -#define FIOQSIZE 0x545E
> +#define TIOCGHAYESESP 0x545E /* Get Hayes ESP configuration */
> +#define TIOCSHAYESESP 0x545F /* Set Hayes ESP configuration */
> +#define FIOQSIZE 0x5460

Umm.. probably better to leave FIOQSIZE where it is ;)

The HAYES ones are obsolete for a specific ancient ISA bus driver so can
probably just be left off

2009-06-12 21:42:18

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH] Blackfin: sync termios header changes with x86

On Friday 12 June 2009, Mike Frysinger wrote:
> > If you want a working version of this file, best copy it from avr32
> > or frv. Or just wait for the asm-generic version.
>
> is asm-generic going to be in 2.6.31 ? otherwise i'd prefer to have
> Blackfin fixed now rather than 2.6.32+

I certainly hope it does. I've sent the pull request yesterday,
so it's probably still in Linus' queue for trees to merge, unless
he dropped it.

Arnd <><

2009-06-13 00:03:17

by Mike Frysinger

[permalink] [raw]
Subject: Re: [PATCH] Blackfin: sync termios header changes with x86

On Fri, Jun 12, 2009 at 17:41, Arnd Bergmann wrote:
> On Friday 12 June 2009, Mike Frysinger wrote:
>> > If you want a working version of this file, best copy it from avr32
>> > or frv. Or just wait for the asm-generic version.
>>
>> is asm-generic going to be in 2.6.31 ?  otherwise i'd prefer to have
>> Blackfin fixed now rather than 2.6.32+
>
> I certainly hope it does. I've sent the pull request yesterday,
> so it's probably still in Linus' queue for trees to merge, unless
> he dropped it.

then i'm fine with fixing the header post your pull by switching to
asm-generic. thanks!
-mike