| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20150926201603.GK2286@localhost>
Date: Sat, 26 Sep 2015 13:16:03 -0700
From: Johan Hovold <johan@...nel.org>
To: Mathieu OTHACEHE <m.othacehe@...il.com>
Cc: johan@...nel.org, gregkh@...uxfoundation.org,
linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH v2] USB: serial: add Moxa UPORT 11x0 driver
On Tue, Sep 22, 2015 at 03:08:29PM +0200, Mathieu OTHACEHE wrote:
> Add a driver which supports :
>
> - UPort 1110 : 1 port RS-232 USB to Serial Hub.
> - UPort 1130 : 1 port RS-422/485 USB to Serial Hub.
> - UPort 1130I : 1 port RS-422/485 USB to Serial Hub with Isolation.
> - UPort 1150 : 1 port RS-232/422/485 USB to Serial Hub.
> - UPort 1150I : 1 port RS-232/422/485 USB to Serial Hub with Isolation.
>
> This driver is based on GPL MOXA driver written by Hen Huang and available
> on MOXA website. The original driver was based on io_ti serial driver.
>
> Signed-off-by: Mathieu OTHACEHE <m.othacehe@...il.com>
<snip>
> +/* Operation modes */
> +#define MXU1_UART_232 0x00
> +#define MXU1_UART_485_RECEIVER_DISABLED 0x01
> +#define MXU1_UART_485_RECEIVER_ENABLED 0x02
> +#define MXU1_TYPE_RS232 (1 << 0)
> +#define MXU1_TYPE_RS422 (1 << 1)
> +#define MXU1_TYPE_RS485 (1 << 2)
You can use BIT(n) for these.
> +/* Pipe transfer mode and timeout */
> +#define MXU1_PIPE_MODE_CONTINUOUS 0x01
> +#define MXU1_PIPE_MODE_MASK 0x03
> +#define MXU1_PIPE_TIMEOUT_MASK 0x7C
> +#define MXU1_PIPE_TIMEOUT_ENABLE 0x80
> +
> +/* Config struct */
> +struct mxu1_uart_config {
> + __be16 wBaudRate;
> + __be16 wFlags;
> + u8 bDataBits;
> + u8 bParity;
> + u8 bStopBits;
> + char cXon;
> + char cXoff;
> + u8 bUartMode;
> +} __packed;
> +
> +/* Purge modes */
> +#define MXU1_PURGE_OUTPUT 0x00
> +#define MXU1_PURGE_INPUT 0x80
> +
> +/* Read/Write data */
> +#define MXU1_RW_DATA_ADDR_SFR 0x10
> +#define MXU1_RW_DATA_ADDR_IDATA 0x20
> +#define MXU1_RW_DATA_ADDR_XDATA 0x30
> +#define MXU1_RW_DATA_ADDR_CODE 0x40
> +#define MXU1_RW_DATA_ADDR_GPIO 0x50
> +#define MXU1_RW_DATA_ADDR_I2C 0x60
> +#define MXU1_RW_DATA_ADDR_FLASH 0x70
> +#define MXU1_RW_DATA_ADDR_DSP 0x80
> +
> +#define MXU1_RW_DATA_UNSPECIFIED 0x00
> +#define MXU1_RW_DATA_BYTE 0x01
> +#define MXU1_RW_DATA_WORD 0x02
> +#define MXU1_RW_DATA_DOUBLE_WORD 0x04
> +
> +struct mxu1_write_data_bytes {
> + u8 bAddrType;
> + u8 bDataType;
> + u8 bDataCounter;
> + __be16 wBaseAddrHi;
> + __be16 wBaseAddrLo;
> + u8 bData[0];
> +} __packed;
> +
> +/* Interrupt codes */
> +#define MXU1_GET_PORT_FROM_CODE(c) (((c) >> 4) - 3)
> +#define MXU1_GET_FUNC_FROM_CODE(c) ((c) & 0x0f)
> +#define MXU1_CODE_HARDWARE_ERROR 0xFF
> +#define MXU1_CODE_DATA_ERROR 0x03
> +#define MXU1_CODE_MODEM_STATUS 0x04
> +
> +/* Download firmware max packet size */
> +#define MXU1_DOWNLOAD_MAX_PACKET_SIZE 64
> +
> +/* Firmware image header */
> +struct mxu1_firmware_header {
> + __le16 wLength;
> + u8 bCheckSum;
> +} __packed;
> +
> +/* UART addresses */
> +/* UART 1 base address */
> +#define MXU1_UART1_BASE_ADDR 0xFFA0
> +/* UART 2 base address*/
> +#define MXU1_UART2_BASE_ADDR 0xFFB0
> +#define MXU1_UART_OFFSET_LCR 0x0002
Why are these unused defines here? This driver is for one-port devices, right?
> +/*UART MCR register offset */
> +#define MXU1_UART_OFFSET_MCR 0x0004
> +
> +#define MXU1_SET_SERIAL_FLAGS 0
> +
> +#define MXU1_TRANSFER_TIMEOUT 2
> +#define MXU1_MSR_WAIT_TIMEOUT (5 * HZ)
This one does not seem to be used either.
> +
> +/* Configuration ids */
> +#define MXU1_BOOT_CONFIG 1
> +#define MXU1_ACTIVE_CONFIG 2
Nor are these. Please drop unused defines unless useful for future feature
additions (e.g. register definitions).
> +
> +#define MXU1_DEFAULT_CLOSING_WAIT 4000 /* in .01 secs */
> +
> +struct mxu1_port {
> + u8 mxp_msr;
> + u8 mxp_lsr;
> + u8 mxp_shadow_mcr;
> + u8 mxp_uart_types;
> + u8 mxp_uart_mode;
> + unsigned int mxp_uart_base_addr;
u32 or u16?
> + int mxp_flags;
> + int mxp_msr_wait_flags;
> + wait_queue_head_t mxp_msr_wait; /* wait for msr change */
> + struct mxu1_device *mxp_mxdev;
> + struct usb_serial_port *mxp_port;
> + spinlock_t mxp_lock;
> + int mxp_send_break;
> + int mxp_set_B0;
> +};
> +
> +struct mxu1_device {
> + struct mutex mxd_lock;
> + struct usb_serial *mxd_serial;
> + u16 mxd_model;
> +};
> +
> +static const struct usb_device_id mxuport11_idtable[] = {
> + { USB_DEVICE(MXU1_VENDOR_ID, MXU1_1110_PRODUCT_ID) },
> + { USB_DEVICE(MXU1_VENDOR_ID, MXU1_1130_PRODUCT_ID) },
> + { USB_DEVICE(MXU1_VENDOR_ID, MXU1_1150_PRODUCT_ID) },
> + { USB_DEVICE(MXU1_VENDOR_ID, MXU1_1151_PRODUCT_ID) },
> + { USB_DEVICE(MXU1_VENDOR_ID, MXU1_1131_PRODUCT_ID) },
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(usb, mxuport11_idtable);
> +
> +/* Write the given buffer out to the control pipe. */
> +static int mxu1_send_ctrl_data_urb(struct usb_serial *serial,
> + u8 request,
> + u16 value, u16 index,
> + u8 *data, size_t size)
> +{
> + int status;
> +
> + status = usb_control_msg(serial->dev,
> + usb_sndctrlpipe(serial->dev, 0),
> + request,
> + (USB_DIR_OUT | USB_TYPE_VENDOR |
> + USB_RECIP_DEVICE), value, index,
> + data, size,
> + 1000);
Please use a define for the timeout.
> + if (status < 0) {
> + dev_err(&serial->interface->dev,
> + "%s - usb_control_msg failed (%d)\n",
> + __func__, status);
> + return status;
> + }
> +
> + if (status != size) {
> + dev_err(&serial->interface->dev,
> + "%s - short write (%d / %zd)\n",
> + __func__, status, size);
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> +/* Send a vendor request without any data */
> +static int mxu1_send_ctrl_urb(struct usb_serial *serial,
> + u8 request, u16 value, u16 index)
> +{
> + return mxu1_send_ctrl_data_urb(serial, request, value, index,
> + NULL, 0);
> +}
> +
> +static int mxu1_download_firmware(struct usb_serial *serial,
> + const struct firmware *fw_p)
> +{
> + int status = 0;
> + int buffer_size;
> + int pos;
> + int len;
> + int done;
> + u8 cs = 0;
> + u8 *buffer;
> + struct usb_device *dev = serial->dev;
> + struct mxu1_firmware_header *header;
> + unsigned int pipe = usb_sndbulkpipe(dev, serial->port[0]->
> + bulk_out_endpointAddress);
Separate declaration and initialisation here.
> +
> + buffer_size = fw_p->size +
> + sizeof(struct mxu1_firmware_header);
Indent continuation lines at least two tabs further, but this line does
appear to need to be broken at all.
> + buffer = kmalloc(buffer_size, GFP_KERNEL);
> + if (!buffer)
> + return -ENOMEM;
> +
> + memcpy(buffer, fw_p->data, fw_p->size);
> + memset(buffer + fw_p->size, 0xff, buffer_size - fw_p->size);
> +
> + for (pos = sizeof(struct mxu1_firmware_header);
sizeof(*header) to avoid having to break the line?
> + pos < buffer_size; pos++)
> + cs = (u8)(cs + buffer[pos]);
Add brackets unless you can avoid the line break above.
> +
> + header = (struct mxu1_firmware_header *)buffer;
> + header->wLength = cpu_to_le16(
> + (__u16)(buffer_size - sizeof(struct mxu1_firmware_header)));
No need for cast.
Use sizeof(*header).
> + header->bCheckSum = cs;
> +
> + dev_dbg(&dev->dev, "%s - downloading firmware", __func__);
> +
> + for (pos = 0; pos < buffer_size; pos += done) {
> + len = min(buffer_size - pos, MXU1_DOWNLOAD_MAX_PACKET_SIZE);
> +
> + status = usb_bulk_msg(dev, pipe, buffer+pos, len, &done, 1000);
Spaces around + operator.
Use a define for the timeout.
> +
Remove blank line.
> + if (status)
> + break;
> + }
> +
> + kfree(buffer);
> +
> + if (status) {
> + dev_err(&dev->dev, "%s - error downloading firmware, %d\n",
> + __func__, status);
No need for __func__ when you have a self-contained error message
(which is preferred). Debug messages can be more brief and rely on
__func__.
If possible try to use a consistent style for reporting errnos (e.g.
"failed to download firmware: %d").
> + return status;
> + }
> +
> + msleep_interruptible(100);
> +
> + status = mxu1_send_ctrl_urb(serial, MXU1_RESET_EXT_DEVICE, 0, 0);
> +
> + dev_dbg(&dev->dev, "%s - download successful (%d)", __func__, status);
> +
> + return 0;
> +}
> +
> +static int mxu1_port_probe(struct usb_serial_port *port)
> +{
> + struct mxu1_port *mxport;
> +
> + mxport = kzalloc(sizeof(struct mxu1_port), GFP_KERNEL);
> +
Don't add newline before checking return values.
> + if (!mxport)
> + return -ENOMEM;
> +
> + spin_lock_init(&mxport->mxp_lock);
> + mxport->mxp_port = port;
> + mxport->mxp_mxdev = usb_get_serial_data(port->serial);
No need to store the serial data pointer in the port data, you can
always access it through port->serial.
> + mxport->mxp_uart_base_addr = MXU1_UART1_BASE_ADDR;
I already asked above, but when is any other base address ever used?
> +
> + init_waitqueue_head(&mxport->mxp_msr_wait);
Use the port msr wait queue instead.
> +
> + switch (mxport->mxp_mxdev->mxd_model) {
> + case MXU1_1110_PRODUCT_ID:
> + mxport->mxp_uart_types = MXU1_TYPE_RS232;
> + mxport->mxp_uart_mode = MXU1_UART_232;
> + break;
> + case MXU1_1130_PRODUCT_ID:
> + case MXU1_1131_PRODUCT_ID:
> + mxport->mxp_uart_types = MXU1_TYPE_RS422 | MXU1_TYPE_RS485;
> + mxport->mxp_uart_mode = MXU1_UART_485_RECEIVER_DISABLED;
> + break;
> + case MXU1_1150_PRODUCT_ID:
> + case MXU1_1151_PRODUCT_ID:
> + mxport->mxp_uart_types =
> + MXU1_TYPE_RS232 | MXU1_TYPE_RS422 | MXU1_TYPE_RS485;
> + mxport->mxp_uart_mode = MXU1_UART_232;
> + break;
> + }
> +
> + usb_set_serial_port_data(port, mxport);
> +
> + port->port.closing_wait =
> + msecs_to_jiffies(MXU1_DEFAULT_CLOSING_WAIT * 10);
Indent further.
> + port->port.drain_delay = 1;
> +
> + return 0;
> +}
> +
> +static int mxu1_startup(struct usb_serial *serial)
> +{
> + struct mxu1_device *mxdev;
> + struct usb_device *dev = serial->dev;
> + char fw_name[32];
> + const struct firmware *fw_p = NULL;
> + int err;
> +
> + dev_dbg(&dev->dev, "%s - product 0x%4X, num configurations %d, configuration value %d",
> + __func__, le16_to_cpu(dev->descriptor.idProduct),
> + dev->descriptor.bNumConfigurations,
> + dev->actconfig->desc.bConfigurationValue);
> +
> + /* create device structure */
> + mxdev = kzalloc(sizeof(struct mxu1_device), GFP_KERNEL);
> + if (mxdev == NULL)
> + return -ENOMEM;
> +
> + mutex_init(&mxdev->mxd_lock);
> + mxdev->mxd_serial = serial;
> + usb_set_serial_data(serial, mxdev);
> +
> + mxdev->mxd_model = le16_to_cpu(dev->descriptor.idProduct);
> +
> + /* if we have only 1 configuration, download firmware */
> + if (dev->config->interface[0]->cur_altsetting->
> + desc.bNumEndpoints == 1) {
Use an intermediate variable for the interface descriptors, which are
accessible through serial->interface->cur_altsetting, to avoid line
break.
> +
> + snprintf(fw_name,
> + sizeof(fw_name) - 1,
No need for -1 as size includes trailing NUL.
> + "moxa/moxa-%04x.fw",
> + mxdev->mxd_model);
> +
> + err = request_firmware(&fw_p, fw_name, &serial->interface->dev);
> + if (err) {
> + dev_err(&serial->interface->dev, "Firmware %s not found\n",
> + fw_name);
> + kfree(mxdev);
> + return err;
> + }
> +
> + err = mxu1_download_firmware(serial, fw_p);
> + if (err) {
> + kfree(mxdev);
> + return err;
> + }
> +
> + }
> +
> + if (fw_p)
> + release_firmware(fw_p);
Move this into the conditional block above and remove the NULL-test.
> +
> + return 0;
> +}
> +
> +static int mxu1_write_byte(struct mxu1_device *mxdev, unsigned long addr,
> + u8 mask, u8 byte)
Pass the usb_serial_port as first parameter for port operations (and use
it for any error or debug messages).
u32 for address?
> +{
> + int status = 0;
> + unsigned int size;
size_t
> + struct mxu1_write_data_bytes *data;
> + struct device *dev = &mxdev->mxd_serial->dev->dev;
> +
> + dev_dbg(dev, "%s - addr 0x%08lX, mask 0x%02X, byte 0x%02X", __func__,
> + addr, mask, byte);
> +
> + size = sizeof(struct mxu1_write_data_bytes) + 2;
> + data = kmalloc(size, GFP_KERNEL);
kzalloc to clear the pad (?) bytes?
> + if (!data) {
> + dev_err(dev, "%s - out of memory\n", __func__);
OOM errors would already have been logged so just return -ENOMEM here.
> + return -ENOMEM;
> + }
> +
> + data->bAddrType = MXU1_RW_DATA_ADDR_XDATA;
> + data->bDataType = MXU1_RW_DATA_BYTE;
> + data->bDataCounter = 1;
> + data->wBaseAddrHi = cpu_to_be16(addr>>16);
Spaces around binary operator missing.
> + data->wBaseAddrLo = cpu_to_be16(addr);
> + data->bData[0] = mask;
> + data->bData[1] = byte;
> +
> + status = mxu1_send_ctrl_data_urb(mxdev->mxd_serial, MXU1_WRITE_DATA, 0,
> + MXU1_RAM_PORT,
> + (u8 *)data,
> + size);
> +
Stray new line again.
> + if (status < 0)
> + dev_err(dev, "%s - failed, %d\n", __func__, status);
> +
> + kfree(data);
> +
> + return status;
> +}
> +
> +static int mxu1_set_mcr(struct mxu1_port *mxport, unsigned int mcr)
So pass usb-serial struct here and elsewhere.
> +{
> + int status = 0;
No need to initialise.
> + unsigned long flags;
> +
> + status = mxu1_write_byte(mxport->mxp_mxdev,
> + mxport->mxp_uart_base_addr +
> + MXU1_UART_OFFSET_MCR,
> + MXU1_MCR_RTS | MXU1_MCR_DTR | MXU1_MCR_LOOP,
> + mcr);
> +
> + spin_lock_irqsave(&mxport->mxp_lock, flags);
> + if (!status)
> + mxport->mxp_shadow_mcr = mcr;
> + spin_unlock_irqrestore(&mxport->mxp_lock, flags);
Use a mutex to protect shadow_mcr so that you can to this atomically.
> +
> + return status;
> +}
> +
> +static void mxu1_set_termios(struct tty_struct *tty1,
> + struct usb_serial_port *port,
> + struct ktermios *old_termios)
> +{
> + struct mxu1_port *mxport = usb_get_serial_port_data(port);
> +
> + struct tty_struct *tty = port->port.tty;
You cannot access the tty like this. Use the passed first argument
instead.
> +
Remove stray newline above.
> + struct mxu1_uart_config *config;
> + tcflag_t cflag, iflag;
> + int baud;
speed_t
> + int status = 0;
> + int port_number = port->port_number - port->minor;
Again, why do you need this as these are all one-port devices?
> + unsigned int mcr;
> +
> + dev_dbg(&port->dev, "%s - port %d", __func__, port->port_number);
> +
> + if (!tty) {
> + dev_dbg(&port->dev, "%s - no tty or termios", __func__);
A lot of debug messages lack a terminating \n. Fix throughout.
> + return;
> + }
> +
> + cflag = tty->termios.c_cflag;
> + iflag = tty->termios.c_iflag;
> +
> + if (old_termios && cflag == old_termios->c_cflag
> + && iflag == old_termios->c_iflag) {
Use tty_termios_hw_change() if appropriate.
Place && before line break.
> + dev_dbg(&port->dev, "%s - nothing to change", __func__);
> + return;
> + }
> +
> + dev_dbg(&port->dev,
> + "%s - clfag %08x, iflag %08x", __func__, cflag, iflag);
> +
> + if (old_termios)
> + dev_dbg(&port->dev, "%s - old clfag %08x, old iflag %08x",
> + __func__,
> + old_termios->c_cflag,
> + old_termios->c_iflag);
Add brackets.
> +
> + if (mxport == NULL)
> + return;
Not needed.
> +
> + config = kmalloc(sizeof(*config), GFP_KERNEL);
Use kzalloc.
> + if (!config)
> + return;
> +
> + config->wFlags = 0;
> +
> + /* these flags must be set */
> + config->wFlags |= MXU1_UART_ENABLE_MS_INTS;
> + config->wFlags |= MXU1_UART_ENABLE_AUTO_START_DMA;
> + if (mxport->mxp_send_break == MXU1_LCR_BREAK)
> + config->wFlags |= MXU1_UART_SEND_BREAK_SIGNAL;
> + config->bUartMode = (u8)(mxport->mxp_uart_mode);
> +
> + switch (cflag & CSIZE) {
> + case CS5:
> + config->bDataBits = MXU1_UART_5_DATA_BITS;
> + break;
> + case CS6:
> + config->bDataBits = MXU1_UART_6_DATA_BITS;
> + break;
> + case CS7:
> + config->bDataBits = MXU1_UART_7_DATA_BITS;
> + break;
> + default:
> + case CS8:
> + config->bDataBits = MXU1_UART_8_DATA_BITS;
> + break;
> + }
> +
> + if (cflag & PARENB) {
> + if (cflag & PARODD) {
> + config->wFlags |= MXU1_UART_ENABLE_PARITY_CHECKING;
Move out of inner conditional.
> + config->bParity = MXU1_UART_ODD_PARITY;
> + } else {
> + config->wFlags |= MXU1_UART_ENABLE_PARITY_CHECKING;
> + config->bParity = MXU1_UART_EVEN_PARITY;
> + }
> + } else {
> + config->wFlags &= ~MXU1_UART_ENABLE_PARITY_CHECKING;
You never set it.
> + config->bParity = MXU1_UART_NO_PARITY;
> + }
You should also clear CMSPAR in the termios structure if you do not
support it.
> +
> + if (cflag & CSTOPB)
> + config->bStopBits = MXU1_UART_2_STOP_BITS;
> + else
> + config->bStopBits = MXU1_UART_1_STOP_BITS;
> +
> + if (cflag & CRTSCTS) {
> + /* RTS flow control must be off to drop RTS for baud rate B0 */
> + if ((cflag & CBAUD) != B0)
> + config->wFlags |= MXU1_UART_ENABLE_RTS_IN;
> + config->wFlags |= MXU1_UART_ENABLE_CTS_OUT;
> + } else {
> + tty->hw_stopped = 0;
No need to update this.
> + }
> +
> + if (I_IXOFF(tty) || I_IXON(tty)) {
> + config->cXon = START_CHAR(tty);
> + config->cXoff = STOP_CHAR(tty);
> +
> + if (I_IXOFF(tty))
> + config->wFlags |= MXU1_UART_ENABLE_X_IN;
> +
> + if (I_IXON(tty))
> + config->wFlags |= MXU1_UART_ENABLE_X_OUT;
> + }
> +
> + baud = tty_get_baud_rate(tty);
> + if (!baud)
> + baud = 9600;
> + config->wBaudRate = (__u16)(923077 / baud);
No need for cast. Missing cpu_to_be16 (store in temporary baud and flags
variables if needed).
Use a define for the baud base.
> +
> + dev_dbg(&port->dev, "%s - BaudRate=%d, wBaudRate=%d, wFlags=0x%04X, bDataBits=%d, bParity=%d, bStopBits=%d, cXon=%d, cXoff=%d, bUartMode=%d",
> + __func__, baud, config->wBaudRate, config->wFlags,
> + config->bDataBits, config->bParity, config->bStopBits,
> + config->cXon, config->cXoff, config->bUartMode);
> +
> + cpu_to_be16s(&config->wBaudRate);
> + cpu_to_be16s(&config->wFlags);
> +
> + status = mxu1_send_ctrl_data_urb(port->serial, MXU1_SET_CONFIG, 0,
> + (u8)(MXU1_UART1_PORT + port_number),
port number again?
> + (u8 *)config,
> + sizeof(*config));
> + if (status)
> + dev_err(&port->dev, "%s - cannot set config on port %d, %d\n",
> + __func__,
> + port_number,
> + status);
Use brackets around multi-line expressions for readability.
> +
> + /* SET_CONFIG asserts RTS and DTR, reset them correctly */
> + mcr = mxport->mxp_shadow_mcr;
Locking?
> + /* if baud rate is B0, clear RTS and DTR */
> + if ((cflag & CBAUD) == B0) {
> +
> + mcr &= ~(MXU1_MCR_DTR | MXU1_MCR_RTS);
> + mxport->mxp_set_B0 = true;
Why do you need this?
You should also disable automatic flow control if enabled.
> + } else {
> + if (mxport->mxp_set_B0)
> + mcr |= MXU1_MCR_DTR;
> +
> + mxport->mxp_set_B0 = false;
Raise DTR/RTS, if changing from B0 (check old termios).
> + }
> +
> + status = mxu1_set_mcr(mxport, mcr);
> +
Stray newline.
> + if (status)
> + dev_err(&port->dev,
> + "%s - cannot set modem control on port %d, %d\n",
> + __func__,
> + port_number,
> + status);
Brackets.
> +
> + kfree(config);
> +}
> +
> +static int mxu1_ioctl_get_rs485(struct mxu1_port *mxport,
> + struct serial_rs485 __user *rs485) {
Bracket on new line (below as well).
> + struct serial_rs485 aux;
> +
> + memset(&aux, 0, sizeof(aux));
> +
> + if (mxport->mxp_uart_mode == MXU1_UART_485_RECEIVER_ENABLED)
> + aux.flags = SER_RS485_ENABLED;
> +
> + if (copy_to_user(rs485, &aux, sizeof(aux)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +static int mxu1_ioctl_set_rs485(struct mxu1_port *mxport,
> + struct serial_rs485 __user *rs485_user) {
> + struct serial_rs485 rs485;
> + struct usb_serial_port *port = mxport->mxp_port;
> +
> + if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user)))
> + return -EFAULT;
> +
> + if (mxport->mxp_uart_types &
> + (MXU1_TYPE_RS422 | MXU1_TYPE_RS485)) {
Don't break line.
> +
> + if (rs485.flags & SER_RS485_ENABLED) {
> + mxport->mxp_uart_mode = MXU1_UART_485_RECEIVER_ENABLED;
> + } else {
> + if (mxport->mxp_uart_types & MXU1_TYPE_RS232)
> + mxport->mxp_uart_mode = MXU1_UART_232;
> + else
> + mxport->mxp_uart_mode = MXU1_UART_485_RECEIVER_DISABLED;
Indent using tabs, not spaces.
> + }
> + } else {
> + dev_err(&port->dev, "%s not handled by MOXA UPort %04x\n",
> + __func__, mxport->mxp_mxdev->mxd_model);
> + return -EINVAL;
> + }
> +
> + mxu1_set_termios(NULL, mxport->mxp_port, NULL);
Why do you need to use set_termios for this update? Why not a dedicated
helper?
> +
> + return 0;
> +}
> +
> +static int mxu1_get_serial_info(struct mxu1_port *mxport,
> + struct serial_struct __user *ret_arg)
> +{
> + struct usb_serial_port *port = mxport->mxp_port;
> + struct serial_struct ret_serial;
> + unsigned cwait;
> +
> + if (!ret_arg)
> + return -EFAULT;
> +
> + cwait = port->port.closing_wait;
> + if (cwait != ASYNC_CLOSING_WAIT_NONE)
> + cwait = jiffies_to_msecs(cwait) / 10;
> +
> + memset(&ret_serial, 0, sizeof(ret_serial));
> +
> + ret_serial.type = PORT_16550A;
> + ret_serial.line = port->minor;
> + ret_serial.port = port->port_number;
> + ret_serial.flags = mxport->mxp_flags;
> + ret_serial.xmit_fifo_size = port->bulk_out_size;
> + ret_serial.baud_base = tty_get_baud_rate(mxport->mxp_port->port.tty);
This is the baud base, not the current the baudrate.
> + ret_serial.close_delay = 5*HZ;
> + ret_serial.closing_wait = cwait;
> +
> + if (copy_to_user(ret_arg, &ret_serial, sizeof(*ret_arg)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +
> +static int mxu1_set_serial_info(struct mxu1_port *mxport,
> + struct serial_struct __user *new_arg)
> +{
> + struct serial_struct new_serial;
> + unsigned cwait;
> +
> + if (copy_from_user(&new_serial, new_arg, sizeof(new_serial)))
> + return -EFAULT;
> +
> + cwait = new_serial.closing_wait;
> + if (cwait != ASYNC_CLOSING_WAIT_NONE)
> + cwait = msecs_to_jiffies(10 * new_serial.closing_wait);
> +
> + mxport->mxp_flags = new_serial.flags & MXU1_SET_SERIAL_FLAGS;
You're not using the flags so drop them (the define above is also 0).
> + mxport->mxp_port->port.closing_wait = cwait;
> +
> + return 0;
> +}
> +
> +static int mxu1_ioctl(struct tty_struct *tty,
> + unsigned int cmd, unsigned long arg)
> +{
> + struct usb_serial_port *port = tty->driver_data;
> +
Stray new line (just fix throughout).
> + struct mxu1_port *mxport = usb_get_serial_port_data(port);
> +
> + if (mxport == NULL)
> + return -ENODEV;
Not needed.
> +
> + switch (cmd) {
> + case TIOCGSERIAL:
> + return mxu1_get_serial_info(mxport,
> + (struct serial_struct __user *)arg);
> +
> + case TIOCSSERIAL:
> + return mxu1_set_serial_info(mxport,
> + (struct serial_struct __user *)arg);
> +
> + case TIOCGRS485:
> + return mxu1_ioctl_get_rs485(mxport,
> + (struct serial_rs485 __user *)
> + arg);
Don't break line before arg.
> + case TIOCSRS485:
> + return mxu1_ioctl_set_rs485(mxport,
> + (struct serial_rs485 __user *)
> + arg);
Ditto.
> + }
> +
> + return -ENOIOCTLCMD;
> +}
> +
> +static int mxu1_tiocmget(struct tty_struct *tty)
> +{
> + struct usb_serial_port *port = tty->driver_data;
> +
> + struct mxu1_port *mxport = usb_get_serial_port_data(port);
> + unsigned int result;
> + unsigned int msr;
> + unsigned int mcr;
> + unsigned long flags;
> +
> + dev_dbg(&port->dev, "%s - port %d", __func__, port->port_number);
> +
> + if (mxport == NULL)
> + return -ENODEV;
Not needed. Fix throughout.
> +
> + spin_lock_irqsave(&mxport->mxp_lock, flags);
> + msr = mxport->mxp_msr;
> + mcr = mxport->mxp_shadow_mcr;
> + spin_unlock_irqrestore(&mxport->mxp_lock, flags);
> +
> + result = ((mcr & MXU1_MCR_DTR) ? TIOCM_DTR : 0)
> + | ((mcr & MXU1_MCR_RTS) ? TIOCM_RTS : 0)
> + | ((mcr & MXU1_MCR_LOOP) ? TIOCM_LOOP : 0)
> + | ((msr & MXU1_MSR_CTS) ? TIOCM_CTS : 0)
> + | ((msr & MXU1_MSR_CD) ? TIOCM_CAR : 0)
> + | ((msr & MXU1_MSR_RI) ? TIOCM_RI : 0)
> + | ((msr & MXU1_MSR_DSR) ? TIOCM_DSR : 0);
But boolean operators before line break and indent further.
> +
> + dev_dbg(&port->dev, "%s - 0x%04X", __func__, result);
> +
> + return result;
> +}
> +
> +static int mxu1_tiocmset(struct tty_struct *tty,
> + unsigned int set, unsigned int clear)
> +{
> + struct usb_serial_port *port = tty->driver_data;
> +
> + struct mxu1_port *mxport = usb_get_serial_port_data(port);
> + unsigned int mcr;
> + unsigned long flags;
> +
> + dev_dbg(&port->dev, "%s - port %d", __func__, port->port_number);
> +
> + if (mxport == NULL)
> + return -ENODEV;
> +
> + spin_lock_irqsave(&mxport->mxp_lock, flags);
> + mcr = mxport->mxp_shadow_mcr;
> +
> + if (set & TIOCM_RTS)
> + mcr |= MXU1_MCR_RTS;
> + if (set & TIOCM_DTR)
> + mcr |= MXU1_MCR_DTR;
> + if (set & TIOCM_LOOP)
> + mcr |= MXU1_MCR_LOOP;
> +
> + if (clear & TIOCM_RTS)
> + mcr &= ~MXU1_MCR_RTS;
> + if (clear & TIOCM_DTR)
> + mcr &= ~MXU1_MCR_DTR;
> + if (clear & TIOCM_LOOP)
> + mcr &= ~MXU1_MCR_LOOP;
> +
> + spin_unlock_irqrestore(&mxport->mxp_lock, flags);
> +
> + return mxu1_set_mcr(mxport, mcr);
> +}
> +
> +static void mxu1_break(struct tty_struct *tty, int break_state)
> +{
> + struct usb_serial_port *port = tty->driver_data;
> +
> + struct mxu1_port *mxport = usb_get_serial_port_data(port);
> +
> + dev_dbg(&port->dev, "%s - state = %d", __func__, break_state);
> +
> + if (mxport == NULL)
> + return;
> +
> + if (break_state == -1)
> + mxport->mxp_send_break = MXU1_LCR_BREAK;
> + else
> + mxport->mxp_send_break = 0;
> +
> + mxu1_set_termios(NULL, mxport->mxp_port, NULL);
> +}
> +
> +static int mxu1_open(struct tty_struct *tty, struct usb_serial_port *port)
> +{
> + struct mxu1_port *mxport = usb_get_serial_port_data(port);
> + struct mxu1_device *mxdev;
> + struct usb_device *dev;
> + struct urb *urb;
> + int port_number;
> + int status;
> + __u16 open_settings = (u8)(MXU1_PIPE_MODE_CONTINUOUS |
> + MXU1_PIPE_TIMEOUT_ENABLE |
> + (MXU1_TRANSFER_TIMEOUT << 2));
Use u16 and split declatation from initialisation. u8 cast looks odd.
> + if (!mxport)
> + return -ENODEV;
> +
> + dev = port->serial->dev;
> + mxdev = mxport->mxp_mxdev;
> +
> + port_number = port->port_number - port->minor;
> +
> + mxport->mxp_msr = 0;
> + mxport->mxp_shadow_mcr |= (MXU1_MCR_RTS | MXU1_MCR_DTR);
Drop parentheses.
> +
> + if (mutex_lock_interruptible(&mxdev->mxd_lock))
> + return -ERESTARTSYS;
Why do you need this? Again, these are one-port devices.
> +
> + dev_dbg(&port->dev, "%s - start interrupt in urb", __func__);
> + urb = mxdev->mxd_serial->port[0]->interrupt_in_urb;
Access directly though port pointer instead.
> + if (!urb) {
> + dev_err(&port->dev,
> + "%s - no interrupt urb\n",
> + __func__);
Too many line breaks. Not needed at all here.
> + status = -EINVAL;
If you expect an interrupt endpoint then make sure it's there already at
probe.
> + goto release_mxd_lock;
> + }
> + urb->context = mxdev;
> + status = usb_submit_urb(urb, GFP_KERNEL);
> + if (status) {
> + dev_err(&port->dev,
> + "%s - submit interrupt urb failed, %d\n",
> + __func__,
> + status);
> + goto release_mxd_lock;
> + }
> +
> + mxu1_set_termios(NULL, port, NULL);
You have a tty argument, which is non-null unless used as a console.
> +
> + dev_dbg(&port->dev, "%s - sending MXU1_OPEN_PORT", __func__);
Drop this debugging, just log any errors.
> + status = mxu1_send_ctrl_urb(mxdev->mxd_serial, MXU1_OPEN_PORT,
> + open_settings,
> + (u8)(MXU1_UART1_PORT + port_number));
Use a intermediate variable for the port number if needed at all.
> + if (status) {
> + dev_err(&port->dev, "%s - cannot send open command, %d\n",
> + __func__,
> + status);
Unnecessary line breaks again.
> + goto unlink_int_urb;
> + }
> +
> + dev_dbg(&port->dev, "%s - sending MXU1_START_PORT", __func__);
> + status = mxu1_send_ctrl_urb(mxdev->mxd_serial, MXU1_START_PORT,
> + 0, (u8)(MXU1_UART1_PORT + port_number));
> + if (status) {
> + dev_err(&port->dev, "%s - cannot send start command, %d\n",
> + __func__,
> + status);
> + goto unlink_int_urb;
> + }
> +
> + dev_dbg(&port->dev, "%s - sending MXU1_PURGE_PORT", __func__);
> + status = mxu1_send_ctrl_urb(mxdev->mxd_serial, MXU1_PURGE_PORT,
> + MXU1_PURGE_INPUT,
> + (u8)(MXU1_UART1_PORT + port_number));
> + if (status) {
> + dev_err(&port->dev,
> + "%s - cannot clear input buffers, %d\n",
> + __func__,
> + status);
> +
> + goto unlink_int_urb;
> + }
> + status = mxu1_send_ctrl_urb(mxdev->mxd_serial, MXU1_PURGE_PORT,
> + MXU1_PURGE_OUTPUT,
> + (u8)(MXU1_UART1_PORT + port_number));
> + if (status) {
> + dev_err(&port->dev,
> + "%s - cannot clear output buffers, %d\n",
> + __func__,
> + status);
> +
> + goto unlink_int_urb;
> + }
> +
> + /* reset the data toggle on the bulk endpoints to work around bug in
> + * host controllers where things get out of sync some times
> + */
Multi-line comments should be on the following format
/*
* ...
*/
> + usb_clear_halt(dev, port->write_urb->pipe);
> + usb_clear_halt(dev, port->read_urb->pipe);
> +
> + mxu1_set_termios(NULL, port, NULL);
> +
> + dev_dbg(&port->dev, "%s - sending MXU1_OPEN_PORT (2)", __func__);
> + status = mxu1_send_ctrl_urb(mxdev->mxd_serial, MXU1_OPEN_PORT,
> + open_settings,
> + (u8)(MXU1_UART1_PORT + port_number));
> + if (status) {
> + dev_err(&port->dev, "%s - cannot send open command, %d\n",
> + __func__,
> + status);
> + goto unlink_int_urb;
> + }
> +
> + dev_dbg(&port->dev, "%s - sending MXU1_START_PORT (2)", __func__);
> + status = mxu1_send_ctrl_urb(mxdev->mxd_serial, MXU1_START_PORT,
> + 0, (u8)(MXU1_UART1_PORT + port_number));
> + if (status) {
> + dev_err(&port->dev, "%s - cannot send start command, %d\n",
> + __func__,
> + status);
> + goto unlink_int_urb;
> + }
> +
> + /* start read urb */
> + dev_dbg(&port->dev, "%s - start read urb", __func__);
> + urb = port->read_urb;
> +
> + if (!urb) {
> + dev_err(&port->dev, "%s - no read urb\n", __func__);
> + status = -EINVAL;
> + goto unlink_int_urb;
> + }
> +
> + urb->context = port;
> + urb->dev = dev;
> + status = usb_submit_urb(urb, GFP_KERNEL);
> +
> + if (status) {
> + dev_err(&port->dev, "%s - submit read urb failed, %d\n",
> + __func__, status);
> + goto unlink_int_urb;
> + }
Use usb_serial_generic_open() to submit the read urb(s).
> +
> + goto release_mxd_lock;
> +
> +unlink_int_urb:
> + usb_kill_urb(port->serial->port[0]->interrupt_in_urb);
> +
> +release_mxd_lock:
> + mutex_unlock(&mxdev->mxd_lock);
> + dev_dbg(&port->dev, "%s - exit %d", __func__, status);
Drop debugging.
> +
> + return status;
> +}
> +
> +static void mxu1_close(struct usb_serial_port *port)
> +{
> + struct mxu1_device *mxdev;
> + struct mxu1_port *mxport;
> + int port_number;
> + unsigned long flags;
> + int status = 0;
> +
> + dev_dbg(&port->dev, "%s - port %d", __func__, port->port_number);
> +
> + mxdev = usb_get_serial_data(port->serial);
> + mxport = usb_get_serial_port_data(port);
> + if (mxdev == NULL || mxport == NULL)
> + return;
Again, not needed (anywhere).
> +
> + usb_kill_urb(port->read_urb);
> + usb_kill_urb(port->write_urb);
> +
> + spin_lock_irqsave(&port->lock, flags);
> + kfifo_reset_out(&port->write_fifo);
> + spin_unlock_irqrestore(&port->lock, flags);
Use generic close implementation instead.
> +
> + port_number = port->port_number - port->minor;
> +
> + dev_dbg(&port->dev, "%s - sending MXU1_CLOSE_PORT", __func__);
> + status = mxu1_send_ctrl_urb(port->serial,
> + MXU1_CLOSE_PORT,
> + 0, (u8)(MXU1_UART1_PORT + port_number));
> + if (status)
> + dev_err(&port->dev,
> + "%s - cannot send close port command, %d\n",
> + __func__,
> + status);
Close before killing urbs?
> +
> + usb_kill_urb(port->serial->port[0]->interrupt_in_urb);
> +
> + dev_dbg(&port->dev, "%s - exit", __func__);
Remove.
> +}
> +
> +static void mxu1_handle_new_msr(struct mxu1_port *mxport, u8 msr)
> +{
> + struct async_icount *icount;
> + struct tty_struct *tty;
> + unsigned long flags;
> +
> + dev_dbg(&mxport->mxp_port->dev, "%s - msr 0x%02X", __func__, msr);
> +
> + if (msr & MXU1_MSR_DELTA_MASK) {
> + spin_lock_irqsave(&mxport->mxp_lock, flags);
> + icount = &mxport->mxp_port->icount;
> + if (msr & MXU1_MSR_DELTA_CTS)
> + icount->cts++;
> + if (msr & MXU1_MSR_DELTA_DSR)
> + icount->dsr++;
> + if (msr & MXU1_MSR_DELTA_CD)
> + icount->dcd++;
> + if (msr & MXU1_MSR_DELTA_RI)
> + icount->rng++;
> + if (mxport->mxp_msr_wait_flags == 1) {
> + mxport->mxp_msr_wait_flags = 0;
> + wake_up_interruptible(&mxport->mxp_msr_wait);
Drop wait flags and use port msr wait queue.
> + }
> + spin_unlock_irqrestore(&mxport->mxp_lock, flags);
> + }
> +
> + mxport->mxp_msr = msr & MXU1_MSR_MASK;
> +
> + /* handle CTS flow control */
> + tty = mxport->mxp_port->port.tty;
You need to use tty_port_tty_get() and put the reference when done.
> +
> + if (tty && C_CRTSCTS(tty)) {
> + if (msr & MXU1_MSR_CTS) {
> + tty->hw_stopped = 0;
> +
> + tty_wakeup(tty);
> + } else {
> + tty->hw_stopped = 1;
> + }
> + }
But I think you drop this bit.
> +}
> +
> +static void mxu1_interrupt_callback(struct urb *urb)
> +{
> + struct mxu1_device *mxdev = (struct mxu1_device *)urb->context;
Cast not needed. Usb-serial core would already have setup the context to
be the usb-serial port.
> + struct usb_serial_port *port;
> + struct usb_serial *serial = mxdev->mxd_serial;
> + struct mxu1_port *mxport;
> + struct device *dev = &urb->dev->dev;
> + unsigned char *data = urb->transfer_buffer;
> + int length = urb->actual_length;
> + int port_number;
> + int function;
> + int status = 0;
> + u8 msr;
> +
> + dev_dbg(&urb->dev->dev, "%s", __func__);
Unless this is a common interrupt urb for a multi-port device, simply
use the port device for all debug and error messages.
> +
> + /* Check port is valid or not */
> + if (mxdev == NULL)
> + return;
Not needed.
> +
> +
Stray newline.
> + switch (urb->status) {
> + case 0:
> + break;
> + case -ECONNRESET:
> + case -ENOENT:
> + case -ESHUTDOWN:
> + dev_dbg(dev, "%s - urb shutting down, %d",
> + __func__, urb->status);
> + return;
> + default:
> + dev_err(dev, "%s - nonzero urb status, %d\n",
> + __func__, urb->status);
These should be debug level as you'd usually get these when unplugging
an open device.
> + goto exit;
> + }
> +
> + if (length != 2) {
> + dev_dbg(dev, "%s - bad packet size, %d", __func__, length);
> + goto exit;
> + }
> +
> + if (data[0] == MXU1_CODE_HARDWARE_ERROR) {
> + dev_err(dev, "%s - hardware error, %d\n", __func__, data[1]);
> + goto exit;
> + }
> +
> + port_number = MXU1_GET_PORT_FROM_CODE(data[0]);
> + function = MXU1_GET_FUNC_FROM_CODE(data[0]);
Is this needed if single port devices? Use static functions rather than
macros.
> +
> + dev_dbg(dev, "%s - port_number %d, function %d, data 0x%02X",
> + __func__,
> + port_number,
> + function,
> + data[1]);
> +
> + if (port_number >= serial->num_ports) {
> + dev_err(dev, "%s - bad port number, %d\n",
> + __func__, port_number);
> + goto exit;
> + }
> +
> + port = serial->port[port_number];
> +
> + mxport = usb_get_serial_port_data(port);
> + if (!mxport)
> + goto exit;
> +
> + switch (function) {
> + case MXU1_CODE_DATA_ERROR:
> + dev_dbg(dev, "%s - DATA ERROR, port %d, data 0x%02X\n",
> + __func__,
> + port_number,
> + data[1]);
> + break;
> +
> + case MXU1_CODE_MODEM_STATUS:
> + msr = data[1];
> + dev_dbg(dev, "%s - port %d, msr 0x%02X",
> + __func__, port_number, msr);
> + mxu1_handle_new_msr(mxport, msr);
> + break;
> +
> + default:
> + dev_err(dev, "%s - unknown interrupt code, 0x%02X\n",
> + __func__, data[1]);
> + break;
> + }
> +
> +exit:
> + status = usb_submit_urb(urb, GFP_ATOMIC);
> + if (status)
> + dev_err(dev, "%s - resubmit interrupt urb failed, %d\n",
> + __func__, status);
> +}
> +
> +static struct usb_serial_driver mxuport11_device = {
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = "mxuport11",
> + },
> + .description = "MOXA UPort 11x0",
> + .id_table = mxuport11_idtable,
Use mxu1 prefix throughout?
> + .num_ports = 1,
> + .port_probe = mxu1_port_probe,
> + .attach = mxu1_startup,
> + .open = mxu1_open,
> + .close = mxu1_close,
> + .ioctl = mxu1_ioctl,
> + .set_termios = mxu1_set_termios,
> + .tiocmget = mxu1_tiocmget,
> + .tiocmset = mxu1_tiocmset,
> + .get_icount = usb_serial_generic_get_icount,
You may want to set tiocmiwait to the generic implementation as well.
> + .break_ctl = mxu1_break,
> + .read_int_callback = mxu1_interrupt_callback,
> +};
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists