| 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
| ||
|
Date: Sun, 08 Feb 2009 23:18:14 -0800 (PST)
From: David Miller <davem@...emloft.net>
To: jie.yang@...eros.com
Cc: jeff@...zik.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] atl1c:Atheros L1C Gigabit Ethernet driver
From: <jie.yang@...eros.com>
Date: Mon, 9 Feb 2009 14:34:31 +0800
> +#define PCI_REG_COMMAND 0x04 /* PCI Command Register */
> +#define CMD_IO_SPACE 0x0001
> +#define CMD_MEMORY_SPACE 0x0002
> +#define CMD_BUS_MASTER 0x0004
Use definitions in linux/pci_regs.h, instead of inventing
your own.
> +
> +#define BAR_0 0
> +#define BAR_1 1
> +#define BAR_5 5
Likewise.
> +/* Error Codes */
> +#define AT_ERR_EEPROM 1
> +#define AT_ERR_PHY 2
> +#define AT_ERR_CONFIG 3
> +#define AT_ERR_PARAM 4
> +#define AT_ERR_MAC_TYPE 5
> +#define AT_ERR_PHY_TYPE 6
> +#define AT_ERR_PHY_SPEED 7
> +#define AT_ERR_PHY_RES 8
> +#define AT_ERR_TIMEOUT 9
Please do not invent your own error code system, use existing ones.
Otherwise your code is hard for others to understand and review.
> +#define AT_RX_BUF_SIZE 1536
How is this number derived? Is it ETH_DATA_LEN or ETH_FRAME_LEN
plus some constant or other importnat value? If so, please
define it as a sum of those descriptive macro values.
> +#define AT_REGS_LEN 75
This definition is really confusing. It's not the length
of the chip register area, it's the number of registers
the chip has.
Every use in the driver multiplies this value by
"sizeof(u32)". Just define this macro to "75 * sizeof(u32)"
and update the macro users accordingly.
> +struct atl1c_tpd_desc {
> + __le16 buffer_len; /* include 4-byte CRC */
> + u16 vlan_tag;
> + __le32 word1;
> + __le64 buffer_addr;
> +};
Is the vlan_tag stored in little or big endian? I find it
unlikely that it is stored in cpu-endianness, so an
endian typed vlan_tag should be specified here.
The AT_TAG_TO_VLAN() macro will need to be updated to match
whatever endianness this value is stored in.
> +struct atl1c_recv_ret_status {
> + __le32 word0;
> + u32 rss_hash;
> + u16 vlan_tag;
> + u16 flag;
> + __le32 word3;
> +};
Well, what endianness are these values stored in? Cpu endianness?
If not, endian specific types and accessors need to be used here.
> + u16 autoneg_advertised;
> +#define ADVERTISE_10_HALF 0x0001
> +#define ADVERTISE_10_FULL 0x0002
> +#define ADVERTISE_100_HALF 0x0004
> +#define ADVERTISE_100_FULL 0x0008
> +#define ADVERTISE_1000_HALF 0x0010 /* Not used, just FYI */
> +#define ADVERTISE_1000_FULL 0x0020
> +#define ADVERTISE_DEFAULT (ADVERTISE_10_HALF |\
> + ADVERTISE_10_FULL |\
> + ADVERTISE_100_HALF |\
> + ADVERTISE_100_FULL |\
> + ADVERTISE_1000_FULL)
Having these self-defined local macros with similar names
as the defines found in linux/mii.h (which also form a bitmask
that fits in 16-bits) is confusing.
Please use the linux/mii.h values here.
> + if (control & EEPROM_CTRL_RW) {
> + AT_READ_REG(hw, REG_EEPROM_CTRL, &data);
> + AT_READ_REG(hw, REG_EEPROM_DATA_LO, p_value);
> + data = data & 0xFFFF;
> + *p_value = swab32((data << 16) | (*p_value >> 16));
> + ret = true;
> + }
Please make sure the endianness works out properly here,
even on big-endian systems.
I think it's right, but it's worth double checking.
> +#if 0
> +int atl1c_phy_commit(struct atl1c_hw *hw)
Please just elide this code from the driver, you can add
it in when a use is created.
> +static void atl1c_phy_magic_data(struct atl1c_hw *hw)
> +{
> + u16 data;
> +
> + atl1c_write_phy_reg(hw, MII_DBG_ADDR, 0x12);
> + atl1c_write_phy_reg(hw, MII_DBG_DATA, 0x4C04);
> +
> + atl1c_write_phy_reg(hw, MII_DBG_ADDR, 0x05);
> + atl1c_write_phy_reg(hw, MII_DBG_DATA, 0x2C46);
> +
> + atl1c_write_phy_reg(hw, MII_DBG_ADDR, 0x36);
> + atl1c_write_phy_reg(hw, MII_DBG_DATA, 0xE12C);
> +
> + atl1c_write_phy_reg(hw, MII_DBG_ADDR, 0x04);
> + atl1c_write_phy_reg(hw, MII_DBG_DATA, 0x88BB);
> +
> + atl1c_write_phy_reg(hw, MII_DBG_ADDR, 0x00);
> + atl1c_write_phy_reg(hw, MII_DBG_DATA, 0x02EF);
No magic constants please, document these MII PHY registers and what
bits and values you are setting into them, using appropriate register
offset and value macros.
I am pretty sure you can find out what these register writes are
doing, and thus be able to document them properly :-)
> +/* register definition */
> +#define REG_PM_CTRLSTAT 0x44
> +#define PM_CTRLSTAT_PME_EN 0x100
All of these PCI config space registers should either use
the definitions in linux/pci_reg.h or the appropriate
extended PCI config space register probing mechanisms such
as pci_find_capability().
pci_find_capability() is the correct way to find the PM,
VPD, PCI-E etc. extended register areas.
> + netif_carrier_on(netdev);
> + netif_wake_queue(netdev);
...
> + netif_carrier_off(netdev);
> + netif_stop_queue(netdev);
Doing a carrier state change as well as a queue wake/stop is
redundant. Setting the carrier off is enough to stop packet
flow on transmit to the device. And likewise setting the
carrier on is enough to (re-)start packet flow for transmit.
> +static int atl1c_clean(struct napi_struct *napi, int budget)
> +{
> + struct atl1c_adapter *adapter =
> + container_of(napi, struct atl1c_adapter, napi);
> + int work_done = 0;
> + int i;
> +
> + /* Keep link state information with original netdev */
> + if (!netif_carrier_ok(adapter->netdev))
> + goto quit_polling;
> +
> + for (i = 0; i < adapter->num_rx_queues; i++)
> + atl1c_clean_rx_irq(adapter, i, &work_done, budget);
This is going to perform very poorly. You should instantiate
a seperate NAPI instance for each RX queue, and only schedule NAPI
for those specific RX queues which indicate RX packets are available
at interrupt time.
Otherwise having seperate RX queues is pointless, as all of the SMP
locking gains from RX seperation will not be realized.
If it is not possible to implement things this way, either because
it is very difficult or impossible due to the interrupt architecture
of the chip, then please just use one RX queue at this time. You
can add the RX multi-queue support back later once it is implemented
properly.
> + netdev->open = &atl1c_open;
> + netdev->stop = &atl1c_close;
> + netdev->hard_start_xmit = &atl1c_xmit_frame;
> + netdev->get_stats = &atl1c_get_stats;
> + netdev->set_multicast_list = &atl1c_set_multi;
> + netdev->set_mac_address = &atl1c_set_mac_addr;
> + netdev->change_mtu = &atl1c_change_mtu;
> + netdev->do_ioctl = &atl1c_ioctl;
> + netdev->tx_timeout = &atl1c_tx_timeout;
Please use net_device_ops, the function pointer initialization method
you are using here is deprecated and will be eliminated at some point
in the future.
> + /* enable device (incl. PCI PM wakeup and hotplug setup) */
> + err = pci_enable_device(pdev);
> + if (err) {
> + dev_err(&pdev->dev, "cannot enable PCI device\n");
> + return err;
> + }
Please use pci_enable_device_mem() if only MEM space register
accesses will be used.
> + /* get user settings */
> + atl1c_check_options(adapter);
Please do not use module parameters for network device
configuration knobs.
Many of the controls you have specified are supported by
ethtool, and that is the consistent interface we have for
users to make these setting changes.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists