lkml.org 
[lkml]   [2012]   [Mar]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 67/68] TTY: isdn/gigaset, do not set tty->driver_data to NULL
    On 03/05/2012 06:05 PM, Tilman Schmidt wrote:
    > Am 05.03.2012 14:52, schrieb Jiri Slaby:
    >> Close the window in open where driver_data is reset to NULL on
    >> each open. It could cause other processes to get invalid retval
    >> from the tty->ops operations because of the checks all over the
    >> code.
    >
    >> With this change we may do other cleanups. Now, the only valid
    >> check for tty->driver_data != NULL is in close. This can happen
    >> only if open fails at gigaset_get_cs_by_tty or try_module_get.
    >> The rest of checks in various tty->ops->* are invalid as
    >> driver_data cannot be NULL there. The same holds for
    >> cs->open_count. So remove them.
    >
    > Thanks for that nice cleanup. It's most welcome. Just one question
    > and a small nit:
    >
    >> --- a/drivers/isdn/gigaset/interface.c +++
    >> b/drivers/isdn/gigaset/interface.c
    > [...]
    >> @@ -178,12 +176,11 @@ static int if_open(struct tty_struct *tty,
    >> struct file *filp)
    >
    >> static void if_close(struct tty_struct *tty, struct file *filp)
    >> { - struct cardstate *cs; + struct cardstate *cs =
    >> tty->driver_data; unsigned long flags;
    >
    >> - cs = (struct cardstate *) tty->driver_data; - if (!cs) { -
    >> pr_err("%s: no cardstate\n", __func__); + if (!cs) { /* happens
    >> if we didn't find cs in open */ + printk(KERN_DEBUG "%s: no
    >> cardstate\n", __func__); return; }
    >
    >
    > Why are you downgrading the error message from KERN_ERR to
    > KERN_DEBUG here? I would think that condition would warrant a
    > message with KERN_ERR severity. Also, the driver does KERN_DEBUG
    > output uniformly through the gig_dbg macro, so if you are sure it
    > should be turned into a debug message then please write it as
    >
    > gig_dbg(DEBUG_IF, "%s: no cardstate", __func__);
    >
    > like four lines later.

    <citing myself from the commit log>
    Now, the only valid check for tty->driver_data != NULL is in close.
    This can happen only if open fails at gigaset_get_cs_by_tty or
    try_module_get.
    </citing>

    I.e. when the module behind the device is going away, driver_data can
    be NULL. In that case we don't want to threaten the user by ERR messages.

    You are maybe right, that we should switch it to gig_dbg or remove the
    print completely (as it is a legitimate path). I'll wait until the
    patches settles down a bit and fix it. If I, by a chance, forget to do
    so, poke me or feel free to do it yourself ;).

    thanks,
    --
    js
    suse labs


    \
     
     \ /
      Last update: 2012-03-05 18:15    [W:0.028 / U:1.104 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site