linux-kernel - Re: [PATCH 1/7] ark3116: (3rd try) Setup some basic infrastructure for new ark3116 driver.

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 27 Oct 2009 22:35:31 +0100
From:	Bart Hartgers <bart.hartgers@...il.com>
To:	Greg KH <greg@...ah.com>
Cc:	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
	Mike McCormack <mikem@...g3k.org>
Subject: Re: [PATCH 1/7] ark3116: (3rd try) Setup some basic infrastructure 
	for new ark3116 driver.

2009/10/27 Greg KH <greg@...ah.com>:
> On Sun, Oct 25, 2009 at 06:50:58PM +0100, bart.hartgers@...il.com wrote:
>> Signed-off-by: Bart Hartgers <bart.hartgers@...il.com>
>> ---
>> Index: linux-2.6.32-rc4/drivers/usb/serial/ark3116.c
>> ===================================================================
>> --- linux-2.6.32-rc4.orig/drivers/usb/serial/ark3116.c        2009-10-18 14:25:02.000000000 +0200
>> +++ linux-2.6.32-rc4/drivers/usb/serial/ark3116.c     2009-10-18 14:25:14.000000000 +0200
>> @@ -1,4 +1,6 @@
>>  /*
>> + * Copyright (C) 2009 by Bart Hartgers (bart.hartgers+ark3116@...il.com)
>> + * Original version:
>>   * Copyright (C) 2006
>>   *   Simon Schulz (ark3116_driver <at> auctionant.de)
>>   *
>> @@ -6,10 +8,13 @@
>>   * - implements a driver for the arkmicro ark3116 chipset (vendor=0x6547,
>>   *   productid=0x0232) (used in a datacable called KQ-U8A)
>>   *
>> - * - based on code by krisfx -> thanks !!
>> - *   (see http://www.linuxquestions.org/questions/showthread.php?p=2184457#post2184457)
>> + * Supports full modem status lines, break, hardware flow control. Does not
>> + * support software flow control, since I do not know how to enable it in hw.
>>   *
>> - *  - based on logs created by usbsnoopy
>> + * This driver is a essentially new implementation. I initially dug
>> + * into the old ark3116.c driver and suddenly realized the ark3116 is
>> + * a 16450 with a USB interface glued to it. See comments at the
>> + * bottom of this file.
>>   *
>>   * This program is free software; you can redistribute it and/or modify it
>>   * under the terms of the GNU General Public License as published by the
>> @@ -19,15 +24,31 @@
>>
>>  #include <linux/kernel.h>
>>  #include <linux/init.h>
>> +#include <asm/atomic.h>
>> +#include <linux/ioctl.h>
>>  #include <linux/tty.h>
>> +#include <linux/tty_flip.h>
>>  #include <linux/module.h>
>>  #include <linux/usb.h>
>>  #include <linux/usb/serial.h>
>>  #include <linux/serial.h>
>> +#include <linux/serial_reg.h>
>>  #include <linux/uaccess.h>
>> -
>> +#include <linux/mutex.h>
>>
>>  static int debug;
>> +/*
>> + * Version information
>> + */
>> +
>> +#define DRIVER_VERSION "v0.3"
>> +#define DRIVER_AUTHOR "Bart Hartgers <bart.hartgers+ark3116@...il.com>"
>> +#define DRIVER_DESC "USB ARK3116 serial/IrDA driver"
>> +#define DRIVER_DEV_DESC "ARK3116 RS232/IrDA"
>> +#define DRIVER_NAME "ark3116"
>> +
>> +/* usb timeout of 1 second */
>> +#define ARK_TIMEOUT (1*HZ)
>>
>>  static struct usb_device_id id_table [] = {
>>       { USB_DEVICE(0x6547, 0x0232) },
>> @@ -45,6 +66,52 @@ static int is_irda(struct usb_serial *se
>>       return 0;
>>  }
>>
>> +struct ark3116_private {
>> +     wait_queue_head_t       delta_msr_wait;
>> +     struct async_icount     icount;
>> +     int                     irda;   /* 1 for irda device */
>> +
>> +     /* protects hw register updates */
>> +     struct mutex            lock;
>> +
>> +     int                     quot;   /* baudrate divisor */
>> +     __u8                    lcr;    /* line control register value */
>> +     __u8                    hcr;    /* handshake control register (0x8)
>> +                                      * value
>> +                                      */
>> +     /* register values - updated asynchronously */
>> +     atomic_t                mcr;
>> +     atomic_t                msr;
>> +     atomic_t                lsr;
>
> These don't need to be atomic, please don't use them if they are not
> needed.  Just use the lock to protect updating them if needed.
>
At least lsr and msr are (or will be in patch 6) updated by the
interrupt_callback. I am happy to add another lock (for
interrupt-context it would need a spinlock rather than a mutex,
right?). I am just surprised that is considered cleaner than using
atomic_t.

For mcr I am not sure whether a race between tiocmget and tiocmset is
possible. I didn't see any obvious locking in usb-serial.c or
tty_io.c, that's why I used an atomic_t. Am I missing something?

> Care to respin this series based on the review so far?
Will do somewhere tomorrow. Mike and I are working out how to combine
our patches.

Groeten,
Bart

>
> thanks,
>
> greg k-h
>



-- 
Bart Hartgers - New e-mail: bart.hartgers@...il.com
--
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/