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
The complete driver in non patch form is at
http://www.microgate.com/ftp/linux/synclink_gt-2.6.14/synclink_gt.c
This driver had been tested for a few months and these
particular patches have been tested against 2.6.14
--
Paul Fulghum
[email protected]
Paul Fulghum <[email protected]> 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...
On Sat, 2005-11-05 at 00:36 -0800, Andrew Morton wrote:
> 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
OK, the view on this seems to change frequently
and depends on who you are talking to, so I'm
never sure which way to go on a particular day.
My preference was to have a single patch, but the
view that patches absolutely must be broken into
tiny pieces even if the pieces make no sense
on their own seems to pop up a lot.
> - 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.
DMA memory (descriptors and data buffers for the rings)
are allocated with pci_alloc_consistent().
> - It seems to be both a tty driver and a net driver, or something. Can
> you please describe the hardware?
Yes, it is both a tty and net (generic hdlc) driver. The hardware
is synchronous and asynchronous and can be used for normal async tty,
hdlc tty, or hdlc networking.
> - 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.
Crap
> eww. Please just open-code these.
These are left over from a vain attempt to decouple the
driver from the kernel API change du jour (as per
your previous comment). I will remove them.
> None of these initialisations to zero are needed.
OK
> MODULE_PARM_DESCs needed, please.
OK
> typedefs are unpopular in new code.
hmmmm, if that is the latest style then I'll change it.
> 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.
It enforces proper endian handling, field access,
and bit positions. As you say, it is more reliable.
It also makes the code more readable for me.
> Please use the
> .field = value,
> form here.
OK
> Is silent droppage on overflow OK?
There is no return code in the API to indicate error.
The caller should monitor the write room before calling.
There is really nothing this function can do if too
much data is blindly pushed at it.
> Unneeded cast from void* (through the whole file, please).
OK
> whitespace funnies.
I read them every Sunday. Ha.
I'll fix it.
> The handling of bh_running seems vague.
It is done so if the handler is already running,
no attempt is made to schedule or run it again.
> 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.
There probably will be a cardbus version.
The definition of that register does not guarantee that
0xffffffff won't happen in normal operation.
But I can add a check to a different register as a
hardware present check similar to the synclink_cs
driver for the 16-bit PCMCIA card.
> del_timer_sync() needed here - otherwise the timer handler could still be
> in progress.
OK
> This happens so often that it's worth creating a new function for it.
> Ditto get_signals().
hmmm, I want to ponder that some and review how often it is used
I've always though that get_signals_locked() type wrappers
were ugly, but if it saves significant code size it make sense.
> kfree(NULL) is legal.
OK
> > 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.
This is the 32 bit physical bus address passed to the hardware
in the descriptor. The hardware DMA engine does not handle
64 bit addresses. The type should be changed to _u32 though.
> The irq_flags seems a bit obfuscated. WHy not just remove it and hard-code
> SA_SHIRQ?
This one driver will (hopefully) handle all the variants of
this family which may or may not share the same flag settings.
Future kernel changes may add or remove more flags which may
or may not apply to a particular variant.
I would prefer to leave it in place now rather than
having to add it back in later to accommodate such changes.
> <attention fades>
>
> Boy, it's a big driver...
It does a lot, but I've worked to reduce the size as much
as possible. It is an improvement over the original synclink
driver, which is a little crufty after being dragged through
the kernel over the last 8 years.
--
Paul Fulghum
Microgate Systems, Ltd.