| 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: <54C99CEE.8070802@gmail.com>
Date: Thu, 29 Jan 2015 10:37:34 +0800
From: Peter Hung <hpeter@...il.com>
To: Johan Hovold <johan@...nel.org>
CC: gregkh@...uxfoundation.org, linux-usb@...r.kernel.org,
linux-kernel@...r.kernel.org, tom_tsai@...tek.com.tw,
peter_hong@...tek.com.tw,
Peter Hung <hpeter+linux_kernel@...il.com>
Subject: Re: [PATCH v3 1/5] usb: serial: add register map for F81232
Hello.
1. For retry Issue:
These patches is referenced from our other usb serial product. That
product maybe not ack the control ep command when It's in very heavily
loading. Our workaround is to modify driver to retry more times if it
timeout because it's f/w can't upgrade with usb protocol. I will remove
the retry mechanic and test for it again.
2. For some function/variable not used:
The original driver is lacking a lot of feature. My first patch strategy
is putting all needed function / variables in patch 1 and apply it in
following patches. Sorry for my wrong strategy, I'll change it with more
meaningful and logical patchset and resend it.
Thanks for your patient and advice.
Johan Hovold 於 2015/1/29 上午 01:55 寫道:
> On Wed, Jan 28, 2015 at 01:57:56PM +0800, Peter Hung wrote:
>> Add register map for F81232. and add some function to operating this device.
>> etc. f81232_get_register()/f81232_set_register() to work with USB control
>> point. and worker f81232_int_work_wq() to read MSR when IIR acquired.
>>
>> Signed-off-by: Peter Hung <hpeter+linux_kernel@...il.com>
>> ---
>> drivers/usb/serial/f81232.c | 229 +++++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 214 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
>> index c5dc233..efd45a7 100644
>> --- a/drivers/usb/serial/f81232.c
>> +++ b/drivers/usb/serial/f81232.c
>> @@ -23,6 +23,8 @@
>> #include <linux/uaccess.h>
>> #include <linux/usb.h>
>> #include <linux/usb/serial.h>
>> +#include <linux/serial_reg.h>
>> +#include <linux/version.h>
>
> You don't need this header.
>
>> static const struct usb_device_id id_table[] = {
>> { USB_DEVICE(0x1934, 0x0706) },
>> @@ -37,19 +39,197 @@ MODULE_DEVICE_TABLE(usb, id_table);
>> #define UART_STATE_TRANSIENT_MASK 0x74
>> #define UART_DCD 0x01
>> #define UART_DSR 0x02
>> -#define UART_BREAK_ERROR 0x04
>> #define UART_RING 0x08
>> -#define UART_FRAME_ERROR 0x10
>> -#define UART_PARITY_ERROR 0x20
>> -#define UART_OVERRUN_ERROR 0x40
>> #define UART_CTS 0x80
>>
>> +#define UART_BREAK_ERROR 0x10
>> +#define UART_FRAME_ERROR 0x08
>> +#define UART_PARITY_ERROR 0x04
>> +#define UART_OVERRUN_ERROR 0x02
>> +#define SERIAL_EVEN_PARITY (UART_LCR_PARITY | UART_LCR_EPAR)
>
> You never use this define.
>
>> +#define REGISTER_REQUEST 0xA0
>> +#define GET_REGISTER 0xc0
>> +#define SET_REGISTER 0x40
>> +#define F81232_USB_TIMEOUT 1000
>> +#define F81232_USB_RETRY 20
>
> Why on earth are you retrying your control requests 20 times? You never
> answered my question to a previous version whether the retries are at
> all needed.
>
>> +
>> +#define SERIAL_BASE_ADDRESS (0x0120)
>> +#define RECEIVE_BUFFER_REGISTER (0x00 + SERIAL_BASE_ADDRESS)
>> +#define TRANSMIT_HOLDING_REGISTER (0x00 + SERIAL_BASE_ADDRESS)
>> +#define INTERRUPT_ENABLE_REGISTER (0x01 + SERIAL_BASE_ADDRESS)
>> +#define INTERRUPT_IDENT_REGISTER (0x02 + SERIAL_BASE_ADDRESS)
>> +#define FIFO_CONTROL_REGISTER (0x02 + SERIAL_BASE_ADDRESS)
>> +#define LINE_CONTROL_REGISTER (0x03 + SERIAL_BASE_ADDRESS)
>> +#define MODEM_CONTROL_REGISTER (0x04 + SERIAL_BASE_ADDRESS)
>> +#define LINE_STATUS_REGISTER (0x05 + SERIAL_BASE_ADDRESS)
>> +#define MODEM_STATUS_REGISTER (0x06 + SERIAL_BASE_ADDRESS)
>> +
>> struct f81232_private {
>> spinlock_t lock;
>> u8 line_control;
>> u8 line_status;
>> +
>> + struct work_struct int_worker;
>
> You never schedule this work.
>
>> + struct usb_serial_port *port;
>> };
>>
>> +static inline int calc_baud_divisor(u32 baudrate)
>> +{
>> + u32 divisor, rem;
>> +
>> + divisor = 115200L / baudrate;
>> + rem = 115200L % baudrate;
>> +
>> + /* Round to nearest divisor */
>> + if (((rem * 2) >= baudrate) && (baudrate != 110))
>> + divisor++;
>> +
>> + return divisor;
>> +}
>
> You never use this either.
>
> You need to take more care when preparing your patches. As I already
> asked, break them up into logical changes and only add things that you
> will actually use.
>
>> +
>> +static inline int f81232_get_register(struct usb_device *dev,
>> + u16 reg, u8 *data)
>> +{
>> + int status;
>> + int i = F81232_USB_RETRY;
>> +
>> + while (i--) {
>> + status = usb_control_msg(dev,
>> + usb_rcvctrlpipe(dev, 0),
>> + REGISTER_REQUEST,
>> + GET_REGISTER,
>> + reg,
>> + 0,
>> + data,
>> + sizeof(*data),
>> + F81232_USB_TIMEOUT);
>> +
>> + if (status < 0) {
>> + dev_dbg(&dev->dev,
>> + "f81232_get_register status: %d, fail:%d\n",
>> + status, i);
>> + } else
>> + break;
>
> Missing brackets { } on the else branch.
>
>> + }
>> +
>> + return status;
>> +}
>> +
>> +
>> +static inline int f81232_set_register(struct usb_device *dev,
>> + u16 reg, u8 data)
>> +{
>> + int status;
>> + int i = F81232_USB_RETRY;
>> +
>> + while (i--) {
>> + status = usb_control_msg(dev,
>> + usb_sndctrlpipe(dev, 0),
>> + REGISTER_REQUEST,
>> + SET_REGISTER,
>> + reg,
>> + 0,
>> + &data,
>> + 1,
>> + F81232_USB_TIMEOUT);
>> +
>> + if (status < 0)
>> + dev_dbg(&dev->dev,
>> + "f81232_set_register status: %d, fail:%d\n",
>> + status, i);
>> + else
>> + break;
>> + }
>> +
>> + return status;
>> +}
>> +
>> +static void f81232_read_msr(struct f81232_private *priv)
>> +{
>> + unsigned long flags;
>> + u8 current_msr, old_msr;
>> + struct usb_device *dev = priv->port->serial->dev;
>> +
>> + f81232_get_register(dev, MODEM_STATUS_REGISTER, ¤t_msr);
>
> Error handling?
>
>> +
>> + spin_lock_irqsave(&priv->lock, flags);
>> + old_msr = priv->line_status;
>> + spin_unlock_irqrestore(&priv->lock, flags);
>> +
>> +
>
> Stray newline.
>
>> + if ((current_msr & 0xf0) ^ (old_msr & 0xf0)) {
>
> Use defines for constants.
>
>> + if (priv->port->port.tty)
>
> You need to acquire a tty reference using tty_port_tty_get().
>
>> + usb_serial_handle_dcd_change(priv->port,
>> + priv->port->port.tty,
>> + current_msr & UART_MSR_DCD);
>> +
>> + spin_lock_irqsave(&priv->lock, flags);
>> + priv->line_status = current_msr;
>> + spin_unlock_irqrestore(&priv->lock, flags);
>> + }
>> +
>> + dev_dbg(&dev->dev, "f81232_read_msr: %x\n", priv->line_status);
>
> Use %s and __func__.
>
>> +}
>> +
>> +
>
> Only one newline. Fix this throughout.
>
>> +static inline int update_mctrl(struct f81232_private *port_priv,
>> + unsigned int set, unsigned int clear)
>> +{
>> + struct usb_device *dev = port_priv->port->serial->dev;
>> + u8 urb_value;
>> + int status;
>> + unsigned long flags;
>> +
>> + if (((set | clear) & (TIOCM_DTR | TIOCM_RTS)) == 0) {
>> + dev_dbg(&dev->dev, "update_mctrl fail - DTR|RTS %d\n",
>> + __LINE__);
>> + return 0; /* no change */
>> + }
>> +
>> +
>> + clear &= ~set; /* 'set' takes precedence over 'clear' */
>> + urb_value = 8 | port_priv->line_control;
>> +
>> +
>> + if (clear & TIOCM_DTR) {
>> + urb_value &= ~UART_MCR_DTR;
>> + dev_dbg(&dev->dev, "clear DTR\n");
>> + }
>> +
>> + if (clear & TIOCM_RTS) {
>> + urb_value &= ~UART_MCR_RTS;
>> + dev_dbg(&dev->dev, "clear RTS\n");
>> + }
>> +
>> + if (set & TIOCM_DTR) {
>> + urb_value |= UART_MCR_DTR;
>> + dev_dbg(&dev->dev, "set DTR\n");
>> + }
>> +
>> + if (set & TIOCM_RTS) {
>> + urb_value |= UART_MCR_RTS;
>> + dev_dbg(&dev->dev, "set RTS\n");
>> + }
>> +
>> + dev_dbg(&dev->dev, "update_mctrl n:%x o:%x\n", urb_value,
>> + port_priv->line_control);
>> +
>> + status = f81232_set_register(dev, MODEM_CONTROL_REGISTER, urb_value);
>> +
>> + if (status < 0) {
>> + dev_dbg(&dev->dev, "MODEM_CONTROL_REGISTER < 0\n");
>> + } else {
>> + spin_lock_irqsave(&port_priv->lock, flags);
>> + port_priv->line_control = urb_value;
>> + spin_unlock_irqrestore(&port_priv->lock, flags);
>> + }
>> +
>> + f81232_read_msr(port_priv);
>> +
>> + return status;
>> +}
>
> You never call this function either.
>
> Missing newline.
>
>> static void f81232_update_line_status(struct usb_serial_port *port,
>> unsigned char *data,
>> unsigned int actual_length)
>> @@ -201,8 +381,8 @@ static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
>>
>> result = usb_submit_urb(port->interrupt_in_urb, GFP_KERNEL);
>> if (result) {
>> - dev_err(&port->dev, "%s - failed submitting interrupt urb,"
>> - " error %d\n", __func__, result);
>> + dev_err(&port->dev, "failed submitting interrupt urb, error %d\n",
>> + result);
>
> I already asked you to fix this separately.
>
>> return result;
>> }
>>
>> @@ -241,6 +421,7 @@ static void f81232_dtr_rts(struct usb_serial_port *port, int on)
>> static int f81232_carrier_raised(struct usb_serial_port *port)
>> {
>> struct f81232_private *priv = usb_get_serial_port_data(port);
>> +
>
> And I also also asked you not to include random white space changes.
>
>> if (priv->line_status & UART_DCD)
>> return 1;
>> return 0;
>> @@ -254,13 +435,18 @@ static int f81232_ioctl(struct tty_struct *tty,
>>
>> switch (cmd) {
>> case TIOCGSERIAL:
>> - memset(&ser, 0, sizeof ser);
>> - ser.type = PORT_16654;
>> + memset(&ser, 0, sizeof(ser));
>> + ser.flags = ASYNC_SKIP_TEST | ASYNC_AUTO_IRQ;
>> + ser.xmit_fifo_size = port->bulk_out_size;
>> + ser.close_delay = 5*HZ;
>> + ser.closing_wait = 30*HZ;
>> +
>> + ser.type = PORT_16550A;
>> ser.line = port->minor;
>> ser.port = port->port_number;
>> - ser.baud_base = 460800;
>> + ser.baud_base = 115200;
>>
>> - if (copy_to_user((void __user *)arg, &ser, sizeof ser))
>> + if (copy_to_user((void __user *)arg, &ser, sizeof(ser)))
>
> This is also unrelated.
>
>> return -EFAULT;
>>
>> return 0;
>> @@ -270,6 +456,17 @@ static int f81232_ioctl(struct tty_struct *tty,
>> return -ENOIOCTLCMD;
>> }
>>
>> +
>> +
>> +
>> +static void f81232_int_work_wq(struct work_struct *work)
>> +{
>> + struct f81232_private *priv =
>> + container_of(work, struct f81232_private, int_worker);
>> +
>> + f81232_read_msr(priv);
>> +}
>> +
>> static int f81232_port_probe(struct usb_serial_port *port)
>> {
>> struct f81232_private *priv;
>> @@ -279,10 +476,11 @@ static int f81232_port_probe(struct usb_serial_port *port)
>> return -ENOMEM;
>>
>> spin_lock_init(&priv->lock);
>> + INIT_WORK(&priv->int_worker, f81232_int_work_wq);
>>
>> usb_set_serial_port_data(port, priv);
>>
>> - port->port.drain_delay = 256;
>
> Why are you removing the drain delay?̈́
>
>> + priv->port = port;
>>
>> return 0;
>> }
>> @@ -304,11 +502,11 @@ static struct usb_serial_driver f81232_device = {
>> },
>> .id_table = id_table,
>> .num_ports = 1,
>> - .bulk_in_size = 256,
>> - .bulk_out_size = 256,
>> + .bulk_in_size = 64,
>> + .bulk_out_size = 64,
>
> You never answered my questions about this change either.
>
>> .open = f81232_open,
>> .close = f81232_close,
>> - .dtr_rts = f81232_dtr_rts,
>> + .dtr_rts = f81232_dtr_rts,
>
> I already asked you to drop this random whitespace change.
>
>> .carrier_raised = f81232_carrier_raised,
>> .ioctl = f81232_ioctl,
>> .break_ctl = f81232_break_ctl,
>> @@ -330,5 +528,6 @@ static struct usb_serial_driver * const serial_drivers[] = {
>> module_usb_serial_driver(serial_drivers, id_table);
>>
>> MODULE_DESCRIPTION("Fintek F81232 USB to serial adaptor driver");
>> -MODULE_AUTHOR("Greg Kroah-Hartman <gregkh@...uxfoundation.org");
>> +MODULE_AUTHOR("Greg Kroah-Hartman <gregkh@...uxfoundation.org>");
>
> Yes, there seems to be a missing bracket here, but please fix that
> separately.
>
>> +MODULE_AUTHOR("Peter Hong <peter_hong@...tek.com.tw>");
>> MODULE_LICENSE("GPL v2");
>
> Make sure to fix the above, including similar issues in the other
> patches in the series. I'm not going to look at the rest of the patches
> until you clean this up and resend.
>
> Please take more care in addressing review comments and answer any
> questions already asked on the previous versions of the series.
>
> Thanks,
> Johan
>
--
With Best Regards,
Peter Hung
--
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