On Tue, Sep 10, 2013 at 07:24:35PM +0200, Andrew Lunn wrote:
> Hi Johan
>
> Thanks for your quick comments. I hopefully have more time to work on
> this code now, so v3 will not take as long as v2 did.
Great. It shouldn't be that much more work.
> > > +/* This structure holds all of the local port information */
> > > +struct mxuport_port {
> > > + int flags; /* for async_struct and serial_struct flags
> > > + field */
> > > + int set_B0;
> > > +
> > > + u8 mcr_state; /* last MCR state */
> > > +
> > > + u8 msr_state; /* last MSR state */
> > > + u8 lsr_state; /* last LSR state */
> > > + unsigned long hold_reason;
> >
> > You go to great lengths to update this hold_reason but I can't see that
> > you ever use it?
>
> Its left over from the old moxa vendor driver. It seems i ripped out
> all the users of it. The vendor driver has some flow control between
> the host and the device. The device has quite big buffers, both read
> and write. So if the host is writing data out fast, it gets buffered,
> rather than pushing back and slowing down the application generating
> the data. Similarly, if the application is being slow reading what has
> been received, it will get buffered in the device, rather than trigger
> flow control over the serial link. For my application, using the 4
> port device for consoles onto my ARM development systems, these
> buffers are not an issue. My usage it interactive. However if somebody
> were to run PPP over the ports, they are likely to see quite bad
> buffer bloat issues.
>
> I will probably rip out the rest of this hold_reason code. If the
> buffers do turn out to be an issue for somebody, we can see how to
> cleanly add back the flow control. Its messy because of the
> multiplexing.
Sounds good. You can leave the definitions in and perhaps log the events
using dev_dbg.
> > > +/*
> > > + * mxuport_process_read_urb_chunk
> > > + *
> > > + * Processes one chunk of data received for a port. If the port is not
> > > + * open, discard the data. Otherwise pass it onto the tty
> > > + * layer. Mostly a copy of usb_serial_generic_process_read_urb().
> > > + */
> >
> > You actually never check that the port is indeed open, which is
> > something you'd want to do (although perhaps below in process_read_urb
> > instead).
>
> I wondered about this. Is it an issue to pass data to the tty layer
> when the port is closed? I had a look at the flip buffer code, and it
> seems like the buffer is freed when the port is closed. What i'm not
> too sure about is when its allocated.
>
> How should i test if the port is opened? Using:
>
> test_bit(ASYNCB_INITIALIZED, &port->flags)
The tty buffer is initialised at probe, but grows dynamically and is
finally freed at release. It is also flushed at every (final) close. We
don't expect new data to arrive while the port isn't open, so using
ASYNCB_INITIALIZED in process_read_urb below is the way to go.
> > > +
> > > + /* if baud rate is B0, drop DTR and RTS */
> > > + if (!(cflag & CBAUD)) {
> > > + mxport->set_B0 = 1;
> > > + mxport->mcr_state &= ~(UART_MCR_DTR | UART_MCR_RTS);
> > > +
> > > + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_MCR,
> > > + mxport->mcr_state,
> > > + port->port_number);
> > > + if (err)
> > > + return err;
> > > + } else {
> > > + if (mxport->set_B0) {
> > > + mxport->set_B0 = 0;
> > > + err = mxuport_send_ctrl_urb(serial->dev,
> > > + RQ_VENDOR_SET_DTR,
> > > + 1, port->port_number);
> > > + if (err)
> > > + return err;
> > > + }
> > > + }
> >
> > You shouldn't need set_B0. Make sure to pass the old termios settings
> > and detect B0 <-> non-B0 transitions using that struct. I see that
> > you're currently calling change_port_settings from setserial in order to
> > handle ASYNC_SPD_MASK. There are some issues with it's current
> > implementation, but if you really want to support it it's of course
> > doable (see below).
>
> I need 115200, since that is what most of my development systems use
> for the serial console. Will the normal path, tty_get_baud_rate(tty),
> handle baud rates above 57600?
Yes, cfsetospeed can be used up to 4Mbaud for example.
> The hardware is also capable of "custom divisor" although you don't
> pass the divisor you need, but the actual baud rate. The moxa vendor
> driver had a custom IOCTL call for that, which i ripped out, but i've
> not yet looked how to implement it correctly.
Isn't that the way you do it today with RQ_VENDOR_SET_BAUD?
> > > + /*
> > > + * Throw away all the allocated write URBs so we can set
> > > + * them up again to fit the multiplexing scheme.
> > > + */
> > > + for (i = 0; i < serial->num_bulk_out; ++i) {
> > > + port = serial->port[i];
> > > + for (j = 0; j < ARRAY_SIZE(port->write_urbs); ++j) {
> > > + usb_free_urb(port->write_urbs[j]);
> > > + kfree(port->bulk_out_buffers[j]);
> > > + port->write_urbs[j] = NULL;
> > > + port->bulk_out_buffers[j] = NULL;
> > > + }
> > > + port->write_urbs_free = 0;
> > > + }
> >
> > There's no need to free and later reallocate the first write urb, right?
>
> It could be kept.
>
> > > +
> > > + /* Find the first bulk out interface - has to exist */
> > > + iface_desc = serial->interface->cur_altsetting;
> > > + for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
> > > + endpoint = &iface_desc->endpoint[i].desc;
> > > + if (usb_endpoint_is_bulk_out(endpoint))
> > > + break;
> > > + }
> >
> > You don't need this, just use the fields of port0 already filled in by
> > usb-serial.
> >
> > > +
> > > + /*
> > > + * All write data is sent over the first bulk out endpoint,
> > > + * with an added header to indicate the port. Allocate URBs
> > > + * for each port to the first bulk out endpoint.
> > > + */
> > > + buffer_size = usb_endpoint_maxp(endpoint);
> >
> > Skip this...
> >
> > > + for (i = 0; i < serial->num_ports; ++i) {
> >
> > ...and only iterate over ports with num > 0...
> >
> > > + port = serial->port[i];
> > > + port->bulk_out_size = buffer_size;
> >
> > ...and then use
> >
> > port->bulk_out_size = port0->bulk_out_size
> >
> > and similar throughout.
>
> So you are assuming the endpoints all have the same buffer size? At
> least for the one device i have, that is true. But is it guaranteed?
> What would happen if the event endpoint actually had a smaller size?
No, remember that we're setting up all ports to use the first bulk-out
endpoint, so the fields can be retrieved from port0.
> > > + /*
> > > + * Add data from the ports is received on the first bulk in
> > > + * endpoint, with a multiplex header. The second bulk in is
> > > + * used for events. Throw away all but the first two bulk in
> > > + * urbs.
> > > + */
> > > + for (i = 2; i < serial->num_bulk_in; ++i) {
> > > + port = serial->port[i];
> > > + for (j = 0; j < ARRAY_SIZE(port->read_urbs); ++j) {
> > > + usb_free_urb(port->read_urbs[j]);
> > > + kfree(port->bulk_in_buffers[j]);
> > > + port->read_urbs[j] = NULL;
> > > + port->bulk_in_buffers[j] = NULL;
> > > + }
> > > + }
> >
> > Do any of these devices actually have more than two bulk-in endpoints?
>
> The one device i have has 3 IN and 3 OUT.
Ok, then you can free the remaining urbs and buffers allocated by
usb-serial if you prefer (you already took care of the OUT endpoints
above).
> > > +
> > > + /* Start to read serial data from the device */
> > > + err = usb_serial_generic_submit_read_urbs(port0, GFP_KERNEL);
> > > + if (err) {
> > > + dev_dbg(&port0->dev,
> > > + "%s - USB submit read bulk urb failed. (status = %d)\n",
> > > + __func__, err);
> > > + goto out;
> > > + }
> > > +
> > > + /* Endpoints on Port1 is used for events */
> > > + err = usb_serial_generic_submit_read_urbs(port1, GFP_KERNEL);
> > > + if (err) {
> > > + dev_dbg(&port1->dev,
> > > + "%s - USB submit read bulk urb failed. (status = %d)\n",
> > > + __func__, err);
> > > + goto out;
> > > + }
> > > + return 0;
> >
> > Hmm. So here you're submitting read urbs for two ports before their
> > port_probe has run and allocated the port private data. Specifically,
> > if you get an early event packet on port1 you end up with a NULL-deref
> > when processing it.
>
> Oh, yes. Not nice. Need to think about that.
If you make sure to never process any packets for ports that have never
been opened (using ASYNCB_INITIALIZED, as discussed above) you should be
fine as far as the NULL-deref goes.
It would still be better if you could submit and kill on first open and
last close, respectively, though.
Either way, you should also kill the port write urb and clear it's fifo
on port close. You can look at the generic close implementation but not
use it directly (as it would kill the read urbs for port 0 and 1).
> > > + static struct usb_serial_driver mxuport_device = {
> > > + .driver = {
> > > + .owner = THIS_MODULE,
> > > + .name = "mx-uport",
> > > + },
> > > + .description = "MOXA UPort",
> > > + .id_table = mxuport_idtable,
> > > + .num_ports = 0,
> > > + .probe = mxuport_probe,
> > > + .port_probe = mxuport_port_probe,
> > > + .port_remove = mxuport_port_remove,
> > > + .attach = mxuport_attach,
> > > + .calc_num_ports = mxuport_calc_num_ports,
> > > + .open = mxuport_open,
> > > + .close = mxuport_close,
> > > + .write = usb_serial_generic_write,
Just noticed this one. No need to set it explicitly.
> > > + .ioctl = mxuport_ioctl,
> > > + .set_termios = mxuport_set_termios,
> > > + .break_ctl = mxuport_break_ctl,
> > > + .tx_empty = mxuport_tx_empty,
> > > + .tiocmiwait = usb_serial_generic_tiocmiwait,
> > > + .get_icount = usb_serial_generic_get_icount,
> > > + .throttle = mxuport_throttle,
> > > + .unthrottle = mxuport_unthrottle,
> > > + .tiocmget = mxuport_tiocmget,
> > > + .tiocmset = mxuport_tiocmset,
> > > + .process_read_urb = mxuport_process_read_urb,
> > > + .prepare_write_buffer = mxuport_prepare_write_buffer,
> > > + };
Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
