> * n_tracerouter_open() - Called when a tty is opened by a SW entity.
> * @tty: terminal device to the ldisc.
> *
> * Return:
> - * 0 for success.
> + * 0 for success.
> + *
> + * Caveats: This should only be opened one time per SW entity.
> */
> -
> static int n_tracerouter_open(struct tty_struct *tty)
> {
> - tty->receive_room = RECEIVE_ROOM;
> - tty_driver_flush_buffer(tty);
> - return 0;
> + struct tracerouter_data *tptr;
> +
> + pr_debug("%s(%s): called.\n", __FILE__, __func__);
> +
> + if (tty->disc_data == NULL) {
This check isn't right - the disc_data is random when you get it and
always yours to use as the line discipline, so you don't need to check
and the check may fail. You can just lose the check.
> + if (copy_from_user(&(tptr->routedata_ptihw), (void
> *) arg,
> + sizeof(tptr->routedata_ptihw)))
> + retval = -EFAULT;
For integers and the like get_user/put_user and into a temporary
variable as you otherwise half copy a value and then get a fault.
> +
> + if (tptr->routedata_ptihw == 0)
> + tptr->routedata = mipi_pti_sinkdata;
Should only be done if the copy worked ?
> + * to use this trace router and the trace sink
> + * for a given system's trace route needs.
> + */
> +#ifdef CONFIG_INTEL_MID_PTI
> + else
> + tptr->routedata = mipi_pti_writedata;
> +#endif
In the longer term I would generalise this to copy a name and have a
function something like
routedata = tracerouter_lookup(name);
if (routedata != NULL)
tptr->routedata = routedata->func;
else
retval = -ENOENT;
> + mutex_unlock(&routelock);
> + return retval;
As at that point it then becomes a very much more generic routing tool
and anything can provide methods.
> + /* If some thead is hanging onto the alloclock, force it to
> release
> + * it because we are shutting down.
> + */
> + if (mutex_is_locked(&routelock) == 1)
> + mutex_unlock(&routelock);
> + mutex_destroy(&routelock);
> +
That seems wrong. If something is hanging onto it then when you unlock
it and destroy it they will then also unlock the freed memory.
> + this_tty = tty_kref_get(tty);
You still have the problem with not preventing two users (or handling
it)
> + tty_kref_put(this_tty);
> + this_tty = NULL;
What if another CPU is currently writing to the ldisc ?
> +/**
> + * mipi_pti_sinkdata() - Kernel API function used to route
> + * trace debugging data to user-defined
> + * port like USB.
> + *
> + * @mc: Not used. Null can be passed. Needed to keep
> + * conformance with mipi_pti_writedata().
> + * @buf: Trace debuging data to write to the PTI HW.
> + * Null value will return with no write occurring.
> + * @count: Size of buf. Value of 0 or a negative number will
> + * return with no write occuring.
> + *
> + * Caveat: If this line discipline does not set the tty it sits
> + * on top of via an open() call, this API function will not
> + * call the tty's write() call because it will have no pointer
> + * to call the write().
> + */
> +void mipi_pti_sinkdata(struct masterchannel *mc, u8 *cp, int count)
> +{
> + pr_debug("%s(%s): called\n", __FILE__, __func__);
> +
> + mutex_lock(&writelock);
> +
> + if ((cp != NULL) && (count > 0) && (this_tty != NULL))
> + this_tty->ops->write(this_tty, cp, count);
And this_tty could go to NULL during that if the tty is being closed or
hung up...
> +
> + mutex_unlock(&writelock);
> +}
> +EXPORT_SYMBOL(mipi_pti_sinkdata);
_GPL
> + this_tty = NULL;
It starts as NULL the compiler sees to that
> + /* If some thead is hanging onto the alloclock, force it to
> release
> + * it because we are shutting down.
> + */
Same comment as before
> + if (mutex_is_locked(&writelock) == 1)
> + mutex_unlock(&writelock);
> + mutex_destroy(&writelock);
> +
> + this_tty = NULL;
and same locking problem
So some trivial stuff but the this_tty bit is rather more of a problem.
The tty itself is safe - your kref ensures the tty won't be destroyed
under you, but your references to this_tty need more care, both to
avoid overwriting it by attaching to two ttys, and to avoid races
between a logging event and a close. I think the races are must a
matter of extending the writelock ?
_______________________________________________
Meego-kernel mailing list
[email protected]
http://lists.meego.com/listinfo/meego-kernel
