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
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
