| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <45C06C71.6000608@garzik.org>
Date: Wed, 31 Jan 2007 05:16:17 -0500
From: Jeff Garzik <jeff@...zik.org>
To: "Amit S. Kale" <amitkale@...xen.com>
CC: netdev@...r.kernel.org, brazilnut@...ibm.com,
netxenproj@...syssoft.com, rob@...xen.com, sanjeev@...xen.com,
wendyx@...ibm.com
Subject: Re: [PATCH 1/2] NetXen: Added ethtool support for user level tools
Amit S. Kale wrote:
> Added ethtool support for user level firmware management utilities.
>
> Signed-off-by: Amit S. Kale <amitkale@...xen.com>
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 majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists