On 02/19/2014 03:17 PM, Dan Carpenter wrote:
> This one has white space problems.  Run scripts/checkpatch.pl on your
> patches before sending.  Comments below.
> 
> On Wed, Feb 19, 2014 at 01:12:15PM -0500, Mark Hounschell wrote:
>> +static int dgap_firmware_load(struct pci_dev *pdev, int card_type)
>> +{
>> +    struct board_t *brd = dgap_Board[dgap_NumBoards - 1];
>> +    const struct firmware *fw;
>> +    char *uaddr;
>> +    int ret;
>> +    
>> +    dgap_get_vpd(brd);
>> +    dgap_do_reset_board(brd);
>> +
>> +    if ((fw_info[card_type].conf_name) && 
>> +        (dgap_driver_state == DRIVER_NEED_CONFIG_LOAD)) {
>> +            ret = request_firmware(&fw, fw_info[card_type].conf_name, 
>> &pdev->dev);
>> +            if (ret) {
>> +                    printk(KERN_ERR "DGAP: request_firmware failed. Make 
>> sure "
>> +                                    "you've placed '%s' file into your 
>> firmware "
>> +                                    "loader directory (e.g. 
>> /lib/firmware)\n",
>> +                                    fw_info[card_type].conf_name);
>> +                    return ret; 
>> +            } else {
>> +                    if (!dgap_config_buf) {
>> +                            uaddr = dgap_config_buf = kmalloc(fw->size + 1, 
>> GFP_ATOMIC);
>> +                            if (!dgap_config_buf) {
>> +                                    DPR_INIT(("DGAP: dgap_firmware_load - 
>> unable to allocate memory for config file\n"));
> 
> Don't print an error if kmalloc() fails.  kmalloc() prints a far more
> useful error message already before returning NULL.
> 
>> +                                    release_firmware(fw);
>> +                                    return -ENOMEM;
>> +                            }
>> +                    }
>> +
>> +                    memcpy(uaddr, fw->data, fw->size);
>> +                    release_firmware(fw);
>> +                    uaddr[fw->size + 1] = '\0';     // The config file is 
>> treated as a string
>> +
>> +                    if (dgap_parsefile(&uaddr, TRUE) != 0) 
>> +                            return -EINVAL;
>> +
>> +                    dgap_driver_state = -1;
>> +            }
> 
> You are going over the 80 character limit unnecessarily.  Change the
> "} else {" to "}" and pull everything in one indent level.
> 
> Get rid of the uaddr temporary variable.  It is not needed.  Actually
> there is a bug where it is used uninitialized.  I haven't tried to
> compile this, doesn't GCC complain?
> 
> 
>> +    }
>> +
>> +    ret = dgap_after_config_loaded(brd->boardnum);
>> +    if (ret != 0)
> 
>       if (ret)
>               return ret;
> 
> The "!= 0" double negative doesn't add anything.
> 
>> +            return ret;
>> +    /*
>> +     * Match this board to a config the user created for us.
>> +     */
>> +    brd->bd_config = dgap_find_config(brd->type, brd->pci_bus, 
>> brd->pci_slot);
>> +
>> +    /*
>> +     * Because the 4 port Xr products share the same PCI ID
>> +     * as the 8 port Xr products, if we receive a NULL config
>> +     * back, and this is a PAPORT8 board, retry with a
>> +     * PAPORT4 attempt as well.
>> +     */
>> +    if (brd->type == PAPORT8 && !brd->bd_config)
>> +            brd->bd_config = dgap_find_config(PAPORT4, brd->pci_bus, 
>> brd->pci_slot);
>> +
>> +    if (!brd->bd_config) {
>> +            printk(KERN_ERR "DGAP: dgap_firmware_load: No valid 
>> configuration found\n");
>> +            return -EINVAL;
>> +    }
>> +    
>> +    dgap_tty_register(brd);
>> +    dgap_finalize_board_init(brd);
>> +
>> +    if (fw_info[card_type].bios_name) {
>> +            ret = request_firmware(&fw, fw_info[card_type].bios_name, 
>> &pdev->dev);
>> +            if (ret) {
>> +                    printk(KERN_ERR "DGAP: request_firmware failed. Make 
>> sure "
>> +                                    "you've placed '%s' file into your 
>> firmware "
>> +                                    "loader directory (e.g. 
>> /lib/firmware)\n",
>> +                                    fw_info[card_type].bios_name);
>> +                    return ret;
>> +            } else {
>> +                    uaddr = (char *)fw->data;
>> +                    dgap_do_bios_load(brd, uaddr, fw->size);
>> +                    release_firmware(fw);
>> +
>> +                    /* Wait for BIOS to test board... */
>> +                    dgap_do_wait_for_bios(brd);
>> +
>> +                    if (brd->state != FINISHED_BIOS_LOAD)
>> +                            return -ENXIO;
>> +            }
> 
> 
> Same thing.  Change "} else {" to "}".  In the kernel we do "error
> handling", this is "error handling followed by success handling."  We
> don't want special success handling.  Success is the default state.
> 
> No need for the uaddr variable.
> 
>> +    }
>> +
>> +        if (fw_info[card_type].fep_name) {
>> +                ret = request_firmware(&fw, fw_info[card_type].fep_name, 
>> &pdev->dev);
>> +                if (ret) {
>> +                        printk(KERN_ERR "DGAP: request_firmware failed. 
>> Make sure "
>> +                                        "you've placed '%s' file into your 
>> firmware "
>> +                                        "loader directory (e.g. 
>> /lib/firmware)\n",
>> +                                        fw_info[card_type].fep_name);
>> +                        return ret;
>> +                } else {
>> +                        uaddr = (char *)fw->data;
>> +                        dgap_do_fep_load(brd, uaddr, fw->size);
>> +                    release_firmware(fw);
>> +
>> +                        /* Wait for FEP to load on board... */
>> +                        dgap_do_wait_for_fep(brd);
>> +
>> +                        if (brd->state != FINISHED_FEP_LOAD)
>> +                                return -ENXIO;
>> +                }
>> +        }
>> +        
>> +#ifdef DIGI_CONCENTRATORS_SUPPORTED
>> +    /*
>> +     * If this is a CX or EPCX, we need to see if the firmware
>> +     * is requesting a concentrator image from us.
>> +     */
>> +    if ((bd->type == PCX) || (bd->type == PEPC)) {
>> +            chk_addr = (u16 *) (vaddr + DOWNREQ);
>> +            /* Nonzero if FEP is requesting concentrator image. */
>> +            check = readw(chk_addr);
>> +            vaddr = brd->re_map_membase;
>> +    }
>> +
>> +    if (fw_info[card_type].con_name && check && vaddr) {
>> +            ret = request_firmware(&fw, fw_info[card_type].con_name, 
>> &pdev->dev);
>> +            if (ret) {
>> +                    printk(KERN_ERR "DGAP: request_firmware failed. Make 
>> sure "
>> +                                    "you've placed '%s' file into your 
>> firmware "
>> +                                    "loader directory (e.g. 
>> /lib/firmware)\n",
>> +                                    fw_info[card_type].con_name);
>> +                    return ret;
>> +            } else {
>> +                    /* Put concentrator firmware loading code here */
>> +                    offset = readw((u16 *) (vaddr + DOWNREQ)); 
>> +                    memcpy_toio(offset, fw->data, fw->size);
>> +
>> +                        uaddr = (char *)fw->data;
>> +                    dgap_do_conc_load(brd, uaddr, fw->size)
>> +                    release_firmware(fw);
>> +            }
>> +    }
>> +#endif
>> +    /*
>> +     * Do tty device initialization.
>> +     */
>> +    ret = dgap_tty_init(brd); 
>> +    if (ret < 0) {
>> +            dgap_tty_uninit(brd);
>> +            printk("DGAP: dgap_firmware_load: Can't init tty devices 
>> (%d)\n", ret);
>> +            return -EIO;
> 
> return ret, don't hard code EIO here.
> 
>> +    }
>> +
>> +    dgap_sysfs_create(brd);
>> +
>> +    brd->state = BOARD_READY;
>> +    brd->dpastatus = BD_RUNNING;
>> +
>> +    return 0;
>> +}
>>  
>>  /*
>>   * Remap PCI memory.
>> @@ -983,37 +1221,18 @@ static void dgap_poll_handler(ulong dummy)
>>      int i;
>>          struct board_t *brd;
>>          unsigned long lock_flags;
>> -        unsigned long lock_flags2;
>>      ulong new_time;
>>  
>>      dgap_poll_counter++;
>>  
>>  
>>      /*
>> -     * If driver needs the config file still,
>> -     * keep trying to wake up the downloader to
>> -     * send us the file.
>> -     */
>> -        if (dgap_driver_state == DRIVER_NEED_CONFIG_LOAD) {
>> -            /*
>> -             * Signal downloader, its got some work to do.
>> -             */
>> -            DGAP_LOCK(dgap_dl_lock, lock_flags2);
>> -            if (dgap_dl_action != 1) {
>> -                    dgap_dl_action = 1;
>> -                    wake_up_interruptible(&dgap_dl_wait);
>> -            }
>> -            DGAP_UNLOCK(dgap_dl_lock, lock_flags2);
>> -            goto schedule_poller;
>> -        }
>> -    /*
>>       * Do not start the board state machine until
>>       * driver tells us its up and running, and has
>>       * everything it needs.
>>       */
>> -    else if (dgap_driver_state != DRIVER_READY) {
>> +    if (dgap_driver_state != DRIVER_READY)
>>              goto schedule_poller;
>> -    }
>>  
>>      /*
>>       * If we have just 1 board, or the system is not SMP,
>> @@ -4656,112 +4875,54 @@ static int dgap_tty_ioctl(struct tty_struct *tty, 
>> unsigned int cmd,
>>              return(-ENOIOCTLCMD);
>>      }
>>  }
>> -/*
>> - * Loads the dgap.conf config file from the user.
>> - */
>> -static void dgap_do_config_load(uchar __user *uaddr, int len)
>> -{
>> -    int orig_len = len;
>> -    char *to_addr;
>> -    uchar __user *from_addr = uaddr;
>> -    char buf[U2BSIZE];
>> -    int n;
>> -
>> -    to_addr = dgap_config_buf = kzalloc(len + 1, GFP_ATOMIC);
>> -    if (!dgap_config_buf) {
>> -            DPR_INIT(("dgap_do_config_load - unable to allocate memory for 
>> file\n"));
>> -            dgap_driver_state = DRIVER_NEED_CONFIG_LOAD;
>> -            return;
>> -    }
>>  
>> -    n = U2BSIZE;
>> -    while (len) {
>> -
>> -            if (n > len)
>> -                    n = len;
>> -
>> -            if (copy_from_user((char *) &buf, from_addr, n) == -1 )
>> -                    return;
>> -
>> -            /* Copy data from buffer to kernel memory */
>> -            memcpy(to_addr, buf, n);
>> -
>> -            /* increment counts */
>> -            len -= n;
>> -            to_addr += n;
>> -            from_addr += n;
>> -            n = U2BSIZE;
>> -    }
>> -
>> -    dgap_config_buf[orig_len] = '\0';
>> -
>> -    to_addr = dgap_config_buf;
>> -    dgap_parsefile(&to_addr, TRUE);
>> -
>> -    DPR_INIT(("dgap_config_load() finish\n"));
>> -
>> -    return;
>> -}
>> -
>> -
>> -static int dgap_after_config_loaded(void)
>> +static int dgap_after_config_loaded(int board)
>>  {
>> -    int i = 0;
>> -    int rc = 0;
>> +        int ret = 0;
>>  
>>      /*
>> -     * Register our ttys, now that we have the config loaded.
>> +     * Initialize KME waitqueues...
>>       */
>> -    for (i = 0; i < dgap_NumBoards; ++i) {
>> +    init_waitqueue_head(&(dgap_Board[board]->kme_wait));
>>  
>> -            /*
>> -             * Initialize KME waitqueues...
>> -             */
>> -            init_waitqueue_head(&(dgap_Board[i]->kme_wait));
>> -
>> -            /*
>> -             * allocate flip buffer for board.
>> -             */
>> -            dgap_Board[i]->flipbuf = kzalloc(MYFLIPLEN, GFP_ATOMIC);
>> -            dgap_Board[i]->flipflagbuf = kzalloc(MYFLIPLEN, GFP_ATOMIC);
>> +    /*
>> +     * allocate flip buffer for board.
>> +     */
>> +    dgap_Board[board]->flipbuf = kmalloc(MYFLIPLEN, GFP_ATOMIC);
>> +    if (!dgap_Board[board]->flipbuf) {
>> +            ret = -ENOMEM;
>> +            goto out;
> 
> Just return -ENOMEM directly.
> 
> 
>>      }
>> +    dgap_Board[board]->flipflagbuf = kmalloc(MYFLIPLEN, GFP_ATOMIC);
>> +    if (!dgap_Board[board]->flipflagbuf) 
>> +            ret = -ENOMEM;
> 
> Free the other allocation and return -ENOMEM.  Don't use a goto since
> there is only one kfree() in the function.
> 
> 
>>  
>> -    return rc;
>> +    out:
>> +        return ret;
> 
> This should be the success path now.  "return 0;"
> 

OK, thanks Dan. I'll followup with a new 19/19 patch. It did compile and
work with no complaints about the above though. I should have checked
that last one better as it is
new code added.

Thanks
Mark

_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Reply via email to