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: Thu, 5 Nov 2015 10:48:03 +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/4 下午 04:38 寫道: > On Wed, 2015-11-04 at 16:19 +0800, Peter Hung wrote: >> Hi >> >> Oliver Neukum 於 2015/11/3 下午 06:03 寫道: >>> On Tue, 2015-11-03 at 11:51 +0800, Peter Hung wrote: >>>> + 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 ? > > No, if it doesn't work, use the current code. OK, I'll use current code. >>>> +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. > > Yes, so why don't you pass an u8 as opposed to a pointer to an u8? I'll re-write it from u8* to u8. >>>> +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. > > OK, but how do you guarantee the delay you need if you get a signal, > which would abort the delay? > Hmm, you are right. It seems to replace *_interruptible to *_killable and return -EINTR to guarantee not abort by normal signal. Thanks for your advices -- 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