Re: [PATCH 09/10] n_tracerouter ldisc driver.

[J Freyensee <james_p_freyensee@linux.intel.com>]

On Thu, 2011-02-24 at 20:20 +0100, Thomas Gleixner wrote:
> On Thu, 24 Feb 2011, james_p_freyensee@linux.intel.com wrote:
> > +
> > +/**
> > + * tracerouter_alloc
> > + *
> > + * Allocates the structure needed for this ldisc.
> > + */
> > +static struct tracerouter_data *tracerouter_alloc(void)
> > +{
> > +	struct tracerouter_data *tptr = kzalloc(
> > +					sizeof(struct tracerouter_data),
> > +					GFP_KERNEL);
> > +	if (tptr == NULL)
> > +		return NULL;
> > +	tptr->opencalled = 0;
> 
> Wheeee!
> 
> You first alloc it with kzalloc and then clear the already zero member
> again ?
> 
> That whole crap condenses down to:
> 
>      return kzalloc(sizeof(*tptr), GFP_KERNEL);
> 
> Which hardly justfies a separate function
> 
> > +	return tptr;
> > +}
> > +
> > +/**
> > + * n_tracerouter_open() - Called when a tty is opened by a SW entity.
> > + * @tty: terminal device to the ldisc.
> > + *
> > + * Return:
> > + *      0 for success.
> > + *
> > + * Caveats: This should only be opened one time per SW entity.
> > + */
> > +static int n_tracerouter_open(struct tty_struct *tty)
> > +{
> > +	int retval = -EEXIST;
> > +
> > +	mutex_lock(&routelock);
> > +	if (tr_data->opencalled == 0) {
> > +
> > +		tr_data->kref_tty = tty_kref_get(tty);
> > +		if (tr_data->kref_tty == NULL)
> > +			retval = -EFAULT;
> > +		else {
> 
> Please use braces for the if as well. It's just irritating not to have
> them before the else.

checkpatch.pl did not care about the lack of braces so that is why this
got missed; I can add it.

> 
> > +			tr_data->opencalled = 1;
> > +			tty->disc_data      = tr_data;
> > +			tty->receive_room   = RECEIVE_ROOM;
> > +			tty_driver_flush_buffer(tty);
> > +			retval = 0;
> > +		}
> > +	}
> > +	mutex_unlock(&routelock);
> > +	return retval;
> > +}
> > +
> > +/**
> > + * n_tracerouter_close() - close connection
> > + * @tty: terminal device to the ldisc.
> > + *
> > + * Called when a software entity wants to close a connection.
> > + */
> > +static void n_tracerouter_close(struct tty_struct *tty)
> > +{
> > +	struct tracerouter_data *tptr = tty->disc_data;
> > +
> > +	WARN_ON(tptr->kref_tty != tr_data->kref_tty);
> > +	tty_driver_flush_buffer(tty);
> 
> That code probably never run with lockdep as you would get a potential
> deadlock warning. See n_tracerouter_open().
> 
> Though the deadlock cannot happen as you are protected by
> tr_data->opencalled it does not make it more correct.
> 

What do you suggest to make it correct for upstream?

Thanks,
jay

> > +	mutex_lock(&routelock);
> > +	tty_kref_put(tr_data->kref_tty);
> > +	tr_data->kref_tty = NULL;
> > +	tr_data->opencalled = 0;
> > +	tty->disc_data = NULL;
> > +	mutex_unlock(&routelock);
> > +}
> > +
> 
> Thanks,
> 
> 	tglx