lkml.org 
[lkml]   [2010]   [Sep]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v3] Gigabit Ethernet driver of Topcliff PCH
Ccing Rafael (please find pch_gbe_resume here)

On 09/10/2010 07:29 AM, Masayuki Ohtake wrote:
> Hi Fujita, Jili and Stephen

Hi, Jili sounds funny :D.

> --- /dev/null
> +++ b/drivers/net/pch_gbe/pch_gbe_ethtool.c
> @@ -0,0 +1,587 @@
...
> +static void pch_gbe_get_regs(struct net_device *netdev,
> + struct ethtool_regs *regs, void *p)
> +{
> + struct pch_gbe_adapter *adapter = netdev_priv(netdev);
> + struct pch_gbe_hw *hw = &adapter->hw;
> + struct pci_dev *pdev = adapter->pdev;
> + u32 *regs_buff = p;
> + u16 i, reg, tmp;
> +
> + regs->version = pdev->revision;
> + regs->version = 0x1000000 | (regs->version << 16) | pdev->device;
> + memset(p, 0, PCH_GBE_REGS_LEN * (int)sizeof(u32));
> + for (i = 0; i < PCH_GBE_MAC_REGS_LEN; i++)
> + regs_buff[i] = ioread32(&hw->reg->INT_ST + i);

Well, you switched to ioremap, but left ioread/iowrite*. I wrote you
either use pci_iomap+ioread/iowrite32 or pci_ioremap_bar+readl/writel
which is faster. (But can be used iff you know it is an MMIO space).

> +static void pch_gbe_get_ethtool_stats(struct net_device *netdev,
> + struct ethtool_stats *stats, u64 *data)
> +{
> + 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;
> +
> + pch_gbe_update_stats(adapter);
> + for (i = 0; i < PCH_GBE_GLOBAL_STATS_LEN; i++) {
> + char *p = hw_stats + gstats->offset;
> + data[i] = gstats->size == sizeof(u64) ? *(u64 *)p:(*(u32 *)p);

Actually, how you protect 64-bit stat numbers on 32-bit to be atomic?
You change adapter->stats from the irq context and it may need to update
2 words on 32-bit. So instead of having 0x100000000 you can get here
0x1ffffffff or 0x0 since read/write of u64 on 32bit are not atomic and
both words may not reach the memory yet. I don't know if that matters --
what are the stats from ethtool used for, but returning invalid values
is not nice. Do you need 64bit stats on 32-bit at all? Wouldn't be
netdev_stats (they are ulong) enough?

Example:
Given stat = 0xffffffff; on 32-bit stat is composed of stat_hi|stat_lo,
compiler generates for instance:
stat_lo++;
if (overflow) (1)
stat_hi++

Then if you read stat from this ethtool function at point (1), you will
get 0x0.

> + gstats++;
> + }
> +}
...
> --- /dev/null
> +++ b/drivers/net/pch_gbe/pch_gbe_main.c
> @@ -0,0 +1,2526 @@
...
> +void pch_gbe_mac_mar_set(struct pch_gbe_hw *hw, u8 * addr, u32 index)
> +{
> + u32 mar_low, mar_high, adrmask;
> +
> + pr_debug("index : 0x%x\n", index);
> +
> + /*
> + * HW expects these in little endian so we reverse the byte order
> + * from network order (big endian) to little endian
> + */
> + mar_high = ((u32) addr[0] | ((u32) addr[1] << 8) |
> + ((u32) addr[2] << 16) | ((u32) addr[3] << 24));
> + mar_low = ((u32) addr[4] | ((u32) addr[5] << 8));
> + /* Stop the MAC Address of index. */
> + adrmask = ioread32(&hw->reg->ADDR_MASK);
> + iowrite32((adrmask | (0x0001 << index)), &hw->reg->ADDR_MASK);
> + /* wait busy */
> + while ((ioread32(&hw->reg->ADDR_MASK) & PCH_GBE_BUSY)) {
> + int tmp = 0;
> + udelay(20);
> + tmp++;
> + if (tmp == 1000) {
> + pr_err("Address mask bit is not cleared\n");
> + break;
> + }
> + }
Maybe it would make sense to create a helper for that.

> +static void pch_gbe_tx_queue(struct pch_gbe_adapter *adapter,
> + struct pch_gbe_tx_ring *tx_ring,
> + struct sk_buff *skb)
> +{
...
> + buffer_info->kernel_skb = skb;

Just curious, why you need ->kernel_skb and ->skb in that structure?
Can't you use just ->skb?

> +int pch_gbe_setup_tx_resources(struct pch_gbe_adapter *adapter,
> + struct pch_gbe_tx_ring *tx_ring)
> +{
> + struct pci_dev *pdev = adapter->pdev;
> + struct pch_gbe_tx_desc *tx_desc;
> + int size;
> + int desNo;
> +
> + size = (int)sizeof(struct pch_gbe_buffer) * tx_ring->count;
> + tx_ring->buffer_info = vmalloc(size);
> + if (!tx_ring->buffer_info) {
> + pr_err("Unable to allocate memory for the buffer infomation\n");
> + return -ENOMEM;
> + }
> + memset(tx_ring->buffer_info, 0, size);
> +
> + tx_ring->size = tx_ring->count * (int)sizeof(struct pch_gbe_tx_desc);
> +
> + tx_ring->desc =
> + pci_alloc_consistent(pdev, tx_ring->size, &tx_ring->dma);

As you can sleep here now, you may use dma_alloc_coherent(..., GFP_KERNEL)
which allows for better memory reclaim and will fail in less cases.
For two reasons:
1) pci_alloc_consistent is deprecated AFAIK
2) pci_alloc_consistent calls page allocator with GFP_ATOMIC and may
fail for huge allocations early. And as I computed last time, you
allocate up to order-4 pages here.

The same for RX dir.

> +static int pch_gbe_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
> +{
> + struct pch_gbe_adapter *adapter = netdev_priv(netdev);
> + struct pch_gbe_tx_ring *tx_ring = adapter->tx_ring;
> + unsigned long flags;
> +
> + if (unlikely(skb->len <= 0)) {

Can this happen at all? ndev people should tell you better...

> + dev_kfree_skb_any(skb);
> + pr_debug("Return : OK skb len : %d\n", skb->len);
> + return NETDEV_TX_OK;
> + }
> + if (unlikely(skb->len > (adapter->hw.mac.max_frame_size - 4))) {
> + pr_err("Transfer length Error: skb len: %d > max: %d\n",
> + skb->len, adapter->hw.mac.max_frame_size);
> + adapter->stats.tx_length_errors++;
> + return NETDEV_TX_BUSY;

Ok, then it's not busy (it will try to send the skb in a loop forever).
You should free it and return OK, I think. Anyway, properly set MTU
should gurantee that, but it is my guess.

> +static struct net_device_stats *pch_gbe_get_stats(
> + struct net_device *netdev)
> +{
> + struct pch_gbe_adapter *adapter = netdev_priv(netdev);
> +
> + /* only return the current stats */
> + return &adapter->net_stats;

Ah, you have your own copy of netstats. You may use netdev->stats all
over the code.

> +#ifdef CONFIG_PM
> +static int pch_gbe_resume(struct pci_dev *pdev)
> +{
> + struct net_device *netdev = pci_get_drvdata(pdev);
> + struct pch_gbe_adapter *adapter = netdev_priv(netdev);
> + struct pch_gbe_hw *hw = &adapter->hw;
> + u32 err;
> +
> + pci_enable_wake(pdev, PCI_D0, 0);
> + pci_set_power_state(pdev, PCI_D0);
> + pci_restore_state(pdev);
> + err = pci_enable_device(pdev);
> + if (err) {
> + pr_err("Cannot enable PCI device from suspend\n");
> + return err;
> + }
> + pci_set_master(pdev);

Not sure which of those you need. Rafael may tell you more. You should
not set power state and restore state, but I might be wrong.

The same you need not to save state from ->suspend.

> + pch_gbe_hal_power_up_phy(hw);
> + pch_gbe_reset(adapter);
> + /* Clear wake on lan control and status */
> + pch_gbe_mac_set_wol_event(hw, 0);
> +
> + if (netif_running(netdev))
> + pch_gbe_up(adapter);
> + netif_device_attach(netdev);
> +
> + return 0;
> +}
> +#endif /* CONFIG_PM */
...
> --- /dev/null
> +++ b/drivers/net/pch_gbe/pch_gbe_param.c
> @@ -0,0 +1,501 @@
...
> +const struct pch_gbe_opt_list speed_list[] = {

static? And all below...

> + { 0, "" },
> + { SPEED_10, "" },
> + { SPEED_100, "" },
> + { SPEED_1000, "" }
> +};
...
> +static void pch_gbe_check_copper_options(struct pch_gbe_adapter *adapter)
> +{
> + struct pch_gbe_hw *hw = &adapter->hw;
> + int speed, dplx;
> +
> + { /* Speed */
> + struct pch_gbe_option opt = {

So may be this static const? And all below...

> + .type = list_option,
> + .name = "Speed",
> + .err = "parameter ignored",
> + .def = 0,
> + .arg = { .l = { .nr = (int)ARRAY_SIZE(speed_list),
> + .p = speed_list } }
> + };
> + speed = Speed;
> + pch_gbe_validate_option(&speed, &opt, adapter);
> + }

regards,
--
js
suse labs


\
 
 \ /
  Last update: 2010-09-10 10:21    [W:0.054 / U:0.152 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site