From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from fraxinus.osuosl.org (fraxinus.osuosl.org [140.211.166.137]) by ash.osuosl.org (Postfix) with ESMTP id 68F7A1CE9D8 for ; Wed, 19 Feb 2014 20:18:00 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id 63F298BD35 for ; Wed, 19 Feb 2014 20:18:00 +0000 (UTC) Received: from fraxinus.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id NIPLHllgPNIN for ; Wed, 19 Feb 2014 20:17:58 +0000 (UTC) Received: from aserp1040.oracle.com (aserp1040.oracle.com [141.146.126.69]) by fraxinus.osuosl.org (Postfix) with ESMTPS id 81F848BD33 for ; Wed, 19 Feb 2014 20:17:58 +0000 (UTC) Date: Wed, 19 Feb 2014 23:17:37 +0300 From: Dan Carpenter Subject: Re: [PATCH 19/19] Add in-kernel firmware loading support Message-ID: <20140219201737.GS26722@mwanda> References: <1392833535-25652-1-git-send-email-markh@compro.net> <1392833535-25652-20-git-send-email-markh@compro.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1392833535-25652-20-git-send-email-markh@compro.net> List-Id: Linux Driver Project Developer List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: driverdev-devel-bounces@linuxdriverproject.org Sender: driverdev-devel-bounces@linuxdriverproject.org To: Mark Hounschell Cc: Greg Kroah-Hartman , driverdev-devel@linuxdriverproject.org 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;" regards, dan carpenter _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel