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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux