On Wed, 2013-02-06 at 14:34 -0500, George Spelvin wrote:
> > Now that N_TTY uses tty->disc_data for its private data,
> > 'subclass' ldiscs cannot use ->disc_data for their own private data.
> >
> > Use a lookup list to associate the tty with the pps source.
>
> Thanks for the cleanup. I fully agree my patch was not a good one;
> I just wanted someone more experienced to make the call on rearchitecting.
>
> In particular, I was nervous about getting flamed by Linus for something that
> was too ambitious.
No problem and I completely understand. That's why I jumped in -- it
looked like some help was needed, both now and maybe even in iterations
before this.
> One thing I'd prefer to do would be to change:
>
> +static struct pps_device *lookup_pps_by_tty(struct tty_struct *tty,
> + struct pps_data **p)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&pps_lock, flags);
> + list_for_each_entry((*p), &pps_list, link) {
> + if ((*p)->tty == tty) {
> + spin_unlock_irqrestore(&pps_lock, flags);
> + return (*p)->pps;
> + }
> + }
> + spin_unlock_irqrestore(&pps_lock, flags);
> + return NULL;
> +}
>
> to:
>
> static struct pps_data *lookup_pps_by_tty(struct tty_struct *tty)
> {
> unsigned long flags;
>
> spin_lock_irqsave(&pps_lock, flags);
> list_for_each_entry(p, &pps_list, link) {
> if (p->tty == tty)
> break;
> }
> spin_unlock_irqrestore(&pps_lock, flags);
> return p;
> }
>
> And do the data->pps dereferencing in the caller.
I did this first and it's a mess -- the patch basically ends up looking
like a rewrite. But feel free to use these patches as a base for a
version you do like and submit those instead for review. I just wanted
to show the way.
(Well, actually that was the second version. When I reviewed the
uart_handle_dcd_change() and saw the separate timestamp, I thought that
maybe the latency was going to be a problem. So the first version used
the same approach but with an rcu 'lockless' list instead -- then I went
back and audited the IRQ path and realized there were 5 bus locks and an
i/o port read already. So total overkill.)
Also, I figured maybe it would be best if it was something maintainable
with basic kernel knowledge.
> A more ambitious cleanup would use the existing pps_device list
> (maintained to allocate minor device numbers) and add an "owner" field
> that can be looked up on, without creating a new data structure and
> allocation.
Didn't see where that was (unless you mean the IDR allocation). Probably
best to keep it separate in the event that relative lifetimes change at
some point in the future.
> (It could either be a generic "void *", or a "struct device *" and
> compare it to tty->dev.)
>
> After all, despite the implementation effort to scale, the total number
> of pps devices in a system is usually at most 1 (I have a computer where
> I run 2, and I doubt there are many others on the planet who do that.)
I thought that was probably the case which is why a lookup list is an
acceptable solution.
Please let us know if you plan to respin the patches, so these patches
don't get pushed.
Regards,
Peter Hurley
--
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
