Thanks for the review. Comments below.
>
>> * 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.
>
I did this to address the comment that n_tracerouter should only be
allowed to sit on one tty port per SW entity and to prevent a multiple tty
port opening. If it's okay to loose this check, does this 2nd revision
address the concern of having only one tty port opening that this ldisc
sits on?
>
>> + 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 ?
>
I'll have to think about that (I'm at a conference right now), but I do
want the default to go mipi_pti_sinkdata which is the n_tracesink driver
because routing to ttyUSB is starting to become the primary use-case
default that will be seen for this entire trace-PTI design. So it being
set even if copy failed could be correct and what I want.
>> + * 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.
When the PTI driver gets a new tty port added (and tested), this whole
ioctl() will be gone and the n_tracerouter will rely on n_tracesink to
route it's data to the PTI driver, if the userapp chooses to route the
data there. That is part of the FIXME found at the beginning of this
code. That should also address the generic problem raised here too.
>
>
>> + /* 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.
>
>
My thinking was if we are exiting, the system is shutting down, and if
there is mutex, I don't want it to hold up system shutdown. I can remove
this block if this is a problem.
>
>> + this_tty = tty_kref_get(tty);
>
> You still have the problem with not preventing two users (or handling
> it)
>
That goes back to the question I raised earlier. How do you suggest I go
about handling this?
>> + 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...
Wouldn't the close() have to wait if the mutex_lock() has locked this
section? then once close() does happen, this_tty is set to NULL which
will prevent an errornous call to this? I assume that if I stick a
mutex_lock() in the close() that would address your concerns (I don't have
the code in front of me this second so I cannot check)?
>
>> +
>> + 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
