Gigabit Ethernet driver of Topcliff PCH

---

drivers/net/pch_gbe/pch_gbe.h new file +17 -0

new file mode 100644
index 0000000..a2b92dd
@@ -0,0 +1,425 @@
+/*
+ * Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307, USA.
+ */
+

drivers/net/pch_gbe/pch_gbe.h new file +10 -0

+
+/* Device Driver infomation */
+#define DRV_STRING      "PCH Network Driver"
+#define DRV_EXT         "-NAPI"
+#define DRV_VERSION     "0.95"DRV_EXT
+#define DRV_DESCRIPTION \
+	"OKI semiconductor sample Linux driver for PCH Gigabit ethernet"
+#define DRV_COPYRIGHT   "Copyright(c) 2010 OKI semiconductor"
+#define FIRM_VERSION    "N/A"
+/* TX/RX descriptor defines */

Hi Masayuki Ohtake.

Just a few comments after a very quick scan over some of the files.

Sam

Quoted 1 line · Masayuki Ohtake

---

"---" is used to truncate your changelog, random comments goes below "---". So there would be no changelog entry in this case.

Quoted 6 lines

drivers/net/Kconfig +0 -0

index 2decc59..fcc6b7b 100644

drivers/net/Kconfig menuconfig NETDEV_1000 · line 2011

Quoted 8 lines

drivers/net/Kconfig +5 -0

@@ -2004,6 +2004,11 @@ menuconfig NETDEV_1000
   If you say N, all options in this submenu will be skipped and disabled.
 
 if NETDEV_1000
+config PCH_GBE
+        tristate "PCH Gigabit Ethernet"
+        ---help---
+          This is an gigabit ethernet driver for PCH.
+          resources.

Naive readers do not know what PCH is.
Google says it is: Potlatch Corporation

Please elaborate much more.

For new stuff we use "help" - not "---help---". But you could not know as this is not documented anywhere.

drivers/net/pch_gbe/Makefile line 3

Quoted 10 lines

drivers/net/pch_gbe/Makefile new file +3 -0

new file mode 100644
index 0000000..008fa22
@@ -0,0 +1,6 @@
+ifeq ($(CONFIG_PCH_GBE_DEBUG_CORE),y)
+EXTRA_CFLAGS += -DDEBUG
+endif

Use:
ccflags-$(CONFIG_PCH_GBE_DEBUG_CORE) := -DDEBUG

Btw. where do CONFIG_PCH_GBE_DEBUG_CORE come from. It was not in the Kconfig file you included.

drivers/net/pch_gbe/Makefile line 6

Quoted 3 lines

drivers/net/pch_gbe/Makefile new file +3 -0

+
+obj-$(CONFIG_PCH_GBE) += pch_gbe.o
+pch_gbe-objs := pch_gbe_phy.o pch_gbe_ethtool.o pch_gbe_param.o pch_gbe_api.o pch_gbe_main.o

Please do not use -objs.
Do like this:

obj-$(CONFIG_PCH_GBE) += pch_gbe.o
pch_gbe-y := pch_gbe_phy.o pch_gbe_ethtool.o pch_gbe_param.o pch_gbe-y += pch_gbe_api.o pch_gbe_main.o

drivers/net/pch_gbe/Module.symvers

Quoted 4 lines

drivers/net/pch_gbe/Module.symvers new file +0 -0

new file mode 100644
index 0000000..e69de29

This file should be dropped.

Quoted 27 lines

drivers/net/pch_gbe/pch_gbe.h new file +20 -0

new file mode 100644
index 0000000..a2b92dd
@@ -0,0 +1,425 @@
+/*
+ * Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307, USA.
+ */
+
+#ifndef _PCH_GBE_H_
+#define _PCH_GBE_H_
+

drivers/net/pch_gbe/pch_gbe.h line 27

Quoted 7 lines

drivers/net/pch_gbe/pch_gbe.h new file +7 -0

+#include <linux/netdevice.h>
+#include <linux/mii.h>
+
+#undef FALSE
+#define FALSE 0
+#undef TRUE
+#define TRUE 1

Use true/false - no reason for these.

drivers/net/pch_gbe/pch_gbe.h line 34

Quoted 7 lines

drivers/net/pch_gbe/pch_gbe.h new file +7 -0

+
+#ifdef DEBUG
+#define PCH_GBE_DEBUG(fmt, args...)\
+ printk(KERN_DEBUG KBUILD_MODNAME ": " fmt, ##args)
+#else
+#define PCH_GBE_DEBUG(fmt, args...) do { } while (0)
+#endif

Use standard logging methods.
pr_* and similar.

Quoted 18 lines

drivers/net/pch_gbe/pch_gbe.h new file +17 -0

+
+/* Device Driver infomation */
+#define DRV_STRING      "PCH Network Driver"
+#define DRV_EXT         "-NAPI"
+#define DRV_VERSION     "0.95"DRV_EXT
+#define DRV_DESCRIPTION \
+ "OKI semiconductor sample Linux driver for PCH Gigabit ethernet"
+#define DRV_COPYRIGHT   "Copyright(c) 2010 OKI semiconductor"
+#define FIRM_VERSION    "N/A"
+/* TX/RX descriptor defines */
+#define PCH_GBE_MAX_TXD                     4096
+#define PCH_GBE_MIN_TXD                        8
+#define PCH_GBE_MAX_RXD                     4096
+#define PCH_GBE_MIN_RXD                        8
+/* Number of Transmit and Receive Descriptors must be a multiple of 8 */
+#define PCH_GBE_TX_DESC_MULTIPLE               8
+#define PCH_GBE_RX_DESC_MULTIPLE               8

drivers/net/pch_gbe/pch_gbe.h line 53

Quoted 2 lines

drivers/net/pch_gbe/pch_gbe.h new file +2 -0

+/* only works for sizes that are powers of 2 */
+#define PCH_GBE_ROUNDUP(i, size) ((i) = (((i) + (size) - 1) & ~((size) - 1)))

We have this already. Search include/linux/*

drivers/net/pch_gbe/pch_gbe.h line 89

Quoted 7 lines

drivers/net/pch_gbe/pch_gbe.h new file +7 -0

+ * @read_mac_addr: for pch_gbe_hal_read_mac_addr
+ */
+struct pch_gbe_functions {
+ void (*get_bus_info) (struct pch_gbe_hw *);
+ s32(*init_hw) (struct pch_gbe_hw *);
+ s32(*read_phy_reg) (struct pch_gbe_hw *, u32, u16 *);
+ s32(*write_phy_reg) (struct pch_gbe_hw *, u32, u16);

missign space after s32

Quoted 7 lines

drivers/net/pch_gbe/pch_gbe.h new file +7 -0

+ void (*reset_phy) (struct pch_gbe_hw *);
+ void (*sw_reset_phy) (struct pch_gbe_hw *);
+ void (*power_up_phy) (struct pch_gbe_hw *hw);
+ void (*power_down_phy) (struct pch_gbe_hw *hw);
+ s32(*read_mac_addr) (struct pch_gbe_hw *);
+};
+

drivers/net/pch_gbe/pch_gbe.h line 68

Quoted 1 line

drivers/net/pch_gbe/pch_gbe.h new file +1 -0

+ printk(KERN_ERR KBUILD_MODNAME ": " "ERROR: Registers not mapped\n");

In several places...
Use standard logging methods to do this simpler.

drivers/net/pch_gbe/pch_gbe.h line 1

Quoted 5 lines

drivers/net/pch_gbe/pch_gbe.h new file +2 -0

+++ b/drivers/net/pch_gbe/pch_gbe_api.h

@@ -0,0 +1,31 @@
+/*

Why is two .h files needed for a single driver?
As this file live in drivers/net/pch_gbe/ this is purely internal stuff.

drivers/net/pch_gbe/pch_gbe.h line 4

Quoted 4 lines

drivers/net/pch_gbe/pch_gbe.h new file +3 -0

+#include "pch_gbe_regs.h"
+#include "pch_gbe.h"
+#include "pch_gbe_api.h"

As per comment above I realise that this single driver has three .h files. Maybe their size warrants this?

Gigabit Ethernet driver of Topcliff PCH
+++ b/drivers/net/pch_gbe/pch_gbe.h

---
Gigabit Ethernet driver of Topcliff PCH

Topcliff PCH is the platform controller hub that is going to be used in Intel's upcoming general embedded platform. All IO peripherals in Topcliff PCH are actually devices sitting on AMBA bus. Topcliff PCH has Gigabit Ethernet I/F. Using this I/F, it is able to access system devices connected to Gigabit Ethernet.

Signed-off-by Masayuki Ohtake <[email protected]>

---

drivers/net/pch_gbe/pch_gbe.h new file +25 -0

+
+struct pch_gbe_hw;
+/**
+ * struct  pch_gbe_functions - HAL APi function pointer
+ * @get_bus_info:	for pch_gbe_hal_get_bus_info
+ * @init_hw:		for pch_gbe_hal_init_hw
+ * @read_phy_reg:	for pch_gbe_hal_read_phy_reg
+ * @write_phy_reg:	for pch_gbe_hal_write_phy_reg
+ * @reset_phy:		for pch_gbe_hal_phy_hw_reset
+ * @sw_reset_phy:	for pch_gbe_hal_phy_sw_reset
+ * @power_up_phy:	for pch_gbe_hal_power_up_phy
+ * @power_down_phy:	for pch_gbe_hal_power_down_phy
+ * @read_mac_addr:	for pch_gbe_hal_read_mac_addr
+ */
+struct pch_gbe_functions {
+	void (*get_bus_info) (struct pch_gbe_hw *);
+	s32(*init_hw) (struct pch_gbe_hw *);
+	s32(*read_phy_reg) (struct pch_gbe_hw *, u32, u16 *);
+	s32(*write_phy_reg) (struct pch_gbe_hw *, u32, u16);
+	void (*reset_phy) (struct pch_gbe_hw *);
+	void (*sw_reset_phy) (struct pch_gbe_hw *);
+	void (*power_up_phy) (struct pch_gbe_hw *hw);
+	void (*power_down_phy) (struct pch_gbe_hw *hw);
+	s32(*read_mac_addr) (struct pch_gbe_hw *);
+};

drivers/net/pch_gbe/pch_gbe.h new file +10 -0

+	struct pch_gbe_mac_info mac;
+	struct pch_gbe_phy_info phy;
+	struct pch_gbe_bus_info bus;
+
+	u16 vendor_id;
+	u16 device_id;
+	u16 subsystem_vendor_id;
+	u16 subsystem_device_id;
+	u8 revision_id;
+};

drivers/net/pch_gbe/pch_gbe_api.c new file +72 -0

@@ -0,0 +1,247 @@
+/*
+ * Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307, USA.
+ */
+
+#include "pch_gbe.h"
+#include "pch_gbe_phy.h"
+
+/* bus type values */
+#define pch_gbe_bus_type_unknown	0
+#define pch_gbe_bus_type_pci		1
+#define pch_gbe_bus_type_pcix		2
+#define pch_gbe_bus_type_pci_express	3
+#define pch_gbe_bus_type_reserved	4
+
+/* bus speed values */
+#define pch_gbe_bus_speed_unknown	0
+#define pch_gbe_bus_speed_33		1
+#define pch_gbe_bus_speed_66		2
+#define pch_gbe_bus_speed_100		3
+#define pch_gbe_bus_speed_120		4
+#define pch_gbe_bus_speed_133		5
+#define pch_gbe_bus_speed_2500		6
+#define pch_gbe_bus_speed_reserved	7
+
+/* bus width values */
+#define pch_gbe_bus_width_unknown	0
+#define pch_gbe_bus_width_pcie_x1	1
+#define pch_gbe_bus_width_pcie_x2	2
+#define pch_gbe_bus_width_pcie_x4	4
+#define pch_gbe_bus_width_32		5
+#define pch_gbe_bus_width_64		6
+#define pch_gbe_bus_width_reserved	7
+
+/**
+ * pch_gbe_plat_get_bus_info - Obtain bus information for adapter
+ * @hw:	Pointer to the HW structure
+ */
+static void pch_gbe_plat_get_bus_info(struct pch_gbe_hw *hw)
+{
+	hw->bus.type  = pch_gbe_bus_type_pci_express;
+	hw->bus.speed = pch_gbe_bus_speed_2500;
+	hw->bus.width = pch_gbe_bus_width_pcie_x1;
+}
+
+/**
+ * pch_gbe_plat_init_hw - Initialize hardware
+ * @hw:	Pointer to the HW structure
+ * Returns
+ *	0:		Successfully
+ *	Negative value:	Failed-EBUSY
+ */
+static s32 pch_gbe_plat_init_hw(struct pch_gbe_hw *hw)
+{
+	s32 ret_val;
+
+	ret_val = pch_gbe_phy_get_id(hw);
+	if (ret_val) {
+		printk(KERN_ERR KBUILD_MODNAME ": "
+			"pch_gbe_phy_get_id error\n");

drivers/net/pch_gbe/pch_gbe_api.c new file +1 -0

+}

drivers/net/pch_gbe/pch_gbe_ethtool.c new file +12 -0

+static int pch_gbe_get_settings(struct net_device *netdev,
+				 struct ethtool_cmd *ecmd)
+{
+	struct pch_gbe_adapter *adapter = netdev_priv(netdev);
+	int ret;
+
+	PCH_GBE_DEBUG("ethtool: %s\n", __func__);
+
+	ret = mii_ethtool_gset(&adapter->mii, ecmd);
+	ecmd->supported &= (u32) (~SUPPORTED_TP);
+	ecmd->supported &= (u32) (~SUPPORTED_1000baseT_Half);
+	ecmd->advertising &= (u32) (~ADVERTISED_TP);

drivers/net/pch_gbe/pch_gbe_ethtool.c new file +10 -0

+
+/**
+ * pch_gbe_set_settings - Set device-specific settings
+ * @netdev: Network interface device structure
+ * @ecmd:   Ethtool command
+ * Returns
+ *	0:			Successful.
+ *	Negative value:		Failed.
+ */
+static int pch_gbe_set_settings(struct net_device *netdev,

drivers/net/pch_gbe/pch_gbe_ethtool.c new file +14 -0

+
+static struct ethtool_ops pch_gbe_ethtool_ops = {
+	.get_settings = pch_gbe_get_settings,
+	.set_settings = pch_gbe_set_settings,
+	.get_drvinfo = pch_gbe_get_drvinfo,
+	.get_regs_len = pch_gbe_get_regs_len,
+	.get_regs = pch_gbe_get_regs,
+	.get_wol = pch_gbe_get_wol,
+	.set_wol = pch_gbe_set_wol,
+	.nway_reset = pch_gbe_nway_reset,
+	.get_link = ethtool_op_get_link,
+	.get_ringparam = pch_gbe_get_ringparam,
+	.set_ringparam = pch_gbe_set_ringparam,
+	.get_pauseparam = pch_gbe_get_pauseparam,

s32 pch_gbe_mac_read_mac_addr(struct pch_gbe_hw *hw)

drivers/net/pch_gbe/pch_gbe_main.c new file +8 -0

+{
+	u32  adr1a, adr1b;
+
+	PCH_GBE_DEBUG("pch_gbe_mac_read_mac_addr\n");
+
+	adr1a = ioread32(&hw->reg->mac_adr[0].high);
+	adr1b = ioread32(&hw->reg->mac_adr[0].low);
+

drivers/net/pch_gbe/pch_gbe_main.c new file +13 -0

+/**
+ * pch_gbe_mac_init_rx_addrs - Initialize receive address's
+ * @hw:	Pointer to the HW structure
+ * @mar_count: Receive address registers
+ */
+void pch_gbe_mac_init_rx_addrs(struct pch_gbe_hw *hw, u16 mar_count)
+{
+	u32 i;
+	PCH_GBE_DEBUG("pch_gbe_mac_init_rx_addrs\n");
+
+	/* Setup the receive address */
+	pch_gbe_mac_mar_set(hw, hw->mac.addr, 0);
+

drivers/net/pch_gbe/pch_gbe_main.c new file +24 -0

+/**
+ * pch_gbe_alloc_queues - Allocate memory for all rings
+ * @adapter:  Board private structure to initialize
+ * Returns
+ *	0:	Successfully
+ *	Negative value:	Failed
+ */
+static int pch_gbe_alloc_queues(struct pch_gbe_adapter *adapter)
+{
+	int size;
+
+	PCH_GBE_DEBUG("%s\n", __func__);
+
+
+	size = (int)sizeof(struct pch_gbe_tx_ring);
+	adapter->tx_ring = kmalloc(size, GFP_KERNEL);
+	if (!adapter->tx_ring)
+		return -ENOMEM;
+	memset(adapter->tx_ring, 0, size);
+
+	size = (int)sizeof(struct pch_gbe_rx_ring);
+	adapter->rx_ring = kmalloc(size, GFP_KERNEL);
+	if (!adapter->rx_ring) {
+		kfree(adapter->tx_ring);

drivers/net/pch_gbe/pch_gbe_main.c new file +50 -0

+static void pch_gbe_tx_queue(struct pch_gbe_adapter *adapter,
+		 struct pch_gbe_tx_ring *tx_ring, struct sk_buff *skb)
+{
+	struct pch_gbe_hw *hw = &adapter->hw;
+	struct pch_gbe_tx_desc *tx_desc;
+	struct pch_gbe_buffer *buffer_info;
+	struct sk_buff *tmp_skb;
+	unsigned int frame_ctrl;
+	unsigned int ring_num;
+	unsigned long flags;
+
+	PCH_GBE_DEBUG("%s\n", __func__);
+
+
+	/*-- Set frame control --*/
+	frame_ctrl = 0;
+	if (unlikely(skb->len < PCH_GBE_SHORT_PKT))
+		frame_ctrl |= PCH_GBE_TXD_CTRL_APAD;
+	if (unlikely(adapter->tx_csum == FALSE))
+		frame_ctrl |= PCH_GBE_TXD_CTRL_TCPIP_ACC_OFF;
+
+	/* Performs checksum processing */
+	/*
+	 * It is because the hardware accelerator does not support a checksum,
+	 * when the received data size is less than 64 bytes.
+	 */
+	if ((skb->len < PCH_GBE_SHORT_PKT) && (adapter->tx_csum == TRUE)) {
+		frame_ctrl |=
+			PCH_GBE_TXD_CTRL_APAD | PCH_GBE_TXD_CTRL_TCPIP_ACC_OFF;
+		if (skb->protocol == htons(ETH_P_IP)) {
+			struct iphdr *iph = ip_hdr(skb);
+			unsigned int offset;
+			iph->check = 0;
+			iph->check = ip_fast_csum((u8 *) iph, iph->ihl);
+			offset = skb_transport_offset(skb);
+			if (iph->protocol == IPPROTO_TCP) {
+				skb->csum = 0;
+				tcp_hdr(skb)->check = 0;
+				skb->csum =
+					skb_checksum(skb, offset,
+						skb->len - offset, 0);
+				tcp_hdr(skb)->check =
+					csum_tcpudp_magic(iph->saddr,
+							iph->daddr,
+							skb->len - offset,
+							IPPROTO_TCP, skb->csum);
+			} else if (iph->protocol == IPPROTO_UDP) {
+				skb->csum = 0;
+				udp_hdr(skb)->check = 0;
+				skb->csum =

drivers/net/pch_gbe/pch_gbe_main.c new file +3 -0

+	for (i = 0; i < 6; i++)
+		printk(KERN_INFO "%2.2x%c",
+			netdev->dev_addr[i], i == 5 ? '\n' : ':');

you just want:

         printk(KERN_INFO "%pM", netdev->dev_addr);

or better yet, convert to dev_printk

Quoted 8 lines · Stephen Hemminger

drivers/net/pch_gbe/pch_gbe_main.c new file +3 -0

+	for (i = 0; i < 6; i++)
+		printk(KERN_INFO "%2.2x%c",
+			netdev->dev_addr[i], i == 5 ? '\n' : ':');

you just want:

         printk(KERN_INFO "%pM", netdev->dev_addr);

or better yet, convert to dev_printk

or perhaps better still convert to netdev_info

My message logging order preference:

if there's a netif_msg_<type> and a struct net_device

	netif_<level>		(struct private *, netif_msg_<type>,
				 struct net_device *, fmt, args...)
if there's a struct net_device
	netdev_<level>		(struct net_device *, fmt, args...)
if there's a struct device
	dev_<level>		(struct device *, fmt, args...)
otherwise
	pr_<level>		(fmt, args...)

Quoted 10 lines · On Thu, 26 Aug 2010 09:16:12 -0700 Joe Perches <[email protected]> wrote:

My message logging order preference:
if there's a netif_msg_<type> and a struct net_device

	netif_<level>		(struct private *, netif_msg_<type>,
				 struct net_device *, fmt, args...)
if there's a struct net_device
	netdev_<level>		(struct net_device *, fmt, args...)
if there's a struct device
	dev_<level>		(struct device *, fmt, args...)
otherwise
	pr_<level>		(fmt, args...)

At this point in process, netdevice isn't registered so it doesn't have a ethX name. so dev_info would be best.

Hi Sam, Joe and Stephen

Thank you for your comments.
We have modified this driver for your comment.

---
Gigabit Ethernet driver of Topcliff PCH

drivers/net/pch_gbe/pch_gbe_main.c new file +10 -0

+			pr_debug
+			    ("Receive skb->ip_summed: %d length: %d\n",
+			     skb->ip_summed, length);
+		}
+dorrop:
+		/* return some buffers to hardware, one at a time is too slow */
+		if (unlikely(cleaned_count >= PCH_GBE_RX_BUFFER_WRITE)) {
+			pch_gbe_alloc_rx_buffers(adapter, rx_ring,
+						 cleaned_count);
+			cleaned_count = 0;

I find hard to believe this driver needs to copy all outgoing frames on pre-allocated skbs.

drivers/net/pch_gbe/pch_gbe_main.c new file +9 -0

+       /* [Header:14][payload] ---> [Header:14][paddong:2][payload]    */
+       memcpy(tmp_skb->data, skb->data, ETH_HLEN);
+       tmp_skb->data[ETH_HLEN] = 0x00;
+       tmp_skb->data[ETH_HLEN + 1] = 0x00;
+       tmp_skb->len = skb->len;
+       memcpy(&tmp_skb->data[ETH_HLEN + 2], &skb->data[ETH_HLEN],
+              (skb->len - ETH_HLEN));
+       buffer_info->kernel_skb = skb;
+       skb = tmp_skb;

Whats the deal here please ?

Le mardi 31 aout 2010 a 23:15 +0900, Masayuki Ohtake a ecrit :

Quoted 10 lines

Hi Sam, Joe and Stephen

Thank you for your comments.
We have modified this driver for your comment.

---
Gigabit Ethernet driver of Topcliff PCH

+ skb->protocol = eth_type_trans(skb, netdev);
+ if ((tcp_ip_status & PCH_GBE_RXD_ACC_STAT_TCPIPOK) ==

drivers/net/pch_gbe/pch_gbe_main.c line 1526

Quoted 8 lines

drivers/net/pch_gbe/pch_gbe_main.c new file +8 -0

+     PCH_GBE_RXD_ACC_STAT_TCPIPOK) {
+ skb->ip_summed = CHECKSUM_UNNECESSARY;
+ } else {
+ skb->ip_summed = CHECKSUM_NONE;
+ }
+
+ if (netif_receive_skb(skb) == NET_RX_DROP) {
+ adapter->stats.rx_dropped++;

dont use pr_err() here, dropping a frame is a normal condition.

Quoted 4 lines

+ pr_err("Receive Netif Receive Dropped Error\n");
+ }
+ (*work_done)++;
+ netdev->last_rx = jiffies;

really ? last_rx is a thing of the past.

Quoted 10 lines

+ pr_debug

drivers/net/pch_gbe/pch_gbe_main.c new file +9 -0

+     ("Receive skb->ip_summed: %d length: %d\n",
+      skb->ip_summed, length);
+ }
+dorrop:
+ /* return some buffers to hardware, one at a time is too slow */
+ if (unlikely(cleaned_count >= PCH_GBE_RX_BUFFER_WRITE)) {
+ pch_gbe_alloc_rx_buffers(adapter, rx_ring,
+ cleaned_count);
+ cleaned_count = 0;

drivers/net/pch_gbe/pch_gbe_main.c line 1550

Quoted 10 lines

drivers/net/pch_gbe/pch_gbe_main.c new file +10 -0

+ }
+ if (++i == rx_ring->count)
+ i = 0;
+ }
+ rx_ring->next_to_clean = i;
+ if (cleaned_count)
+ pch_gbe_alloc_rx_buffers(adapter, rx_ring, cleaned_count);
+ return cleaned;
+}
+

I find hard to believe this driver needs to copy all outgoing frames on pre-allocated skbs.

drivers/net/pch_gbe/pch_gbe_main.c new file +9 -0

+       /* [Header:14][payload] ---> [Header:14][paddong:2][payload]    */
+       memcpy(tmp_skb->data, skb->data, ETH_HLEN);
+       tmp_skb->data[ETH_HLEN] = 0x00;
+       tmp_skb->data[ETH_HLEN + 1] = 0x00;
+       tmp_skb->len = skb->len;
+       memcpy(&tmp_skb->data[ETH_HLEN + 2], &skb->data[ETH_HLEN],
+              (skb->len - ETH_HLEN));
+       buffer_info->kernel_skb = skb;
+       skb = tmp_skb;

Whats the deal here please ?

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Eric

Thank you for your comments.

Quoted 13 lines

I find hard to believe this driver needs to copy all outgoing frames on pre-allocated skbs.

drivers/net/pch_gbe/pch_gbe_main.c new file +9 -0

+       /* [Header:14][payload] ---> [Header:14][paddong:2][payload]    */
+       memcpy(tmp_skb->data, skb->data, ETH_HLEN);
+       tmp_skb->data[ETH_HLEN] = 0x00;
+       tmp_skb->data[ETH_HLEN + 1] = 0x00;
+       tmp_skb->len = skb->len;
+       memcpy(&tmp_skb->data[ETH_HLEN + 2], &skb->data[ETH_HLEN],
+              (skb->len - ETH_HLEN));
+       buffer_info->kernel_skb = skb;
+       skb = tmp_skb;

Quoted 1 line

Whats the deal here please ?

This processing depends on hardware specification.

At the time of transmission.
Hardware accepts a packet in the following format.
[Header: 14octet] + [padding: 2octet] + [payload]
Also, it is necessary to align the head of a [Header: 14octet] at 64byte.

In my knowledge, SKB received by kernel are the following format.
[padding: 2octet] + [Header:14octet] + [payload] Also, The head of [payload] has aligned at 16 byte.

So, it has adjusted with the format of hardware by a copy.

Quoted 26 lines · Masayuki Ohtake

Hi Eric

Thank you for your comments.

Quoted 13 lines

I find hard to believe this driver needs to copy all outgoing frames on pre-allocated skbs.

drivers/net/pch_gbe/pch_gbe_main.c new file +9 -0

+       /* [Header:14][payload] ---> [Header:14][paddong:2][payload]    */
+       memcpy(tmp_skb->data, skb->data, ETH_HLEN);
+       tmp_skb->data[ETH_HLEN] = 0x00;
+       tmp_skb->data[ETH_HLEN + 1] = 0x00;
+       tmp_skb->len = skb->len;
+       memcpy(&tmp_skb->data[ETH_HLEN + 2], &skb->data[ETH_HLEN],
+              (skb->len - ETH_HLEN));
+       buffer_info->kernel_skb = skb;
+       skb = tmp_skb;

Quoted 1 line

Whats the deal here please ?

This processing depends on hardware specification.

At the time of transmission.
Hardware accepts a packet in the following format.
[Header: 14octet] + [padding: 2octet] + [payload]
Also, it is necessary to align the head of a [Header: 14octet] at 64byte.

Quoted 5 lines

In my knowledge, SKB received by kernel are the following format.
[padding: 2octet] + [Header:14octet] + [payload] Also, The head of [payload] has aligned at 16 byte.

So, it has adjusted with the format of hardware by a copy.

The two bytes padding can be handled by network stack, if you force in your setup phase :

	dev->hard_header_len = ETH_HLEN + 2;

then, you can do a single memcpy:

memcpy(tmp_skb->data, skb->data, skb->len);

About the 64 byte alignement, it might be a bit more complex :)

Something like:

      skb = skb_realloc_headroom(skb, 2);
      if (!skb)
          goto drop;
__skb_push(skb, 2);
       memmove(skb->data, skb->data + 2, ETH_HLEN);

skb->data[ETH_HLEN] = 0;
skb->data[ETH_HLEN+1] = 0;

       if (!IS_ALIGNED(skb->data, 16)) {
   nskb = netdev_alloc_skb(dev, skb->len + 16);
   if (!nskb)
               goto drop2;
           skb_reserve(nskb, PTR_ALIGN(skb->data, 16) - skb->data);
   skb_put(skb, skb->len);
           memcpy(nskb->data, skb->data, skb->len);
   dev_kfree_skb(skb);
           skb = nskb;
       }

Quoted 26 lines · Masayuki Ohtake

Hi Eric

Thank you for your comments.

Quoted 13 lines

I find hard to believe this driver needs to copy all outgoing frames on pre-allocated skbs.

drivers/net/pch_gbe/pch_gbe_main.c new file +9 -0

+       /* [Header:14][payload] ---> [Header:14][paddong:2][payload]    */
+       memcpy(tmp_skb->data, skb->data, ETH_HLEN);
+       tmp_skb->data[ETH_HLEN] = 0x00;
+       tmp_skb->data[ETH_HLEN + 1] = 0x00;
+       tmp_skb->len = skb->len;
+       memcpy(&tmp_skb->data[ETH_HLEN + 2], &skb->data[ETH_HLEN],
+              (skb->len - ETH_HLEN));
+       buffer_info->kernel_skb = skb;
+       skb = tmp_skb;

Quoted 1 line

Whats the deal here please ?

This processing depends on hardware specification.

At the time of transmission.
Hardware accepts a packet in the following format.
[Header: 14octet] + [padding: 2octet] + [payload]
Also, it is necessary to align the head of a [Header: 14octet] at 64byte.

Quoted 5 lines

In my knowledge, SKB received by kernel are the following format.
[padding: 2octet] + [Header:14octet] + [payload] Also, The head of [payload] has aligned at 16 byte.

So, it has adjusted with the format of hardware by a copy.

The two bytes padding can be handled by network stack, if you force in your setup phase :
dev->hard_header_len = ETH_HLEN + 2;

then, you can do a single memcpy:

memcpy(tmp_skb->data, skb->data, skb->len);

About the 64 byte alignement, it might be a bit more complex :)

Hi Eric and Stephen

Thank you for your suggestion.

I tried modification of "dev->hard_header_len".
However, there was no changed to a format of SKB received by kernel.
[Header:14octet] + [payload]

So, I can not a single memcpy.

Le vendredi 03 septembre 2010 a 22:32 +0900, Masayuki Ohtake a ecrit :

Quoted 9 lines

Hi Eric and Stephen

Thank you for your suggestion.

I tried modification of "dev->hard_header_len".
However, there was no changed to a format of SKB received by kernel.
[Header:14octet] + [payload]

So, I can not a single memcpy.

Hmm, maybe you did this at the wrong place ?

Take a look at other drivers how they do this

(network stack default hard_header_len to 14, in net/ethernet/eth.c, function ether_setup())

find drivers/net | xargs grep -n hard_header_len

Example in drivers/net/wan/hdlc.c

static void hdlc_setup_dev(struct net_device *dev)

{
        /* Re-init all variables changed by HDLC protocol drivers,
         * including ether_setup() called from hdlc_raw_eth.c.
         */
        dev->flags               = IFF_POINTOPOINT | IFF_NOARP;
        dev->priv_flags          = IFF_WAN_HDLC;
        dev->mtu                 = HDLC_MAX_MTU;
        dev->type                = ARPHRD_RAWHDLC;
        dev->hard_header_len     = 16;
        dev->addr_len            = 0;
        dev->header_ops          = &hdlc_null_ops;
}

Hi Sam, Joe and Stephen

Thank you for your comments.
We have modified this driver for your comment.

---
Gigabit Ethernet driver of Topcliff PCH
Patch created against 2.6.35

Topcliff PCH is the platform controller hub that is going to be used in Intel's upcoming general embedded platform. All IO peripherals in Topcliff PCH are actually devices sitting on AMBA bus. Topcliff PCH has Gigabit Ethernet I/F. Using this I/F, it is able to access system devices connected to Gigabit Ethernet.

Signed-off-by Masayuki Ohtake <[email protected]>

---
 drivers/net/Kconfig                   |   10 +
 drivers/net/Makefile                  |    1 +
 drivers/net/pch_gbe/Makefile          |    4 +
 drivers/net/pch_gbe/pch_gbe.h         |  683 +++++++++
 drivers/net/pch_gbe/pch_gbe_api.c     |  246 ++++
 drivers/net/pch_gbe/pch_gbe_api.h     |   34 +
 drivers/net/pch_gbe/pch_gbe_ethtool.c |  618 ++++++++
 drivers/net/pch_gbe/pch_gbe_main.c    | 2591 +++++++++++++++++++++++++++++++++
 drivers/net/pch_gbe/pch_gbe_param.c   |  519 +++++++
 drivers/net/pch_gbe/pch_gbe_phy.c     |  276 ++++
 drivers/net/pch_gbe/pch_gbe_phy.h     |   35 +
 11 files changed, 5017 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/pch_gbe/Makefile
 create mode 100644 drivers/net/pch_gbe/pch_gbe.h
 create mode 100644 drivers/net/pch_gbe/pch_gbe_api.c
 create mode 100644 drivers/net/pch_gbe/pch_gbe_api.h
 create mode 100644 drivers/net/pch_gbe/pch_gbe_ethtool.c
 create mode 100644 drivers/net/pch_gbe/pch_gbe_main.c
 create mode 100644 drivers/net/pch_gbe/pch_gbe_param.c
 create mode 100644 drivers/net/pch_gbe/pch_gbe_phy.c
 create mode 100644 drivers/net/pch_gbe/pch_gbe_phy.h

drivers/net/Kconfig +0 -0

index 2decc59..f8102de 100644

drivers/net/Kconfig +2 -0

+	  access system devices connected to Gigabit Ethernet.
+	  This driver enables Gigabit Ethernet function.
 
 config ACENIC
 	tristate "Alteon AceNIC/3Com 3C985/NetGear GA620 Gigabit support"

We have modified this driver for your comment.

drivers/net/pch_gbe/pch_gbe_ethtool.c line 37

Quoted 8 lines · On Tue, 31 Aug 2010 11:38:35 -0700 Joe Perches <[email protected]> wrote:

struct pch_gbe_stats {

drivers/net/pch_gbe/pch_gbe_ethtool.c +3 -3

-	signed char stat_string[ETH_GSTRING_LEN];
-	int sizeof_stat;
-	int stat_offset;
+	signed char string[ETH_GSTRING_LEN];
+	size_t size;
+	size_t offset;
 };

Is this driver really using 'signed char' here?
Should just be 'char' for a simple string.

Gigabit Ethernet driver of Topcliff PCH
Patch created against 2.6.35

Topcliff PCH is the platform controller hub that is going to be used in Intel's upcoming general embedded platform. All IO peripherals in Topcliff PCH are actually devices sitting on AMBA bus. Topcliff PCH has Gigabit Ethernet I/F. Using this I/F, it is able to access system devices connected to Gigabit Ethernet.

Signed-off-by Masayuki Ohtake <[email protected]>

---
 drivers/net/Kconfig                   |   10 +
 drivers/net/Makefile                  |    1 +
 drivers/net/pch_gbe/Makefile          |    4 +
 drivers/net/pch_gbe/pch_gbe.h         |  683 +++++++++
 drivers/net/pch_gbe/pch_gbe_api.c     |  246 ++++
 drivers/net/pch_gbe/pch_gbe_api.h     |   34 +
 drivers/net/pch_gbe/pch_gbe_ethtool.c |  618 ++++++++
 drivers/net/pch_gbe/pch_gbe_main.c    | 2591 +++++++++++++++++++++++++++++++++
 drivers/net/pch_gbe/pch_gbe_param.c   |  519 +++++++
 drivers/net/pch_gbe/pch_gbe_phy.c     |  276 ++++
 drivers/net/pch_gbe/pch_gbe_phy.h     |   35 +
 11 files changed, 5017 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/pch_gbe/Makefile
 create mode 100644 drivers/net/pch_gbe/pch_gbe.h
 create mode 100644 drivers/net/pch_gbe/pch_gbe_api.c
 create mode 100644 drivers/net/pch_gbe/pch_gbe_api.h
 create mode 100644 drivers/net/pch_gbe/pch_gbe_ethtool.c
 create mode 100644 drivers/net/pch_gbe/pch_gbe_main.c
 create mode 100644 drivers/net/pch_gbe/pch_gbe_param.c
 create mode 100644 drivers/net/pch_gbe/pch_gbe_phy.c
 create mode 100644 drivers/net/pch_gbe/pch_gbe_phy.h

Quoted 4 lines

+ if (buffer_info->dma) {
+ pci_unmap_single(adapter->pdev, buffer_info->dma,
+ buffer_info->length, PCI_DMA_FROMDEVICE);
+ buffer_info->dma = 0;

Probably, this trick doesn't work because zero could be a valid DMA address on some architctures.

drivers/net/pch_gbe/pch_gbe_main.c line 1055

Quoted 5 lines

+ /*-- Set Buffer infomation --*/
+ buffer_info->length = skb->len;
+ buffer_info->dma =

drivers/net/pch_gbe/pch_gbe_main.c new file +2 -0

+     pci_map_single(adapter->pdev, skb->data, buffer_info->length,
+    PCI_DMA_TODEVICE);

Needs to handle mapping failure (dma_mapping_errro)?

Can you use dma_map_* API instead of pci_map_*? Please read:

Quoted 32 lines · On Tue, 31 Aug 2010 23:15:22 +0900 Masayuki Ohtake <[email protected]>…

Gigabit Ethernet driver of Topcliff PCH
Patch created against 2.6.35

Topcliff PCH is the platform controller hub that is going to be used in Intel's upcoming general embedded platform. All IO peripherals in Topcliff PCH are actually devices sitting on AMBA bus. Topcliff PCH has Gigabit Ethernet I/F. Using this I/F, it is able to access system devices connected to Gigabit Ethernet.

Signed-off-by Masayuki Ohtake <[email protected]>

---
 drivers/net/Kconfig                   |   10 +
 drivers/net/Makefile                  |    1 +
 drivers/net/pch_gbe/Makefile          |    4 +
 drivers/net/pch_gbe/pch_gbe.h         |  683 +++++++++
 drivers/net/pch_gbe/pch_gbe_api.c     |  246 ++++
 drivers/net/pch_gbe/pch_gbe_api.h     |   34 +
 drivers/net/pch_gbe/pch_gbe_ethtool.c |  618 ++++++++
 drivers/net/pch_gbe/pch_gbe_main.c    | 2591 +++++++++++++++++++++++++++++++++
 drivers/net/pch_gbe/pch_gbe_param.c   |  519 +++++++
 drivers/net/pch_gbe/pch_gbe_phy.c     |  276 ++++
 drivers/net/pch_gbe/pch_gbe_phy.h     |   35 +
 11 files changed, 5017 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/pch_gbe/Makefile
 create mode 100644 drivers/net/pch_gbe/pch_gbe.h
 create mode 100644 drivers/net/pch_gbe/pch_gbe_api.c
 create mode 100644 drivers/net/pch_gbe/pch_gbe_api.h
 create mode 100644 drivers/net/pch_gbe/pch_gbe_ethtool.c
 create mode 100644 drivers/net/pch_gbe/pch_gbe_main.c
 create mode 100644 drivers/net/pch_gbe/pch_gbe_param.c
 create mode 100644 drivers/net/pch_gbe/pch_gbe_phy.c
 create mode 100644 drivers/net/pch_gbe/pch_gbe_phy.h

(snip)

drivers/net/pch_gbe/pch_gbe_main.c line 803

Quoted 13 lines

drivers/net/pch_gbe/pch_gbe_main.c new file +13 -0

+/**
+ * pch_gbe_unmap_and_free_rx_resource - Unmap and free rx socket buffer
+ * @adapter:      Board private structure
+ * @buffer_info:  Buffer information structure
+ */
+static void pch_gbe_unmap_and_free_rx_resource(
+ struct pch_gbe_adapter *adapter,
+ struct pch_gbe_buffer *buffer_info)
+{
+ if (buffer_info->dma) {
+ pci_unmap_single(adapter->pdev, buffer_info->dma,
+ buffer_info->length, PCI_DMA_FROMDEVICE);
+ buffer_info->dma = 0;

Probably, this trick doesn't work because zero could be a valid DMA address on some architctures.

drivers/net/pch_gbe/pch_gbe_main.c line 1055

Quoted 5 lines

+ /*-- Set Buffer infomation --*/
+ buffer_info->length = skb->len;
+ buffer_info->dma =

drivers/net/pch_gbe/pch_gbe_main.c new file +2 -0

+     pci_map_single(adapter->pdev, skb->data, buffer_info->length,
+    PCI_DMA_TODEVICE);

Needs to handle mapping failure (dma_mapping_errro)?

I found the same problems in other places.

Can you use dma_map_* API instead of pci_map_*? Please read:

Documentation/DMA-API.txt
Documentation/DMA-API-HOWTO.txt

Thanks,

Quoted 3 lines

Probably, this trick doesn't work because zero could be a valid DMA address on some architctures.

The flag which shows mapped will be added. And it is used instead of "if (buffer_info->dma) ".

Quoted 7 lines

drivers/net/pch_gbe/pch_gbe_main.c line 1055

Quoted 5 lines

+ /*-- Set Buffer infomation --*/
+ buffer_info->length = skb->len;
+ buffer_info->dma =

drivers/net/pch_gbe/pch_gbe_main.c new file +2 -0

+     pci_map_single(adapter->pdev, skb->data, buffer_info->length,
+    PCI_DMA_TODEVICE);

Needs to handle mapping failure (dma_mapping_errro)?

Error process will be added.

Quoted 2 lines

Can you use dma_map_* API instead of pci_map_*? Please read:

This DMA of device is not standard. Personal use of Gigabit Ethernet device. So, I guess that I do not need to use DMA-API.

Quoted 2 lines

This DMA of device is not standard. Personal use of Gigabit Ethernet device. So, I guess that I do not need to use DMA-API.

Sorry, I'm not sure what you mean. If a driver does DMA, it must use the DMA API. Your driver already uses the DMA API (pci_dma_*), however, pci_map_* API is obsolete so please use dma_map_* API instead.

Quoted 6 lines · On Tue, 7 Sep 2010 10:13:22 +0900 "Masayuki Ohtake"…

Quoted 3 lines

Probably, this trick doesn't work because zero could be a valid DMA address on some architctures.

The flag which shows mapped will be added. And it is used instead of "if (buffer_info->dma) ".

Sounds fine. That's what other drivers do.

Quoted 9 lines

Quoted 7 lines

drivers/net/pch_gbe/pch_gbe_main.c line 1055

Quoted 5 lines

+ /*-- Set Buffer infomation --*/
+ buffer_info->length = skb->len;
+ buffer_info->dma =

drivers/net/pch_gbe/pch_gbe_main.c new file +2 -0

+     pci_map_single(adapter->pdev, skb->data, buffer_info->length,
+    PCI_DMA_TODEVICE);

Needs to handle mapping failure (dma_mapping_errro)?

Error process will be added.

Nice.

Quoted 5 lines

Quoted 2 lines

Can you use dma_map_* API instead of pci_map_*? Please read:

This DMA of device is not standard. Personal use of Gigabit Ethernet device. So, I guess that I do not need to use DMA-API.

Sorry, I'm not sure what you mean. If a driver does DMA, it must use the DMA API. Your driver already uses the DMA API (pci_dma_*), however, pci_map_* API is obsolete so please use dma_map_* API instead.

Thanks,
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html