Ueimor, Jeff,
Thanks for reviewing. We are integrating these into our code and will send an
update tomorrow.
-Amit
On Wednesday 31 January 2007 15:46, Jeff Garzik wrote:
> Amit S. Kale wrote:
> > Added ethtool support for user level firmware management utilities.
> >
> > Signed-off-by: Amit S. Kale <[EMAIL PROTECTED]>
>
> 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;
> > }
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
