From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <53051BC7.6050100@compro.net> Date: Wed, 19 Feb 2014 16:01:59 -0500 From: Mark Hounschell Reply-To: markh@compro.net MIME-Version: 1.0 Subject: Re: [PATCH 19/19] Add in-kernel firmware loading support References: <1392833535-25652-1-git-send-email-markh@compro.net> <1392833535-25652-20-git-send-email-markh@compro.net> <2024027765.46695.1392841072739.JavaMail.root@mx2.compro.net> In-Reply-To: <2024027765.46695.1392841072739.JavaMail.root@mx2.compro.net> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit List-ID: To: Dan Carpenter Cc: driverdev-devel@linuxdriverproject.org, Greg Kroah-Hartman 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