Re: [PATCH 67/68] TTY: isdn/gigaset, do not set tty->driver_data to NULL

[Jiri Slaby <jslaby@suse.cz>]

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