Message-Id: <1261599746.2782.66.camel@achroite.uk.solarflarecom.com>
Date:	Wed, 23 Dec 2009 20:22:26 +0000
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Jeff Kirsher <jeffrey.t.kirsher@...el.com>
Cc:	netdev@...r.kernel.org, gospo@...hat.com,
	Greg Rose <gregory.v.rose@...el.com>
Subject: Re: [RFC PATCH v2 02/12] ixgbevf: 82599 Virtual Function core
	functions and header

On Fri, 2009-12-18 at 14:51 -0800, Jeff Kirsher wrote:
> From: Greg Rose <gregory.v.rose@...el.com>
> 
> This module and header file contain the core functions for the 82599
> virtual function device.
[...]
> diff --git a/drivers/net/ixgbevf/vf.c b/drivers/net/ixgbevf/vf.c
> new file mode 100644
> index 0000000..38784eb
> --- /dev/null
> +++ b/drivers/net/ixgbevf/vf.c
[...]
> +/**
> + *  ixgbevf_set_rar_vf - set device MAC address
> + *  @hw: pointer to hardware structure
> + *  @index: Receive address register to write
> + *  @addr: Address to put into receive address register
> + *  @vmdq: VMDq "set" or "pool" index
> + **/
> +static s32 ixgbevf_set_rar_vf(struct ixgbe_hw *hw, u32 index, u8 *addr,
> +			      u32 vmdq)
> +{
> +	struct ixgbe_mbx_info *mbx = &hw->mbx;
> +	u32 msgbuf[3];
> +	u8 *msg_addr = (u8 *)(&msgbuf[1]);
> +	s32 ret_val;
> +
> +	memset(msgbuf, 0, 12);

Seems like it would be clearer to write sizeof(msgbuf) rather than 12.
But does it even matter that you clear the padding bytes?

[...]
> +/**
> + *  ixgbevf_update_mc_addr_list_vf - Update Multicast addresses
> + *  @hw: pointer to the HW structure
> + *  @mc_addr_list: array of multicast addresses to program
> + *  @mc_addr_count: number of multicast addresses to program
> + *  @next: caller supplied function to return next address in list
> + *
> + *  Updates the Multicast Table Array.
> + **/
> +static s32 ixgbevf_update_mc_addr_list_vf(struct ixgbe_hw *hw, u8 *mc_addr_list,
> +					u32 mc_addr_count,
> +					ixgbe_mc_addr_itr next)
> +{
> +	struct ixgbe_mbx_info *mbx = &hw->mbx;
> +	u32 msgbuf[IXGBE_VFMAILBOX_SIZE];
> +	u16 *vector_list = (u16 *)&msgbuf[1];
> +	u32 vector;
> +	u32 cnt, i;
> +	u32 vmdq;
> +
> +	/* Each entry in the list uses 1 16 bit word.  We have 30
> +	 * 16 bit words available in our HW msg buffer (minus 1 for the
> +	 * msg type).  That's 30 hash values if we pack 'em right.  If
> +	 * there are more than 30 MC addresses to add then punt the
> +	 * extras for now and then add code to handle more than 30 later.
> +	 * It would be unusual for a server to request that many multi-cast
> +	 * addresses except for in large enterprise network environments.
> +	 */
> +	cnt = (mc_addr_count > 30) ? 30 : mc_addr_count;

What, you don't want to support large enterprise networks now? ;-)
You should switch to all-multicast behaviour rather than dropping
multicast addresses.

[...]
> +/**
> + *  ixgbevf_get_mac_addr_vf - Read device MAC address
> + *  @hw: pointer to the HW structure

The mac_addr parameter is missing.

> + **/
> +static s32 ixgbevf_get_mac_addr_vf(struct ixgbe_hw *hw, u8 *mac_addr)
> +{
> +	int i;
> +
> +	for (i = 0; i < IXGBE_ETH_LENGTH_OF_ADDRESS; i++)
> +		mac_addr[i] = hw->mac.perm_addr[i];

memcpy()?

[...]
> +/**
> + *  ixgbevf_setup_mac_link_vf - Setup MAC link settings
> + *  @hw: pointer to hardware structure
> + *  @speed: new link speed
> + *  @autoneg: true if autonegotiation enabled
> + *  @autoneg_wait_to_complete: true when waiting for completion is needed
> + *
> + *  Set the link speed in the AUTOC register and restarts link.
> + **/

This description is obviously not correct.

> +static s32 ixgbevf_setup_mac_link_vf(struct ixgbe_hw *hw,
> +				     ixgbe_link_speed speed, bool autoneg,
> +				     bool autoneg_wait_to_complete)
> +{
> +	return 0;
> +}
[...]
> diff --git a/drivers/net/ixgbevf/vf.h b/drivers/net/ixgbevf/vf.h
> new file mode 100644
> index 0000000..799600e
> --- /dev/null
> +++ b/drivers/net/ixgbevf/vf.h
[...]
> +struct ixgbe_mac_operations {
> +	s32 (*init_hw)(struct ixgbe_hw *);
> +	s32 (*reset_hw)(struct ixgbe_hw *);
> +	s32 (*start_hw)(struct ixgbe_hw *);
> +	s32 (*clear_hw_cntrs)(struct ixgbe_hw *);
> +	enum ixgbe_media_type (*get_media_type)(struct ixgbe_hw *);
> +	u32 (*get_supported_physical_layer)(struct ixgbe_hw *);
> +	s32 (*get_mac_addr)(struct ixgbe_hw *, u8 *);
> +	s32 (*stop_adapter)(struct ixgbe_hw *);
> +	s32 (*get_bus_info)(struct ixgbe_hw *);
> +
> +	/* Link */
> +	s32 (*setup_link)(struct ixgbe_hw *, ixgbe_link_speed, bool, bool);
> +	s32 (*check_link)(struct ixgbe_hw *, ixgbe_link_speed *, bool *, bool);
> +	s32 (*get_link_capabilities)(struct ixgbe_hw *, ixgbe_link_speed *,
> +				     bool *);
> +
> +	/* RAR, Multicast, VLAN */
> +	s32 (*set_rar)(struct ixgbe_hw *, u32, u8 *, u32);
> +	s32 (*init_rx_addrs)(struct ixgbe_hw *);
> +	s32 (*update_mc_addr_list)(struct ixgbe_hw *, u8 *, u32,
> +				   ixgbe_mc_addr_itr);
> +	s32 (*enable_mc)(struct ixgbe_hw *);
> +	s32 (*disable_mc)(struct ixgbe_hw *);
> +	s32 (*clear_vfta)(struct ixgbe_hw *);
> +	s32 (*set_vfta)(struct ixgbe_hw *, u32, u32, bool);
> +};
> +
> +enum ixgbe_mac_type {
> +	ixgbe_mac_unknown = 0,
> +	ixgbe_mac_82599_vf,
> +	ixgbe_num_macs
> +};

Is this abstraction really justified, given you have only one
implementation?

[...]
> +struct ixgbe_mbx_operations {
> +	s32 (*init_params)(struct ixgbe_hw *hw);
> +	s32 (*read)(struct ixgbe_hw *, u32 *, u16);
> +	s32 (*write)(struct ixgbe_hw *, u32 *, u16);
> +	s32 (*read_posted)(struct ixgbe_hw *, u32 *, u16);
> +	s32 (*write_posted)(struct ixgbe_hw *, u32 *, u16);
> +	s32 (*check_for_msg)(struct ixgbe_hw *);
> +	s32 (*check_for_ack)(struct ixgbe_hw *);
> +	s32 (*check_for_rst)(struct ixgbe_hw *);
> +};
[...]

Ditto for this.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

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