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  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 21 Jul 2020 10:36:36 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Tony Nguyen <anthony.l.nguyen@...el.com>
Cc:     davem@...emloft.net, Alice Michael <alice.michael@...el.com>,
        netdev@...r.kernel.org, nhorman@...hat.com, sassmann@...hat.com,
        jeffrey.t.kirsher@...el.com, Alan Brady <alan.brady@...el.com>,
        Phani Burra <phani.r.burra@...el.com>,
        Joshua Hay <joshua.a.hay@...el.com>,
        Madhu Chittim <madhu.chittim@...el.com>,
        Pavan Kumar Linga <pavan.kumar.linga@...el.com>,
        Donald Skidmore <donald.c.skidmore@...el.com>,
        Jesse Brandeburg <jesse.brandeburg@...el.com>,
        Sridhar Samudrala <sridhar.samudrala@...el.com>
Subject: Re: [net-next v4 02/15] iecm: Add framework set of header files

On Mon, 20 Jul 2020 17:37:57 -0700 Tony Nguyen wrote:
> diff --git a/include/linux/net/intel/iecm_alloc.h b/include/linux/net/intel/iecm_alloc.h
> new file mode 100644
> index 000000000000..fd13bf085663
> --- /dev/null
> +++ b/include/linux/net/intel/iecm_alloc.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2020, Intel Corporation. */
> +
> +#ifndef _IECM_ALLOC_H_
> +#define _IECM_ALLOC_H_
> +
> +/* Memory types */
> +enum iecm_memset_type {
> +	IECM_NONDMA_MEM = 0,
> +	IECM_DMA_MEM
> +};
> +
> +/* Memcpy types */
> +enum iecm_memcpy_type {
> +	IECM_NONDMA_TO_NONDMA = 0,
> +	IECM_NONDMA_TO_DMA,
> +	IECM_DMA_TO_DMA,
> +	IECM_DMA_TO_NONDMA
> +};

These enums are never used. Please remove all dead code in the submission.
(Unused HW interface defines are okay.)

> diff --git a/include/linux/net/intel/iecm.h b/include/linux/net/intel/iecm.h
> new file mode 100644
> index 000000000000..ef6e4d79f51f
> --- /dev/null
> +++ b/include/linux/net/intel/iecm.h
> @@ -0,0 +1,430 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright (C) 2020 Intel Corporation */
> +
> +#ifndef _IECM_H_
> +#define _IECM_H_
> +
> +#include <linux/aer.h>
> +#include <linux/bitmap.h>
> +#include <linux/compiler.h>
> +#include <linux/cpumask.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/errno.h>
> +#include <linux/etherdevice.h>
> +#include <linux/ethtool.h>
> +#include <linux/if_bridge.h>
> +#include <linux/if_vlan.h>
> +#include <linux/interrupt.h>
> +#include <linux/ip.h>
> +#include <linux/ipv6.h>
> +#include <linux/jiffies.h>
> +#include <linux/kernel.h>
> +#include <linux/log2.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/netdevice.h>
> +#include <linux/pci.h>
> +#include <linux/pkt_sched.h>
> +#include <linux/rtnetlink.h>
> +#include <linux/sctp.h>
> +#include <linux/skbuff.h>
> +#include <linux/string.h>
> +#include <linux/timer.h>
> +#include <linux/types.h>
> +#include <linux/workqueue.h>
> +#include <net/tcp.h>
> +#include <net/ipv6.h>
> +#include <net/ip6_checksum.h>
> +#include <linux/prefetch.h>
> +#include <net/pkt_sched.h>
> +
> +#include <net/gre.h>
> +#include <net/udp_tunnel.h>
> +#include <linux/avf/virtchnl.h>
> +
> +#include <linux/net/intel/iecm_osdep.h>
> +#include <linux/net/intel/iecm_controlq_api.h>
> +#include <linux/net/intel/iecm_lan_txrx.h>
> +
> +#include <linux/net/intel/iecm_txrx.h>
> +#include <linux/net/intel/iecm_type.h>

Please trim this litany of include files. You don't need most of them.

> diff --git a/include/linux/net/intel/iecm_osdep.h b/include/linux/net/intel/iecm_osdep.h
> new file mode 100644
> index 000000000000..33e01582f516
> --- /dev/null
> +++ b/include/linux/net/intel/iecm_osdep.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2020, Intel Corporation. */
> +
> +#ifndef _IECM_OSDEP_H_
> +#define _IECM_OSDEP_H_

OS abstraction layers are not allowed in Linux drivers. Please rename
this to something meaningful or marge the contents to appropriate
headers and remove this one.

> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/net/intel/iecm_alloc.h>

None of these headers are required here. Yet you fail to include io.h..

> +#define wr32(a, reg, value)	writel((value), ((a)->hw_addr + (reg)))
> +#define rd32(a, reg)		readl((a)->hw_addr + (reg))
> +#define wr64(a, reg, value)	writeq((value), ((a)->hw_addr + (reg)))
> +#define rd64(a, reg)		readq((a)->hw_addr + (reg))
> +
> +struct iecm_dma_mem {
> +	void *va;
> +	dma_addr_t pa;
> +	size_t size;
> +};
> +
> +#define iecm_wmb() wmb() /* memory barrier */

Please remove this define and use wmb() directly.

> +#endif /* _IECM_OSDEP_H_ */

> diff --git a/include/linux/net/intel/iecm_controlq.h b/include/linux/net/intel/iecm_controlq.h
> new file mode 100644
> index 000000000000..4cba637042cd
> --- /dev/null
> +++ b/include/linux/net/intel/iecm_controlq.h
> @@ -0,0 +1,95 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2020, Intel Corporation. */
> +
> +#ifndef _IECM_CONTROLQ_H_
> +#define _IECM_CONTROLQ_H_
> +
> +#include <linux/net/intel/iecm_controlq_api.h>
> +
> +/* Maximum buffer lengths for all control queue types */
> +#define IECM_CTLQ_MAX_RING_SIZE	1024
> +#define IECM_CTLQ_MAX_BUF_LEN	4096
> +
> +#define IECM_CTLQ_DESC(R, i) \
> +	(&(((struct iecm_ctlq_desc *)((R)->desc_ring.va))[i]))
> +
> +#define IECM_CTLQ_DESC_UNUSED(R) \
> +	(u16)((((R)->next_to_clean > (R)->next_to_use) ? 0 : (R)->ring_size) + \
> +	      (R)->next_to_clean - (R)->next_to_use - 1)
> +
> +/* Data type manipulation macros. */
> +#define IECM_HI_DWORD(x)	((u32)((((x) >> 16) >> 16) & 0xFFFFFFFF))
> +#define IECM_LO_DWORD(x)	((u32)((x) & 0xFFFFFFFF))

Please use lower_32_bits() / upper_32_bits() directly.

> +#define IECM_HI_WORD(x)		((u16)(((x) >> 16) & 0xFFFF))
> +#define IECM_LO_WORD(x)		((u16)((x) & 0xFFFF))

unused

Powered by blists - more mailing lists