On Sun, May 26, 2013 at 07:19:03PM +0200, Andrew Lunn wrote:
> On Sun, May 26, 2013 at 06:04:24PM +0200, Johan Hovold wrote:
> > On Sun, May 26, 2013 at 01:00:51PM +0200, Andrew Lunn wrote:
[...]
> > > +/*
> > > + * mxuport_prepare_write_buffer - fill in the buffer, ready for
> > > + * sending
> > > + *
> > > + * Add a four byte header containing the port number and the number of
> > > + * bytes of data in the message. Return the number of bytes in the
> > > + * buffer.
> > > + */
> > > +static int mxuport_prepare_write_buffer(struct usb_serial_port *port,
> > > + void *dest, size_t size)
> > > +{
> > > + struct mxuport_port *mx_port = usb_get_serial_port_data(port);
> > > + unsigned char *buf = dest;
> > > + int count;
> > > +
> > > + count = kfifo_out_locked(&port->write_fifo, buf + 4, size - 4,
> > > + &port->lock);
> > > +
> > > + put_unaligned_be16(mx_port->portno, buf);
> > > + put_unaligned_be16(count, buf + 2);
> > > +
> > > + dev_dbg(&port->dev, "%s - port %d, size %d count %d", __func__,
> > > + mx_port->portno, size, count);
> > > +
> > > + return count + 4;
> > > +}
> > > +
> > > +/*
> > > + * mxuport_write_start - kick off an URB write
> > > + * @port: Pointer to the &struct usb_serial_port data
> > > + *
> > > + * Returns zero on success, or a negative errno value
> > > + */
> > > +static int mxuport_write_start(struct usb_serial_port *port)
> > > +{
> > > + struct usb_serial *serial = port->serial;
> > > + struct urb *urb;
> > > + int count, result;
> > > + unsigned long flags;
> > > + int i;
> > > + struct usb_serial_port *port0;
> > > +
> > > + if (test_and_set_bit_lock(USB_SERIAL_WRITE_BUSY, &port->flags))
> > > + return 0;
> > > +
> > > + /* All data goes over port 0 endpoints */
> > > + port0 = serial->port[0];
> > > +
> > > +retry:
> > > + spin_lock_irqsave(&port->lock, flags);
> > > + if (!port0->write_urbs_free || !kfifo_len(&port->write_fifo)) {
> > > + clear_bit_unlock(USB_SERIAL_WRITE_BUSY, &port->flags);
> > > + spin_unlock_irqrestore(&port->lock, flags);
> > > + return 0;
> > > + }
> > > + i = (int)find_first_bit(&port0->write_urbs_free,
> > > + ARRAY_SIZE(port0->write_urbs));
> > > + spin_unlock_irqrestore(&port->lock, flags);
> > > +
> > > + urb = port0->write_urbs[i];
> > > + count = mxuport_prepare_write_buffer(port, urb->transfer_buffer,
> > > + port0->bulk_out_size);
> > > + urb->transfer_buffer_length = count;
> > > + if (debug)
> > > + usb_serial_debug_data(&port->dev, __func__, count,
> > > + urb->transfer_buffer);
> > > + spin_lock_irqsave(&port->lock, flags);
> > > + port->tx_bytes += count;
> > > + spin_unlock_irqrestore(&port->lock, flags);
> > > +
> > > + clear_bit(i, &port0->write_urbs_free);
> > > +
> > > + /* Set the context to point to the real port, not port 0 */
> > > + urb->context = port;
> > > +
> > > + result = usb_submit_urb(urb, GFP_ATOMIC);
> > > + if (result) {
> > > + dev_err_console(port, "%s - error submitting urb: %d\n",
> > > + __func__, result);
> > > + set_bit(i, &port0->write_urbs_free);
> > > + spin_lock_irqsave(&port->lock, flags);
> > > + port->tx_bytes -= count;
> > > + spin_unlock_irqrestore(&port->lock, flags);
> > > +
> > > + clear_bit_unlock(USB_SERIAL_WRITE_BUSY, &port->flags);
> > > + return result;
> > > + }
> > > +
> > > + /* Try sending off another urb, unless in irq context (in which case
> > > + * there will be no free urb). */
> > > + if (!in_irq())
> > > + goto retry;
> > > +
> > > + clear_bit_unlock(USB_SERIAL_WRITE_BUSY, &port->flags);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/*
> > > + * mxuport_write - write function
> > > + * @tty: Pointer to &struct tty_struct for the device
> > > + * @port: Pointer to the &usb_serial_port structure for the device
> > > + * @buf: Pointer to the data to write
> > > + * @count: Number of bytes to write
> > > + *
> > > + * Returns the number of characters actually written, which may be anything
> > > + * from zero to @count. If an error occurs, it returns the negative errno
> > > + * value.
> > > + */
> > > +static int mxuport_write(struct tty_struct *tty, struct usb_serial_port *port,
> > > + const unsigned char *buf, int count)
> > > +{
> > > + int result;
> > > +
> > > + if (!count)
> > > + return 0;
> > > +
> > > + count = kfifo_in_locked(&port->write_fifo, buf, count, &port->lock);
> > > + result = mxuport_write_start(port);
> > > + if (result)
> > > + return result;
> > > +
> > > + return count;
> > > +}
> > > +
> > > +/*
> > > + * mxuport_write_bulk_callback - A write has completed
> > > + *
> > > + * Now that the write is complete, update the count of transmitted
> > > + * characters and make the URB is available to be reused. Start the
> > > + * next transmission.
> > > + */
> > > +
> > > +static void mxuport_write_bulk_callback(struct urb *urb)
> > > +{
> > > + unsigned long flags;
> > > + struct usb_serial_port *port = urb->context;
> > > + struct usb_serial_port *port0 = port->serial->port[0];
> > > +
> > > + int err = urb->status;
> > > + int i;
> > > +
> > > + dev_dbg(&port->dev, "%s\n", __func__);
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(port0->write_urbs); ++i)
> > > + if (port0->write_urbs[i] == urb)
> > > + break;
> > > +
> > > + spin_lock_irqsave(&port->lock, flags);
> > > + port->tx_bytes -= urb->transfer_buffer_length;
> > > + set_bit(i, &port0->write_urbs_free);
> > > + spin_unlock_irqrestore(&port->lock, flags);
> > > +
> > > + if (err) {
> > > + dev_dbg(&port->dev, "%s - non-zero urb status: %d\n",
> > > + __func__, err);
> > > +
> > > + spin_lock_irqsave(&port->lock, flags);
> > > + kfifo_reset_out(&port->write_fifo);
> > > + spin_unlock_irqrestore(&port->lock, flags);
> > > + } else {
> > > + mxuport_write_start(port);
> > > + }
> > > +
> > > + usb_serial_port_softint(port);
> > > +}
> > > +
> > > +/*
> > > + * mxuport_write_room
> > > + *
> > > + * Return how much space is available in the buffer.
> > > + */
> > > +static int mxuport_write_room(struct tty_struct *tty)
> > > +{
> > > + struct usb_serial_port *port = tty->driver_data;
> > > + unsigned long flags;
> > > + int room;
> > > +
> > > + spin_lock_irqsave(&port->lock, flags);
> > > + room = kfifo_avail(&port->write_fifo);
> > > + spin_unlock_irqrestore(&port->lock, flags);
> > > +
> > > + dev_dbg(&port->dev, "%s - returns %d\n", __func__, room);
> > > + return room;
> > > +}
> > > +
> > > +/*
> > > + * mxuport_chars_in_buffer
> > > + *
> > > + * Return how many charactors are currenty in the write buffer
> > > + */
> > > +static int mxuport_chars_in_buffer(struct tty_struct *tty)
> > > +{
> > > + struct usb_serial_port *port = tty->driver_data;
> > > + unsigned long flags;
> > > + int chars;
> > > +
> > > + spin_lock_irqsave(&port->lock, flags);
> > > + chars = kfifo_len(&port->write_fifo) + port->tx_bytes;
> > > + spin_unlock_irqrestore(&port->lock, flags);
> > > +
> > > + dev_dbg(&port->dev, "%s - returns %d\n", __func__, chars);
> > > + return chars;
> > > +}
> >
> > It seems it could be possible to reuse the generic write implementations
> > of all functions but prepare_write_buffer above rather than the modified
> > copies.
> >
> > When you set up the "fake ports", simply allocate the port write urbs
> > along with the write fifos and initialise them to use the first (and
> > only) bulk-out end point.
>
> I didn't think about initializing them for the first port. Yes, that
> could help. Thanks.
Note that you need to set the port->bulk_out_size as well for the "fake"
ports in order to be able to use the generic implementation. (You must
not set bulk_in_size, however.)
[...]
> > > +/*
> > > + * mxuport_wait_until_sent - wait while transmit buffer empties
> > > + *
> > > + * Parameter timeout is a flag (1 or 0).
> > > + * If timeout = 1, using a best effort transmit approach
> > > + * = 0, wait until firmware transmit buffer really empties
> > > + */
> > > +static void mxuport_wait_until_sent(struct usb_serial_port *port, int timeout)
> >
> > Hmmm. You're defining your own wait_until_sent semantics here. Why no use
> > the generic implementation instead (in v3.10-rc3)? Then all you need to
> > do is implement tx_empty using RQ_VENDOR_GET_OUTQUEUE.
>
> Ah, new functionality. I did the development work with v3.9 and just
> rebased onto v3.10 without looking to see what had changed. I will
> take a look at the commit logs for v3.10.
Yes, brand new. :)
> > The implementation below might never return...
> >
> > > +{
> > > + struct mxuport_port *mx_port = usb_get_serial_port_data(port);
> > > + struct usb_serial *serial = port->serial;
> > > + int err, count;
> > > + int equal_cnt, pass_cnt;
> > > + unsigned long txlen;
> > > + unsigned char len_buf[4];
> >
> > You need to allocate the buffer using kmalloc as some systems can't do
> > DMA from the stack.
>
> Ah, O.K. I developed on ARM, and it was fine. Is it necessary to pass
> GFP_DMA? I don't use any users of this flag in drivers/usb/serial, so
> i guess not.
No that's not needed, as you guessed.
[...]
> > > +/*
> > > + * mxuport_set_termios
> > > + *
> > > + * Change the port configuration based on termios. Check there is
> > > + * something to change, and call the lower level function if there is.
> > > + */
> > > +static void mxuport_set_termios(struct tty_struct *tty,
> > > + struct usb_serial_port *port,
> > > + struct ktermios *old_termios)
> > > +{
> > > + struct mxuport_port *mx_port = usb_get_serial_port_data(port);
> > > + struct ktermios *termios = &tty->termios;
> > > + unsigned int cflag, iflag;
> > > +
> > > + dev_dbg(&port->dev, "%s - port %d", __func__, mx_port->portno);
> > > +
> > > + if (!tty) {
> > > + dev_dbg(&port->dev, "%s - no tty", __func__);
> > > + return;
> > > + }
> > > +
> > > + mxuport_wait_until_sent(port, 0);
> >
> > If you use the generic wait_until_sent implementation (or implement the
> > standard semantics) then this has already been handled by the tty layer.
>
> I need to look deeper, but one issues is the size of the write buffer
> in the device itself. It can be very large so you do need to wait
> around for it to flush stuff out, before making changes. There is a
> microprocessor inside the box and it seems like all the space RAM is
> dedicated to buffers. When using 9600 it can take many multiple of
> seconds to send out what it has buffered.
That's were the new generic wait_until_sent-support come in handy.
Simply implement tx_empty and usb-serial core and the tty layer will
take care of the rest.
> > > +static int mxuport_get_icount(struct tty_struct *tty,
> > > + struct serial_icounter_struct *icount)
> > > +{
> > > + struct usb_serial_port *port = tty->driver_data;
> > > + struct mxuport_port *mx_port = usb_get_serial_port_data(port);
> > > + struct async_icount *ic = &mx_port->icount;
> > > +
> > > + icount->cts = ic->cts;
> > > + icount->dsr = ic->dsr;
> > > + icount->rng = ic->rng;
> > > + icount->dcd = ic->dcd;
> > > + icount->tx = ic->tx;
> > > + icount->rx = ic->rx;
> > > + icount->frame = ic->frame;
> > > + icount->parity = ic->parity;
> > > + icount->overrun = ic->overrun;
> > > + icount->brk = ic->brk;
> > > + icount->buf_overrun = ic->buf_overrun;
> > > + return 0;
> > > +}
> >
> > You should remove this one and reuse the port icount struct and the
> > generic implementation.
>
> Is new in v3.10?
Yes, it's also new.
> > > +
> > > +/*
> > > + * mxuport_ioctl - I/O control function of driver
> > > + *
> > > + * This function handles any ioctl calls to the driver.
> > > + */
> > > +static int mxuport_ioctl(struct tty_struct *tty, unsigned int cmd,
> > > + unsigned long arg)
> > > +{
> > > + struct usb_serial_port *port = tty->driver_data;
> > > + struct usb_serial *serial = port->serial;
> > > + struct mxuport_port *mx_port = usb_get_serial_port_data(port);
> > > + int err;
> > > + struct async_icount cnow;
> > > + struct async_icount cprev;
> > > +
> > > + if (mx_port == NULL)
> > > + return -ENODEV;
> > > +
> > > + dev_dbg(&port->dev, "%s - port %d, cmd = 0x%x", __func__,
> > > + mx_port->portno, cmd);
> > > +
> > > + switch (cmd) {
> > > +
> > > + case TIOCMIWAIT:
> > > + dev_dbg(&port->dev, "%s - port %d : TIOCMIWAIT", __func__,
> > > + mx_port->portno);
> > > +
> > > + cprev = mx_port->icount;
> > > + atomic_set(&mx_port->wait_msr, 1);
> > > +
> > > + wait_event_interruptible_timeout(mx_port->delta_msr_wait,
> > > + (atomic_read
> > > + (&mx_port->wait_msr) == 0),
> > > + WAIT_MSR_TIMEOUT);
> > > +
> > > + /* if no change, return error */
> > > + cnow = mx_port->icount;
> > > + if (cnow.rng == cprev.rng && cnow.dsr == cprev.dsr &&
> > > + cnow.dcd == cprev.dcd && cnow.cts == cprev.cts)
> > > + return -EIO;
> > > +
> > > + /* status change, return 0 */
> > > + if (((arg & TIOCM_RNG) && (cnow.rng != cprev.rng)) ||
> > > + ((arg & TIOCM_DSR) && (cnow.dsr != cprev.dsr)) ||
> > > + ((arg & TIOCM_CD) && (cnow.dcd != cprev.dcd)) ||
> > > + ((arg & TIOCM_CTS) && (cnow.cts != cprev.cts))) {
> > > + return 0;
> > > + }
> > > + break;
> >
> > You should remove this one as well and reuse the generic implementation.
> > Simply use the port counters and wait queue, instead.
>
> Will do.
>
> >
> > > + case TCFLSH:
> > > + switch (arg) {
> > > + case TCIFLUSH:
> > > + err = mxuport_send_ctrl_urb(serial->dev,
> > > + RQ_VENDOR_PURGE,
> > > + 0x8, mx_port->portno);
> > > + if (err) {
> > > + dev_dbg(&port->dev,
> > > + "%s - fail to flush port %d input buffer",
> > > + __func__, mx_port->portno);
> > > + return err;
> > > + }
> > > + break;
> > > +
> > > + case TCOFLUSH:
> > > + if (tty->driver->ops->flush_buffer)
> > > + tty->driver->ops->flush_buffer(tty);
> > > + err = mxuport_send_ctrl_urb(serial->dev,
> > > + RQ_VENDOR_PURGE,
> > > + 0x4, mx_port->portno);
> > > + if (err) {
> > > + dev_dbg(&port->dev,
> > > + "%s - fail to flush port %d output buffer",
> > > + __func__, mx_port->portno);
> > > + return err;
> > > + }
> > > + break;
> > > +
> > > + case TCIOFLUSH:
> > > + err = mxuport_send_ctrl_urb(serial->dev,
> > > + RQ_VENDOR_PURGE,
> > > + 0xC, mx_port->portno);
> > > + if (err) {
> > > + dev_dbg(&port->dev,
> > > + "%s - fail to flush port %d input/output buffer",
> > > + __func__, mx_port->portno);
> > > + return err;
> > > + }
> > > + break;
> > > + }
> > > + break;
> >
> > You probably just let the line discipline handle TCFLSH. If you need the
> > hardware output buffer to be flushed we can add the flush_buffer operation
> > to usb-serial.
>
> The output buffer can be large, so flushing it probably is needed.
Ok, I'll add flush_buffer to usb-serial and look at how best to handle
this then.
> > > + err = request_firmware(&fw_p, buf, &udev->dev);
> > > + if (err) {
> > > + dev_warn(&udev->dev, "Firmware %s not found\n", buf);
> > > + /* Use the firmware already in the device */
> > > + return 0;
> > > + } else {
> > > + local_ver = (fw_p->data[VER_ADDR_1] * 65536 +
> > > + fw_p->data[VER_ADDR_2] * 256 +
> > > + fw_p->data[VER_ADDR_3]);
> >
> > Use bitshift and or?
>
> Won't the compiler do that anyway?
Sure, I just thought it would be more clear.
> >
> > > + dev_dbg(&udev->dev, "Available firmware version v%x.%x.%x\n",
> > > + fw_p->data[VER_ADDR_1], fw_p->data[VER_ADDR_2],
> > > + fw_p->data[VER_ADDR_3]);
> > > + if (local_ver <= dev_ver) {
> > > + release_firmware(fw_p);
> > > + return 0;
> > > + }
> > > + }
> >
> > How about splitting the actual firmware download below into a separate
> > function?
>
> Can do. It is clear more than a vt100 screenfull.
>
> > > + err = mxuport_send_ctrl_urb(udev, RQ_VENDOR_QUERY_FW_READY, 0, 0);
> > > + if (err)
> > > + goto out;
> > > +
> > > + dev_dbg(&udev->dev, "Download firmware completely.");
> > > +
> > > + err = mxuport_recv_ctrl_urb(udev, RQ_VENDOR_GET_VERSION, 0, 0,
> > > + ver_buf, sizeof(ver_buf));
> > > + if (err < 0)
> > > + dev_dbg(&udev->dev,
> > > + "%s - receive control URB failed. (err = %d)",
> > > + __func__, err);
> > > + err = 0;
> > > +
> > > + dev_ver = ver_buf[0] * 65536 + ver_buf[1] * 256 + ver_buf[2];
> > > +
> > > + dev_dbg(&udev->dev, "Installed device firmware version v%x.%x.%x\n",
> > > + ver_buf[0], ver_buf[1], ver_buf[2]);
> >
> > dev_info?
>
> I can do. But i noticed dev_info is not used very much in
> drivers/usb/serial so i avoided it. Also, 'Installed' is maybe the
> wrong word. Downloaded might be better, since it goes into RAM and
> will be lost on power cycle.
You're right, "using" is perhaps a better choice? You can decide whether
you want to use dev_dbg or dev_info, but printing a firmware version
once at probe would be ok if it's useful for anyone.
Perhaps you should merge it with the "available" version debug above as
well, so that regardless of whether any new firmware is actually
downloaded, the used version is printed once, and once only, at probe.
[...]
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
