login
Search
--- 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 <masa-korg@dsn.okisemi.com> --- drivers/net/Kconfig | 5 + drivers/net/Makefile | 1 + drivers/net/pch_gbe/Makefile | 6 + drivers/net/pch_gbe/pch_gbe.h | 425 ++++++ drivers/net/pch_gbe/pch_gbe_api.c | 247 +++ drivers/net/pch_gbe/pch_gbe_api.h | 31 + drivers/net/pch_gbe/pch_gbe_ethtool.c | 623 ++++++++ drivers/net/pch_gbe/pch_gbe_main.c | 2695 +++++++++++++++++++++++++++++++++ drivers/net/pch_gbe/pch_gbe_param.c | 516 +++++++ drivers/net/pch_gbe/pch_gbe_phy.c | 277 ++++ drivers/net/pch_gbe/pch_gbe_phy.h | 32 + drivers/net/pch_gbe/pch_gbe_regs.h | 305 ++++ 12 files changed, 5163 insertions(+), 0 deletions(-) create mode 100644 drivers/net/pch_gbe/Makefile create mode 100644 drivers/net/pch_gbe/Module.symvers 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 create mode 100644 drivers/net/pch_gbe/pch_gbe_regs.h diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig index 2decc59..fcc6b7b 100644 --- a/drivers/net/Kconfig +++ b/drivers/net/Kconfig @@ -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 ...
Hi Masayuki Ohtake. Just a few comments after a very quick scan over some of the files. Sam "---" is used to truncate your changelog, random comments goes below "---". 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---". Use: ccflags-$(CONFIG_PCH_GBE_DEBUG_CORE) := -DDEBUG Btw. where do CONFIG_PCH_GBE_DEBUG_CORE come from. It was not in the Kconfig 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 Use standard logging methods. In several places... Why is two .h files needed for a single driver? As per comment above I realise that this single driver has three .h files. Maybe their size warrants this? --
Hi Sam Thank you for your comment. The reply's comment was buried in following. I will modify this patch. Thanks, Ohtake(OKISEMI) ----- Original Message ----- From: "Sam Ravnborg" <sam@ravnborg.org> To: "Masayuki Ohtake" <masa-korg@dsn.okisemi.com> Cc: "LKML" <linux-kernel@vger.kernel.org>; "ML netdev" <netdev@vger.kernel.org>; "Greg Rose" <gregory.v.rose@intel.com>; "Maxime Bizon" <mbizon@freebox.fr>; "Kristoffer Glembo" <kristoffer@gaisler.com>; "Ralf Baechle" <ralf@linux-mips.org>; "John Linn" <john.linn@xilinx.com>; "Randy Dunlap" <randy.dunlap@oracle.com>; "David S. Miller" <davem@davemloft.net>; "MeeGo" <meego-dev@meego.com>; "Wang, Qi" <qi.wang@intel.com>; "Wang, Yong Y" <yong.y.wang@intel.com>; "Andrew" <andrew.chih.howe.khor@intel.com>; "Intel OTC" <joel.clark@intel.com>; "Foster, Margie" <margie.foster@intel.com>; "Toshiharu Okada" <okada533@dsn.okisemi.com>; "Tomoya Morinaga" <morinaga526@dsn.okisemi.com>; "Takahiro Shimizu" <shimizu394@dsn.okisemi.com> Sent: Thursday, August 26, 2010 7:28 PM <masa> <masa> Topcliff PCH is the platform controller hub that is going to be used in Intel's upcoming general embedded platform. <masa> This deletes. <masa> <masa> <masa> <masa> <masa> <masa> <masa> <masa> This driver is designed be easy to add other PHY. <masa> "pch_gbe_regs.h" shows the register of MAC. "pch_gbe_regs.h" is merged into "pch_gbe.h". --
Better to use
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
and
pr_debug(fmt, ...);
Trivial: Inconsistent spacing (a few times)
Perhaps:
[]
Convert all the
printk(KERN_<level> KBUILD_MODNAME ": " etc
to
pr_err("ERROR: Registers not mapped\n");
These are more commonly written as:
if (!fn_ptr) {
[ report_error_condition ]
return -ERRNO;
}
rtn = fn_ptr(foo);
if (rtn is err) {
[ report_error_condition ]
return -ERRNO2;
}
return 0;
For more complicated styles where some unwinding
is necessary
if (!fn_ptr) {
[ report_error_condition ]
err = -ERRNO;
goto out;
}
windup
rtn = fn_ptr(foo);
if (rtn is err) {
[ report_error_condition ]
err = ERRNO2
goto out2;
}
return 0;
outn:
..
out2:
unwind;
out:
pr_err("ERROR: configuration\n");
[]
Add #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
If you really want to track entering functions,
it might be better to use something like:
#define FUNC_ENTER() pr_devel("ethtool: %s enter\n", __func__)
#define FUNC_EXIT() pr_devel("ethtool: %s exit\n", __func__)
Unlike pr_debug, pr_devel does not remain active in
There's a vsprintf pointer extension "%pM" used for mac addresses
pr_debug("hw->mac.addr: %pM\n, hw->mac.addr);
etc.
--
On Thu, 26 Aug 2010 18:56:55 +0900
No need for cast to u32 on the masking:
ecmd->supported &= ~(SUPPORTED_TP | SUPPORTED_1000baseT_Half);
ecmd->advertising &= ~(ADVERTISED_TP | ADVERTISED_1000baseT_Half);
Awkward use of if, and don't set other bits in duplex
if (!netif_carrier_ok(adapter->netdev))
ecmd->speed = -1;
--
--
On Thu, 26 Aug 2010 18:56:55 +0900 Don't invent your own mutex mechanism. Either use spin lock or mutex. -- --
On Thu, 26 Aug 2010 18:56:55 +0900 Should be const -- --
On Thu, 26 Aug 2010 18:56:55 +0900 can use new %pM printk format here. --
Use cpu_relax here to allow other hyper-thread to run. /* wait busy */ while ((ioread32(&hw->reg->ADDR_MASK) & PCH_GBE_BUSY)) cpu_relax(); --
On Thu, 26 Aug 2010 18:56:55 +0900 This is not allowed, you can not just allocate up a network device structure out of kmalloc space. Not sure what you are doing with the polling_netdev? Is it left over from when net_device and NAPI were more closely bound? -- --
On Thu, 26 Aug 2010 18:56:55 +0900
Why not something simpler using skb_checksum_help which is
safer since it handles case of shared skb correctly.
if (skb->ip_summed = CHECKSUM_COMPLETE &&
skb->len < PCH_GBE_SHORT_PKT) {
frame_ctrl |=
PCH_GBE_TXD_CTRL_APAD | PCH_GBE_TXD_CTRL_TCPIP_ACC_OFF;
skb->ip_summed = CHECKSUM_NONE;
if (skb_checksum_help(skb))
goto drop; /* could not expand header to find space */
}
--
--
+static int pch_gbe_probe(struct pci_dev *pdev,
+ const struct pci_device_id *pci_id)
+{
+ struct net_device *netdev;
+ struct pch_gbe_adapter *adapter;
+ unsigned long mmio_start;
+ unsigned long mmio_len;
+ int i, ret;
+
+ PCH_GBE_DEBUG("pch_gbe_probe\n");
+ ret = pci_enable_device(pdev);
+ if (ret)
+ return ret;
+
+ if (!dma_set_mask(&pdev->dev, DMA_BIT_MASK(64))
+ && !dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64))) {
+ ;
+ } else {
+ ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
+ if (ret) {
+ ret = dma_set_coherent_mask(&pdev->dev,
+ DMA_BIT_MASK(32));
+ if (ret) {
+ printk(KERN_ERR KBUILD_MODNAME ": ERR: "
+ "No usable DMA configuration, aborting\n");
+ goto err_disable_device;
+ }
+ }
+ }
+ ret = pci_request_regions(pdev, KBUILD_MODNAME);
+ if (ret) {
+ printk(KERN_ERR KBUILD_MODNAME ": "
+ "ERR: Can't reserve PCI I/O and memory resources\n");
+ goto err_disable_device;
+ }
+ pci_set_master(pdev);
+
+ netdev = alloc_etherdev((int)sizeof(struct pch_gbe_adapter));
+ if (!netdev) {
+ ret = -ENOMEM;
+ printk(KERN_ERR KBUILD_MODNAME ": "
+ "ERR: Can't allocates and sets up an Ethernet device\n");
+ goto err_release_pci;
+ }
+ SET_NETDEV_DEV(netdev, &pdev->dev);
+
+ pci_set_drvdata(pdev, netdev);
+ adapter = netdev_priv(netdev);
+ adapter->netdev = netdev;
+ adapter->pdev = pdev;
+ adapter->hw.back = adapter;
+ mmio_start = pci_resource_start(pdev, PCH_GBE_PCI_BAR);
+ mmio_len = pci_resource_len(pdev, PCH_GBE_PCI_BAR);
+ adapter->hw.reg = ioremap_nocache(mmio_start, mmio_len);
+ if (!adapter->hw.reg) {
+ ret = -EIO;
+ printk(KERN_ERR KBUILD_MODNAME ": " "Can't ioremap\n");
+ goto err_free_netdev;
+ }
+
+ netdev->netdev_ops = &pch_gbe_netdev_ops;
+ netdev->watchdog_timeo = PCH_GBE_WATCHDOG_PERIOD;
+ netif_napi_add(netdev, &adapter->napi,
+ pch_gbe_napi_poll, PCH_GBE_RX_WEIGHT);
+ strncpy(netdev->name, pci_name(pdev), (int)sizeof(netdev->name) - 1);
+ netdev->mem_start = ...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...) --
On Thu, 26 Aug 2010 09:16:12 -0700 At this point in process, netdevice isn't registered so it doesn't have a ethX name. so dev_info would be best. -- --
Yes, likely true. Ideally netdev_<level> is used after register_netdev. The default "eth%d" is printed after alloc_netdev but before register_netdev. Before commmit 256df2f3879efdb2e9808bdb1b54b16fbb11fa38 when netdev_<level>s were converted to functions, they would also dereference a NULL pointer if used before register_netdev. --
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 <masa-korg@dsn.okisemi.com> --- 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 diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig index 2decc59..f8102de 100644 --- a/drivers/net/Kconfig +++ b/drivers/net/Kconfig @@ -2004,6 +2004,16 @@ 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 ...
This is a gigabit You aren't going to modify this text after the product is released, are you? Topcliff PCH is the platform controller hub that is used in Intel's -- ~Randy *** Remember to use Documentation/SubmitChecklist when testing your code *** --
I find hard to believe this driver needs to copy all outgoing frames on pre-allocated skbs. + /* [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 ? --
Hi Eric 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. Thanks, Ohtake(OKISemi) ----- Original Message ----- From: "Eric Dumazet" <eric.dumazet@gmail.com> To: "Masayuki Ohtake" <masa-korg@dsn.okisemi.com> Cc: "Stephen Hemminger" <shemminger@vyatta.com>; "Sam Ravnborg" <sam@ravnborg.org>; "Joe Perches" <joe@perches.com>; "LKML" <linux-kernel@vger.kernel.org>; "ML netdev" <netdev@vger.kernel.org>; "Greg Rose" <gregory.v.rose@intel.com>; "Maxime Bizon" <mbizon@freebox.fr>; "Kristoffer Glembo" <kristoffer@gaisler.com>; "Ralf Baechle" <ralf@linux-mips.org>; "John Linn" <john.linn@xilinx.com>; "Randy Dunlap" <randy.dunlap@oracle.com>; "David S. Miller" <davem@davemloft.net>; "MeeGo" <meego-dev@meego.com>; "Wang, Qi" <qi.wang@intel.com>; "Wang, Yong Y" <yong.y.wang@intel.com>; "Andrew" <andrew.chih.howe.khor@intel.com>; "Intel OTC" <joel.clark@intel.com>; "Foster, Margie" <margie.foster@intel.com>; "Toshiharu Okada" <okada533@dsn.okisemi.com>; "Tomoya Morinaga" <morinaga526@dsn.okisemi.com>; "Takahiro Shimizu" <shimizu394@dsn.okisemi.com> Sent: Tuesday, August 31, 2010 11:51 PM --
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 :) --
On Thu, 02 Sep 2010 15:40:47 +0200
With Eric's suggestion, you might find most packets are going
to be aligned but the code should not depend on it always being true.
Packets that get forwarded have header determined by the receiving
interface.
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;
}
--
--
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 Eric Thank you for your information. I will confirm other drivers. Thanks, Ohtake(OKISemi) ----- Original Message ----- From: "Eric Dumazet" <eric.dumazet@gmail.com> To: "Masayuki Ohtake" <masa-korg@dsn.okisemi.com> Cc: "Stephen Hemminger" <shemminger@vyatta.com>; "Sam Ravnborg" <sam@ravnborg.org>; "Joe Perches" <joe@perches.com>; "LKML" <linux-kernel@vger.kernel.org>; "ML netdev" <netdev@vger.kernel.org>; "Greg Rose" <gregory.v.rose@intel.com>; "Maxime Bizon" <mbizon@freebox.fr>; "Kristoffer Glembo" <kristoffer@gaisler.com>; "Ralf Baechle" <ralf@linux-mips.org>; "John Linn" <john.linn@xilinx.com>; "Randy Dunlap" <randy.dunlap@oracle.com>; "David S. Miller" <davem@davemloft.net>; "MeeGo" <meego-dev@meego.com>; "Wang, Qi" <qi.wang@intel.com>; "Wang, Yong Y" <yong.y.wang@intel.com>; "Andrew" <andrew.chih.howe.khor@intel.com>; "Intel OTC" <joel.clark@intel.com>; "Foster, Margie" <margie.foster@intel.com>; "Toshiharu Okada" <okada533@dsn.okisemi.com>; "Tomoya Morinaga" <morinaga526@dsn.okisemi.com>; "Takahiro Shimizu" <shimizu394@dsn.okisemi.com> Sent: Friday, September 03, 2010 10:43 PM --
Some trivial logging message cleanups:
Missing newline
Alignment
Grammar
80 column coalescing
Signed-off-by: Joe Perches <joe@perches.com>
---
drivers/net/pch_gbe/pch_gbe_main.c | 180 ++++++++++++++++-------------------
drivers/net/pch_gbe/pch_gbe_param.c | 36 +++-----
drivers/net/pch_gbe/pch_gbe_phy.c | 2 +-
3 files changed, 94 insertions(+), 124 deletions(-)
diff --git a/drivers/net/pch_gbe/pch_gbe_main.c b/drivers/net/pch_gbe/pch_gbe_main.c
index ce0c38e..264f275 100644
--- a/drivers/net/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/pch_gbe/pch_gbe_main.c
@@ -284,7 +284,7 @@ s32 pch_gbe_mac_force_mac_fc(struct pch_gbe_hw *hw)
rx_fctrl &= ~PCH_GBE_FL_CTRL_EN;
iowrite32(rx_fctrl, &hw->reg->RX_FCTRL);
pr_debug("RX_FCTRL reg : 0x%08x mac->tx_fc_enable : %d\n",
- ioread32(&hw->reg->RX_FCTRL), mac->tx_fc_enable);
+ ioread32(&hw->reg->RX_FCTRL), mac->tx_fc_enable);
return 0;
}
@@ -299,7 +299,7 @@ void pch_gbe_mac_set_wol_event(struct pch_gbe_hw *hw, u32 wu_evt)
FUNC_ENTER();
pr_debug("wu_evt : 0x%08x ADDR_MASK reg : 0x%08x\n",
- wu_evt, ioread32(&hw->reg->ADDR_MASK));
+ wu_evt, ioread32(&hw->reg->ADDR_MASK));
if (wu_evt) {
/* Set Wake-On-Lan address mask */
@@ -360,8 +360,8 @@ u16 pch_gbe_mac_ctrl_miim(struct pch_gbe_hw *hw, u32 addr, u32 dir, u32 reg,
spin_unlock_irqrestore(&hw->miim_lock, flags);
pr_debug("PHY %s: reg=%d, data=0x%04X\n",
- dir == PCH_GBE_MIIM_OPER_READ ? "READ" : "WRITE", reg,
- dir == PCH_GBE_MIIM_OPER_READ ? data_out : data);
+ dir == PCH_GBE_MIIM_OPER_READ ? "READ" : "WRITE", reg,
+ dir == PCH_GBE_MIIM_OPER_READ ? data_out : data);
return (u16) data_out;
}
@@ -394,9 +394,9 @@ void pch_gbe_mac_set_pause_packet(struct pch_gbe_hw *hw)
iowrite32(PCH_GBE_PS_PKT_RQ, &hw->reg->PAUSE_REQ);
pr_debug("PAUSE_PKT1-5 reg : 0x%08x 0x%08x 0x%08x 0x%08x 0x%08x\n",
- ioread32(&hw->reg->PAUSE_PKT1), ioread32(&hw->reg->PAUSE_PKT2),
- ioread32(&hw->reg->PAUSE_PKT3), ...Perhaps this is clearer.
Signed-off-by: Joe Perches <joe@perches.com>
---
drivers/net/pch_gbe/pch_gbe.h | 6 +++---
drivers/net/pch_gbe/pch_gbe_main.c | 33 +++++++++++++++++----------------
2 files changed, 20 insertions(+), 19 deletions(-)
diff --git a/drivers/net/pch_gbe/pch_gbe.h b/drivers/net/pch_gbe/pch_gbe.h
index a615ddc..a2b2c55 100644
--- a/drivers/net/pch_gbe/pch_gbe.h
+++ b/drivers/net/pch_gbe/pch_gbe.h
@@ -638,9 +638,9 @@ struct pch_gbe_adapter {
struct pch_gbe_rx_ring *rx_ring;
unsigned long rx_buffer_len;
unsigned long tx_queue_len;
- unsigned char rx_csum;
- unsigned char tx_csum;
- unsigned char have_msi;
+ bool rx_csum;
+ bool tx_csum;
+ bool have_msi;
};
/**
diff --git a/drivers/net/pch_gbe/pch_gbe_main.c b/drivers/net/pch_gbe/pch_gbe_main.c
index 264f275..8195702 100644
--- a/drivers/net/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/pch_gbe/pch_gbe_main.c
@@ -711,7 +711,7 @@ static void pch_gbe_setup_rctl(struct pch_gbe_adapter *adapter)
tcpip = ioread32(&hw->reg->TCPIP_ACC);
- if (adapter->rx_csum == true) {
+ if (adapter->rx_csum) {
tcpip &= ~PCH_GBE_RX_TCPIPACC_OFF;
tcpip |= PCH_GBE_RX_TCPIPACC_EN;
} else {
@@ -1005,7 +1005,7 @@ static void pch_gbe_tx_queue(struct pch_gbe_adapter *adapter,
frame_ctrl = 0;
if (unlikely(skb->len < PCH_GBE_SHORT_PKT))
frame_ctrl |= PCH_GBE_TXD_CTRL_APAD;
- if (unlikely(adapter->tx_csum == false))
+ if (unlikely(!adapter->tx_csum))
frame_ctrl |= PCH_GBE_TXD_CTRL_TCPIP_ACC_OFF;
/* Performs checksum processing */
@@ -1013,7 +1013,7 @@ static void pch_gbe_tx_queue(struct pch_gbe_adapter *adapter,
* 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)) {
+ if ((skb->len < PCH_GBE_SHORT_PKT) && (adapter->tx_csum)) {
frame_ctrl |= PCH_GBE_TXD_CTRL_APAD |
PCH_GBE_TXD_CTRL_TCPIP_ACC_OFF;
if (skb->protocol ...Use c90 initializers
Use memset
Use FIELD_SIZEOF
Remove stat_ from struct pch_gbe_stats members
Save offset from start of struct pch_gbe_hw_stats
All stat members are u64, so maybe the size field can be removed
Signed-off-by: Joe Perches <joe@perches.com>
---
drivers/net/pch_gbe/pch_gbe_ethtool.c | 32 +++++++++++++++++---------------
drivers/net/pch_gbe/pch_gbe_main.c | 28 +---------------------------
2 files changed, 18 insertions(+), 42 deletions(-)
diff --git a/drivers/net/pch_gbe/pch_gbe_ethtool.c b/drivers/net/pch_gbe/pch_gbe_ethtool.c
index 25b9357..f61b314 100644
--- a/drivers/net/pch_gbe/pch_gbe_ethtool.c
+++ b/drivers/net/pch_gbe/pch_gbe_ethtool.c
@@ -31,13 +31,18 @@
* pch_gbe_stats - Stats item infomation
*/
struct pch_gbe_stats {
- 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;
};
-#define PCH_GBE_STAT(m) {#m,\
- sizeof(((struct pch_gbe_adapter *)0)->stats.m),\
- offsetof(struct pch_gbe_adapter, stats.m)}
+
+#define PCH_GBE_STAT(m) \
+{ \
+ .string = #m, \
+ .size = FIELD_SIZEOF(struct pch_gbe_hw_stats, m), \
+ .offset = offsetof(struct pch_gbe_hw_stats, m), \
+}
+
/**
* pch_gbe_gstrings_stats - ethtool information status name list
*/
@@ -545,7 +550,7 @@ static void pch_gbe_get_strings(struct net_device *netdev, u32 stringset,
switch (stringset) {
case (u32) ETH_SS_STATS:
for (i = 0; i < PCH_GBE_GLOBAL_STATS_LEN; i++) {
- memcpy(p, pch_gbe_gstrings_stats[i].stat_string,
+ memcpy(p, pch_gbe_gstrings_stats[i].string,
ETH_GSTRING_LEN);
p += ETH_GSTRING_LEN;
}
@@ -564,16 +569,15 @@ static void pch_gbe_get_ethtool_stats(struct net_device *netdev,
{
struct pch_gbe_adapter *adapter = netdev_priv(netdev);
int i;
+ const struct pch_gbe_stats *gstats = pch_gbe_gstrings_stats;
+ char *hw_stats = (char *)&adapter->stats;
FUNC_ENTER();
...On Tue, 31 Aug 2010 11:38:35 -0700 Is this driver really using 'signed char' here? Should just be 'char' for a simple string. --
You're right. Probably all the below should just be char.
$ grep -Pn "\bsigned\s+char\b" drivers/net/pch_gbe/*.[ch]
pch_gbe_ethtool.c:34: signed char string[ETH_GSTRING_LEN];
pch_gbe_param.c:155: signed char *name;
pch_gbe_param.c:156: signed char *err;
pch_gbe_param.c:165: struct pch_gbe_opt_list { int i; signed char *str; } *p;
--
On Tue, 31 Aug 2010 23:15:22 +0900 Probably, this trick doesn't work because zero could be a valid DMA 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, --
Hi Fujita The flag which shows mapped will be added. 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. Thanks, Ohtake ----- Original Message ----- From: "FUJITA Tomonori" <fujita.tomonori@lab.ntt.co.jp> To: <masa-korg@dsn.okisemi.com> Cc: <shemminger@vyatta.com>; <sam@ravnborg.org>; <joe@perches.com>; <linux-kernel@vger.kernel.org>; <netdev@vger.kernel.org>; <gregory.v.rose@intel.com>; <mbizon@freebox.fr>; <kristoffer@gaisler.com>; <ralf@linux-mips.org>; <john.linn@xilinx.com>; <randy.dunlap@oracle.com>; <davem@davemloft.net>; <meego-dev@meego.com>; <qi.wang@intel.com>; <yong.y.wang@intel.com>; <andrew.chih.howe.khor@intel.com>; <joel.clark@intel.com>; <margie.foster@intel.com>; <okada533@dsn.okisemi.com>; <morinaga526@dsn.okisemi.com>; <shimizu394@dsn.okisemi.com> Sent: Friday, September 03, 2010 11:23 AM --
On Tue, 7 Sep 2010 10:13:22 +0900 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, --
Thank you for the explanation. I understood and use dma_map_* . Thanks Ohtake ----- Original Message ----- From: "FUJITA Tomonori" <fujita.tomonori@lab.ntt.co.jp> To: <masa-korg@dsn.okisemi.com> Cc: <fujita.tomonori@lab.ntt.co.jp>; <shemminger@vyatta.com>; <sam@ravnborg.org>; <joe@perches.com>; <linux-kernel@vger.kernel.org>; <netdev@vger.kernel.org>; <gregory.v.rose@intel.com>; <mbizon@freebox.fr>; <kristoffer@gaisler.com>; <ralf@linux-mips.org>; <john.linn@xilinx.com>; <randy.dunlap@oracle.com>; <davem@davemloft.net>; <meego-dev@meego.com>; <qi.wang@intel.com>; <yong.y.wang@intel.com>; <andrew.chih.howe.khor@intel.com>; <joel.clark@intel.com>; <margie.foster@intel.com>; <okada533@dsn.okisemi.com>; <morinaga526@dsn.okisemi.com>; <shimizu394@dsn.okisemi.com> Sent: Tuesday, September 07, 2010 12:21 PM --
