netdev - Re: [PATCH net-next v7 01/10] qed: Add module with basic common support

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: <20151024.051149.1911984823506391327.davem@davemloft.net>
Date:	Sat, 24 Oct 2015 05:11:49 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	Yuval.Mintz@...gic.com
Cc:	netdev@...r.kernel.org, Ariel.Elior@...gic.com
Subject: Re: [PATCH net-next v7 01/10] qed: Add module with basic common
 support

From: Yuval Mintz <Yuval.Mintz@...gic.com>
Date: Thu, 22 Oct 2015 08:06:48 +0300

> diff --git a/drivers/net/ethernet/qlogic/qed/Makefile b/drivers/net/ethernet/qlogic/qed/Makefile
> new file mode 100644
> index 0000000..5bbe0c7
> --- /dev/null
> +++ b/drivers/net/ethernet/qlogic/qed/Makefile
> @@ -0,0 +1,3 @@
> +obj-$(CONFIG_QED) := qed.o
> +
> +qed-y := qed_cxt.o qed_dev.o qed_hw.o qed_init_fw_funcs.o qed_init_ops.o qed_int.o qed_main.o qed_mcp.o qed_sp_commands.o qed_spq.o

Please break this qed-y assignment into multiple lines using "\".

> +#ifndef _QED_H
> +#define _QED_H
> +#include <linux/types.h>

Please put an empty line after "#define _QED_H"

> +/* helpers */
> +static inline u32 DB_ADDR(u32 cid, u32 DEMS)
> +{
> +	u32 db_addr = FIELD_VALUE(DB_LEGACY_ADDR_DEMS, DEMS) |
> +		      FIELD_VALUE(DB_LEGACY_ADDR_ICID, cid);
> +
> +	return db_addr;
> +}

Generally speaking, inline functions should be named with lowercase
letters, CPP macros that act like functions can be uppercase.

> +#define MAP_WORD_SIZE sizeof(unsigned long)
> +#define BITS_PER_MAP_WORD (MAP_WORD_SIZE * 8)

Please don't reimplement something as fundamental as a bitmap of words, use
the faciltiies and helpers in linux/bitmap.h instead.

> +/* counts the iids for the CDU/CDUC ILT client configuration */
> +struct qed_cdu_iids {
> +	u32	pf_cids;
> +};

This one member structure is excessive, it's only ever instantiated
as a local variable in this file, so just use "u32" directly.

> +	p_info->p_cxt = (u8 *)p_mngr->ilt_shadow[line].p_virt +
> +			      p_info->iid % cxts_per_p * conn_cxt_size;

p_virt is a void pointer, therefore you don't need to cast it to have
arithmatic on it work properly.

> +	if (qed_mcp_is_init(p_hwfn)) {
> +		ether_addr_copy(p_hwfn->hw_info.hw_mac_addr,
> +				p_hwfn->mcp_info->func_info.mac);
> +	} else {
> +		static u8 mcp_hw_mac[6] = { 0, 2, 3, 4, 5, 6 };
> +
> +		ether_addr_copy(p_hwfn->hw_info.hw_mac_addr, mcp_hw_mac);
> +		p_hwfn->hw_info.hw_mac_addr[5] = p_hwfn->abs_pf_id;
> +	}

In this else branch you should probably use a random ethernet address.
If that is not correct here, this is not expected and requires a
detailed comment explaining why this fixed ethernet address is being
used.

> +static int qed_get_dev_info(struct qed_dev *cdev)
 ...
> +	return 0;

This never returns anything other than zero, make it return void.

> +int qed_hw_prepare(struct qed_dev	*cdev,
> +		   int			personality)

Please do not declare function variables with tab characters
separating the type from the variable name.  Likewise when these
functions are externally declared in header files.

> +		void __iomem *p_regview, *p_doorbell;
> +
> +		p_regview = (void __iomem *)
> +			    ((u8 __iomem *)cdev->regview + i *

Again, void pointers do not need to be cast to "u8 *" in order
to perform byte arithmatic on them.
--
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