Re: 2.6.12-rc3-mm1 (a-new-10gb-ethernet-driver-by-chelsio-communications.patch)

[Alexey Dobriyan]

> --- /dev/null
> +++ 25-akpm/drivers/net/chelsio/common.h

> +#define DIMOF(x) (sizeof(x)/sizeof(x[0]))

ARRAY_SIZE in include/linux/kernel.h

> --- /dev/null
>+++ 25-akpm/drivers/net/chelsio/cpl5_cmd.h

> +struct cpl_tx_pkt {
> +	__u16 vlan;

__be16.

> +struct cpl_tx_pkt_lso {
> +	__u32 len;

__be32

> +	__u16 eth_type_mss;

__be16

> +struct cpl_rx_pkt {
> +	__u16 vlan;

_be16 really. You do ntohs(p->vlan).

If any other fields of these structs are in network order I'd mark them as
such.

> --- /dev/null
> +++ 25-akpm/drivers/net/chelsio/cxgb2.c

> +#if BITS_PER_LONG == 64 && !defined(CONFIG_X86_64)
> +# define FMT64 "l"
> +#else
> +# define FMT64 "ll"
> +#endif

FMT64 is unused.

> +#define PCI_DMA_64BIT ~0ULL
> +#define PCI_DMA_32BIT 0xffffffffULL

In include/linux/dma-mapping.h DMA_64BIT_MASK and DMA_32BIT_MASK, respectively.

> +static const char pci_speed[][4] = {
> +	"33", "66", "100", "133"
> +};

Unused.

> +static void get_stats(struct net_device *dev, struct ethtool_stats *stats,
> +		      u64 *data)
> +{

> +	*data++ = s->TxOctetsOK;
> +	*data++ = s->TxOctetsBad;
> +	*data++ = s->TxUnicastFramesOK;
> +	*data++ = s->TxMulticastFramesOK;
> +	*data++ = s->TxBroadcastFramesOK;
> +	*data++ = s->TxPauseFrames;
> +	*data++ = s->TxFramesWithDeferredXmissions;
> +	*data++ = s->TxLateCollisions;
> +	*data++ = s->TxTotalCollisions;
> +	*data++ = s->TxFramesAbortedDueToXSCollisions;
> +	*data++ = s->TxUnderrun;
> +	*data++ = s->TxLengthErrors;
> +	*data++ = s->TxInternalMACXmitError;
> +	*data++ = s->TxFramesWithExcessiveDeferral;
> +	*data++ = s->TxFCSErrors;
> +
> +	*data++ = s->RxOctetsOK;
> +	*data++ = s->RxOctetsBad;
> +	*data++ = s->RxUnicastFramesOK;
> +	*data++ = s->RxMulticastFramesOK;
> +	*data++ = s->RxBroadcastFramesOK;
> +	*data++ = s->RxPauseFrames;
> +	*data++ = s->RxFCSErrors;
> +	*data++ = s->RxAlignErrors;
> +	*data++ = s->RxSymbolErrors;
> +	*data++ = s->RxDataErrors;
> +	*data++ = s->RxSequenceErrors;
> +	*data++ = s->RxRuntErrors;
> +	*data++ = s->RxJabberErrors;
> +	*data++ = s->RxInternalMACRcvError;
> +	*data++ = s->RxInRangeLengthErrors;
> +	*data++ = s->RxOutOfRangeLengthField;
> +	*data++ = s->RxFrameTooLongErrors;

It seems you want one big memcpy.

> +static int ethtool_ioctl(struct net_device *dev, void *useraddr)

void __user *addr. To make sparse happy.

> +{

> +	if (copy_from_user(&cmd, useraddr, sizeof(cmd)))

> +static int __devinit init_one(struct pci_dev *pdev,
> +			      const struct pci_device_id *ent)
> +{

> +	if (!pci_set_dma_mask(pdev, PCI_DMA_64BIT)) {

DMA_64BIT_MASK

> +		if (pci_set_consistent_dma_mask(pdev, PCI_DMA_64BIT)) {

DMA_64BIT_MASK

> +	} else if ((err = pci_set_dma_mask(pdev, PCI_DMA_32BIT)) != 0) {

DMA_32BIT_MASK

> --- /dev/null
> +++ 25-akpm/drivers/net/chelsio/cxgb2.h

> +struct adapter {
> +	u8 *regs;

void __iomem *regs; You do ->regs = ioremap(...);

> --- /dev/null
> +++ 25-akpm/drivers/net/chelsio/espi.c

> +/* 1. Deassert rx_reset_core. */
> +/* 2. Program TRICN_CNFG registers. */
> +/* 3. Deassert rx_reset_link */
> +static int tricn_init(adapter_t *adapter)
> +{

> +	/* 1 */
> +	timeout=1000;

> +	/* 2 */
> +	if (sme) {

> +	/* 3 */
> +	t1_write_reg_4(adapter, A_ESPI_RX_RESET, 0x3);

So move comments where they should be. ;-)

> +/* T2 Init part --  */
> +/* 1. Set T_ESPI_MISCCTRL_ADDR */
> +/* 2. Init ESPI registers. */
> +/* 3. Init TriCN Hard Macro */
> +int t1_espi_init(struct peespi *espi, int mac_type, int nports)
> +{

> --- /dev/null
> +++ 25-akpm/drivers/net/chelsio/mv88x201x.c

> +	if (do_enable & LINK_ENABLE_BIT) {
> +		led |= LINK_ENABLE_BIT;
> +		mdio_write(cphy, 0x1, 0x7, led);
> +	} else {
> +		led &= ~LINK_ENABLE_BIT;
> +		mdio_write(cphy, 0x1, 0x7, led);
> +	}

Common mdio_write line.

> --- /dev/null
> +++ 25-akpm/drivers/net/chelsio/pm3393.c

> +static int pm3393_macaddress_set(struct cmac *cmac, u8 ma[6])
> +{
> +	u32 val, lo, mid, hi

> +	 * MAC addr: 00:07:43:00:13:09
> +	 *
> +	 * ma[5] = 0x09
> +	 * ma[4] = 0x13
> +	 * ma[3] = 0x00
> +	 * ma[2] = 0x43
> +	 * ma[1] = 0x07
> +	 * ma[0] = 0x00
> +	 *
> +	 * The PM3393 requires byte swapping and reverse order entry
> +	 * when programming MAC addresses:
> +	 *
> +	 * low_bits[15:0]    = ma[1]:ma[0]
> +	 * mid_bits[31:16]   = ma[3]:ma[2]
> +	 * high_bits[47:32]  = ma[5]:ma[4]

> +	lo = ((u32) ma[1] << 8) | (u32) ma[0];
> +	mid = ((u32) ma[3] << 8) | (u32) ma[2];
> +	hi = ((u32) ma[5] << 8) | (u32) ma[4];

This is

	lo  = (u32) le16_to_cpu((__le16 *) &ma[0]);
	mid = (u32) le16_to_cpu((__le16 *) &ma[2]);
	hi  = (u32) le16_to_cpu((__le16 *) &ma[4]);

Right?

> +	/* The following steps are required to properly reset
> +	 * the PM3393. This information is provided in the
> +	 * PM3393 datasheet (Issue 2: November 2002)
> +	 * section 13.1 -- Device Reset.
> +	 *
> +	 * The PM3393 has three types of components that are
> +	 * individually reset:
> +	 *
> +	 * DRESETB      - Digital circuitry
> +	 * PL4_ARESETB  - PL4 analog circuitry
> +	 * XAUI_ARESETB - XAUI bus analog circuitry
> +	 *
> +	 * Steps to reset PM3393 using RSTB pin:
> +	 *
> +	 * 1. Assert RSTB pin low ( write 0 )
> +	 * 2. Wait at least 1ms to initiate a complete initialization of device.
> +	 * 3. Wait until all external clocks and REFSEL are stable.
> +	 * 4. Wait minimum of 1ms. (after external clocks and REFEL are stable)
> +	 * 5. De-assert RSTB ( write 1 )
> +	 * 6. Wait until internal timers to expires after ~14ms.
> +	 *    - Allows analog clock synthesizer(PL4CSU) to stabilize to
> +	 *      selected reference frequency before allowing the digital
> +	 *      portion of the device to operate.
> +	 * 7. Wait at least 200us for XAUI interface to stabilize.
> +	 * 8. Verify the PM3393 came out of reset successfully.
> +	 *    Set successful reset flag if everything worked else try again
> +	 *    a few more times.

Same comment about comments.

--- /dev/null
+++ 25-akpm/drivers/net/chelsio/regs.h

> +/* Do not edit this file */

OK.

--- /dev/null
+++ 25-akpm/drivers/net/chelsio/subr.c

> +static struct board_info t1_board[] = {
> +
> +{ CHBT_BOARD_N110, 1/*ports#*/,
> +  SUPPORTED_10000baseT_Full | SUPPORTED_FIBRE /*caps*/, CHBT_TERM_T1,
> +  CHBT_MAC_PM3393, CHBT_PHY_88X2010,
> +  125000000/*clk-core*/, 0/*clk-mc3*/, 0/*clk-mc4*/,
> +  1/*espi-ports*/, 0/*clk-cspi*/, 44/*clk-elmer0*/, 0/*mdien*/,
> +  0/*mdiinv*/, 1/*mdc*/, 0/*phybaseaddr*/, &t1_pm3393_ops,
> +  &t1_mv88x201x_ops, &mi1_mdio_ext_ops,
> +  "Chelsio N110 1x10GBaseX NIC" },
> +
> +{ CHBT_BOARD_N210, 1/*ports#*/,
> +  SUPPORTED_10000baseT_Full | SUPPORTED_FIBRE /*caps*/, CHBT_TERM_T2,
> +  CHBT_MAC_PM3393, CHBT_PHY_88X2010,
> +  125000000/*clk-core*/, 0/*clk-mc3*/, 0/*clk-mc4*/,
> +  1/*espi-ports*/, 0/*clk-cspi*/, 44/*clk-elmer0*/, 0/*mdien*/,
> +  0/*mdiinv*/, 1/*mdc*/, 0/*phybaseaddr*/, &t1_pm3393_ops,
> +  &t1_mv88x201x_ops, &mi1_mdio_ext_ops,
> +  "Chelsio N210 1x10GBaseX NIC" },
> +
> +};

That's what C99 initializers are for:

	static struct board_info t1_board[] = {
		{
			.board = CHBT_BOARD_N110,
			.port_number = 1,
			.caps = SUPPORTED_10000baseT_Full | SUPPORTED_FIBRE,
			.chip_term = CHBT_TERM_T1,
				...
		}, {
				...
		},
	};

> +const struct board_info *t1_get_board_info(unsigned int board_id)
> +{
> +	return board_id < DIMOF(t1_board) ? &t1_board[board_id] : NULL;

ARRAY_SIZE