lkml.org 
[lkml]   [2005]   [Nov]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] new driver synclink_gt
Paul Fulghum <paulkf@microgate.com> wrote:
>
> Patches for a new character device driver for
> the SyncLink GT and SyncLink AC families of
> synchronous and asynchronous serial adapters
> are located at (patches too large for mailing list):
>
> http://www.microgate.com/ftp/linux/synclink_gt-2.6.14/patch-2.6.14-Kconfig
> http://www.microgate.com/ftp/linux/synclink_gt-2.6.14/patch-2.6.14-Makefile
> http://www.microgate.com/ftp/linux/synclink_gt-2.6.14/patch-2.6.14-synclink.h
> http://www.microgate.com/ftp/linux/synclink_gt-2.6.14/patch-2.6.14-synclink_gt.c

There's not much point in presenting it as a patch-per-file, really - we
need all four patches to have a buildable driver, so it may as well be a
single patch.

We need to get a driverdude onto this - I'm not one any more. Quick notes:

- Tx (at least) appears to DMA straight out of kmalloced memory. If I
read it right, it needs to use the PCI DMA API fully - pci_map_single()
and friends. <checks> Maybe I misread it.

- It seems to be both a tty driver and a net driver, or something. Can
you please describe the hardware?

- Be aware that the tty ldisc APIs were redone in -mm, so this driver
probably won't compile in -mm and fixups will be needed sometime.

>
> /*
> * compatibility and utility macros
> */
> #define GET_USER(error,value,addr) error = get_user(value,addr)
> #define COPY_FROM_USER(error,dest,src,size) error = copy_from_user(dest,src,size) ? -EFAULT : 0
> #define PUT_USER(error,value,addr) error = put_user(value,addr)
> #define COPY_TO_USER(error,dest,src,size) error = copy_to_user(dest,src,size) ? -EFAULT : 0

eww. Please just open-code these.

> /*
> * module configuration and status
> */
> static int pci_registered = 0;
> ...
> static struct _slgt_info *slgt_device_list = NULL;
> static int slgt_device_count = 0;
>
> static int ttymajor=0;
> static int debug_level = 0;
> static int maxframe[MAX_DEVICES] = {0,};
> static int dosyncppp[MAX_DEVICES] = {0,};

None of these initialisations to zero are needed.

> module_param(ttymajor, int, 0);
> module_param(debug_level, int, 0);
> module_param_array(maxframe, int, NULL, 0);
> module_param_array(dosyncppp, int, NULL, 0);

MODULE_PARM_DESCs needed, please.

>
> /*
> * device specific structures, macros and functions
> */
>
> #define SLGT_MAX_PORTS 4
> #define SLGT_REG_SIZE 256
>
> /*
> * DMA buffer descriptor and access macros
> */
> typedef struct
> {
> unsigned short count;
> unsigned short status;
> unsigned int pbuf; /* physical address of data buffer */
> unsigned int next; /* physical address of next descriptor */
>
> /* driver book keeping */
> char *buf; /* virtual address of data buffer */
> unsigned int pdesc; /* physical address of this descriptor */
> dma_addr_t buf_dma_addr;
> } SLGT_DESC;

typedefs are unpopular in new code. Just use `struct slgt_desc' everywhere.

> #define set_desc_buffer(a,b) (a).pbuf = cpu_to_le32((unsigned int)(b))
> #define set_desc_next(a,b) (a).next = cpu_to_le32((unsigned int)(b))
> #define set_desc_count(a,b)(a).count = cpu_to_le16((unsigned short)(b))
> #define set_desc_eof(a,b) (a).status = cpu_to_le16((b) ? (le16_to_cpu((a).status) | BIT0) : (le16_to_cpu((a).status) & ~BIT0))
> #define desc_count(a) (le16_to_cpu((a).count))
> #define desc_status(a) (le16_to_cpu((a).status))
> #define desc_complete(a) (le16_to_cpu((a).status) & BIT15)
> #define desc_eof(a) (le16_to_cpu((a).status) & BIT2)
> #define desc_crc_error(a) (le16_to_cpu((a).status) & BIT1)
> #define desc_abort(a) (le16_to_cpu((a).status) & BIT0)
> #define desc_residue(a) ((le16_to_cpu((a).status) & 0x38) >> 3)

That makes the driver harder to follow, but I guess as long as it's
followed religiously then it can make working on the driver more reliable.

> typedef struct _slgt_info {

Another typedef to remove, please.

> #ifdef CONFIG_HDLC
> struct net_device *netdev;
> #endif
>
> } SLGT_INFO;

Ah, so the netdev is for hdlc transport.

> static MGSL_PARAMS default_params = {
> MGSL_MODE_HDLC, /* unsigned long mode */
> 0, /* unsigned char loopback; */
> HDLC_FLAG_UNDERRUN_ABORT15, /* unsigned short flags; */
> HDLC_ENCODING_NRZI_SPACE, /* unsigned char encoding; */
> 0, /* unsigned long clock_speed; */
> 0xff, /* unsigned char addr_filter; */
> HDLC_CRC_16_CCITT, /* unsigned short crc_type; */
> HDLC_PREAMBLE_LENGTH_8BITS, /* unsigned char preamble_length; */
> HDLC_PREAMBLE_PATTERN_NONE, /* unsigned char preamble; */
> 9600, /* unsigned long data_rate; */
> 8, /* unsigned char data_bits; */
> 1, /* unsigned char stop_bits; */
> ASYNC_PARITY_NONE /* unsigned char parity; */
> };

Please use the

.field = value,

form here.

> static void set_termios(struct tty_struct *tty, struct termios *old_termios)
> {
> SLGT_INFO *info = (SLGT_INFO *)tty->driver_data;
> unsigned long flags;
>
> DBGINFO(("%s set_termios\n", tty->driver->name));
>
> /* just return if nothing has changed */
> if ((tty->termios->c_cflag == old_termios->c_cflag)
> && (RELEVANT_IFLAG(tty->termios->c_iflag)
> == RELEVANT_IFLAG(old_termios->c_iflag)))
> return;

whitespace went weird.

> static void put_char(struct tty_struct *tty, unsigned char ch)
> {
> SLGT_INFO *info = (SLGT_INFO *)tty->driver_data;
> SLGT_INFO *info = (SLGT_INFO *)tty->driver_data;
> unsigned long flags;
>
> if (sanity_check(info, tty->name, "put_char"))
> return;
> DBGINFO(("%s put_char(%d)\n", info->device_name, ch));
> if (!tty || !info->tx_buf)
> return;
> spin_lock_irqsave(&info->lock,flags);
> if (!info->tx_active && (info->tx_count < info->max_frame_size))
> info->tx_buf[info->tx_count++] = ch;
> spin_unlock_irqrestore(&info->lock,flags);
> }

Is silent droppage on overflow OK?

> /*
> * Service an IOCTL request
> *
> * Arguments
> *
> * tty pointer to tty instance data
> * file pointer to associated file object for device
> * cmd IOCTL command code
> * arg command argument/context
> *
> * Return 0 if success, otherwise error code
> */
> static int ioctl(struct tty_struct *tty, struct file *file,
> unsigned int cmd, unsigned long arg)
> {
> SLGT_INFO *info = (SLGT_INFO *)tty->driver_data;

Unneeded cast from void* (through the whole file, please).

> /* Append serial signal status to end */
> ret += sprintf(buf+ret, " %s\n", stat_buf+1);
>
> ret += sprintf(buf+ret, "\ttxactive=%d bh_req=%d bh_run=%d pending_bh=%x\n",
> info->tx_active,info->bh_requested,info->bh_running,
> info->pending_bh);

whitespace funnies.

> static void bh_handler(void* context)
> {
> SLGT_INFO *info = (SLGT_INFO*)context;
> int action;
>
> if (!info)
> return;
> info->bh_running = 1;

The handling of bh_running seems vague. Is some locking needed around it?
Memory barriers, at least?

> static void ri_change(SLGT_INFO *info)
> {
> get_signals(info);
> DBGISR(("ri_change %s signals=%04X\n", info->device_name, info->signals));
> if ((info->ri_chkcount)++ == IO_PIN_SHUTDOWN_LIMIT) {
> slgt_irq_off(info, IRQ_RI);
> return;
> }
> info->icount.dcd++;
> if (info->signals & SerialSignal_RI) {
> info->input_signal_events.ri_up++;
> } else {
> info->input_signal_events.ri_down++;
> }

whitespace

> /* interrupt service routine
> *
> * irq interrupt number
> * dev_id device ID supplied during interrupt registration
> * regs interrupted processor context
> */
> static irqreturn_t slgt_interrupt(int irq, void *dev_id, struct pt_regs * regs)
> {
> SLGT_INFO *info;
> unsigned int gsr;
> unsigned int i;
>
> DBGISR(("slgt_interrupt irq=%d entry\n", irq));
>
> info = (SLGT_INFO *)dev_id;
> if (!info)
> return IRQ_NONE;
>
> spin_lock(&info->lock);
>
> while((gsr = rd_reg32(info, GSR) & 0xffffff00)) {
> DBGISR(("%s gsr=%08x\n", info->device_name, gsr));
> info->irq_occurred = 1;
> for(i=0; i < info->port_count ; i++) {
> if (info->port_array[i] == NULL)
> continue;
> if (gsr & (BIT8 << i))
> isr_serial(info->port_array[i]);
> if (gsr & (BIT16 << (i*2)))
> isr_rdma(info->port_array[i]);
> if (gsr & (BIT17 << (i*2)))
> isr_tdma(info->port_array[i]);
> }
> }

Many ISRs are designed to special-case an irq-pending value of 0xffffffff
so they gracefully handle device unplug. I guess this device doesn't come
in cardbus (yet), but maybe the same handling is needed for PCI hotplug.

> static void shutdown(SLGT_INFO * info)
> {
> unsigned long flags;
>
> if (!(info->flags & ASYNC_INITIALIZED))
> return;
>
> DBGINFO(("%s shutdown\n", info->device_name));
>
> /* clear status wait queue because status changes */
> /* can't happen after shutting down the hardware */
> wake_up_interruptible(&info->status_event_wait_q);
> wake_up_interruptible(&info->event_wait_q);
>
> del_timer(&info->tx_timer);
> del_timer(&info->rx_timer);

del_timer_sync() needed here - otherwise the timer handler could still be
in progress.

> spin_lock_irqsave(&info->lock,flags);
> set_signals(info);
> spin_unlock_irqrestore(&info->lock,flags);

This happens so often that it's worth creating a new function for it.
Ditto get_signals().

>
> static void free_tmp_rbuf(SLGT_INFO *info)
> {
> if (info->tmp_rbuf)
> kfree(info->tmp_rbuf);

kfree(NULL) is legal.

> memset(info->bufs, 0, DESC_LIST_SIZE);
>
> info->rbufs = (SLGT_DESC*)info->bufs;
> info->tbufs = ((SLGT_DESC*)info->bufs) + info->rbuf_count;
>
> pbufs = (unsigned int)info->bufs_dma_addr;

No, it shouldn't put a dma_addr_t into an unsigned int. dma_addr_t is
sometimes 64-bit on 32-bit machines. Basically, it should remain opaque.

> if (request_irq(port_array[0]->irq_level,
> slgt_interrupt,
> port_array[0]->irq_flags,
> port_array[0]->device_name,
> port_array[0]) < 0) {

The irq_flags seems a bit obfuscated. WHy not just remove it and hard-code
SA_SHIRQ?

<attention fades>

Boy, it's a big driver...
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-11-05 09:39    [W:0.681 / U:0.216 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site