[PATCH] tty_ioctl: Soft carrier handling - Kernel

This is a discussion on [PATCH] tty_ioctl: Soft carrier handling - Kernel ; First cut at moving the soft carrier handling knowledge entirely into the core code. One or two drivers still needed to snoop these functions to track CLOCAL internally. Instead make TIOCSSOFTCAR generate the same driver calls as other termios ioctls ...

+ Reply to Thread
Results 1 to 4 of 4

Thread: [PATCH] tty_ioctl: Soft carrier handling

  1. [PATCH] tty_ioctl: Soft carrier handling

    First cut at moving the soft carrier handling knowledge entirely into the
    core code. One or two drivers still needed to snoop these functions to
    track CLOCAL internally. Instead make TIOCSSOFTCAR generate the same
    driver calls as other termios ioctls changing the clocal flag. This
    allows us to remove any driver knowledge and special casing. Also while
    we are at it we can fix the error handling.

    Signed-off-by: Alan Cox

    diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c linux-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c
    --- linux.vanilla-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c 2008-03-10 12:57:53.000000000 +0000
    +++ linux-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c 2008-03-10 13:27:09.000000000 +0000
    @@ -753,6 +756,32 @@
    }

    /**
    + * tty_change_softcar - carrier change ioctl helper
    + * @tty: tty to update
    + * @arg: enable/disable CLOCAL
    + *
    + * Perform a change to the CLOCAL state and call into the driver
    + * layer to make it visible. All done with the termios mutex
    + */
    +
    +static int tty_change_softcar(struct tty_struct *tty, int arg)
    +{
    + int ret = 0;
    + int bit = arg ? CLOCAL : 0;
    + struct ktermios old = *tty->termios;
    +
    + mutex_lock(&tty->termios_mutex);
    + tty->termios->c_cflag &= ~CLOCAL;
    + tty->termios->c_cflag |= bit;
    + if (tty->driver->set_termios)
    + tty->driver->set_termios(tty, &old);
    + if ((tty->termios->c_cflag & CLOCAL) != bit)
    + ret = -EINVAL;
    + mutex_unlock(&tty->termios_mutex);
    + return ret;
    +}
    +
    +/**
    * tty_mode_ioctl - mode related ioctls
    * @tty: tty for the ioctl
    * @file: file pointer for the tty
    @@ -862,12 +891,7 @@
    case TIOCSSOFTCAR:
    if (get_user(arg, (unsigned int __user *) arg))
    return -EFAULT;
    - mutex_lock(&tty->termios_mutex);
    - tty->termios->c_cflag =
    - ((tty->termios->c_cflag & ~CLOCAL) |
    - (arg ? CLOCAL : 0));
    - mutex_unlock(&tty->termios_mutex);
    - return 0;
    + return tty_change_softcar(tty, arg);
    default:
    return -ENOIOCTLCMD;
    }
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH] tty_ioctl: Soft carrier handling

    On 03/10/2008 10:56 PM, Alan Cox wrote:
    > First cut at moving the soft carrier handling knowledge entirely into the
    > core code. One or two drivers still needed to snoop these functions to
    > track CLOCAL internally. Instead make TIOCSSOFTCAR generate the same
    > driver calls as other termios ioctls changing the clocal flag. This
    > allows us to remove any driver knowledge and special casing. Also while
    > we are at it we can fix the error handling.
    >
    > Signed-off-by: Alan Cox
    >
    > diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c linux-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c
    > --- linux.vanilla-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c 2008-03-10 12:57:53.000000000 +0000
    > +++ linux-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c 2008-03-10 13:27:09.000000000 +0000
    > @@ -753,6 +756,32 @@

    [...]
    > +static int tty_change_softcar(struct tty_struct *tty, int arg)
    > +{
    > + int ret = 0;
    > + int bit = arg ? CLOCAL : 0;
    > + struct ktermios old = *tty->termios;


    Maybe a dumb question, but shouldn't be the snapshot done in the critical
    section too?

    > +
    > + mutex_lock(&tty->termios_mutex);
    > + tty->termios->c_cflag &= ~CLOCAL;
    > + tty->termios->c_cflag |= bit;
    > + if (tty->driver->set_termios)
    > + tty->driver->set_termios(tty, &old);
    > + if ((tty->termios->c_cflag & CLOCAL) != bit)
    > + ret = -EINVAL;
    > + mutex_unlock(&tty->termios_mutex);
    > + return ret;
    > +}

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

  3. Re: [PATCH] tty_ioctl: Soft carrier handling

    On Mon, 10 Mar 2008 21:56:13 +0000 Alan Cox wrote:

    > First cut at moving the soft carrier handling knowledge entirely into the
    > core code. One or two drivers still needed to snoop these functions to
    > track CLOCAL internally. Instead make TIOCSSOFTCAR generate the same
    > driver calls as other termios ioctls changing the clocal flag. This
    > allows us to remove any driver knowledge and special casing. Also while
    > we are at it we can fix the error handling.
    >
    > Signed-off-by: Alan Cox
    >
    > diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c linux-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c
    > --- linux.vanilla-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c 2008-03-10 12:57:53.000000000 +0000
    > +++ linux-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c 2008-03-10 13:27:09.000000000 +0000
    > @@ -753,6 +756,32 @@
    > }
    >
    > /**
    > + * tty_change_softcar - carrier change ioctl helper
    > + * @tty: tty to update
    > + * @arg: enable/disable CLOCAL
    > + *
    > + * Perform a change to the CLOCAL state and call into the driver
    > + * layer to make it visible. All done with the termios mutex
    > + */
    > +
    > +static int tty_change_softcar(struct tty_struct *tty, int arg)
    > +{
    > + int ret = 0;
    > + int bit = arg ? CLOCAL : 0;
    > + struct ktermios old = *tty->termios;
    > +
    > + mutex_lock(&tty->termios_mutex);


    change_termios() modifies tty->termios under tty->termios_mutex, which is
    very sensible.

    Hence (surely?) all code which takes a copy of tty->termios should do so
    under tty->termios_mutex so that it doesn't get a copy of an internally
    inconsistent, half-overwritten structure.

    But this code doesn't do that, nor does change_termios(), set_termios() and
    perhaps other places.

    Confused.

    > + tty->termios->c_cflag &= ~CLOCAL;
    > + tty->termios->c_cflag |= bit;
    > + if (tty->driver->set_termios)
    > + tty->driver->set_termios(tty, &old);
    > + if ((tty->termios->c_cflag & CLOCAL) != bit)
    > + ret = -EINVAL;
    > + mutex_unlock(&tty->termios_mutex);
    > + return ret;
    > +}


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

  4. Re: [PATCH] tty_ioctl: Soft carrier handling


    > > + int bit = arg ? CLOCAL : 0;
    > > + struct ktermios old = *tty->termios;

    >
    > Maybe a dumb question, but shouldn't be the snapshot done in the critical
    > section too?


    Yes it should. I'll fix that in the next bits and double check the cases
    Andrew asks about (although I think they are ok with caller locking from
    memory)

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

+ Reply to Thread