[ patch 1/7] drivers/serial/jsm: new serial device driver
Wen Xiong
We are submitting a new serial driver for the 2.6 kernel. This device
driver is for the Digi Neo serial port adapter.
We made some changes based on great comments from linux community. We
used the Russell's serial_core interface on 2.6 kernel, handled all
initialization of module correctly and used fs/seq_file.c interface for
/proc entry. And we made some changes based on Greg KH's comments also
including removing whitespace, changing some function comments, removing
msleep() etc.
Per requests, I split it up in smaller chunks and sent them in several
emails for you. You are able to read the code and comment the code
easily. I added more adapter descriptions for you.
Neo adapter description:
This adapter is a 920K baud non-intelligent asynchronous serial
communications adapter for PCI bus. The adapter is based on Exar 17D152
Universal PCI Dual UART IC. The adapter is mapped into the PCI bus
memory space, and offers extened UART register mapping beyond the
standard 16c554 registers. This driver used serial_core.c interface on
2.6 serials of kernel.
Thanks for all your help!
wendy
This first patch included jsm_driver.c. This routine included all PCI
initialization functions for this driver.
Signed-off-by: Wen Xiong <wen...@us.ltcfwd.linux.ibm.com>
Kyle Moffett
> +/*
> + * jsm_init_globals()
> + *
> + * This is where we initialize the globals from the static insmod
> + * configuration variables. These are declared near the head of
> + * this file.
> + */
> +static void jsm_init_globals(void)
> +{
> + int i = 0;
> +
> + jsm_rawreadok = rawreadok;
> + jsm_trcbuf_size = trcbuf_size;
> + jsm_debug = debug;
> +
> + for (i = 0; i < MAXBOARDS; i++) {
> + jsm_Board[i] = NULL;
> + jsm_board_slot[i] = (char *)kmalloc(20, GFP_KERNEL);
> + memset(jsm_board_slot[i], 0, 20);
> + }
Instead of several 20-byte kmallocs, you could help reduce memory
usage and fragmentation with something like this:
static void *jsm_board_slot_mem;
jsm_board_slot_mem = kmalloc(20*MAXBOARDS, GFP_KERNEL);
memset(jsm_board_slot_mem, 0, 20*MAXBOARDS)
for (i = 0; i < MAXBOARDS; i++) {
jsm_Board[i] = NULL;
jsm_board_slot[i] = &jsm_board_slot_mem[20*i];
}
Then free like this:
kfree(jsm_board_slot_mem);
On the other hand, it might be nice to have all these structures
dynamically allocated, so that the no-boards case only uses the 8 or
16 bytes of RAM required for a struct list_head. In that case you
could clump the other board info into a single struct instead of
multiple independent static arrays. Something like this might work:
struct jsm_board_instance {
struct list_head board_list;
struct board_t board;
char slot[20];
[...etc...]
};
static struct list_head jsm_board_list = LIST_HEAD_INIT(jsm_board_list);
static spinlock_t jsm_board_list_lock = SPIN_LOCK_UNLOCKED;
Then when doing hardware add/remove, take the lock and do the list
manipulation, then unlock.
Cheers,
Kyle Moffett
-----BEGIN GEEK CODE BLOCK-----
Version: 3.12
GCM/CS/IT/U d- s++: a18 C++++>$ UB/L/X/*++++(+)>$ P+++(++++)>$
L++++(+++) E W++(+) N+++(++) o? K? w--- O? M++ V? PS+() PE+(-) Y+
PGP+++ t+(+++) 5 X R? tv-(--) b++++(++) DI+ D+ G e->++++$ h!*()>++$ r
!y?(-)
------END GEEK CODE BLOCK------
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Jeff Garzik
General comment: This driver has -way- too many global variables.
Normal drivers should associate state information with each instance of
a device (e.g. each struct pci_dev), not globally.
> +/*
> + * Globals
> + */
> +uint jsm_NumBoards,current_NumBoards;
> +struct board_t *jsm_Board[MAXBOARDS];
> +char *jsm_board_slot[MAXBOARDS];
> +spinlock_t jsm_global_lock = SPIN_LOCK_UNLOCKED;
> +int jsm_driver_state = DRIVER_INITIALIZED;
> +ulong jsm_poll_counter;
> +uint jsm_Major;
> +int jsm_debug;
> +int jsm_rawreadok;
> +int jsm_trcbuf_size;
Eliminate MAXBOARDS. Such static limits are completely unnecessary.
> +/*
> + * Static vars.
> + */
> +static uint jsm_major_control_registered = FALSE;
> +static uint jsm_driver_start = FALSE;
Eliminate jsm_driver_start. This is unnecessary also.
> +/*
> + * Poller stuff
> + */
> +static spinlock_t jsm_poll_lock = SPIN_LOCK_UNLOCKED;
> +static ulong jsm_poll_time; /* Time of next poll */
> +static ulong jsm_poll_tick = 50; /* Poll interval - 20 ms */
> +
> +static struct timer_list jsm_poll_timer = {
> + .function = jsm_poll_handler
> +};
> +
> +static struct pci_device_id jsm_pci_tbl[] = {
> + { DIGI_VID, PCI_DEVICE_NEO_4_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
> + { DIGI_VID, PCI_DEVICE_NEO_8_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 1 },
> + { DIGI_VID, PCI_DEVICE_NEO_2DB9_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 2 },
> + { DIGI_VID, PCI_DEVICE_NEO_2DB9PRI_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 3 },
> + { DIGI_VID, PCI_DEVICE_NEO_2RJ45_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 4 },
> + { DIGI_VID, PCI_DEVICE_NEO_2RJ45PRI_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 5 },
> + { DIGI_VID, PCI_DEVICE_NEO_1_422_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 6 },
> + { DIGI_VID, PCI_DEVICE_NEO_1_422_485_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 7 },
> + { DIGI_VID, PCI_DEVICE_NEO_2_422_485_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 8 },
> + {0,} /* 0 terminated list. */
Don't create your own constants, use standard linux/pci_ids.h ones.
> +MODULE_DEVICE_TABLE(pci, jsm_pci_tbl);
> +
> +struct board_id {
> + uchar *name;
> + uint maxports;
> +};
> +
> +static struct board_id jsm_Ids[] = {
> + { PCI_DEVICE_NEO_4_PCI_NAME, 4 },
> + { PCI_DEVICE_NEO_8_PCI_NAME, 8 },
> + { PCI_DEVICE_NEO_2DB9_PCI_NAME, 2 },
> + { PCI_DEVICE_NEO_2DB9PRI_PCI_NAME, 2 },
> + { PCI_DEVICE_NEO_2RJ45_PCI_NAME, 2 },
> + { PCI_DEVICE_NEO_2RJ45PRI_PCI_NAME, 2 },
> + { PCI_DEVICE_NEO_1_422_PCI_NAME, 1 },
> + { PCI_DEVICE_NEO_1_422_485_PCI_NAME, 1 },
> + { PCI_DEVICE_NEO_2_422_485_PCI_NAME, 2 },
> + { NULL, 0 }
> +};
> +
> +struct pci_driver jsm_driver = {
> + .name = "jsm",
> + .probe = jsm_init_one,
> + .id_table = jsm_pci_tbl,
> + .remove = __devexit_p(jsm_remove_one),
> +};
> +
> +char *jsm_state_text[] = {
> + "Board Failed",
> + "Board Found",
> + "Board READY",
> +};
> +
> +char *jsm_driver_state_text[] = {
> + "Driver Initialized",
> + "Driver Ready."
> +};
> +
> +/*
> + * jsm_init_globals()
> + *
> + * This is where we initialize the globals from the static insmod
> + * configuration variables. These are declared near the head of
> + * this file.
> + */
> +static void jsm_init_globals(void)
> +{
> + int i = 0;
> +
> + jsm_rawreadok = rawreadok;
> + jsm_trcbuf_size = trcbuf_size;
> + jsm_debug = debug;
> +
> + for (i = 0; i < MAXBOARDS; i++) {
> + jsm_Board[i] = NULL;
> + jsm_board_slot[i] = (char *)kmalloc(20, GFP_KERNEL);
> + memset(jsm_board_slot[i], 0, 20);
> + }
> +
> + init_timer(&jsm_poll_timer);
> +}
When MAXBOARDS is eliminated, and the poll timer is made per-device,
this code will go away.
> +/*
> + * jsm_poll_handler
> + *
> + * As each timer expires, it determines (a) whether the "transmit"
> + * waiter needs to be woken up, and (b) whether the poller needs to
> + * be rescheduled.
> + */
> +static void jsm_poll_handler(ulong dummy)
> +{
> + struct board_t *brd;
> + unsigned long lock_flags;
> + int i;
> +
> + jsm_poll_counter++;
> +
> + /*
> + * Do not start the board state machine until
> + * driver tells us its up and running, and has
> + * everything it needs.
> + */
> + if (jsm_driver_state != DRIVER_READY)
> + goto schedule_poller;
> +
> + /* Go thru each board, kicking off a tasklet for each if needed */
> + for (i = 0; i < jsm_NumBoards; i++) {
> + if (jsm_Board[i] == NULL) continue;
> + brd = jsm_Board[i];
> +
> + spin_lock_irqsave(&brd->bd_lock, lock_flags);
> +
> + /* If board is in a failed state, don't bother scheduling a tasklet */
> + if (brd->state == BOARD_FAILED) {
> + spin_lock_irqsave(&brd->bd_lock, lock_flags);
> + continue;
> + }
> +
> + spin_unlock_irqrestore(&brd->bd_lock, lock_flags);
> + }
> +
> +schedule_poller:
> +
> + /*
> + * Schedule ourself back at the nominal wakeup interval.
> + */
> + if (current_NumBoards >= 0) {
> + ulong time;
> +
> + spin_lock_irqsave(&jsm_poll_lock, lock_flags);
> + jsm_poll_time += jsm_jiffies_from_ms(jsm_poll_tick);
> +
> + time = jsm_poll_time - jiffies;
> +
> + if ((ulong) time >= 2 * jsm_poll_tick)
> + jsm_poll_time = jiffies + jsm_jiffies_from_ms(jsm_poll_tick);
> +
> + jsm_poll_timer.expires = jsm_poll_time;
> + spin_unlock_irqrestore(&jsm_poll_lock, lock_flags);
> +
> + add_timer(&jsm_poll_timer);
> + }
> +}
Don't use a single timer for all boards. This makes information
needlessly global.
> +/*
> + * Start of driver.
> + */
> +static int jsm_start(void)
> +{
> + int rc = 0;
> + unsigned long lock_flags;
> +
> + if (!jsm_driver_start) {
> + jsm_driver_start = TRUE;
As noted earlier, jsm_driver_start is completely pointless.
jsm_start() is the first function called in module_init(), and is never
called from anywhere else.
> + /* make sure that the globals are init'd before we do anything else */
> + jsm_init_globals();
> + jsm_NumBoards = 0;
> + current_NumBoards = -1;
> +
> + APR(("For the tools package or updated drivers please visit http://www.digi.com\n"));
> +
> + /*
> + * Register our base character device into the kernel.
> + * This allows the download daemon to connect to the downld device
> + * before any of the boards are init'ed.
> + */
> + if (!jsm_major_control_registered) {
> + /*
> + * Register management/dpa devices
> + */
> + rc = register_chrdev(0, "jsm", &jsm_BoardFops);
> + if (rc <= 0) {
> + APR(("Can't register jsm driver device (%d)\n", rc));
> + return -ENXIO;
> + }
> + jsm_Major = rc;
> + jsm_major_control_registered = TRUE;
> + }
> +
> + /*
> + * Register our basic stuff in /proc/jsm
> + */
> + jsm_proc_register_basic_prescan();
> +
> + if (rc < 0) {
> + APR(("tty preinit - not enough memory (%d)\n", rc));
> + return rc;
> + }
Why is this not in sysfs?
> + /* Start the poller */
> + spin_lock_irqsave(&jsm_poll_lock, lock_flags);
> + jsm_poll_time = jiffies + jsm_jiffies_from_ms(jsm_poll_tick);
> + jsm_poll_timer.expires = jsm_poll_time;
> + spin_unlock_irqrestore(&jsm_poll_lock, lock_flags);
> +
> + add_timer(&jsm_poll_timer);
It's silly to add a timer and start polling, when you have nothing to poll.
But that's ok... this code will go away when the poll timer is made
per-device.
> + jsm_driver_state = DRIVER_READY;
> + }
> + return rc;
> +}
> +
> +static int jsm_finalize_board_init(struct board_t *brd)
> +{
> + int rc = 0;
> +
> + DPR_INIT(("jsm_finalize_board_init() - start\n"));
> +
> + if (!brd || brd->magic != JSM_BOARD_MAGIC)
> + return -ENODEV;
> +
> + DPR_INIT(("jsm_finalize_board_init() - start #2\n"));
> +
> + if (brd->irq) {
> + rc = request_irq(brd->irq, brd->bd_ops->intr, SA_INTERRUPT|SA_SHIRQ, "JSM", brd);
> +
> + if (rc) {
> + printk(KERN_WARNING "Failed to hook IRQ %d\n",brd->irq);
> + brd->state = BOARD_FAILED;
> + brd->dpastatus = BD_NOFEP;
> + rc = -ENODEV;
> + } else {
> + DPR_INIT(("Requested and received usage of IRQ %d\n", brd->irq));
> + }
> + }
> + return rc;
> +}
> +
> +/*
> + * jsm_found_board()
> + *
> + * A board has been found, init it.
> + */
> +static int jsm_found_board(struct pci_dev *pdev, int id)
> +{
> + struct board_t *brd;
> + unsigned int pci_irq;
> + int i = 0;
> + int rc = 0;
> + int index;
> + int wen_board;
> + int hotplug = 0;
> +
> + wen_board = jsm_NumBoards;
> + for (index = 0; index < jsm_NumBoards; index++) {
> + if (!strcmp(jsm_board_slot[index], pdev->slot_name)) {
> + wen_board = index;
> + hotplug = 1;
> + break;
> + }
> + }
It is very wrong to store boards based on pdev->slot_name.
However, this goes away when more state is made per-device.
> + brd = jsm_Board[wen_board] =
> + (struct board_t *)kmalloc(sizeof(struct board_t), GFP_KERNEL);
> + if (!brd) {
> + APR(("memory allocation for board structure failed\n"));
> + return -ENOMEM;
> + }
> + memset(brd, 0, sizeof(struct board_t));
> +
> + /* store the info for the board we've found */
> + brd->magic = JSM_BOARD_MAGIC;
> + brd->boardnum = wen_board;
> + brd->vendor = jsm_pci_tbl[id].vendor;
> + brd->device = jsm_pci_tbl[id].device;
> + brd->pci_bus = pdev->bus->number;
> + brd->pci_slot = PCI_SLOT(pdev->devfn);
You should not need this information.
> + brd->pci_dev = pdev;
> + brd->name = jsm_Ids[id].name;
> + brd->maxports = jsm_Ids[id].maxports;
> + brd->dpastatus = BD_NOFEP;
> + strcpy(jsm_board_slot[wen_board],pdev->slot_name);
> + init_waitqueue_head(&brd->state_wait);
> +
> + spin_lock_init(&brd->bd_lock);
> + spin_lock_init(&brd->bd_intr_lock);
> +
> + brd->state = BOARD_FOUND;
> +
> + for (i = 0; i < MAXPORTS; i++)
> + brd->channels[i] = NULL;
> +
> + /* store which card & revision we have */
> + pci_read_config_word(pdev, PCI_SUBSYSTEM_VENDOR_ID, &brd->subvendor);
> + pci_read_config_word(pdev, PCI_SUBSYSTEM_ID, &brd->subdevice);
Obtain this information from struct pci_dev. Do not read it manually.
> + pci_irq = pdev->irq;
> + brd->irq = pci_irq;
> +
> + switch(brd->device) {
> +
> + case PCI_DEVICE_NEO_4_DID:
> + case PCI_DEVICE_NEO_8_DID:
> + case PCI_DEVICE_NEO_2DB9_DID:
> + case PCI_DEVICE_NEO_2DB9PRI_DID:
> + case PCI_DEVICE_NEO_2RJ45_DID:
> + case PCI_DEVICE_NEO_2RJ45PRI_DID:
> + case PCI_DEVICE_NEO_1_422_DID:
> + case PCI_DEVICE_NEO_1_422_485_DID:
> + case PCI_DEVICE_NEO_2_422_485_DID:
use standard pci_ids.h constants.
> + /*
> + * This chip is set up 100% when we get to it.
> + * No need to enable global interrupts or anything.
> + */
> + brd->dpatype = T_NEO | T_PCIBUS;
> +
> + DPR_INIT(("jsm_found_board - NEO.\n"));
> +
> + /* get the PCI Base Address Registers */
> + brd->membase = pci_resource_start(pdev, 0);
> + brd->membase_end = pci_resource_end(pdev, 0);
> +
> + if (brd->membase & 1)
> + brd->membase &= ~3;
> + else
> + brd->membase &= ~15;
> +
> + /* Assign the board_ops struct */
> + brd->bd_ops = &jsm_neo_ops;
> +
> + brd->bd_uart_offset = 0x200;
> + brd->bd_dividend = 921600;
> +
> + brd->re_map_membase = ioremap(brd->membase, 0x1000);
> + DPR_INIT(("remapped mem: 0x%p\n", brd->re_map_membase));
> + if (!brd->re_map_membase) {
> + kfree(brd);
> + APR(("card has no PCI Memory resources, failing board.\n"));
> + return -ENOMEM;
> + }
> + break;
> +
> + default:
> + APR(("Did not find any compatible Neo or Classic PCI boards in system.\n"));
> + kfree(brd);
> + return -ENXIO;
> + }
> +
> + /*
> + * Do tty device initialization.
> + */
> + rc = jsm_finalize_board_init(brd);
> + if (rc < 0) {
> + APR(("Can't finalize board init (%d)\n", rc));
> + brd->state = BOARD_FAILED;
> + brd->dpastatus = BD_NOFEP;
> + goto failed;
> + }
> +
> + rc = jsm_tty_register(brd);
> + if (rc < 0) {
> + jsm_tty_uninit(brd);
> + APR(("Can't register tty devices (%d)\n", rc));
> + brd->state = BOARD_FAILED;
> + brd->dpastatus = BD_NOFEP;
> + goto failed;
> + }
> +
> + rc = jsm_tty_init(brd);
> + if (rc < 0) {
> + jsm_tty_uninit(brd);
> + APR(("Can't init tty devices (%d)\n", rc));
> + brd->state = BOARD_FAILED;
> + brd->dpastatus = BD_NOFEP;
> + goto failed;
> + }
> +
> + rc = jsm_uart_port_init(brd);
> + if (rc < 0)
> + goto failed;
> +
> + brd->state = BOARD_READY;
> + brd->dpastatus = BD_RUNNING;
> +
> + /* init our poll helper tasklet */
> + tasklet_init(&brd->helper_tasklet, brd->bd_ops->tasklet, (unsigned long) brd);
> +
> + /* Log the information about the board */
> + APR(("board %d: %s (rev %d), irq %d\n",wen_board, brd->name, brd->rev, brd->irq));
> +
> + /*
> + * allocate flip buffer for board.
> + *
> + * Okay to malloc with GFP_KERNEL, we are not at interrupt
> + * context, and there are no locks held.
> + */
> + brd->flipbuf = kmalloc(MYFLIPLEN, GFP_KERNEL);
> + if (!brd->flipbuf) {
> + APR(("memory allocation for flipbuf failed\n"));
> + return -ENOMEM;
> + }
leak on error
> + memset(brd->flipbuf, 0, MYFLIPLEN);
> +
> + jsm_proc_register_basic_postscan(wen_board);
> + wake_up_interruptible(&brd->state_wait);
> +
> + if (!hotplug)
> + jsm_NumBoards++;
> +
> + current_NumBoards++;
> +
> + return 0;
> +
> +failed:
> + kfree(brd);
> + iounmap((void *) brd->re_map_membase);
> + return -ENXIO;
> +}
> +
> +
> +static int jsm_probe1(struct pci_dev *pdev, int card_type)
> +{
> + return jsm_found_board(pdev, card_type);
> +}
eliminate this needless function.
> +/* returns count (>= 0), or negative on error */
> +static int jsm_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> +{
> + int rc;
> +
> + rc = pci_enable_device(pdev);
> + if (rc) {
> + dev_err(&pdev->dev, "Device enable FAILED\n");
> + return rc;
> + }
> +
> + if ((rc = jsm_probe1(pdev, ent->driver_data))) {
> + dev_err(&pdev->dev, "jsm_probe1 FAILED\n");
> + pci_disable_device(pdev);
> + return rc;
> + }
> + return rc;
> +}
Missing pci_request_regions().
> +/*
> + * jsm_cleanup_board()
> + *
> + * Free all the memory associated with a board
> + */
> +static void jsm_cleanup_board(struct board_t *brd)
> +{
> + int i = 0;
> +
> + if (!brd || brd->magic != JSM_BOARD_MAGIC)
> + return ;
> +
> + if (brd->irq)
> + free_irq(brd->irq, brd);
> +
> + tasklet_kill(&brd->helper_tasklet);
> +
> + if (brd->re_map_membase) {
> + iounmap(brd->re_map_membase);
> + brd->re_map_membase = NULL;
> + }
When will this 'if' test ever fail?
> + /* Free all allocated channels structs */
> + for (i = 0; i < MAXPORTS; i++) {
> + if (brd->channels[i]) {
> + if (brd->channels[i]->ch_rqueue)
> + kfree(brd->channels[i]->ch_rqueue);
> + if (brd->channels[i]->ch_equeue)
> + kfree(brd->channels[i]->ch_equeue);
> + if (brd->channels[i]->ch_wqueue)
> + kfree(brd->channels[i]->ch_wqueue);
> +
> + kfree(brd->channels[i]);
> + brd->channels[i] = NULL;
> + }
> + }
> +
> + if (brd->flipbuf)
> + kfree(brd->flipbuf);
> +
> + jsm_Board[brd->boardnum] = NULL;
> +
> + kfree(brd);
> + current_NumBoards--;
> +}
> +
> +static void jsm_remove_one(struct pci_dev *dev)
> +{
> + int i;
> +
> + for (i = 0; i < jsm_NumBoards; i++) {
> + if ((jsm_Board[i] != NULL) && (jsm_Board[i]->pci_dev == dev)) {
> + jsm_proc_unregister_brd(i);
> + jsm_remove_uart_port(jsm_Board[i]);
> + jsm_tty_uninit(jsm_Board[i]);
> + jsm_cleanup_board(jsm_Board[i]);
> + }
> + }
> +}
pci_disable_device(), pci_release_regions()
> +/*
> + * jsm_init_module()
> + *
> + * Module load. This is where it all starts.
> + */
> +static int __init
> +jsm_init_module(void)
> +{
> + int rc = 0;
> +
> + APR(("%s, Digi International Part Number %s\n", "jsm: 1.1-1-INKERNEL", "40002438_A-INKERNEL"));
> +
> + /*
> + * Initialize global stuff
> + */
> + rc = jsm_start();
> + if (rc < 0)
> + return rc;
> +
> + rc = uart_register_driver(&jsm_uart_driver);
> + if (rc)
> + return rc;
leak on error
> + rc = pci_register_driver(&jsm_driver);
> + if (rc < 0)
> + uart_unregister_driver(&jsm_uart_driver);
leak on error
> + return rc;
> +}
> +
> +module_init(jsm_init_module);
> +
> +/*
> + * jsm_exit_module()
> + *
> + * Module unload. This is where it all ends.
> + */
> +static void __exit
> +jsm_exit_module(void)
> +{
> + int i;
> +
> + del_timer_sync(&jsm_poll_timer);
> +
> + if (jsm_major_control_registered)
> + unregister_chrdev(jsm_Major, "jsm");
> +
> + pci_unregister_driver(&jsm_driver);
> +
> + jsm_proc_unregister_all();
> +
> + for (i = 0; i < MAXBOARDS; i++) {
> + jsm_board_slot[i] = NULL;
> + kfree(jsm_board_slot[i]);
> + }
> + uart_unregister_driver(&jsm_uart_driver);
> +}
> +module_exit(jsm_exit_module);
> +MODULE_LICENSE("GPL");
> +
> +/*
> + * jsm_ioctl_name()
> + *
> + * Returns a text version of each ioctl value.
> + */
> +char *jsm_ioctl_name(int cmd)
> +{
> + switch(cmd) {
> +
> + case TCGETA: return("TCGETA");
> + case TCGETS: return("TCGETS");
> + case TCSETA: return("TCSETA");
> + case TCSETS: return("TCSETS");
> + case TCSETAW: return("TCSETAW");
> + case TCSETSW: return("TCSETSW");
> + case TCSETAF: return("TCSETAF");
> + case TCSETSF: return("TCSETSF");
> + case TCSBRK: return("TCSBRK");
> + case TCXONC: return("TCXONC");
> + case TCFLSH: return("TCFLSH");
> + case TIOCGSID: return("TIOCGSID");
> +
> + case TIOCGETD: return("TIOCGETD");
> + case TIOCSETD: return("TIOCSETD");
> + case TIOCGWINSZ: return("TIOCGWINSZ");
> + case TIOCSWINSZ: return("TIOCSWINSZ");
> +
> + case TIOCMGET: return("TIOCMGET");
> + case TIOCMSET: return("TIOCMSET");
> + case TIOCMBIS: return("TIOCMBIS");
> + case TIOCMBIC: return("TIOCMBIC");
> +
> + /* from digi.h */
> + case DIGI_SETA: return("DIGI_SETA");
> + case DIGI_SETAW: return("DIGI_SETAW");
> + case DIGI_SETAF: return("DIGI_SETAF");
> + case DIGI_SETFLOW: return("DIGI_SETFLOW");
> + case DIGI_SETAFLOW: return("DIGI_SETAFLOW");
> + case DIGI_GETFLOW: return("DIGI_GETFLOW");
> + case DIGI_GETAFLOW: return("DIGI_GETAFLOW");
> + case DIGI_GETA: return("DIGI_GETA");
> + case DIGI_GEDELAY: return("DIGI_GEDELAY");
> + case DIGI_SEDELAY: return("DIGI_SEDELAY");
> + case DIGI_GETCUSTOMBAUD: return("DIGI_GETCUSTOMBAUD");
> + case DIGI_SETCUSTOMBAUD: return("DIGI_SETCUSTOMBAUD");
> + case TIOCMODG: return("TIOCMODG");
> + case TIOCMODS: return("TIOCMODS");
> + case TIOCSDTR: return("TIOCSDTR");
> + case TIOCCDTR: return("TIOCCDTR");
> +
> + default: return("unknown");
> + }
Eliminate this, or make it debug-only.
Jeff
Jeff Garzik
Agree with your initial comment, but you missed an overall point:
MAXBOARDS must be eliminated completely. We do not impose such limits
in Linux.
Information should be allocated on a per-device basis.
> On the other hand, it might be nice to have all these structures
> dynamically allocated, so that the no-boards case only uses the 8 or
> 16 bytes of RAM required for a struct list_head. In that case you
> could clump the other board info into a single struct instead of
> multiple independent static arrays. Something like this might work:
>
> struct jsm_board_instance {
> struct list_head board_list;
> struct board_t board;
> char slot[20];
> [...etc...]
> };
> static struct list_head jsm_board_list = LIST_HEAD_INIT(jsm_board_list);
> static spinlock_t jsm_board_list_lock = SPIN_LOCK_UNLOCKED;
>
> Then when doing hardware add/remove, take the lock and do the list
> manipulation, then unlock.
Correct-a-mundo!
This is the way it should be done.
Jeff
Greg KH
> +static struct pci_device_id jsm_pci_tbl[] = {
> + { DIGI_VID, PCI_DEVICE_NEO_4_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
> + { DIGI_VID, PCI_DEVICE_NEO_8_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 1 },
> + { DIGI_VID, PCI_DEVICE_NEO_2DB9_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 2 },
> + { DIGI_VID, PCI_DEVICE_NEO_2DB9PRI_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 3 },
> + { DIGI_VID, PCI_DEVICE_NEO_2RJ45_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 4 },
> + { DIGI_VID, PCI_DEVICE_NEO_2RJ45PRI_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 5 },
> + { DIGI_VID, PCI_DEVICE_NEO_1_422_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 6 },
> + { DIGI_VID, PCI_DEVICE_NEO_1_422_485_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 7 },
> + { DIGI_VID, PCI_DEVICE_NEO_2_422_485_DID, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 8 },
Use the PCI_DEVICE() macro to make this smaller.
thanks,
greg k-h
Wen Xiong
>> + *
>> + * Okay to malloc with GFP_KERNEL, we are not at interrupt
>> + * context, and there are no locks held.
>> + */
>> + brd->flipbuf = kmalloc(MYFLIPLEN, GFP_KERNEL);
>> + if (!brd->flipbuf) {
>> + APR(("memory allocation for flipbuf failed\n"));
>> + return -ENOMEM;
>> + }
>
>
> leak on error
>
>
>>
>> +{
>> + return jsm_found_board(pdev, card_type);
>> +}
>
>
> eliminate this needless function.
>
>
>
>
>
>> +/*
>> + * jsm_cleanup_board()
>> + *
>> + * Free all the memory associated with a board
>> + */
>> +static void jsm_cleanup_board(struct board_t *brd)
>> +{
>> + int i = 0;
>> +
>> + if (!brd || brd->magic != JSM_BOARD_MAGIC)
>> + return ;
>> +
>> + if (brd->irq)
>> + free_irq(brd->irq, brd);
>> +
>> + tasklet_kill(&brd->helper_tasklet);
>> +
>> + if (brd->re_map_membase) {
>> + iounmap(brd->re_map_membase);
>> + brd->re_map_membase = NULL;
>> + }
>
>
> When will this 'if' test ever fail?
>
>
>>
>> +
Hi All.
Based on very detail comments from Jeff, Greg, Christoph Hellwig, Rik
and Nish, I modified these codes and tested it succesfully in our lab.
For patch1, major changes included:
1. removed static board limit to use dynamic list to control board
structure.
2. leak on errors.
3. removed some global variables
lots of others.
Thanks for all your help!
wendy
Signed-off-by: Wen Xiong <wen...@us.ltcfwd.linux.ibm.com>
Jeff Garzik
Please resubmit the entire series to LKML, so we can review it in total.
Jeff