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
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
