Jeff Garzik wrote:

> Wen Xiong wrote:
>
>
> 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.
>
>
>>
>> +     *
>> +     * 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?
>
>
>>
>> +
>> +    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
>
>
>
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 <wendyx@us.ltcfwd.linux.ibm.com>