Message-ID: <001801cb54d3$3c0711c0$66f8800a@maildom.okisemi.com>
Date:	Wed, 15 Sep 2010 21:20:12 +0900
From:	"Masayuki Ohtake" <masa-korg@....okisemi.com>
To:	"Jiri Slaby" <jslaby@...e.cz>
Cc:	"David S. Miller" <davem@...emloft.net>,
	"Randy Dunlap" <randy.dunlap@...cle.com>,
	"John Linn" <john.linn@...inx.com>,
	"Ralf Baechle" <ralf@...ux-mips.org>,
	"Kristoffer Glembo" <kristoffer@...sler.com>,
	"Maxime Bizon" <mbizon@...ebox.fr>,
	"Greg Rose" <gregory.v.rose@...el.com>,
	"ML netdev" <netdev@...r.kernel.org>,
	"LKML" <linux-kernel@...r.kernel.org>,
	"MeeGo" <meego-dev@...go.com>,
	"Stephen Hemminger" <shemminger@...tta.com>,
	"FUJITA Tomonori" <fujita.tomonori@....ntt.co.jp>,
	"Wang, Qi" <qi.wang@...el.com>,
	"Wang, Yong Y" <yong.y.wang@...el.com>,
	"Andrew" <andrew.chih.howe.khor@...el.com>,
	"Intel OTC" <joel.clark@...el.com>,
	"Foster, Margie" <margie.foster@...el.com>,
	"Arjan" <arjan@...ux.intel.com>,
	"Toshiharu Okada" <okada533@....okisemi.com>,
	"Takahiro Shimizu" <shimizu394@....okisemi.com>,
	"Tomoya Morinaga" <morinaga526@....okisemi.com>,
	"Rafael J. Wysocki" <rjw@...k.pl>
Subject: Re: [PATCH v3] Gigabit Ethernet driver of Topcliff PCH

Hi Jiri

Thank you for your comments.

My reply comments are included in the following.
I will resubmit after modified.

Thanks Ohtake
-----
Date: Fri, 10 Sep 2010 10:19:30 +0200
From: "Jiri Slaby" <jslaby@...e.cz>
> 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.

[masa]
Sorry, :-}

>
> > --- /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).
>

[masa]
This will be modified.
"pci_iomap+ioread/iowrite32" will be used.


> > +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.

[masa]
This will be changed to 32bit stats .


>
> > + 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.

[masa]
The function for wait will be added.

>
> > +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?

[masa]
"->kernel_skb " will be deleted.

>
> > +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.

[masa]
This will be modified so.


>
> > +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...

[masa]
This will be deleted.

>
> > + 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.

[masa]
dev_kfree_skb_any(skb);
return NETDEV_TX_OK;

>
> > +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.

[masa]
"netdev->stats" will be used

>
> > +#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.
>

[masa]
I confirm Rafael's comment.


> > + 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...

[masa]
This will be modified.

>
> > + { 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...

[masa]
This will be modified.




--
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

Powered by blists - more mailing lists