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
| ||
|
Date: Wed, 4 Nov 2015 16:19:39 +0800 From: Peter Hung <hpeter@...il.com> To: Oliver Neukum <oneukum@...e.com> Cc: johan@...nel.org, 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 V6 1/1] usb:serial: Add Fintek F81532/534 driver Hi Oliver Neukum 於 2015/11/3 下午 06:03 寫道: > On Tue, 2015-11-03 at 11:51 +0800, Peter Hung wrote: >> +static int f81534_attach(struct usb_serial *serial) >> +{ >> + struct f81534_serial_private *serial_priv = NULL; >> + int status; >> + int i; >> + int offset; >> + uintptr_t setting_idx = (uintptr_t) usb_get_serial_data(serial); >> + >> + serial_priv = kzalloc(sizeof(*serial_priv), GFP_KERNEL); >> + if (!serial_priv) >> + return -ENOMEM; >> + >> + usb_set_serial_data(serial, serial_priv); >> + serial_priv->setting_idx = setting_idx; >> + >> + for (i = 0; i < F81534_NUM_PORT; ++i) { >> + /* Disable all interrupt before submit URB */ >> + status = f81534_setregister(serial->dev, i, >> + INTERRUPT_ENABLE_REGISTER, 0x00); >> + if (status) { >> + dev_err(&serial->dev->dev, "%s: IER disable failed\n", >> + __func__); >> + goto failed; >> + } >> + } >> + >> + for (i = 0; i < F81534_NUM_PORT; ++i) >> + atomic_set(&serial_priv->port_active[i], 0); > > Should be ATOMIC_INIT() > ATOMIC_INIT() seems to be used only for variable initializer, It cant be used for dynamic allocation. Should I change it to a normal boolean flag protecting with spin_lock ? >> +static int f81534_port_remove(struct usb_serial_port *port) >> +{ >> + struct f81534_port_private *port_priv; >> + >> + f81534_release_gpio(port); >> + port_priv = usb_get_serial_port_data(port); >> + kfree(port_priv); >> + return 0; >> +} >> + >> +static void f81534_compare_msr(struct usb_serial_port *port, u8 *msr, > > Is the point of passing a pointer to msr locking? > >> + bool is_port_open) This function is used only with URB callback function. The *msr is reported by H/W with newest MSR. The USB-Serial generic system will re-submit read URB when callback complete. So this function should run once on the same time. >> +static int f81534_tiocmget(struct tty_struct *tty) >> +{ >> + struct usb_serial_port *port = tty->driver_data; >> + struct f81534_port_private *port_priv = usb_get_serial_port_data(port); >> + unsigned long flags; >> + int r; >> + u8 msr, mcr; >> + >> + /* >> + * We'll avoid to direct read MSR register. The IC will read the MSR >> + * changed and report it f81534_process_per_serial_block() by >> + * F81534_TOKEN_MSR_CHANGE. >> + * >> + * When this device in heavy loading (e.g., BurnInTest Loopback Test) >> + * The report of MSR register will delay received a bit. It's due to >> + * MSR interrupt is lowest priority in 16550A. So we decide to sleep >> + * a little time to pass the test. >> + */ >> + if (schedule_timeout_interruptible( >> + msecs_to_jiffies(F81534_DELAY_READ_MSR))) { >> + dev_info(&port->dev, "%s: breaked !!\n", __func__); >> + } > > Is the delay necessary or isn't it? > If it is necessary you should do something about the signal. > We add this delay due to stress test (Loop-back & 921600bps with BurnInTest). It'll receive MSR with some delay when connecting with DTR-DSR & RTS/CTS, but the delay smaller than 10ms. So we decided to delay some time to pass the test. >> +static int f81534_prepare_write_buffer(struct usb_serial_port *port, >> + void *dest, size_t size) >> +{ >> + unsigned char *ptr = (unsigned char *) dest; >> + struct f81534_port_private *port_priv = usb_get_serial_port_data(port); >> + int port_num = port_priv->phy; >> + struct usb_serial *serial = port->serial; >> + >> + WARN_ON(size != serial->port[0]->bulk_out_size); >> + >> + if (size != F81534_WRITE_BUFFER_SIZE) { >> + WARN_ON(size != F81534_WRITE_BUFFER_SIZE); > > What is the sense of this? > I'll remove the double-check section with next version patch. >> + ptr[F81534_RECEIVE_BLOCK_SIZE * 0] = 0; >> + ptr[F81534_RECEIVE_BLOCK_SIZE * 1] = 1; >> + ptr[F81534_RECEIVE_BLOCK_SIZE * 2] = 2; >> + ptr[F81534_RECEIVE_BLOCK_SIZE * 3] = 3; > > Either these ... > >> + ptr[F81534_RECEIVE_BLOCK_SIZE * port_num + 0] = port_num; > > .. or that is redundant > I'll remove it too. Thanks for your advice. -- 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