Re: [PATCH 19/19] Add in-kernel firmware loading support

[Dan Carpenter <dan.carpenter@oracle.com>]

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