Subject: Re: [PATCH 1/2] NetXen: Added ethtool support for user level tools

From: Jeff Garzik <[email-masked]>
Date: Wed, 31 Jan 2007 05:16:17 -0500

Amit S. Kale wrote:
> Added ethtool support for user level firmware management utilities.
> 
> Signed-off-by: Amit S. Kale <[email-masked]>

You will need to resend against netdev#upstream, which now contains Al 
Viro's netxen annotations.

Please add sparse checking (read Documentation/sparse.txt) to your build 
process, after updating for Al Viro's changes.

> 
> ---
> 
>  netxen_nic.h         |   16 ++-
>  netxen_nic_ethtool.c |   87 +++++++++++++---
>  netxen_nic_init.c    |  268 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 351 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/netxen/netxen_nic.h b/drivers/net/netxen/netxen_nic.h
> index 59324b1..f188b59 100644
> --- a/drivers/net/netxen/netxen_nic.h
> +++ b/drivers/net/netxen/netxen_nic.h
> @@ -63,12 +63,16 @@ #include <asm/pgtable.h>
>  
>  #include "netxen_nic_hw.h"
>  
> -#define NETXEN_NIC_BUILD_NO     "2"
> +#define NETXEN_NIC_BUILD_NO     "3"

As mentioned previously, this constant should be removed

>  #define _NETXEN_NIC_LINUX_MAJOR 3
>  #define _NETXEN_NIC_LINUX_MINOR 3
>  #define _NETXEN_NIC_LINUX_SUBVERSION 3
>  #define NETXEN_NIC_LINUX_VERSIONID  "3.3.3" "-" NETXEN_NIC_BUILD_NO
>  
> +#define NUM_FLASH_SECTORS (64)
> +#define FLASH_SECTOR_SIZE (64*1024)
> +#define FLASH_TOTAL_SIZE  (NUM_FLASH_SECTORS*FLASH_SECTOR_SIZE)
> +
>  #define RCV_DESC_RINGSIZE	\
>  	(sizeof(struct rcv_desc) * adapter->max_rx_desc_count)
>  #define STATUS_DESC_RINGSIZE	\
> @@ -85,6 +89,7 @@ #define NETXEN_NETDEV_STATUS		0x1
>  #define NETXEN_RCV_PRODUCER_OFFSET	0
>  #define NETXEN_RCV_PEG_DB_ID		2
>  #define NETXEN_HOST_DUMMY_DMA_SIZE 1024
> +#define FLASH_SUCCESS 0
>  
>  #define ADDR_IN_WINDOW1(off)	\
>  	((off > NETXEN_CRB_PCIX_HOST2) && (off < NETXEN_CRB_MAX)) ? 1 : 0
> @@ -1034,6 +1039,15 @@ void netxen_phantom_init(struct netxen_a
>  void netxen_load_firmware(struct netxen_adapter *adapter);
>  int netxen_pinit_from_rom(struct netxen_adapter *adapter, int verbose);
>  int netxen_rom_fast_read(struct netxen_adapter *adapter, int addr, int *valp);
> +int netxen_rom_fast_read_words(struct netxen_adapter *adapter, int addr, 
> +				u8 *bytes, size_t size);
> +int netxen_rom_fast_write_words(struct netxen_adapter *adapter, int addr, 
> +				u8 *bytes, size_t size);
> +int netxen_flash_unlock(struct netxen_adapter *adapter);
> +int netxen_backup_crbinit(struct netxen_adapter *adapter);
> +int netxen_flash_erase_secondary(struct netxen_adapter *adapter);
> +int netxen_flash_erase_primary(struct netxen_adapter *adapter);
> +
>  int netxen_rom_fast_write(struct netxen_adapter *adapter, int addr, int data);
>  int netxen_rom_se(struct netxen_adapter *adapter, int addr);
>  int netxen_do_rom_se(struct netxen_adapter *adapter, int addr);
> diff --git a/drivers/net/netxen/netxen_nic_ethtool.c b/drivers/net/netxen/netxen_nic_ethtool.c
> index 3404461..49b3b4c 100644
> --- a/drivers/net/netxen/netxen_nic_ethtool.c
> +++ b/drivers/net/netxen/netxen_nic_ethtool.c
> @@ -32,6 +32,7 @@
>   */
>  
>  #include <linux/types.h>
> +#include <linux/delay.h>
>  #include <asm/uaccess.h>
>  #include <linux/pci.h>
>  #include <asm/io.h>
> @@ -94,17 +95,7 @@ #define NETXEN_MAX_EEPROM_LEN   1024
>  
>  static int netxen_nic_get_eeprom_len(struct net_device *dev)
>  {
> -	struct netxen_port *port = netdev_priv(dev);
> -	struct netxen_adapter *adapter = port->adapter;
> -	int n;
> -
> -	if ((netxen_rom_fast_read(adapter, 0, &n) == 0)
> -	    && (n & NETXEN_ROM_ROUNDUP)) {
> -		n &= ~NETXEN_ROM_ROUNDUP;
> -		if (n < NETXEN_MAX_EEPROM_LEN)
> -			return n;
> -	}
> -	return 0;
> +	return FLASH_TOTAL_SIZE;
>  }
>  
>  static void
> @@ -445,13 +436,78 @@ netxen_nic_get_eeprom(struct net_device 
>  		return -EINVAL;
>  
>  	eeprom->magic = (port->pdev)->vendor | ((port->pdev)->device << 16);
> -	for (offset = 0; offset < eeprom->len; offset++)
> -		if (netxen_rom_fast_read
> -		    (adapter, (8 * offset) + 8, (int *)eeprom->data) == -1)
> -			return -EIO;
> +	offset = eeprom->offset;
> +
> +	if (netxen_rom_fast_read_words
> +		(adapter, offset, bytes, eeprom->len) == -1){
> +		return -EIO;
> +	}

two non-standard style elements:

1) function arguments should follow the function on the same line.  wrap 
the line at the first argument that crosses the 72-column barrier

2) do not use braces '{' '}' to enclose a single C statement

>  	return 0;
>  }
>  
> +static int
> +netxen_nic_set_eeprom(struct net_device *dev, struct ethtool_eeprom *eeprom,
> +			u8 * bytes)
> +{
> +	struct netxen_port *port = netdev_priv(dev);
> +	struct netxen_adapter *adapter = port->adapter;
> +	int offset = eeprom->offset;
> +	static int first_write = 1;
> +	int ret;
> +	static int ready_to_flash = 0;
> +
> +	if(first_write == 1){
> +		netxen_flash_unlock(adapter);
> +		printk("%s: flash unlocked. \n", netxen_nic_driver_name);
> +		if ((ret = netxen_flash_erase_secondary(adapter)) 
> +			!= FLASH_SUCCESS) {

do not combine a test and a C statement into the same line

> +			printk("%s: Flash erase failed.\n", 
> +				netxen_nic_driver_name);
> +			return(ret);

1) return is not a function.  do not enclose its arguments in parens.

2) is it ok to return without re-locking flash?

> +		}
> +		printk("%s: secondary flash erased successfully.\n", 
> +			netxen_nic_driver_name);

Do not add printk() calls without a KERN_xxx message prefix.

Moreover, you should probably be using dev_{err,info,...} instead of 
printk()

> +		first_write = 0;
> +		return 0;

ditto (re-locking flash)

> +	}
> +
> +	if(offset == BOOTLD_START){

add spaces following "if", this is not Java ;-)

> +		if ((ret = netxen_flash_erase_primary(adapter)) 
> +			!= FLASH_SUCCESS) {
> +			printk("%s: Flash erase failed.\n", 
> +				netxen_nic_driver_name);
> +			return ret;

ditto (re-locking flash)

> +		}
> +		if((ret = netxen_rom_se(adapter, USER_START)) != FLASH_SUCCESS)
> +			return ret;
> +		if((ret = netxen_rom_se(adapter, FIXED_START)) != FLASH_SUCCESS)
> +			return ret;

1) more statement + tests that should be split into two lines apiece

2) re-locking flash?

> +		printk("%s: primary flash erased successfully\n", 
> +			netxen_nic_driver_name);
> +		udelay (500);
> +
> +		if((ret = netxen_backup_crbinit(adapter)) != FLASH_SUCCESS){
> +			printk("%s: CRBinit backup failed.\n", 
> +				netxen_nic_driver_name);
> +			return ret;
> +		}
> +		printk("%s: CRBinit backup done.\n", netxen_nic_driver_name);
> +		ready_to_flash = 1;
> +		udelay (500);
> +	}
> +
> +	if(!ready_to_flash){
> +		printk("%s: Invalid write sequence, returning...\n",
> +			netxen_nic_driver_name);
> +		return -EINVAL;

more naked printk's

> +
> +	udelay (500);
> +
> +	return netxen_rom_fast_write_words(adapter, offset, bytes, eeprom->len);
> +}
> +
>  static void
>  netxen_nic_get_ringparam(struct net_device *dev, struct ethtool_ringparam *ring)
>  {
> @@ -721,6 +777,7 @@ struct ethtool_ops netxen_nic_ethtool_op
>  	.get_link = netxen_nic_get_link,
>  	.get_eeprom_len = netxen_nic_get_eeprom_len,
>  	.get_eeprom = netxen_nic_get_eeprom,
> +	.set_eeprom = netxen_nic_set_eeprom,
>  	.get_ringparam = netxen_nic_get_ringparam,
>  	.get_pauseparam = netxen_nic_get_pauseparam,
>  	.set_pauseparam = netxen_nic_set_pauseparam,
> diff --git a/drivers/net/netxen/netxen_nic_init.c b/drivers/net/netxen/netxen_nic_init.c
> index c3e41f3..069436f 100644
> --- a/drivers/net/netxen/netxen_nic_init.c
> +++ b/drivers/net/netxen/netxen_nic_init.c
> @@ -391,6 +391,7 @@ static inline int do_rom_fast_write(stru
>  	netxen_nic_reg_write(adapter, NETXEN_ROMUSB_ROM_ABYTE_CNT, 3);
>  	netxen_nic_reg_write(adapter, NETXEN_ROMUSB_ROM_INSTR_OPCODE,
>  			     M25P_INSTR_PP);
> +	udelay(100);

why?

maybe just a PCI posting bug?

>  	if (netxen_wait_rom_done(adapter)) {
>  		netxen_nic_reg_write(adapter, NETXEN_ROMUSB_ROM_ABYTE_CNT, 0);
>  		return -1;
> @@ -399,12 +400,13 @@ static inline int do_rom_fast_write(stru
>  	return netxen_rom_wip_poll(adapter);
>  }
>  
> +
>  static inline int
>  do_rom_fast_read(struct netxen_adapter *adapter, int addr, int *valp)
>  {
>  	netxen_nic_reg_write(adapter, NETXEN_ROMUSB_ROM_ADDRESS, addr);
>  	netxen_nic_reg_write(adapter, NETXEN_ROMUSB_ROM_ABYTE_CNT, 3);
> -	udelay(100);		/* prevent bursting on CRB */
> +	udelay(70);		/* prevent bursting on CRB */

why?

>  	netxen_nic_reg_write(adapter, NETXEN_ROMUSB_ROM_DUMMY_BYTE_CNT, 0);
>  	netxen_nic_reg_write(adapter, NETXEN_ROMUSB_ROM_INSTR_OPCODE, 0xb);
>  	if (netxen_wait_rom_done(adapter)) {
> @@ -413,13 +415,45 @@ do_rom_fast_read(struct netxen_adapter *
>  	}
>  	/* reset abyte_cnt and dummy_byte_cnt */
>  	netxen_nic_reg_write(adapter, NETXEN_ROMUSB_ROM_ABYTE_CNT, 0);
> -	udelay(100);		/* prevent bursting on CRB */
> +	udelay(70);		/* prevent bursting on CRB */

why?

>  	netxen_nic_reg_write(adapter, NETXEN_ROMUSB_ROM_DUMMY_BYTE_CNT, 0);
>  
>  	*valp = netxen_nic_reg_read(adapter, NETXEN_ROMUSB_ROM_RDATA);
>  	return 0;
>  }
>  
> +static inline int 
> +do_rom_fast_read_words(struct netxen_adapter *adapter, int addr,
> +			u8 *bytes, size_t size)
> +{
> +	int addridx = addr;
> +	int ret = 0;
> +
> +	while (addridx < (addr + size)) {
> +		ret = do_rom_fast_read(adapter, addridx, (int *)bytes);
> +		if(ret != 0)

add space after 'if'

> +			break;
> +		bytes += 4;
> +		addridx += 4;
> +	}
> +	return ret;
> +}
> +
> +int
> +netxen_rom_fast_read_words (struct netxen_adapter *adapter, int addr, u8 *bytes, 
> +			size_t size)
> +{
> +	int ret;

add blank line after definition of automatic variable

> +	if (rom_lock(adapter) != 0) {
> +		return -1;
> +	}

remove braces

> +	ret = do_rom_fast_read_words(adapter, addr, bytes, size);
> +
> +	netxen_rom_unlock(adapter);
> +	return ret;
> +}
> +
>  int netxen_rom_fast_read(struct netxen_adapter *adapter, int addr, int *valp)
>  {
>  	int ret;
> @@ -443,20 +477,189 @@ int netxen_rom_fast_write(struct netxen_
>  	netxen_rom_unlock(adapter);
>  	return ret;
>  }
> +
> +static inline int do_rom_fast_write_words(struct netxen_adapter *adapter, 
> +						int addr, u8 *bytes, size_t size)
> +{
> +	int addridx = addr;
> +	int data1;
> +	int data;
> +	int ret = 0;
> +	int timeout = 0;
> +
> +	while (addridx < (addr + size)) {
> +		data = *(u32*)bytes;
> +
> +		ret = do_rom_fast_write(adapter, addridx, data);
> +		if(ret == -1){

add space

> +			printk("do_rom_fast_write returned error \n");

naked printk

> +			return ret;
> +			
> +		}
> +		timeout = 0;
> +
> +		while(1){
> +			do_rom_fast_read(adapter, addridx, &data1);
> +
> +			if(data1 == data){
> +				break;
> +			}

kill braces

> +			if(timeout++ >= 300) {
> +				printk("netxen_nic: Data write didn't succeed"
> +					" at address %x\n", addridx);
> +				break;

naked printk

> +			}
> +		}
> +
> +		bytes += 4;
> +		addridx += 4;
> +	}
> +
> +	return ret;
> +}
> +
> +int netxen_rom_fast_write_words(struct netxen_adapter *adapter, int addr, 
> +					u8 *bytes, size_t size)
> +{
> +	int ret = 0;
> +
> +	if (rom_lock(adapter) != 0) {
> +		return -EAGAIN;
> +	}

kill braces

> +	ret = do_rom_fast_write_words(adapter, addr, bytes, size);
> +	netxen_rom_unlock(adapter);
> +	return ret;
> +}
> +
> +int netxen_rom_wrsr(struct netxen_adapter *adapter, int data)
> +{
> +	if (netxen_rom_wren(adapter)){
> +		return -1;
> +	}

ditto

> +	netxen_crb_writelit_adapter(adapter, NETXEN_ROMUSB_ROM_WDATA, data);
> +	netxen_crb_writelit_adapter(adapter, NETXEN_ROMUSB_ROM_INSTR_OPCODE, 0x1);
> +
> +	if (netxen_wait_rom_done(adapter)) {
> +		return -1;
> +	}

ditto

> +	return netxen_rom_wip_poll(adapter);
> +}
> +
> +int netxen_rom_rdsr(struct netxen_adapter *adapter)
> +{
> +	int ret;
> +
> +	if (rom_lock(adapter) != 0){
> +		return -1;
> +	}

ditto

> +	ret = netxen_do_rom_rdsr(adapter);
> +	netxen_rom_unlock(adapter);
> +	return ret;
> +}
> +
> +int netxen_backup_crbinit(struct netxen_adapter *adapter)
> +{
> +	int ret = FLASH_SUCCESS;
> +	int val;
> +	char *buffer = kmalloc(FLASH_SECTOR_SIZE, GFP_KERNEL);

check for NULL

> +	/* unlock sector 63 */
> +	val = netxen_rom_rdsr(adapter);
> +	val = val & 0xe3;
> +	ret = netxen_rom_wrsr(adapter, val);
> +	if(ret != FLASH_SUCCESS){
> +		kfree(buffer);
> +		return -1;
> +	}
> +	ret = netxen_rom_wip_poll(adapter);
> +	if(ret != FLASH_SUCCESS){
> +		kfree(buffer);
> +		return -1;
> +	}
> +	/* copy  sector 0 to sector 63 */
> +
> +	if (netxen_rom_fast_read_words
> +		(adapter, CRBINIT_START, buffer, FLASH_SECTOR_SIZE) == -1){

function args start on previous line

> +		printk("get_eeprom() fails...\n");
> +		kfree(buffer);
> +		return -EIO;
> +	}
> +
> +	ret = netxen_rom_fast_write_words(adapter, FIXED_START, buffer,
> +					   FLASH_SECTOR_SIZE);

like this

> +	if(ret != FLASH_SUCCESS){

add space after 'if'

> +		kfree(buffer);
> +		return -1;
> +	}
> +
> +	/* lock sector 63 */
> +	val = netxen_rom_rdsr(adapter);
> +	if (!(val & 0x8)) {
> +		val |= (0x1 << 2);
> +		/* lock sector 63 */
> +		if (netxen_rom_wrsr(adapter, val) == 0) {
> +			ret = netxen_rom_wip_poll(adapter);
> +			if(ret != FLASH_SUCCESS){
> +				kfree(buffer);
> +				return -1;
> +			}
> +			/* lock SR writes */
> +			val = val | 0x80;
> +			ret = netxen_rom_wip_poll(adapter);
> +			if(ret != FLASH_SUCCESS){
> +				kfree(buffer);
> +				return -1;
> +			}
> +		}
> +	}
> +	kfree(buffer);
> +	return ret;

overall, WAY TOO MANY duplicate 'kfree(buffer)' calls.  it is kernel 
style to use 'goto' to jump to a single error handling callsite, rather 
than all this duplication

>  int netxen_do_rom_se(struct netxen_adapter *adapter, int addr)
>  {
> -	netxen_rom_wren(adapter);
> +	if(netxen_rom_wren(adapter) != 0)
> +		return -1;
>  	netxen_nic_reg_write(adapter, NETXEN_ROMUSB_ROM_ADDRESS, addr);
> +	udelay(200);
>  	netxen_nic_reg_write(adapter, NETXEN_ROMUSB_ROM_ABYTE_CNT, 3);
> +	udelay(200);
>  	netxen_nic_reg_write(adapter, NETXEN_ROMUSB_ROM_INSTR_OPCODE,
> -			     M25P_INSTR_SE);
> +				M25P_INSTR_SE);
> +	udelay(200);

why?

PCI posting bugs?

>  	if (netxen_wait_rom_done(adapter)) {
>  		netxen_nic_reg_write(adapter, NETXEN_ROMUSB_ROM_ABYTE_CNT, 0);
>  		return -1;
>  	}
> +
>  	return netxen_rom_wip_poll(adapter);
>  }
>  
> +void check_erased_flash(struct netxen_adapter *adapter, int addr)
> +{
> +	int i;
> +	int val;
> +	int count = 0, erased_errors =0;
> +	int range;
> +
> +	if(addr == 0x3e8000)
> +		range = 0x3f0000;	

what do these magic numbers mean?  why aren't they named constants?

> +	else range = addr + FLASH_SECTOR_SIZE;
> +	
> +	for(i = addr; i < range; i+=4){
> +		netxen_rom_fast_read(adapter, i, &val);
> +		if(val != 0xffffffff)
> +			erased_errors++;
> +		count++;
> +	}
> +
> +	if(erased_errors)

space after 'if'

> +		printk("0x%x out of 0x%x words fail to be erased "
> +			"for sector address: %x\n", erased_errors, count, addr);

naked printk

>  int netxen_rom_se(struct netxen_adapter *adapter, int addr)
>  {
>  	int ret = 0;
> @@ -465,6 +668,63 @@ int netxen_rom_se(struct netxen_adapter 
>  	}
>  	ret = netxen_do_rom_se(adapter, addr);
>  	netxen_rom_unlock(adapter);
> +	schedule();
> +	check_erased_flash(adapter, addr);
> +	return ret;

why are you calling schedule() here?  more likely you want to call 
msleep(), I'm guessing.

> +int
> +netxen_flash_erase_sections(struct netxen_adapter *adapter, int start, int end)
> +{
> +	int ret = FLASH_SUCCESS;
> +	int i;
> +	for (i = start; i < end; i++) {
> +		ret = netxen_rom_se(adapter, i*FLASH_SECTOR_SIZE);
> +		if (ret)
> +			break;
> +		if (netxen_rom_wip_poll(adapter) != 0) {
> +			ret = -1;
> +			break;
> +		}
> +	}
> +	return(ret);
> +}
> +
> +int
> +netxen_flash_erase_secondary(struct netxen_adapter *adapter)
> +{
> +	int ret = FLASH_SUCCESS;
> +	int start, end;
> +
> +	start = SECONDARY_START/FLASH_SECTOR_SIZE;
> +	end   = USER_START/FLASH_SECTOR_SIZE;
> +	netxen_flash_erase_sections(adapter, start, end);
> +
> +	return(ret);
> +}
> +
> +int
> +netxen_flash_erase_primary(struct netxen_adapter *adapter)
> +{
> +	int ret = FLASH_SUCCESS;
> +	int start, end;
> +
> +	start = PRIMARY_START/FLASH_SECTOR_SIZE;
> +	end   = SECONDARY_START/FLASH_SECTOR_SIZE;
> +	ret = netxen_flash_erase_sections(adapter, start, end);
> +
> +	return(ret);
> +}
> +
> +int netxen_flash_unlock(struct netxen_adapter *adapter)
> +{
> +	int ret = 0;
> +
> +	if (netxen_rom_wrsr(adapter, 0) != 0)
> +		ret = -1;
> +	if (netxen_rom_wren(adapter) != 0)
> +		ret = -1;
> +
>  	return ret;
>  }
>  
>