netdev - Re: [net-next 1/8] i40e: main driver core

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: <1371187762.31941.42.camel@joe-AO722>
Date:	Thu, 13 Jun 2013 22:29:22 -0700
From:	Joe Perches <joe@...ches.com>
To:	Jeff Kirsher <jeffrey.t.kirsher@...el.com>
Cc:	davem@...emloft.net, Jesse Brandeburg <jesse.brandeburg@...el.com>,
	netdev@...r.kernel.org, gospo@...hat.com, sassmann@...hat.com,
	Shannon Nelson <shannon.nelson@...el.com>,
	PJ Waskiewicz <peter.p.waskiewicz.jr@...el.com>,
	e1000-devel@...ts.sourceforge.net
Subject: Re: [net-next 1/8] i40e: main driver core

On Thu, 2013-06-13 at 20:55 -0700, Jeff Kirsher wrote:
> From: Jesse Brandeburg <jesse.brandeburg@...el.com>
> 
> This is the driver for the Intel(R) Ethernet Controller XL710 Family.

This code looks very generic and not tailored to linux.
Are you intending to fix it to be more linux-kernel like?

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
[]
> +enum i40e_status_code i40e_allocate_dma_mem_d(struct i40e_hw *hw,

i40e_status?  int is more common.

> +	if (mem->va)
> +		return I40E_SUCCESS;
> +	else
> +		return I40E_ERR_NO_MEMORY;

	if (!mem->va)
		return -ENOMEM;

	return 0;

but returning the pointer or NULL is much more normal.

> +enum i40e_status_code i40e_allocate_virt_mem_d(struct i40e_hw *hw,
> +					       struct i40e_virt_mem *mem,
> +					       u32 size)
[]
> 	if (mem->va)
> +		return I40E_SUCCESS;
> +	else
> +		return I40E_ERR_NO_MEMORY;
> +}

same as above.

> +/**
> + * i40e_debug_d - OS dependent version of debug printing
> + * @hw:  pointer to the HW structure
> + * @mask: debug level mask
> + * @fmt_str: printf-type format description
> + **/
> +void i40e_debug_d(void *hw, u32 mask, char *fmt_str, ...)
> +{
> +	char *buf;
> +	int buf_len = 512;
> +	va_list argptr;
> +	struct i40e_pf *pf = (struct i40e_pf *)(((struct i40e_hw *)hw)->back);
> +
> +	if (!(mask & ((struct i40e_hw *)hw)->debug_mask))
> +		return;
> +
> +	buf = kzalloc(buf_len, GFP_KERNEL);
> +
> +	if (!buf) {
> +		dev_err(&pf->pdev->dev, "%s - Out of memory\n", __func__);
> +		return;
> +	}
> +
> +	va_start(argptr, fmt_str);
> +	vsnprintf(buf, buf_len, fmt_str, argptr);
> +	va_end(argptr);
> +
> +	/* the debug string is already formatted with a newline */
> +	dev_info(&pf->pdev->dev, "%s", buf);
> +	kfree(buf);

%pV and avoid the malloc/free.

Also, it seems that perhaps all of these uses
try to use "%s: ", __func__, so maybe using
%pf, __builtin_return_address(0) might be better.

> +void i40e_update_stats(struct i40e_vsi *vsi)
> +{
[]
> +	for (q = 0; q < vsi->num_queue_pairs; q++) {
> +		rx_b += vsi->rx_rings[q].rx_stats.bytes;

better to use a temporary for vs->rx_rings[q]

> +

> struct i40e_mac_filter *i40e_add_filter(struct i40e_vsi *vsi,
> +					u8 *macaddr, u16 vlan,
> +					bool is_vf, bool is_netdev)
> +{
[]
> +		f = kzalloc(sizeof(*f), GFP_ATOMIC);
> +		if (NULL == f) {
> +			if (is_netdev)
> +				netdev_err(vsi->netdev,
> +					   "%s: no memory for new filter\n",
> +					   __func__);
> +			else
> +				dev_err(&vsi->back->pdev->dev,
> +					 "%s: no memory for new filter\n",
> +					 __func__);

No need for OOM messages, there are a lot
of these.

> +static int i40e_set_mac(struct net_device *netdev, void *p)
> +{
> +	struct i40e_netdev_priv *np = netdev_priv(netdev);
> +	struct i40e_vsi *vsi = np->vsi;
> +	struct sockaddr *addr = p;
> +	struct i40e_mac_filter *f;
> +
> +	netdev_info(netdev, "%s: address=%pM\n", __func__, addr->sa_data);

sa_addr?  Is family guaranteed to be ARPHDR_ETHER here?

[]

> +static void i40e_set_rx_mode(struct net_device *netdev)
> +{

> +		if (f->macaddr[0] & 0x01) {

is_multicast_ether_addr()

Sorry, this is very long and I wonder if you really
intend this to be merged as-is.


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