netdev - Re: [PATCHv2 NEXT 1/2] qlcnic: Qlogic ethernet driver for CNA devices

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100106111655.207e7414@nehalam>
Date:	Wed, 6 Jan 2010 11:16:55 -0800
From:	Stephen Hemminger <shemminger@...tta.com>
To:	Amit Kumar Salecha <amit.salecha@...gic.com>
Cc:	davem@...emloft.net, netdev@...r.kernel.org,
	dhananjay.phadke@...gic.com
Subject: Re: [PATCHv2 NEXT 1/2] qlcnic: Qlogic ethernet driver for CNA
 devices

Questions:
* Are rings really so big that they need to be allocated with vmalloc()
  rather than kmalloc()? vmalloc() space is restricted and use kmalloc()
  if possible.

* Is there some documentation on bridge mode?  controlling it via sysfs is
  okay, but is there a more logical place?

* Do you need overhead of tx_clean_lock if you only call it from NAPI
  context?

* crb_lock is setup as rwlock but you only use as writer;
  also isn't this already covered by mem_lock?

* mem_lock could/should be mutex
  can ioremap want to sleep?

Minor stuff:

* the following are only used in one file and should be static
    init_module
    cleanup_module
    qlcnic_get_stats
    qlcnic_set_mac

* Could qlcnic_process_cmd_ring be moved to qlcnic_main.c
  and made static so gcc can inline

* strings can be const
   qlcnic_driver_name, qlcnic_driver_string

* other things that can be const
   pci_device table
   diag_registers
   qlcnic_config_rss::key
   fw_name
   msi_tgt_status
   legacy_intr?

* why does diag_register[] use sentinel rather than ARRAY_SIZE()

* no need for static inline (let compiler decide)
    qlnic_update_cmd_consumer

* get_brd_name_by_type
    why is this an inline, it is not in fast path
    just move to qlcnic_main.c
    name can be const char *

* inconsistent use of extern
+int qlcnic_get_flash_mac_addr(struct qlcnic_adapter *adapter, __le64 *mac);
+int qlcnic_p3_get_mac_addr(struct qlcnic_adapter *adapter, __le64 *mac);
+extern void qlcnic_change_ringparam(struct qlcnic_adapter *adapter);
+extern int qlcnic_rom_fast_read(struct qlcnic_adapter *adapter, int addr,
+				int *valp);

* Polling should be aligned to safe power (see round_jiffies)

-- 
--
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