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.
> > All data transfers are made on port0, yet the locks are taken on PortN.
> > urb->context points to PortN, even though the URL is for port0.
>
> You probably meant "URB" here.
Yep. Changelog fixed.
> > +/* 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.
> > +
> > + struct usb_serial_port *port; /* loop back to the owner of this
> > + object */
> > + struct mx_uart_config *uart_cfg; /* configuration of UART */
> > +};
> > +
> > +/* Configuration of UART */
> > +struct mx_uart_config {
> > + u8 data_bits; /* 5..8 - data bits per character */
> > + u8 parity; /* parity settings */
> > + u8 stop_bits; /* stop bits settings */
> > + u8 flow_ctrl; /* flow control settings */
> > + int interface; /* interface is defined */
>
> This description isn't very clear.
Agreed. After reading the code, it indicates RS232, 485 etc. I will
change the comment.
> > +/*
> > + * mxuport_control_callback
> > + *
> > + * This is the callback function for when we have finished sending
> > + * control urb on the control pipe.
> > + */
> > +static void mxuport_control_callback(struct urb *urb)
> > +{
> > + struct usb_ctrlrequest *req = (struct usb_ctrlrequest *)urb->context;
>
> You should pass the usb_serial_port (or usb_serial) rather than the
> ctrlrequest as context in order to get uniform and more informative
> error messages if anything goes wrong. You can still access the
> ctrlrequest as urb->setup_packet.
>
> > + struct device *dev = &urb->dev->dev;
>
> Use &port->dev (or &serial->interface->dev).
>
> > +
> > + switch (urb->status) {
> > + case 0:
> > + break;
> > +
> > + case -ECONNRESET:
> > + case -ENOENT:
> > + case -ESHUTDOWN:
> > + /* This urb is terminated, clean up */
> > + dev_dbg(dev, "%s - urb shutting down with status: %d\n",
> > + __func__, urb->status);
> > + break;
> > +
> > + default:
> > + dev_err(dev, "%s - nonzero control status received: %d\n",
> > + __func__, urb->status);
> > + break;
> > + }
> > +
> > + kfree(req);
> > + usb_free_urb(urb);
>
> You don't free the transfer_buffer even though you allow control
> requests to pass a buffer. You don't use that part of the interface at
> the moment, so either disallow buffers to be passed below or make sure
> to free them here.
I will remove data and size. As you say, the current callers don't
actually pass any data.
> > +}
> > +
> > +/*
> > + * mxuport_send_aysnc_ctrl_urb - send asynchronously a vendor request with
> > + * data
> > + *
> > + * This function writes the given buffer out to the control pipe, but
> > + * does not wait around for it to complete.
> > + */
> > +static void mxuport_send_async_ctrl_urb(struct usb_device *udev,
> > + u8 request,
> > + u16 value,
> > + u16 index, u8 *data, size_t size)
...
> If you think this could be useful in other contexts than
> throttle/unthrottle where you currently call it while holding a
> spinlock, I'd pass the memory flag as a parameter (instead of always
> using GFP_ATOMIC).
At the moment i don't see any other potential users. We can add the
memory flag parameter later if needed.
> > + int status = usb_control_msg(udev,
> > + usb_rcvctrlpipe(udev, 0),
> > + request,
> > + (USB_DIR_IN | USB_TYPE_VENDOR |
> > + USB_RECIP_DEVICE), value, index,
> > + data, size,
> > + HZ * USB_CTRL_GET_TIMEOUT);
>
> Please split definition and initialisation here and below.
>
> You don't want to use HZ here as USB_CTRL_GET_TIMEOUT is already
> specified in ms.
Ah, interesting. Bug in the vendor driver, which i blindly copied. I
will fix both instances.
> > +/*
> > + * mxuport_throttle - throttle function of driver
> > + *
> > + * This function is called by the tty driver when it wants to stop the
> > + * data being read from the port. Since all the data comers over one
> > + * bulk in endpoint, we cannot stop submitting urbs by setting
> > + * port->throttle. Instead tell the device to stop sending us data for
> > + * the port.
> > + */
> > +static void mxuport_throttle(struct tty_struct *tty)
> > +{
> > + struct usb_serial_port *port = tty->driver_data;
> > + struct usb_serial *serial = port->serial;
> > + struct mxuport_port *mxport = usb_get_serial_port_data(port);
> > + unsigned long flags;
> > +
> > + dev_dbg(&port->dev, "%s\n", __func__);
> > +
> > + spin_lock_irqsave(&port->lock, flags);
> > + mxport->hold_reason |= MX_WAIT_FOR_UNTHROTTLE;
> > +
> > + mxuport_send_async_ctrl_urb(serial->dev,
> > + RQ_VENDOR_SET_RX_HOST_EN,
> > + 0, port->port_number, NULL, 0);
> > + spin_unlock_irqrestore(&port->lock, flags);
> > +}
>
> I don't see you ever checking for MX_WAIT_FOR_UNTHROTTLE. Why not get
> rid of it? Then you should be able to use the synchronous interface to
> disable rx for the port in question, and also get rid of the
> asynchronous control interface above.
Good point. I don't see any particular reason for this being async to
start with.
> > +
> > +/*
> > + * 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)
> > +/*
> > + * mxuport_init_port - Initialize the port
> > + *
> > + * Initialize one port, by enabling its FIFO, set to RS-232 mode, open
> > + * the port and enable reception.
> > + */
> > +static int mxuport_init_port(struct usb_serial_port *port)
> > +{
> > + struct mxuport_port *mxport = usb_get_serial_port_data(port);
> > + struct usb_serial *serial = port->serial;
> > + int err = 0;
> > +
> > + dev_dbg(&port->dev, "%s\n", __func__);
> > +
> > + /* Send vendor request - set FIFO (Enable) */
> > + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_FIFO_DISABLE,
> > + 0, port->port_number);
> > + if (err)
> > + return err;
> > +
> > + /* Send vendor request - set transmition mode (Hi-Performance) */
> > + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_HIGH_PERFOR,
> > + 0, port->port_number);
> > + if (err)
> > + return err;
> > +
> > + /* Send vendor request - set interface (RS-232) */
> > + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_INTERFACE,
> > + mxport->uart_cfg->interface,
> > + port->port_number);
> > + if (err)
> > + return err;
> > +
> > + /* Send vendor request - port open */
> > +
> > + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_OPEN,
> > + 1, port->port_number);
> > + if (err)
> > + return err;
> > +
> > + /* Send vendor request - set receive host (enable) */
> > + err = mxuport_send_ctrl_urb(serial->dev, RQ_VENDOR_SET_RX_HOST_EN,
> > + 1, port->port_number);
> > + return err;
> > +}
>
> Shouldn't this be split up over port_probe and open, where you initialise
> (first three requests) at probe, but enable reception (last two
> requests) at open?
Yes, that would make sense.
> > +
> > + /* 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?
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.
> > + /* set port interface */
> > + if ((productid == MX_UPORT1250_PID) ||
> > + (productid == MX_UPORT1251_PID) ||
> > + (productid == MX_UPORT1450_PID) ||
> > + (productid == MX_UPORT1451_PID) ||
> > + (productid == MX_UPORT1658_PID) ||
> > + (productid == MX_UPORT1653_PID)) {
>
> As I mentioned in my review of v1, try to keep product specific details
> in the id_table above by using the driver_info field to store quirks or
> features such as
>
> #define MX_UPORT_QUIRK_RS232_ONLY 0x01
>
> ...
> { USB_DEVICE(MX_USBSERIAL_VID, MX_UPORT1410_PID),
> .driver_info = MX_UPORT_QUIRK_RS232_ONLY },
>
> for the product IDs that only support rs232 mode.
>
> > + if ((new_serial.port >= 0) && (new_serial.port <= 3)) {
> > + if (old_cfg.uart_cfg->interface != new_serial.port) {
> > + mxport->uart_cfg->interface = new_serial.port;
> > + err = mxuport_send_ctrl_urb(
> > + udev,
> > + RQ_VENDOR_SET_INTERFACE,
> > + mxport->uart_cfg->
> > + interface, port->port_number);
> > + if (err)
> > + return err;
> > + }
> > + }
> > + } else {
> > + if (old_cfg.uart_cfg->interface != new_serial.port)
> > + return -EINVAL;
> > + }
>
> Ok, here you're abusing the setserial interface to set the Moxa-specific
> interface-mode (rs232, 2-wire rs485, rs422, 4-wire rs485). We already
> have an ioctl to enable rs-485:
>
> Documentation/serial/serial-rs485.txt
>
> Do you think you could use that interface? Perhaps those ioctls along
> with a custom moxa "2-wire" sysfs flag would suffice to be able to
> select one of the four modes?
Ah, i missed that ioctl. I will take a look.
> > +
> > + /* set alternative baud rate */
> > + if ((old_cfg.flags & ASYNC_SPD_MASK) !=
> > + (mxport->flags & ASYNC_SPD_MASK)) {
> > + if ((mxport->flags & ASYNC_SPD_MASK) == ASYNC_SPD_HI)
> > + tty->alt_speed = 57600;
> > + if ((mxport->flags & ASYNC_SPD_MASK) == ASYNC_SPD_VHI)
> > + tty->alt_speed = 115200;
> > + if ((mxport->flags & ASYNC_SPD_MASK) == ASYNC_SPD_SHI)
> > + tty->alt_speed = 230400;
> > + if ((mxport->flags & ASYNC_SPD_MASK) == ASYNC_SPD_WARP)
> > + tty->alt_speed = 460800;
> > + mxuport_change_port_settings(tty, port, mxport);
> > + }
>
> Do you really want and need this? Especially as you don't implement
> custom divisor support. The above implementation is not correct either
> way as these flags should only take effect when the user requests
> 38.4kb. I'd just drop it, if I where you.
If 115200 works without it, i will drop it.
> > + /*
> > + * 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?
> > + /*
> > + * 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.
> You should make sure to set bulk_in_size to 0 as well (used during
> resume).
>
> Actually, you will have to override the generic resume implementation
> anyway, as the generic one will fail to resubmit the read urbs unless
> port 0 and 1 are open.
O.K. I will add that to my TODO list.
> > +
> > + /* 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.
Thanks
Andrew
--
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
