[PATCH 0/5] sky2: driver update

[PATCH 0/5] sky2: driver update

[Stephen Hemminger]

Some patches for 2.6.26 and future versions

-- 

[PATCH 1/5] sky2: restore vlan acceleration on reset

[Stephen Hemminger]

[-- Attachment #1: sky2-vlan-restore.patch --]
[-- Type: text/plain, Size: 1720 bytes --]

If device has to be reset by sky2_restart, then need to restore
the VLAN acceleration settings.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
Please apply to 2.6.26


--- a/drivers/net/sky2.c	2008-05-14 16:28:22.000000000 -0700
+++ b/drivers/net/sky2.c	2008-05-14 16:29:39.000000000 -0700
@@ -1159,17 +1159,9 @@ static int sky2_ioctl(struct net_device 
 }
 
 #ifdef SKY2_VLAN_TAG_USED
-static void sky2_vlan_rx_register(struct net_device *dev, struct vlan_group *grp)
+static void sky2_set_vlan_mode(struct sky2_hw *hw, u16 port, bool onoff)
 {
-	struct sky2_port *sky2 = netdev_priv(dev);
-	struct sky2_hw *hw = sky2->hw;
-	u16 port = sky2->port;
-
-	netif_tx_lock_bh(dev);
-	napi_disable(&hw->napi);
-
-	sky2->vlgrp = grp;
-	if (grp) {
+	if (onoff) {
 		sky2_write32(hw, SK_REG(port, RX_GMF_CTRL_T),
 			     RX_VLAN_STRIP_ON);
 		sky2_write32(hw, SK_REG(port, TX_GMF_CTRL_T),
@@ -1180,6 +1172,19 @@ static void sky2_vlan_rx_register(struct
 		sky2_write32(hw, SK_REG(port, TX_GMF_CTRL_T),
 			     TX_VLAN_TAG_OFF);
 	}
+}
+
+static void sky2_vlan_rx_register(struct net_device *dev, struct vlan_group *grp)
+{
+	struct sky2_port *sky2 = netdev_priv(dev);
+	struct sky2_hw *hw = sky2->hw;
+	u16 port = sky2->port;
+
+	netif_tx_lock_bh(dev);
+	napi_disable(&hw->napi);
+
+	sky2->vlgrp = grp;
+	sky2_set_vlan_mode(hw, port, grp != NULL);
 
 	sky2_read32(hw, B0_Y2_SP_LISR);
 	napi_enable(&hw->napi);
@@ -1418,6 +1423,10 @@ static int sky2_up(struct net_device *de
 	sky2_prefetch_init(hw, txqaddr[port], sky2->tx_le_map,
 			   TX_RING_SIZE - 1);
 
+#ifdef SKY2_VLAN_TAG_USED
+	sky2_set_vlan_mode(hw, port, sky2->vlgrp != NULL);
+#endif
+
 	err = sky2_rx_start(sky2);
 	if (err)
 		goto err_out;

-- 

[PATCH 2/5] sky2: don't warn if page allocation fails

[Stephen Hemminger]

[-- Attachment #1: sky2-nowarn-alloc-page.patch --]
[-- Type: text/plain, Size: 662 bytes --]

Since driver handles page allocation failure, no need for extra
warnings. 

Note: it only allocates single pages so if the system is
so tight it can't one page it is probably hosed anyway.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
Please apply to 2.6.26
--- a/drivers/net/sky2.c	2008-05-14 16:29:39.000000000 -0700
+++ b/drivers/net/sky2.c	2008-05-14 16:29:44.000000000 -0700
@@ -1223,7 +1223,7 @@ static struct sk_buff *sky2_rx_alloc(str
 	}
 
 	for (i = 0; i < sky2->rx_nfrags; i++) {
-		struct page *page = alloc_page(GFP_ATOMIC);
+		struct page *page = alloc_page(GFP_ATOMIC | __GFP_NOWARN);
 
 		if (!page)
 			goto free_partial;

-- 

[PATCH 3/5] sky2: split phy power into two functions

[Stephen Hemminger]

[-- Attachment #1: sky2-power-split.patch --]
[-- Type: text/plain, Size: 2961 bytes --]

Later changes add more code to PHY power changes so refactor now.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
This can wait for next release (#upstream) and go in -mm

--- a/drivers/net/sky2.c	2008-05-14 16:29:44.000000000 -0700
+++ b/drivers/net/sky2.c	2008-05-14 16:29:46.000000000 -0700
@@ -619,28 +619,35 @@ static void sky2_phy_init(struct sky2_hw
 		gm_phy_write(hw, port, PHY_MARV_INT_MASK, PHY_M_DEF_MSK);
 }
 
-static void sky2_phy_power(struct sky2_hw *hw, unsigned port, int onoff)
+static const u32 phy_power[] = { PCI_Y2_PHY1_POWD, PCI_Y2_PHY2_POWD };
+static const u32 coma_mode[] = { PCI_Y2_PHY1_COMA, PCI_Y2_PHY2_COMA };
+
+static void sky2_phy_power_up(struct sky2_hw *hw, unsigned port)
 {
 	u32 reg1;
-	static const u32 phy_power[] = { PCI_Y2_PHY1_POWD, PCI_Y2_PHY2_POWD };
-	static const u32 coma_mode[] = { PCI_Y2_PHY1_COMA, PCI_Y2_PHY2_COMA };
 
 	sky2_write8(hw, B2_TST_CTRL1, TST_CFG_WRITE_ON);
 	reg1 = sky2_pci_read32(hw, PCI_DEV_REG1);
-	/* Turn on/off phy power saving */
-	if (onoff)
-		reg1 &= ~phy_power[port];
-	else
-		reg1 |= phy_power[port];
+	reg1 &= ~phy_power[port];
 
-	if (onoff && hw->chip_id == CHIP_ID_YUKON_XL && hw->chip_rev > 1)
+	if (hw->chip_id == CHIP_ID_YUKON_XL && hw->chip_rev > 1)
 		reg1 |= coma_mode[port];
 
 	sky2_pci_write32(hw, PCI_DEV_REG1, reg1);
 	sky2_write8(hw, B2_TST_CTRL1, TST_CFG_WRITE_OFF);
 	sky2_pci_read32(hw, PCI_DEV_REG1);
+}
+
+static void sky2_phy_power_down(struct sky2_hw *hw, unsigned port)
+{
+	u32 reg1;
+
+	sky2_write8(hw, B2_TST_CTRL1, TST_CFG_WRITE_ON);
+	reg1 = sky2_pci_read32(hw, PCI_DEV_REG1);
+	reg1 |= phy_power[port];
 
-	udelay(100);
+	sky2_pci_write32(hw, PCI_DEV_REG1, reg1);
+	sky2_write8(hw, B2_TST_CTRL1, TST_CFG_WRITE_OFF);
 }
 
 /* Force a renegotiation */
@@ -675,8 +682,11 @@ static void sky2_wol_init(struct sky2_po
 
 	sky2->advertising &= ~(ADVERTISED_1000baseT_Half|ADVERTISED_1000baseT_Full);
 	sky2->flow_mode = FC_NONE;
-	sky2_phy_power(hw, port, 1);
-	sky2_phy_reinit(sky2);
+
+	spin_lock_bh(&sky2->phy_lock);
+	sky2_phy_power_up(hw, port);
+	sky2_phy_init(hw, port);
+	spin_unlock_bh(&sky2->phy_lock);
 
 	sky2->flow_mode = save_mode;
 	sky2->advertising = ctrl;
@@ -781,6 +791,7 @@ static void sky2_mac_init(struct sky2_hw
 	sky2_write8(hw, SK_REG(port, GMAC_IRQ_MSK), GMAC_DEF_MSK);
 
 	spin_lock_bh(&sky2->phy_lock);
+	sky2_phy_power_up(hw, port);
 	sky2_phy_init(hw, port);
 	spin_unlock_bh(&sky2->phy_lock);
 
@@ -1385,8 +1396,6 @@ static int sky2_up(struct net_device *de
 	if (!sky2->rx_ring)
 		goto err_out;
 
-	sky2_phy_power(hw, port, 1);
-
 	sky2_mac_init(hw, port);
 
 	/* Register is number of 4K blocks on internal RAM buffer. */
@@ -1767,7 +1776,7 @@ static int sky2_down(struct net_device *
 	sky2_write8(hw, SK_REG(port, RX_GMF_CTRL_T), GMF_RST_SET);
 	sky2_write8(hw, SK_REG(port, TX_GMF_CTRL_T), GMF_RST_SET);
 
-	sky2_phy_power(hw, port, 0);
+	sky2_phy_power_down(hw, port);
 
 	netif_carrier_off(dev);
 

-- 

[PATCH 4/5] sky2: put PHY in sleep when down

[Stephen Hemminger]

[-- Attachment #1: sky2-deep-sleep.patch --]
[-- Type: text/plain, Size: 2838 bytes --]

Put PHY int sleep mode (from vendor sk98lin 10.50 driver) when the
network device is brought down.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
---
This can wait for next release (#upstream) and go in -mm

--- a/drivers/net/sky2.c	2008-05-14 16:29:46.000000000 -0700
+++ b/drivers/net/sky2.c	2008-05-14 16:29:48.000000000 -0700
@@ -641,11 +641,47 @@ static void sky2_phy_power_up(struct sky
 static void sky2_phy_power_down(struct sky2_hw *hw, unsigned port)
 {
 	u32 reg1;
+	u16 ctrl;
+
+	/* release GPHY Control reset */
+	sky2_write8(hw, SK_REG(port, GPHY_CTRL), GPC_RST_CLR);
+
+	/* release GMAC reset */
+	sky2_write8(hw, SK_REG(port, GMAC_CTRL), GMC_RST_CLR);
+
+	if (hw->flags & SKY2_HW_NEWER_PHY) {
+		/* select page 2 to access MAC control register */
+		gm_phy_write(hw, port, PHY_MARV_EXT_ADR, 2);
+
+		ctrl = gm_phy_read(hw, port, PHY_MARV_PHY_CTRL);
+		/* allow GMII Power Down */
+		ctrl &= ~PHY_M_MAC_GMIF_PUP;
+		gm_phy_write(hw, port, PHY_MARV_PHY_CTRL, ctrl);
+
+		/* set page register back to 0 */
+		gm_phy_write(hw, port, PHY_MARV_EXT_ADR, 0);
+	}
+
+	/* setup General Purpose Control Register */
+	gma_write16(hw, port, GM_GP_CTRL,
+		    GM_GPCR_FL_PASS | GM_GPCR_SPEED_100 | GM_GPCR_AU_ALL_DIS);
+
+	if (hw->chip_id != CHIP_ID_YUKON_EC) {
+		if (hw->chip_id == CHIP_ID_YUKON_EC_U) {
+			ctrl = gm_phy_read(hw, port, PHY_MARV_PHY_CTRL);
+
+			/* enable Power Down */
+			ctrl |= PHY_M_PC_POW_D_ENA;
+			gm_phy_write(hw, port, PHY_MARV_PHY_CTRL, ctrl);
+		}
+
+		/* set IEEE compatible Power Down Mode (dev. #4.99) */
+		gm_phy_write(hw, port, PHY_MARV_CTRL, PHY_CT_PDOWN);
+	}
 
 	sky2_write8(hw, B2_TST_CTRL1, TST_CFG_WRITE_ON);
 	reg1 = sky2_pci_read32(hw, PCI_DEV_REG1);
-	reg1 |= phy_power[port];
-
+	reg1 |= phy_power[port];		/* set PHY to PowerDown/COMA Mode */
 	sky2_pci_write32(hw, PCI_DEV_REG1, reg1);
 	sky2_write8(hw, B2_TST_CTRL1, TST_CFG_WRITE_OFF);
 }
--- a/drivers/net/sky2.h	2008-05-14 16:29:12.000000000 -0700
+++ b/drivers/net/sky2.h	2008-05-14 16:29:48.000000000 -0700
@@ -1143,6 +1143,12 @@ enum {
 	PHY_M_PC_ENA_AUTO	= 3, /* 11 = Enable Automatic Crossover */
 };
 
+/* for Yukon-EC Ultra Gigabit Ethernet PHY (88E1149 only) */
+enum {
+	PHY_M_PC_COP_TX_DIS	= 1<<3, /* Copper Transmitter Disable */
+	PHY_M_PC_POW_D_ENA	= 1<<2,	/* Power Down Enable */
+};
+
 /* for 10/100 Fast Ethernet PHY (88E3082 only) */
 enum {
 	PHY_M_PC_ENA_DTE_DT	= 1<<15, /* Enable Data Terminal Equ. (DTE) Detect */
@@ -1411,6 +1417,7 @@ enum {
 /*****  PHY_MARV_PHY_CTRL (page 2)		16 bit r/w	MAC Specific Ctrl *****/
 enum {
 	PHY_M_MAC_MD_MSK	= 7<<7, /* Bit  9.. 7: Mode Select Mask */
+	PHY_M_MAC_GMIF_PUP	= 1<<3,	/* GMII Power Up (88E1149 only) */
 	PHY_M_MAC_MD_AUTO	= 3,/* Auto Copper/1000Base-X */
 	PHY_M_MAC_MD_COPPER	= 5,/* Copper only */
 	PHY_M_MAC_MD_1000BX	= 7,/* 1000Base-X only */

-- 

[PATCH 5/5] sky2: pci power savings

[Stephen Hemminger]

[-- Attachment #1: pci-power.patch --]
[-- Type: text/plain, Size: 6403 bytes --]

Turn on special bits to save more power when device is shutdown.
Tested on a limited range of hardware, some of the bits are for hardware
that probably isn't even in production (like Yukon Supreme) and was ported
from the vendor driver.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
This is experimental so wait for 2.6.27 (#upstream) and go in -mm


--- a/drivers/net/sky2.c	2008-05-14 16:29:48.000000000 -0700
+++ b/drivers/net/sky2.c	2008-05-14 16:29:50.000000000 -0700
@@ -284,6 +284,86 @@ static void sky2_power_aux(struct sky2_h
 			     PC_VAUX_ON | PC_VCC_OFF));
 }
 
+static void sky2_power_state(struct sky2_hw *hw, pci_power_t state)
+{
+	u16 power_control = sky2_pci_read16(hw, hw->pm_cap + PCI_PM_CTRL);
+	int pex = pci_find_capability(hw->pdev, PCI_CAP_ID_EXP);
+	u32 reg;
+
+	sky2_write8(hw, B2_TST_CTRL1, TST_CFG_WRITE_ON);
+
+	switch (state) {
+	case PCI_D0:
+		break;
+
+	case PCI_D1:
+		power_control |= 1;
+		break;
+
+	case PCI_D2:
+		power_control |= 2;
+		break;
+
+	case PCI_D3hot:
+	case PCI_D3cold:
+		power_control |= 3;
+		if (hw->flags & SKY2_HW_ADV_POWER_CTL) {
+			/* additional power saving measurements */
+			reg = sky2_pci_read32(hw, PCI_DEV_REG4);
+
+			/* set gating core clock for LTSSM in L1 state */
+			reg |= P_PEX_LTSSM_STAT(P_PEX_LTSSM_L1_STAT) |
+				/* auto clock gated scheme controlled by CLKREQ */
+				P_ASPM_A1_MODE_SELECT |
+				/* enable Gate Root Core Clock */
+				P_CLK_GATE_ROOT_COR_ENA;
+
+			if (pex && (hw->flags & SKY2_HW_CLK_POWER)) {
+				/* enable Clock Power Management (CLKREQ) */
+				u16 ctrl = sky2_pci_read16(hw, pex + PCI_EXP_DEVCTL);
+
+				ctrl |= PCI_EXP_DEVCTL_AUX_PME;
+				sky2_pci_write16(hw, pex + PCI_EXP_DEVCTL, ctrl);
+			} else
+				/* force CLKREQ Enable in Our4 (A1b only) */
+				reg |= P_ASPM_FORCE_CLKREQ_ENA;
+
+			/* set Mask Register for Release/Gate Clock */
+			sky2_pci_write32(hw, PCI_DEV_REG5,
+					 P_REL_PCIE_EXIT_L1_ST | P_GAT_PCIE_ENTER_L1_ST |
+					 P_REL_PCIE_RX_EX_IDLE | P_GAT_PCIE_RX_EL_IDLE |
+					 P_REL_GPHY_LINK_UP | P_GAT_GPHY_LINK_DOWN);
+		} else
+			sky2_write8(hw, B28_Y2_ASF_STAT_CMD, Y2_ASF_CLK_HALT);
+
+		/* put CPU into reset state */
+		sky2_write8(hw,  B28_Y2_ASF_STAT_CMD, HCU_CCSR_ASF_RESET);
+		if (hw->chip_id == CHIP_ID_YUKON_SUPR && hw->chip_rev == CHIP_REV_YU_SU_A0)
+			/* put CPU into halt state */
+			sky2_write8(hw, B28_Y2_ASF_STAT_CMD, HCU_CCSR_ASF_HALTED);
+
+		if (pex && !(hw->flags & SKY2_HW_RAM_BUFFER)) {
+			reg = sky2_pci_read32(hw, PCI_DEV_REG1);
+			/* force to PCIe L1 */
+			reg |= PCI_FORCE_PEX_L1;
+			sky2_pci_write32(hw, PCI_DEV_REG1, reg);
+		}
+		break;
+
+	default:
+		dev_warn(&hw->pdev->dev, PFX "Invalid power state (%d) ",
+		       state);
+		return;
+	}
+
+	power_control |= PCI_PM_CTRL_PME_ENABLE;
+	/* Finally, set the new power state. */
+	sky2_pci_write32(hw, hw->pm_cap + PCI_PM_CTRL, power_control);
+
+	sky2_write8(hw, B2_TST_CTRL1, TST_CFG_WRITE_OFF);
+	sky2_pci_read32(hw, B0_CTST);
+}
+
 static void sky2_gmac_reset(struct sky2_hw *hw, unsigned port)
 {
 	u16 reg;
@@ -2786,6 +2866,10 @@ static int __devinit sky2_init(struct sk
 		hw->flags = SKY2_HW_GIGABIT
 			| SKY2_HW_NEWER_PHY
 			| SKY2_HW_ADV_POWER_CTL;
+
+		/* check for Rev. A1 dev 4200 */
+		if (sky2_read16(hw, Q_ADDR(Q_XA1, Q_WM)) == 0)
+			hw->flags |= SKY2_HW_CLK_POWER;
 		break;
 
 	case CHIP_ID_YUKON_EX:
@@ -2836,6 +2920,11 @@ static int __devinit sky2_init(struct sk
 	if (hw->pmd_type == 'L' || hw->pmd_type == 'S' || hw->pmd_type == 'P')
 		hw->flags |= SKY2_HW_FIBRE_PHY;
 
+	hw->pm_cap = pci_find_capability(hw->pdev, PCI_CAP_ID_PM);
+	if (hw->pm_cap == 0) {
+		dev_err(&hw->pdev->dev, "cannot find PowerManagement capability\n");
+		return -EIO;
+	}
 
 	hw->ports = 1;
 	t8 = sky2_read8(hw, B2_Y2_HW_RES);
@@ -4407,7 +4496,7 @@ static int sky2_suspend(struct pci_dev *
 
 	pci_save_state(pdev);
 	pci_enable_wake(pdev, pci_choose_state(pdev, state), wol);
-	pci_set_power_state(pdev, pci_choose_state(pdev, state));
+	sky2_power_state(hw, pci_choose_state(pdev, state));
 
 	return 0;
 }
@@ -4420,9 +4509,7 @@ static int sky2_resume(struct pci_dev *p
 	if (!hw)
 		return 0;
 
-	err = pci_set_power_state(pdev, PCI_D0);
-	if (err)
-		goto out;
+	sky2_power_state(hw, PCI_D0);
 
 	err = pci_restore_state(pdev);
 	if (err)
@@ -4490,8 +4577,7 @@ static void sky2_shutdown(struct pci_dev
 	pci_enable_wake(pdev, PCI_D3cold, wol);
 
 	pci_disable_device(pdev);
-	pci_set_power_state(pdev, PCI_D3hot);
-
+	sky2_power_state(hw, PCI_D3hot);
 }
 
 static struct pci_driver sky2_driver = {
--- a/drivers/net/sky2.h	2008-05-14 16:29:48.000000000 -0700
+++ b/drivers/net/sky2.h	2008-05-14 16:29:50.000000000 -0700
@@ -28,6 +28,11 @@ enum pci_dev_reg_1 {
 	PCI_Y2_PHY2_POWD = 1<<27, /* Set PHY 2 to Power Down (YUKON-2) */
 	PCI_Y2_PHY1_POWD = 1<<26, /* Set PHY 1 to Power Down (YUKON-2) */
 	PCI_Y2_PME_LEGACY= 1<<15, /* PCI Express legacy power management mode */
+
+	PCI_PHY_LNK_TIM_MSK= 3L<<8,/* Bit  9.. 8:	GPHY Link Trigger Timer */
+	PCI_ENA_L1_EVENT = 1<<7, /* Enable PEX L1 Event */
+	PCI_ENA_GPHY_LNK = 1<<6, /* Enable PEX L1 on GPHY Link down */
+	PCI_FORCE_PEX_L1 = 1<<5, /* Force to PEX L1 */
 };
 
 enum pci_dev_reg_2 {
@@ -45,7 +50,11 @@ enum pci_dev_reg_2 {
 
 /*	PCI_OUR_REG_4		32 bit	Our Register 4 (Yukon-ECU only) */
 enum pci_dev_reg_4 {
-					/* (Link Training & Status State Machine) */
+				/* (Link Training & Status State Machine) */
+	P_PEX_LTSSM_STAT_MSK	= 0x7fL<<25,	/* Bit 31..25:	PEX LTSSM Mask */
+#define P_PEX_LTSSM_STAT(x)	((x << 25) & P_PEX_LTSSM_STAT_MSK)
+	P_PEX_LTSSM_L1_STAT	= 0x34,
+	P_PEX_LTSSM_DET_STAT	= 0x01,
 	P_TIMER_VALUE_MSK	= 0xffL<<16,	/* Bit 23..16:	Timer Value Mask */
 					/* (Active State Power Management) */
 	P_FORCE_ASPM_REQUEST	= 1<<15, /* Force ASPM Request (A1 only) */
@@ -454,6 +463,9 @@ enum yukon_ex_rev {
 	CHIP_REV_YU_EX_A0    = 1,
 	CHIP_REV_YU_EX_B0    = 2,
 };
+enum yukon_supr_rev {
+	CHIP_REV_YU_SU_A0    = 0,
+};
 
 
 /*	B2_Y2_CLK_GATE	 8 bit	Clock Gating (Yukon-2 only) */
@@ -2059,7 +2071,9 @@ struct sky2_hw {
 #define SKY2_HW_NEW_LE		0x00000020	/* new LSOv2 format */
 #define SKY2_HW_AUTO_TX_SUM	0x00000040	/* new IP decode for Tx */
 #define SKY2_HW_ADV_POWER_CTL	0x00000080	/* additional PHY power regs */
+#define SKY2_HW_CLK_POWER	0x00000100	/* clock power management */
 
+	int		     pm_cap;
 	u8	     	     chip_id;
 	u8		     chip_rev;
 	u8		     pmd_type;

-- 

Re: [PATCH 1/5] sky2: restore vlan acceleration on reset

[Stephen Hemminger]

On Wed, 14 May 2008 17:04:13 -0700
Stephen Hemminger <shemminger@vyatta.com> wrote:

> If device has to be reset by sky2_restart, then need to restore
> the VLAN acceleration settings.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> ---
> Please apply to 2.6.26

Okay, it has been a week with no reply, what is the status
of these patches?

Re: [PATCH 1/5] sky2: restore vlan acceleration on reset

[Jeff Garzik]

Stephen Hemminger wrote:
> On Wed, 14 May 2008 17:04:13 -0700
> Stephen Hemminger <shemminger@vyatta.com> wrote:
> 
>> If device has to be reset by sky2_restart, then need to restore
>> the VLAN acceleration settings.
>>
>> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
>>
>> ---
>> Please apply to 2.6.26
> 
> Okay, it has been a week with no reply, what is the status
> of these patches?

Ouch, I missed it, sorry.  Dug it out...

Re: [PATCH 2/5] sky2: don't warn if page allocation fails

[Jeff Garzik]

>  
>  	for (i = 0; i < sky2->rx_nfrags; i++) {
> -		struct page *page = alloc_page(GFP_ATOMIC);
> +		struct page *page = alloc_page(GFP_ATOMIC | __GFP_NOWARN);
>  
>  		if (!page)
>  			goto free_partial;


IMO it's inappropriate to add these warnings to net drivers that 
properly check all return values.

This approach is too maintenance intensive, and winds up fixing the same 
problem over and over again -- a hint that the fix is in the wrong place.

	Jeff

Re: [PATCH 1/5] sky2: restore vlan acceleration on reset

[Jeff Garzik]

applied

Re: [PATCH 2/5] sky2: don't warn if page allocation fails

[Stephen Hemminger]

On Thu, 22 May 2008 05:57:44 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

> >  
> >  	for (i = 0; i < sky2->rx_nfrags; i++) {
> > -		struct page *page = alloc_page(GFP_ATOMIC);
> > +		struct page *page = alloc_page(GFP_ATOMIC | __GFP_NOWARN);
> >  
> >  		if (!page)
> >  			goto free_partial;
> 
> 
> IMO it's inappropriate to add these warnings to net drivers that 
> properly check all return values.
> 
> This approach is too maintenance intensive, and winds up fixing the same 
> problem over and over again -- a hint that the fix is in the wrong place.
> 
> 	Jeff
> 
> 

So the __GFP_NOWARN should go away and get replaced by GFP_WARN?

Re: [PATCH 3/5] sky2: split phy power into two functions

[Jeff Garzik]

applied 3-5

Re: [PATCH 2/5] sky2: don't warn if page allocation fails

[Nick Piggin]

On Wednesday 28 May 2008 04:47, Stephen Hemminger wrote:
> On Thu, 22 May 2008 05:57:44 -0400
>
> Jeff Garzik <jgarzik@pobox.com> wrote:
> > >  	for (i = 0; i < sky2->rx_nfrags; i++) {
> > > -		struct page *page = alloc_page(GFP_ATOMIC);
> > > +		struct page *page = alloc_page(GFP_ATOMIC | __GFP_NOWARN);
> > >
> > >  		if (!page)
> > >  			goto free_partial;
> >
> > IMO it's inappropriate to add these warnings to net drivers that
> > properly check all return values.
> >
> > This approach is too maintenance intensive, and winds up fixing the same
> > problem over and over again -- a hint that the fix is in the wrong place.
> >
> > 	Jeff
>
> So the __GFP_NOWARN should go away and get replaced by GFP_WARN?

We actually still want to see the messages, regardless of what is causing
them, because they can indicate problems in the VM.

The best way is not to add __GFP_NOWARN at all, and just improve the
throttling/thresholding/reporting of the page allocation failure warnings.