Re: [PATCH 09/10] ieee802154: add serial dongle driver

[Alan Cox <alan@lxorguk.ukuu.org.uk>]

> > > +	return _send_pending_data(zbdev);
> > 
> > Where do you check that the tty has enough space ?
> 
> Basically that means checking for ->write result, doesn't it?

You should check write_room before writing if you want some control - you
can also then respect internal flow control via the wakeup mechanism.

Something like:
                /* Wait for space */
                while ((space = tty_write_room(tty)) < len) {
                        if (file->f_flags & O_NONBLOCK) {
                                err = -EAGAIN;
                                break;
                        }
                        interruptible_sleep_on(&tty->write_wait);
                        if (signal_pending(current)) {
                                err = -EINTR;
                                break;
                        }
                }
                /* Shove the entire frame down */
                tty->ops->write(tty, data, len);
 
> > In all these ERR cases what stops a remote device from having fun spewing
> > garbage at you and filling the log ?
> 
> And what stops your precious IDE controller from spewing garbage and
> filling the log? Nothing I think. I'll lower the priority of the
> messages though.

The fact the drive is entirely under my control and not potentially being
spewed at from a hostile network. Also the fact that it takes about 30
seconds to spank a misbehaving drive and reset it. The network laye
generally uses time checks (search for ratelimit()).

> any RS-232 device which implements IEEE 802.15.4 PHY layer and doesn't
> follow this protocol, we can extend the ldisc to be more like hci-uart:
> a multiplexor of protocols.

Ok

> > > +	zbdev->tty = tty;
> > 
> > Refcounting ?
> 
> Sample code? SLIP, hci-uart, ppp don't do it.

No I'm still running around clobbering them all - and slip needed a
partial rewrite first.

	zbdev->tty = tty_kref_get(tty);

	tty_kref_put(foo);

> > You can't go around mashing driver internal values - many drivers don't
> > support low_latency mode and this will make them crash.
> 
> C&P from drivers/bluetooth/hci-ldisc.c, around line 466. We had problems
> working with low-latency unset.

The bluetooth one needs killing too. I will do that tomorrow ...

How many releases ago - the entire tty buffering layer has been rewritten
over time. tty->low_latency requires driver specific support that most
don't have. Also as of 2.6.30rc you'll get debug spew if you misuse it.

If you still need it we need to know why.


> That's not a 'no, I won't do it', but rather 'wait, all those are also
> buggy ?!'

The tty layer has a lot of "yes, those are also buggy" - which is why I'm
currently half way through systematically brutalising it.


> > > +	default:
> > > +		pr_debug("Unknown ioctl cmd: %u\n", cmd);
> > > +		return -EINVAL;
> > 
> > This will break default ioctl processing. You probably want to call into
> > some of the generic handlers at this point depending upon your hardware.
> > Either way -EINVAL is wrong.
> 
> 
> n_tty_ioctl_helper ?

That depends what you need. tty_mode_ioctl() gives you all the mode
stuff, n_tty_ioctl_helper adds things like software flow management which
don't actually look relevant to your ldisc ?

Any other queries let me know, and if the tty->low_latency is still
needed let me know and we'll figure out why between us, as it should not
be.

Alan