Re: [PATCH v3] net: mvneta: support suspend and resume

2017-03-16 Thread Jane Li 



On 03/16/2017 03:48 PM, Jisheng Zhang wrote:

On Thu, 16 Mar 2017 15:11:48 +0800 Jane Li wrote:


Add basic support for handling suspend and resume.

Signed-off-by: Jane Li <j...@marvell.com>
---
Since v2:
- use SIMPLE_DEV_PM_OPS instead of SET_LATE_SYSTEM_SLEEP_PM_OPS

Since v1:
- add mvneta_conf_mbus_windows() and mvneta_bm_port_init() in mvneta_resume()

  drivers/net/ethernet/marvell/mvneta.c | 60 ---
  1 file changed, 56 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 61dd446..b0fea26 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -431,6 +431,7 @@ struct mvneta_port {
/* Flags for special SoC configurations */
bool neta_armada3700;
u16 rx_offset_correction;
+   const struct mbus_dram_target_info *dram_target_info;
  };
  
  /* The mvneta_tx_desc and mvneta_rx_desc structures describe the

@@ -4118,7 +4119,6 @@ static int mvneta_port_power_up(struct mvneta_port *pp, 
int phy_mode)
  /* Device initialization routine */
  static int mvneta_probe(struct platform_device *pdev)
  {
-   const struct mbus_dram_target_info *dram_target_info;
struct resource *res;
struct device_node *dn = pdev->dev.of_node;
struct device_node *phy_node;
@@ -4267,13 +4267,13 @@ static int mvneta_probe(struct platform_device *pdev)
  
  	pp->tx_csum_limit = tx_csum_limit;
  
-	dram_target_info = mv_mbus_dram_info();

+   pp->dram_target_info = mv_mbus_dram_info();
/* Armada3700 requires setting default configuration of Mbus
 * windows, however without using filled mbus_dram_target_info
 * structure.
 */
-   if (dram_target_info || pp->neta_armada3700)
-   mvneta_conf_mbus_windows(pp, dram_target_info);
+   if (pp->dram_target_info || pp->neta_armada3700)
+   mvneta_conf_mbus_windows(pp, pp->dram_target_info);
  
  	pp->tx_ring_size = MVNETA_MAX_TXD;

pp->rx_ring_size = MVNETA_MAX_RXD;
@@ -4405,6 +4405,57 @@ static int mvneta_remove(struct platform_device *pdev)
return 0;
  }
  
+#ifdef CONFIG_PM_SLEEP

+static int mvneta_suspend(struct device *device)
+{
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+
+   if (netif_running(dev))
+   mvneta_stop(dev);
+   netif_device_detach(dev);
+   clk_disable_unprepare(pp->clk_bus);
+   clk_disable_unprepare(pp->clk);
+   return 0;
+}
+
+static int mvneta_resume(struct device *device)
+{
+   struct platform_device *pdev = to_platform_device(device);
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+   int err;
+
+   clk_prepare_enable(pp->clk);
+   clk_prepare_enable(pp->clk_bus);

hmm, since clk_bus is optional, it's better to add check here.

Except the above, you could add

Reviewed-by: Jisheng Zhang <jszh...@marvell.com>

Done. Add this in patch v4.

+   if (pp->dram_target_info || pp->neta_armada3700)
+   mvneta_conf_mbus_windows(pp, pp->dram_target_info);
+   if (pp->bm_priv) {
+   err = mvneta_bm_port_init(pdev, pp);
+   if (err < 0) {
+   dev_info(>dev, "use SW buffer management\n");
+   pp->bm_priv = NULL;
+   }
+   }
+   mvneta_defaults_set(pp);
+   err = mvneta_port_power_up(pp, pp->phy_interface);
+   if (err < 0) {
+   dev_err(device, "can't power up port\n");
+   return err;
+   }
+
+   if (pp->use_inband_status)
+   mvneta_fixed_link_update(pp, dev->phydev);
+
+   netif_device_attach(dev);
+   if (netif_running(dev))
+   mvneta_open(dev);
+   return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(mvneta_pm_ops, mvneta_suspend, mvneta_resume);
+
  static const struct of_device_id mvneta_match[] = {
{ .compatible = "marvell,armada-370-neta" },
{ .compatible = "marvell,armada-xp-neta" },
@@ -4419,6 +4470,7 @@ static int mvneta_remove(struct platform_device *pdev)
.driver = {
.name = MVNETA_DRIVER_NAME,
.of_match_table = mvneta_match,
+   .pm = _pm_ops,
},
  };

Re: [PATCH v3] net: mvneta: support suspend and resume

2017-03-16 Thread Jane Li 



On 03/16/2017 03:48 PM, Jisheng Zhang wrote:

On Thu, 16 Mar 2017 15:11:48 +0800 Jane Li wrote:


Add basic support for handling suspend and resume.

Signed-off-by: Jane Li 
---
Since v2:
- use SIMPLE_DEV_PM_OPS instead of SET_LATE_SYSTEM_SLEEP_PM_OPS

Since v1:
- add mvneta_conf_mbus_windows() and mvneta_bm_port_init() in mvneta_resume()

  drivers/net/ethernet/marvell/mvneta.c | 60 ---
  1 file changed, 56 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 61dd446..b0fea26 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -431,6 +431,7 @@ struct mvneta_port {
/* Flags for special SoC configurations */
bool neta_armada3700;
u16 rx_offset_correction;
+   const struct mbus_dram_target_info *dram_target_info;
  };
  
  /* The mvneta_tx_desc and mvneta_rx_desc structures describe the

@@ -4118,7 +4119,6 @@ static int mvneta_port_power_up(struct mvneta_port *pp, 
int phy_mode)
  /* Device initialization routine */
  static int mvneta_probe(struct platform_device *pdev)
  {
-   const struct mbus_dram_target_info *dram_target_info;
struct resource *res;
struct device_node *dn = pdev->dev.of_node;
struct device_node *phy_node;
@@ -4267,13 +4267,13 @@ static int mvneta_probe(struct platform_device *pdev)
  
  	pp->tx_csum_limit = tx_csum_limit;
  
-	dram_target_info = mv_mbus_dram_info();

+   pp->dram_target_info = mv_mbus_dram_info();
/* Armada3700 requires setting default configuration of Mbus
 * windows, however without using filled mbus_dram_target_info
 * structure.
 */
-   if (dram_target_info || pp->neta_armada3700)
-   mvneta_conf_mbus_windows(pp, dram_target_info);
+   if (pp->dram_target_info || pp->neta_armada3700)
+   mvneta_conf_mbus_windows(pp, pp->dram_target_info);
  
  	pp->tx_ring_size = MVNETA_MAX_TXD;

pp->rx_ring_size = MVNETA_MAX_RXD;
@@ -4405,6 +4405,57 @@ static int mvneta_remove(struct platform_device *pdev)
return 0;
  }
  
+#ifdef CONFIG_PM_SLEEP

+static int mvneta_suspend(struct device *device)
+{
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+
+   if (netif_running(dev))
+   mvneta_stop(dev);
+   netif_device_detach(dev);
+   clk_disable_unprepare(pp->clk_bus);
+   clk_disable_unprepare(pp->clk);
+   return 0;
+}
+
+static int mvneta_resume(struct device *device)
+{
+   struct platform_device *pdev = to_platform_device(device);
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+   int err;
+
+   clk_prepare_enable(pp->clk);
+   clk_prepare_enable(pp->clk_bus);

hmm, since clk_bus is optional, it's better to add check here.

Except the above, you could add

Reviewed-by: Jisheng Zhang 

Done. Add this in patch v4.

+   if (pp->dram_target_info || pp->neta_armada3700)
+   mvneta_conf_mbus_windows(pp, pp->dram_target_info);
+   if (pp->bm_priv) {
+   err = mvneta_bm_port_init(pdev, pp);
+   if (err < 0) {
+   dev_info(>dev, "use SW buffer management\n");
+   pp->bm_priv = NULL;
+   }
+   }
+   mvneta_defaults_set(pp);
+   err = mvneta_port_power_up(pp, pp->phy_interface);
+   if (err < 0) {
+   dev_err(device, "can't power up port\n");
+   return err;
+   }
+
+   if (pp->use_inband_status)
+   mvneta_fixed_link_update(pp, dev->phydev);
+
+   netif_device_attach(dev);
+   if (netif_running(dev))
+   mvneta_open(dev);
+   return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(mvneta_pm_ops, mvneta_suspend, mvneta_resume);
+
  static const struct of_device_id mvneta_match[] = {
{ .compatible = "marvell,armada-370-neta" },
{ .compatible = "marvell,armada-xp-neta" },
@@ -4419,6 +4470,7 @@ static int mvneta_remove(struct platform_device *pdev)
.driver = {
.name = MVNETA_DRIVER_NAME,
.of_match_table = mvneta_match,
+   .pm = _pm_ops,
},
  };

[PATCH v4] net: mvneta: support suspend and resume

2017-03-16 Thread Jane Li 
Add basic support for handling suspend and resume.

Signed-off-by: Jane Li <j...@marvell.com>
Reviewed-by: Jisheng Zhang <jszh...@marvell.com>
---
Since v3:
- check clk_bus before enabling it during resume

Since v2:
- use SIMPLE_DEV_PM_OPS instead of SET_LATE_SYSTEM_SLEEP_PM_OPS

Since v1:
- add mvneta_conf_mbus_windows() and mvneta_bm_port_init() in mvneta_resume()

 drivers/net/ethernet/marvell/mvneta.c | 61 ---
 1 file changed, 57 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 61dd446..aebbc53 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -431,6 +431,7 @@ struct mvneta_port {
/* Flags for special SoC configurations */
bool neta_armada3700;
u16 rx_offset_correction;
+   const struct mbus_dram_target_info *dram_target_info;
 };
 
 /* The mvneta_tx_desc and mvneta_rx_desc structures describe the
@@ -4118,7 +4119,6 @@ static int mvneta_port_power_up(struct mvneta_port *pp, 
int phy_mode)
 /* Device initialization routine */
 static int mvneta_probe(struct platform_device *pdev)
 {
-   const struct mbus_dram_target_info *dram_target_info;
struct resource *res;
struct device_node *dn = pdev->dev.of_node;
struct device_node *phy_node;
@@ -4267,13 +4267,13 @@ static int mvneta_probe(struct platform_device *pdev)
 
pp->tx_csum_limit = tx_csum_limit;
 
-   dram_target_info = mv_mbus_dram_info();
+   pp->dram_target_info = mv_mbus_dram_info();
/* Armada3700 requires setting default configuration of Mbus
 * windows, however without using filled mbus_dram_target_info
 * structure.
 */
-   if (dram_target_info || pp->neta_armada3700)
-   mvneta_conf_mbus_windows(pp, dram_target_info);
+   if (pp->dram_target_info || pp->neta_armada3700)
+   mvneta_conf_mbus_windows(pp, pp->dram_target_info);
 
pp->tx_ring_size = MVNETA_MAX_TXD;
pp->rx_ring_size = MVNETA_MAX_RXD;
@@ -4405,6 +4405,58 @@ static int mvneta_remove(struct platform_device *pdev)
return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int mvneta_suspend(struct device *device)
+{
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+
+   if (netif_running(dev))
+   mvneta_stop(dev);
+   netif_device_detach(dev);
+   clk_disable_unprepare(pp->clk_bus);
+   clk_disable_unprepare(pp->clk);
+   return 0;
+}
+
+static int mvneta_resume(struct device *device)
+{
+   struct platform_device *pdev = to_platform_device(device);
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+   int err;
+
+   clk_prepare_enable(pp->clk);
+   if (!IS_ERR(pp->clk_bus))
+   clk_prepare_enable(pp->clk_bus);
+   if (pp->dram_target_info || pp->neta_armada3700)
+   mvneta_conf_mbus_windows(pp, pp->dram_target_info);
+   if (pp->bm_priv) {
+   err = mvneta_bm_port_init(pdev, pp);
+   if (err < 0) {
+   dev_info(>dev, "use SW buffer management\n");
+   pp->bm_priv = NULL;
+   }
+   }
+   mvneta_defaults_set(pp);
+   err = mvneta_port_power_up(pp, pp->phy_interface);
+   if (err < 0) {
+   dev_err(device, "can't power up port\n");
+   return err;
+   }
+
+   if (pp->use_inband_status)
+   mvneta_fixed_link_update(pp, dev->phydev);
+
+   netif_device_attach(dev);
+   if (netif_running(dev))
+   mvneta_open(dev);
+   return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(mvneta_pm_ops, mvneta_suspend, mvneta_resume);
+
 static const struct of_device_id mvneta_match[] = {
{ .compatible = "marvell,armada-370-neta" },
{ .compatible = "marvell,armada-xp-neta" },
@@ -4419,6 +4471,7 @@ static int mvneta_remove(struct platform_device *pdev)
.driver = {
.name = MVNETA_DRIVER_NAME,
.of_match_table = mvneta_match,
+   .pm = _pm_ops,
},
 };
 
-- 
1.9.1

[PATCH v4] net: mvneta: support suspend and resume

2017-03-16 Thread Jane Li 
Add basic support for handling suspend and resume.

Signed-off-by: Jane Li 
Reviewed-by: Jisheng Zhang 
---
Since v3:
- check clk_bus before enabling it during resume

Since v2:
- use SIMPLE_DEV_PM_OPS instead of SET_LATE_SYSTEM_SLEEP_PM_OPS

Since v1:
- add mvneta_conf_mbus_windows() and mvneta_bm_port_init() in mvneta_resume()

 drivers/net/ethernet/marvell/mvneta.c | 61 ---
 1 file changed, 57 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 61dd446..aebbc53 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -431,6 +431,7 @@ struct mvneta_port {
/* Flags for special SoC configurations */
bool neta_armada3700;
u16 rx_offset_correction;
+   const struct mbus_dram_target_info *dram_target_info;
 };
 
 /* The mvneta_tx_desc and mvneta_rx_desc structures describe the
@@ -4118,7 +4119,6 @@ static int mvneta_port_power_up(struct mvneta_port *pp, 
int phy_mode)
 /* Device initialization routine */
 static int mvneta_probe(struct platform_device *pdev)
 {
-   const struct mbus_dram_target_info *dram_target_info;
struct resource *res;
struct device_node *dn = pdev->dev.of_node;
struct device_node *phy_node;
@@ -4267,13 +4267,13 @@ static int mvneta_probe(struct platform_device *pdev)
 
pp->tx_csum_limit = tx_csum_limit;
 
-   dram_target_info = mv_mbus_dram_info();
+   pp->dram_target_info = mv_mbus_dram_info();
/* Armada3700 requires setting default configuration of Mbus
 * windows, however without using filled mbus_dram_target_info
 * structure.
 */
-   if (dram_target_info || pp->neta_armada3700)
-   mvneta_conf_mbus_windows(pp, dram_target_info);
+   if (pp->dram_target_info || pp->neta_armada3700)
+   mvneta_conf_mbus_windows(pp, pp->dram_target_info);
 
pp->tx_ring_size = MVNETA_MAX_TXD;
pp->rx_ring_size = MVNETA_MAX_RXD;
@@ -4405,6 +4405,58 @@ static int mvneta_remove(struct platform_device *pdev)
return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int mvneta_suspend(struct device *device)
+{
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+
+   if (netif_running(dev))
+   mvneta_stop(dev);
+   netif_device_detach(dev);
+   clk_disable_unprepare(pp->clk_bus);
+   clk_disable_unprepare(pp->clk);
+   return 0;
+}
+
+static int mvneta_resume(struct device *device)
+{
+   struct platform_device *pdev = to_platform_device(device);
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+   int err;
+
+   clk_prepare_enable(pp->clk);
+   if (!IS_ERR(pp->clk_bus))
+   clk_prepare_enable(pp->clk_bus);
+   if (pp->dram_target_info || pp->neta_armada3700)
+   mvneta_conf_mbus_windows(pp, pp->dram_target_info);
+   if (pp->bm_priv) {
+   err = mvneta_bm_port_init(pdev, pp);
+   if (err < 0) {
+   dev_info(>dev, "use SW buffer management\n");
+   pp->bm_priv = NULL;
+   }
+   }
+   mvneta_defaults_set(pp);
+   err = mvneta_port_power_up(pp, pp->phy_interface);
+   if (err < 0) {
+   dev_err(device, "can't power up port\n");
+   return err;
+   }
+
+   if (pp->use_inband_status)
+   mvneta_fixed_link_update(pp, dev->phydev);
+
+   netif_device_attach(dev);
+   if (netif_running(dev))
+   mvneta_open(dev);
+   return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(mvneta_pm_ops, mvneta_suspend, mvneta_resume);
+
 static const struct of_device_id mvneta_match[] = {
{ .compatible = "marvell,armada-370-neta" },
{ .compatible = "marvell,armada-xp-neta" },
@@ -4419,6 +4471,7 @@ static int mvneta_remove(struct platform_device *pdev)
.driver = {
.name = MVNETA_DRIVER_NAME,
.of_match_table = mvneta_match,
+   .pm = _pm_ops,
},
 };
 
-- 
1.9.1

Re: [PATCH v2] net: mvneta: support suspend and resume

2017-03-16 Thread Jane Li 



On 03/16/2017 11:37 AM, Jisheng Zhang wrote:

On Thu, 16 Mar 2017 11:19:10 +0800 Jane Li wrote:


Add basic support for handling suspend and resume.

Signed-off-by: Jane Li <j...@marvell.com>
---
Since v1:
- add mvneta_conf_mbus_windows() and mvneta_bm_port_init() in mvneta_resume()

  drivers/net/ethernet/marvell/mvneta.c | 62 ---
  1 file changed, 58 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 61dd446..250017b 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -431,6 +431,7 @@ struct mvneta_port {
/* Flags for special SoC configurations */
bool neta_armada3700;
u16 rx_offset_correction;
+   const struct mbus_dram_target_info *dram_target_info;
  };
  
  /* The mvneta_tx_desc and mvneta_rx_desc structures describe the

@@ -4118,7 +4119,6 @@ static int mvneta_port_power_up(struct mvneta_port *pp, 
int phy_mode)
  /* Device initialization routine */
  static int mvneta_probe(struct platform_device *pdev)
  {
-   const struct mbus_dram_target_info *dram_target_info;
struct resource *res;
struct device_node *dn = pdev->dev.of_node;
struct device_node *phy_node;
@@ -4267,13 +4267,13 @@ static int mvneta_probe(struct platform_device *pdev)
  
  	pp->tx_csum_limit = tx_csum_limit;
  
-	dram_target_info = mv_mbus_dram_info();

+   pp->dram_target_info = mv_mbus_dram_info();
/* Armada3700 requires setting default configuration of Mbus
 * windows, however without using filled mbus_dram_target_info
 * structure.
 */
-   if (dram_target_info || pp->neta_armada3700)
-   mvneta_conf_mbus_windows(pp, dram_target_info);
+   if (pp->dram_target_info || pp->neta_armada3700)
+   mvneta_conf_mbus_windows(pp, pp->dram_target_info);
  
  	pp->tx_ring_size = MVNETA_MAX_TXD;

pp->rx_ring_size = MVNETA_MAX_RXD;
@@ -4405,6 +4405,59 @@ static int mvneta_remove(struct platform_device *pdev)
return 0;
  }
  
+#ifdef CONFIG_PM_SLEEP

+static int mvneta_suspend(struct device *device)
+{
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+
+   if (netif_running(dev))
+   mvneta_stop(dev);
+   netif_device_detach(dev);
+   clk_disable_unprepare(pp->clk_bus);
+   clk_disable_unprepare(pp->clk);
+   return 0;
+}
+
+static int mvneta_resume(struct device *device)
+{
+   struct platform_device *pdev = to_platform_device(device);
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+   int err;
+
+   clk_prepare_enable(pp->clk);
+   clk_prepare_enable(pp->clk_bus);
+   if (pp->dram_target_info || pp->neta_armada3700)
+   mvneta_conf_mbus_windows(pp, pp->dram_target_info);
+   if (pp->bm_priv) {
+   err = mvneta_bm_port_init(pdev, pp);
+   if (err < 0) {
+   dev_info(>dev, "use SW buffer management\n");
+   pp->bm_priv = NULL;
+   }
+   }
+   mvneta_defaults_set(pp);
+   err = mvneta_port_power_up(pp, pp->phy_interface);
+   if (err < 0) {
+   dev_err(device, "can't power up port\n");
+   return err;
+   }
+
+   if (pp->use_inband_status)
+   mvneta_fixed_link_update(pp, dev->phydev);
+
+   netif_device_attach(dev);
+   if (netif_running(dev))
+   mvneta_open(dev);
+   return 0;
+}
+#endif
+
+static const struct dev_pm_ops mvneta_pm_ops = {
+   SET_LATE_SYSTEM_SLEEP_PM_OPS(mvneta_suspend, mvneta_resume)
+};

we could make use of SIMPLE_DEV_PM_OPS to simplify the code

Thanks

Done. Modify this in patch v3.

Thanks,
Jane

Re: [PATCH v2] net: mvneta: support suspend and resume

2017-03-16 Thread Jane Li 



On 03/16/2017 11:37 AM, Jisheng Zhang wrote:

On Thu, 16 Mar 2017 11:19:10 +0800 Jane Li wrote:


Add basic support for handling suspend and resume.

Signed-off-by: Jane Li 
---
Since v1:
- add mvneta_conf_mbus_windows() and mvneta_bm_port_init() in mvneta_resume()

  drivers/net/ethernet/marvell/mvneta.c | 62 ---
  1 file changed, 58 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 61dd446..250017b 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -431,6 +431,7 @@ struct mvneta_port {
/* Flags for special SoC configurations */
bool neta_armada3700;
u16 rx_offset_correction;
+   const struct mbus_dram_target_info *dram_target_info;
  };
  
  /* The mvneta_tx_desc and mvneta_rx_desc structures describe the

@@ -4118,7 +4119,6 @@ static int mvneta_port_power_up(struct mvneta_port *pp, 
int phy_mode)
  /* Device initialization routine */
  static int mvneta_probe(struct platform_device *pdev)
  {
-   const struct mbus_dram_target_info *dram_target_info;
struct resource *res;
struct device_node *dn = pdev->dev.of_node;
struct device_node *phy_node;
@@ -4267,13 +4267,13 @@ static int mvneta_probe(struct platform_device *pdev)
  
  	pp->tx_csum_limit = tx_csum_limit;
  
-	dram_target_info = mv_mbus_dram_info();

+   pp->dram_target_info = mv_mbus_dram_info();
/* Armada3700 requires setting default configuration of Mbus
 * windows, however without using filled mbus_dram_target_info
 * structure.
 */
-   if (dram_target_info || pp->neta_armada3700)
-   mvneta_conf_mbus_windows(pp, dram_target_info);
+   if (pp->dram_target_info || pp->neta_armada3700)
+   mvneta_conf_mbus_windows(pp, pp->dram_target_info);
  
  	pp->tx_ring_size = MVNETA_MAX_TXD;

pp->rx_ring_size = MVNETA_MAX_RXD;
@@ -4405,6 +4405,59 @@ static int mvneta_remove(struct platform_device *pdev)
return 0;
  }
  
+#ifdef CONFIG_PM_SLEEP

+static int mvneta_suspend(struct device *device)
+{
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+
+   if (netif_running(dev))
+   mvneta_stop(dev);
+   netif_device_detach(dev);
+   clk_disable_unprepare(pp->clk_bus);
+   clk_disable_unprepare(pp->clk);
+   return 0;
+}
+
+static int mvneta_resume(struct device *device)
+{
+   struct platform_device *pdev = to_platform_device(device);
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+   int err;
+
+   clk_prepare_enable(pp->clk);
+   clk_prepare_enable(pp->clk_bus);
+   if (pp->dram_target_info || pp->neta_armada3700)
+   mvneta_conf_mbus_windows(pp, pp->dram_target_info);
+   if (pp->bm_priv) {
+   err = mvneta_bm_port_init(pdev, pp);
+   if (err < 0) {
+   dev_info(>dev, "use SW buffer management\n");
+   pp->bm_priv = NULL;
+   }
+   }
+   mvneta_defaults_set(pp);
+   err = mvneta_port_power_up(pp, pp->phy_interface);
+   if (err < 0) {
+   dev_err(device, "can't power up port\n");
+   return err;
+   }
+
+   if (pp->use_inband_status)
+   mvneta_fixed_link_update(pp, dev->phydev);
+
+   netif_device_attach(dev);
+   if (netif_running(dev))
+   mvneta_open(dev);
+   return 0;
+}
+#endif
+
+static const struct dev_pm_ops mvneta_pm_ops = {
+   SET_LATE_SYSTEM_SLEEP_PM_OPS(mvneta_suspend, mvneta_resume)
+};

we could make use of SIMPLE_DEV_PM_OPS to simplify the code

Thanks

Done. Modify this in patch v3.

Thanks,
Jane

[PATCH v3] net: mvneta: support suspend and resume

2017-03-16 Thread Jane Li 
Add basic support for handling suspend and resume.

Signed-off-by: Jane Li <j...@marvell.com>
---
Since v2:
- use SIMPLE_DEV_PM_OPS instead of SET_LATE_SYSTEM_SLEEP_PM_OPS

Since v1:
- add mvneta_conf_mbus_windows() and mvneta_bm_port_init() in mvneta_resume()

 drivers/net/ethernet/marvell/mvneta.c | 60 ---
 1 file changed, 56 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 61dd446..b0fea26 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -431,6 +431,7 @@ struct mvneta_port {
/* Flags for special SoC configurations */
bool neta_armada3700;
u16 rx_offset_correction;
+   const struct mbus_dram_target_info *dram_target_info;
 };
 
 /* The mvneta_tx_desc and mvneta_rx_desc structures describe the
@@ -4118,7 +4119,6 @@ static int mvneta_port_power_up(struct mvneta_port *pp, 
int phy_mode)
 /* Device initialization routine */
 static int mvneta_probe(struct platform_device *pdev)
 {
-   const struct mbus_dram_target_info *dram_target_info;
struct resource *res;
struct device_node *dn = pdev->dev.of_node;
struct device_node *phy_node;
@@ -4267,13 +4267,13 @@ static int mvneta_probe(struct platform_device *pdev)
 
pp->tx_csum_limit = tx_csum_limit;
 
-   dram_target_info = mv_mbus_dram_info();
+   pp->dram_target_info = mv_mbus_dram_info();
/* Armada3700 requires setting default configuration of Mbus
 * windows, however without using filled mbus_dram_target_info
 * structure.
 */
-   if (dram_target_info || pp->neta_armada3700)
-   mvneta_conf_mbus_windows(pp, dram_target_info);
+   if (pp->dram_target_info || pp->neta_armada3700)
+   mvneta_conf_mbus_windows(pp, pp->dram_target_info);
 
pp->tx_ring_size = MVNETA_MAX_TXD;
pp->rx_ring_size = MVNETA_MAX_RXD;
@@ -4405,6 +4405,57 @@ static int mvneta_remove(struct platform_device *pdev)
return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int mvneta_suspend(struct device *device)
+{
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+
+   if (netif_running(dev))
+   mvneta_stop(dev);
+   netif_device_detach(dev);
+   clk_disable_unprepare(pp->clk_bus);
+   clk_disable_unprepare(pp->clk);
+   return 0;
+}
+
+static int mvneta_resume(struct device *device)
+{
+   struct platform_device *pdev = to_platform_device(device);
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+   int err;
+
+   clk_prepare_enable(pp->clk);
+   clk_prepare_enable(pp->clk_bus);
+   if (pp->dram_target_info || pp->neta_armada3700)
+   mvneta_conf_mbus_windows(pp, pp->dram_target_info);
+   if (pp->bm_priv) {
+   err = mvneta_bm_port_init(pdev, pp);
+   if (err < 0) {
+   dev_info(>dev, "use SW buffer management\n");
+   pp->bm_priv = NULL;
+   }
+   }
+   mvneta_defaults_set(pp);
+   err = mvneta_port_power_up(pp, pp->phy_interface);
+   if (err < 0) {
+   dev_err(device, "can't power up port\n");
+   return err;
+   }
+
+   if (pp->use_inband_status)
+   mvneta_fixed_link_update(pp, dev->phydev);
+
+   netif_device_attach(dev);
+   if (netif_running(dev))
+   mvneta_open(dev);
+   return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(mvneta_pm_ops, mvneta_suspend, mvneta_resume);
+
 static const struct of_device_id mvneta_match[] = {
{ .compatible = "marvell,armada-370-neta" },
{ .compatible = "marvell,armada-xp-neta" },
@@ -4419,6 +4470,7 @@ static int mvneta_remove(struct platform_device *pdev)
.driver = {
.name = MVNETA_DRIVER_NAME,
.of_match_table = mvneta_match,
+   .pm = _pm_ops,
},
 };
 
-- 
1.9.1

[PATCH v3] net: mvneta: support suspend and resume

2017-03-16 Thread Jane Li 
Add basic support for handling suspend and resume.

Signed-off-by: Jane Li 
---
Since v2:
- use SIMPLE_DEV_PM_OPS instead of SET_LATE_SYSTEM_SLEEP_PM_OPS

Since v1:
- add mvneta_conf_mbus_windows() and mvneta_bm_port_init() in mvneta_resume()

 drivers/net/ethernet/marvell/mvneta.c | 60 ---
 1 file changed, 56 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 61dd446..b0fea26 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -431,6 +431,7 @@ struct mvneta_port {
/* Flags for special SoC configurations */
bool neta_armada3700;
u16 rx_offset_correction;
+   const struct mbus_dram_target_info *dram_target_info;
 };
 
 /* The mvneta_tx_desc and mvneta_rx_desc structures describe the
@@ -4118,7 +4119,6 @@ static int mvneta_port_power_up(struct mvneta_port *pp, 
int phy_mode)
 /* Device initialization routine */
 static int mvneta_probe(struct platform_device *pdev)
 {
-   const struct mbus_dram_target_info *dram_target_info;
struct resource *res;
struct device_node *dn = pdev->dev.of_node;
struct device_node *phy_node;
@@ -4267,13 +4267,13 @@ static int mvneta_probe(struct platform_device *pdev)
 
pp->tx_csum_limit = tx_csum_limit;
 
-   dram_target_info = mv_mbus_dram_info();
+   pp->dram_target_info = mv_mbus_dram_info();
/* Armada3700 requires setting default configuration of Mbus
 * windows, however without using filled mbus_dram_target_info
 * structure.
 */
-   if (dram_target_info || pp->neta_armada3700)
-   mvneta_conf_mbus_windows(pp, dram_target_info);
+   if (pp->dram_target_info || pp->neta_armada3700)
+   mvneta_conf_mbus_windows(pp, pp->dram_target_info);
 
pp->tx_ring_size = MVNETA_MAX_TXD;
pp->rx_ring_size = MVNETA_MAX_RXD;
@@ -4405,6 +4405,57 @@ static int mvneta_remove(struct platform_device *pdev)
return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int mvneta_suspend(struct device *device)
+{
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+
+   if (netif_running(dev))
+   mvneta_stop(dev);
+   netif_device_detach(dev);
+   clk_disable_unprepare(pp->clk_bus);
+   clk_disable_unprepare(pp->clk);
+   return 0;
+}
+
+static int mvneta_resume(struct device *device)
+{
+   struct platform_device *pdev = to_platform_device(device);
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+   int err;
+
+   clk_prepare_enable(pp->clk);
+   clk_prepare_enable(pp->clk_bus);
+   if (pp->dram_target_info || pp->neta_armada3700)
+   mvneta_conf_mbus_windows(pp, pp->dram_target_info);
+   if (pp->bm_priv) {
+   err = mvneta_bm_port_init(pdev, pp);
+   if (err < 0) {
+   dev_info(>dev, "use SW buffer management\n");
+   pp->bm_priv = NULL;
+   }
+   }
+   mvneta_defaults_set(pp);
+   err = mvneta_port_power_up(pp, pp->phy_interface);
+   if (err < 0) {
+   dev_err(device, "can't power up port\n");
+   return err;
+   }
+
+   if (pp->use_inband_status)
+   mvneta_fixed_link_update(pp, dev->phydev);
+
+   netif_device_attach(dev);
+   if (netif_running(dev))
+   mvneta_open(dev);
+   return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(mvneta_pm_ops, mvneta_suspend, mvneta_resume);
+
 static const struct of_device_id mvneta_match[] = {
{ .compatible = "marvell,armada-370-neta" },
{ .compatible = "marvell,armada-xp-neta" },
@@ -4419,6 +4470,7 @@ static int mvneta_remove(struct platform_device *pdev)
.driver = {
.name = MVNETA_DRIVER_NAME,
.of_match_table = mvneta_match,
+   .pm = _pm_ops,
},
 };
 
-- 
1.9.1

[PATCH v2] net: mvneta: support suspend and resume

2017-03-15 Thread Jane Li 
Add basic support for handling suspend and resume.

Signed-off-by: Jane Li <j...@marvell.com>
---
Since v1:
- add mvneta_conf_mbus_windows() and mvneta_bm_port_init() in mvneta_resume()

 drivers/net/ethernet/marvell/mvneta.c | 62 ---
 1 file changed, 58 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 61dd446..250017b 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -431,6 +431,7 @@ struct mvneta_port {
/* Flags for special SoC configurations */
bool neta_armada3700;
u16 rx_offset_correction;
+   const struct mbus_dram_target_info *dram_target_info;
 };
 
 /* The mvneta_tx_desc and mvneta_rx_desc structures describe the
@@ -4118,7 +4119,6 @@ static int mvneta_port_power_up(struct mvneta_port *pp, 
int phy_mode)
 /* Device initialization routine */
 static int mvneta_probe(struct platform_device *pdev)
 {
-   const struct mbus_dram_target_info *dram_target_info;
struct resource *res;
struct device_node *dn = pdev->dev.of_node;
struct device_node *phy_node;
@@ -4267,13 +4267,13 @@ static int mvneta_probe(struct platform_device *pdev)
 
pp->tx_csum_limit = tx_csum_limit;
 
-   dram_target_info = mv_mbus_dram_info();
+   pp->dram_target_info = mv_mbus_dram_info();
/* Armada3700 requires setting default configuration of Mbus
 * windows, however without using filled mbus_dram_target_info
 * structure.
 */
-   if (dram_target_info || pp->neta_armada3700)
-   mvneta_conf_mbus_windows(pp, dram_target_info);
+   if (pp->dram_target_info || pp->neta_armada3700)
+   mvneta_conf_mbus_windows(pp, pp->dram_target_info);
 
pp->tx_ring_size = MVNETA_MAX_TXD;
pp->rx_ring_size = MVNETA_MAX_RXD;
@@ -4405,6 +4405,59 @@ static int mvneta_remove(struct platform_device *pdev)
return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int mvneta_suspend(struct device *device)
+{
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+
+   if (netif_running(dev))
+   mvneta_stop(dev);
+   netif_device_detach(dev);
+   clk_disable_unprepare(pp->clk_bus);
+   clk_disable_unprepare(pp->clk);
+   return 0;
+}
+
+static int mvneta_resume(struct device *device)
+{
+   struct platform_device *pdev = to_platform_device(device);
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+   int err;
+
+   clk_prepare_enable(pp->clk);
+   clk_prepare_enable(pp->clk_bus);
+   if (pp->dram_target_info || pp->neta_armada3700)
+   mvneta_conf_mbus_windows(pp, pp->dram_target_info);
+   if (pp->bm_priv) {
+   err = mvneta_bm_port_init(pdev, pp);
+   if (err < 0) {
+   dev_info(>dev, "use SW buffer management\n");
+   pp->bm_priv = NULL;
+   }
+   }
+   mvneta_defaults_set(pp);
+   err = mvneta_port_power_up(pp, pp->phy_interface);
+   if (err < 0) {
+   dev_err(device, "can't power up port\n");
+   return err;
+   }
+
+   if (pp->use_inband_status)
+   mvneta_fixed_link_update(pp, dev->phydev);
+
+   netif_device_attach(dev);
+   if (netif_running(dev))
+   mvneta_open(dev);
+   return 0;
+}
+#endif
+
+static const struct dev_pm_ops mvneta_pm_ops = {
+   SET_LATE_SYSTEM_SLEEP_PM_OPS(mvneta_suspend, mvneta_resume)
+};
+
 static const struct of_device_id mvneta_match[] = {
{ .compatible = "marvell,armada-370-neta" },
{ .compatible = "marvell,armada-xp-neta" },
@@ -4419,6 +4472,7 @@ static int mvneta_remove(struct platform_device *pdev)
.driver = {
.name = MVNETA_DRIVER_NAME,
.of_match_table = mvneta_match,
+   .pm = _pm_ops,
},
 };
 
-- 
1.9.1

[PATCH v2] net: mvneta: support suspend and resume

2017-03-15 Thread Jane Li 
Add basic support for handling suspend and resume.

Signed-off-by: Jane Li 
---
Since v1:
- add mvneta_conf_mbus_windows() and mvneta_bm_port_init() in mvneta_resume()

 drivers/net/ethernet/marvell/mvneta.c | 62 ---
 1 file changed, 58 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 61dd446..250017b 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -431,6 +431,7 @@ struct mvneta_port {
/* Flags for special SoC configurations */
bool neta_armada3700;
u16 rx_offset_correction;
+   const struct mbus_dram_target_info *dram_target_info;
 };
 
 /* The mvneta_tx_desc and mvneta_rx_desc structures describe the
@@ -4118,7 +4119,6 @@ static int mvneta_port_power_up(struct mvneta_port *pp, 
int phy_mode)
 /* Device initialization routine */
 static int mvneta_probe(struct platform_device *pdev)
 {
-   const struct mbus_dram_target_info *dram_target_info;
struct resource *res;
struct device_node *dn = pdev->dev.of_node;
struct device_node *phy_node;
@@ -4267,13 +4267,13 @@ static int mvneta_probe(struct platform_device *pdev)
 
pp->tx_csum_limit = tx_csum_limit;
 
-   dram_target_info = mv_mbus_dram_info();
+   pp->dram_target_info = mv_mbus_dram_info();
/* Armada3700 requires setting default configuration of Mbus
 * windows, however without using filled mbus_dram_target_info
 * structure.
 */
-   if (dram_target_info || pp->neta_armada3700)
-   mvneta_conf_mbus_windows(pp, dram_target_info);
+   if (pp->dram_target_info || pp->neta_armada3700)
+   mvneta_conf_mbus_windows(pp, pp->dram_target_info);
 
pp->tx_ring_size = MVNETA_MAX_TXD;
pp->rx_ring_size = MVNETA_MAX_RXD;
@@ -4405,6 +4405,59 @@ static int mvneta_remove(struct platform_device *pdev)
return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int mvneta_suspend(struct device *device)
+{
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+
+   if (netif_running(dev))
+   mvneta_stop(dev);
+   netif_device_detach(dev);
+   clk_disable_unprepare(pp->clk_bus);
+   clk_disable_unprepare(pp->clk);
+   return 0;
+}
+
+static int mvneta_resume(struct device *device)
+{
+   struct platform_device *pdev = to_platform_device(device);
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+   int err;
+
+   clk_prepare_enable(pp->clk);
+   clk_prepare_enable(pp->clk_bus);
+   if (pp->dram_target_info || pp->neta_armada3700)
+   mvneta_conf_mbus_windows(pp, pp->dram_target_info);
+   if (pp->bm_priv) {
+   err = mvneta_bm_port_init(pdev, pp);
+   if (err < 0) {
+   dev_info(>dev, "use SW buffer management\n");
+   pp->bm_priv = NULL;
+   }
+   }
+   mvneta_defaults_set(pp);
+   err = mvneta_port_power_up(pp, pp->phy_interface);
+   if (err < 0) {
+   dev_err(device, "can't power up port\n");
+   return err;
+   }
+
+   if (pp->use_inband_status)
+   mvneta_fixed_link_update(pp, dev->phydev);
+
+   netif_device_attach(dev);
+   if (netif_running(dev))
+   mvneta_open(dev);
+   return 0;
+}
+#endif
+
+static const struct dev_pm_ops mvneta_pm_ops = {
+   SET_LATE_SYSTEM_SLEEP_PM_OPS(mvneta_suspend, mvneta_resume)
+};
+
 static const struct of_device_id mvneta_match[] = {
{ .compatible = "marvell,armada-370-neta" },
{ .compatible = "marvell,armada-xp-neta" },
@@ -4419,6 +4472,7 @@ static int mvneta_remove(struct platform_device *pdev)
.driver = {
.name = MVNETA_DRIVER_NAME,
.of_match_table = mvneta_match,
+   .pm = _pm_ops,
},
 };
 
-- 
1.9.1

Re: [PATCH] net: mvneta: support suspend and resume

2017-03-15 Thread Jane Li 

Hi Jisheng,


On 2017年03月15日 15:18, Jisheng Zhang wrote:

Hi Jane,

On Wed, 15 Mar 2017 15:08:34 +0800 Jane Li  wrote:


Add basic support for handling suspend and resume.

Signed-off-by: Jane Li <j...@marvell.com>
---
  drivers/net/ethernet/marvell/mvneta.c | 44 +++
  1 file changed, 44 insertions(+)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 61dd446..4f16342 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -4405,6 +4405,49 @@ static int mvneta_remove(struct platform_device *pdev)
return 0;
  }
  
+#ifdef CONFIG_PM_SLEEP

+static int mvneta_suspend(struct device *device)
+{
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+
+   if (netif_running(dev))
+   mvneta_stop(dev);
+   netif_device_detach(dev);
+   clk_disable_unprepare(pp->clk_bus);
+   clk_disable_unprepare(pp->clk);
+   return 0;
+}
+
+static int mvneta_resume(struct device *device)
+{
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+   int err;
+
+   clk_prepare_enable(pp->clk);
+   clk_prepare_enable(pp->clk_bus);

we may miss the necessary registers setting in mvneta_bm_port_init() and
mvneta_conf_mbus_windows(). those registers also need to be restored.


Done. Add them in patch v2.

+   mvneta_defaults_set(pp);

before restore the default setting, is it safer to mvneta_port_disable()?

Thanks,
Jisheng

During suspend, mvneta_port_disable() has been executed as bellow path.
It seems it is not need to do mvneta_port_disable() during resume.

mvneta_suspend() -> mvneta_stop() -> mvneta_stop_dev() -> 
mvneta_port_disable()


Thanks,
Jane

+   err = mvneta_port_power_up(pp, pp->phy_interface);
+   if (err < 0) {
+   dev_err(device, "can't power up port\n");
+   return err;
+   }
+
+   if (pp->use_inband_status)
+   mvneta_fixed_link_update(pp, dev->phydev);
+
+   netif_device_attach(dev);
+   if (netif_running(dev))
+   mvneta_open(dev);
+   return 0;
+}
+#endif
+
+static const struct dev_pm_ops mvneta_pm_ops = {
+   SET_LATE_SYSTEM_SLEEP_PM_OPS(mvneta_suspend, mvneta_resume)
+};
+
  static const struct of_device_id mvneta_match[] = {
{ .compatible = "marvell,armada-370-neta" },
{ .compatible = "marvell,armada-xp-neta" },
@@ -4419,6 +4462,7 @@ static int mvneta_remove(struct platform_device *pdev)
.driver = {
.name = MVNETA_DRIVER_NAME,
.of_match_table = mvneta_match,
+   .pm = _pm_ops,
},
  };

Re: [PATCH] net: mvneta: support suspend and resume

2017-03-15 Thread Jane Li 

Hi Jisheng,


On 2017年03月15日 15:18, Jisheng Zhang wrote:

Hi Jane,

On Wed, 15 Mar 2017 15:08:34 +0800 Jane Li  wrote:


Add basic support for handling suspend and resume.

Signed-off-by: Jane Li 
---
  drivers/net/ethernet/marvell/mvneta.c | 44 +++
  1 file changed, 44 insertions(+)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 61dd446..4f16342 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -4405,6 +4405,49 @@ static int mvneta_remove(struct platform_device *pdev)
return 0;
  }
  
+#ifdef CONFIG_PM_SLEEP

+static int mvneta_suspend(struct device *device)
+{
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+
+   if (netif_running(dev))
+   mvneta_stop(dev);
+   netif_device_detach(dev);
+   clk_disable_unprepare(pp->clk_bus);
+   clk_disable_unprepare(pp->clk);
+   return 0;
+}
+
+static int mvneta_resume(struct device *device)
+{
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+   int err;
+
+   clk_prepare_enable(pp->clk);
+   clk_prepare_enable(pp->clk_bus);

we may miss the necessary registers setting in mvneta_bm_port_init() and
mvneta_conf_mbus_windows(). those registers also need to be restored.


Done. Add them in patch v2.

+   mvneta_defaults_set(pp);

before restore the default setting, is it safer to mvneta_port_disable()?

Thanks,
Jisheng

During suspend, mvneta_port_disable() has been executed as bellow path.
It seems it is not need to do mvneta_port_disable() during resume.

mvneta_suspend() -> mvneta_stop() -> mvneta_stop_dev() -> 
mvneta_port_disable()


Thanks,
Jane

+   err = mvneta_port_power_up(pp, pp->phy_interface);
+   if (err < 0) {
+   dev_err(device, "can't power up port\n");
+   return err;
+   }
+
+   if (pp->use_inband_status)
+   mvneta_fixed_link_update(pp, dev->phydev);
+
+   netif_device_attach(dev);
+   if (netif_running(dev))
+   mvneta_open(dev);
+   return 0;
+}
+#endif
+
+static const struct dev_pm_ops mvneta_pm_ops = {
+   SET_LATE_SYSTEM_SLEEP_PM_OPS(mvneta_suspend, mvneta_resume)
+};
+
  static const struct of_device_id mvneta_match[] = {
{ .compatible = "marvell,armada-370-neta" },
{ .compatible = "marvell,armada-xp-neta" },
@@ -4419,6 +4462,7 @@ static int mvneta_remove(struct platform_device *pdev)
.driver = {
.name = MVNETA_DRIVER_NAME,
.of_match_table = mvneta_match,
+   .pm = _pm_ops,
},
  };

[PATCH] net: mvneta: support suspend and resume

2017-03-15 Thread Jane Li 
Add basic support for handling suspend and resume.

Signed-off-by: Jane Li <j...@marvell.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 44 +++
 1 file changed, 44 insertions(+)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 61dd446..4f16342 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -4405,6 +4405,49 @@ static int mvneta_remove(struct platform_device *pdev)
return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int mvneta_suspend(struct device *device)
+{
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+
+   if (netif_running(dev))
+   mvneta_stop(dev);
+   netif_device_detach(dev);
+   clk_disable_unprepare(pp->clk_bus);
+   clk_disable_unprepare(pp->clk);
+   return 0;
+}
+
+static int mvneta_resume(struct device *device)
+{
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+   int err;
+
+   clk_prepare_enable(pp->clk);
+   clk_prepare_enable(pp->clk_bus);
+   mvneta_defaults_set(pp);
+   err = mvneta_port_power_up(pp, pp->phy_interface);
+   if (err < 0) {
+   dev_err(device, "can't power up port\n");
+   return err;
+   }
+
+   if (pp->use_inband_status)
+   mvneta_fixed_link_update(pp, dev->phydev);
+
+   netif_device_attach(dev);
+   if (netif_running(dev))
+   mvneta_open(dev);
+   return 0;
+}
+#endif
+
+static const struct dev_pm_ops mvneta_pm_ops = {
+   SET_LATE_SYSTEM_SLEEP_PM_OPS(mvneta_suspend, mvneta_resume)
+};
+
 static const struct of_device_id mvneta_match[] = {
{ .compatible = "marvell,armada-370-neta" },
{ .compatible = "marvell,armada-xp-neta" },
@@ -4419,6 +4462,7 @@ static int mvneta_remove(struct platform_device *pdev)
.driver = {
.name = MVNETA_DRIVER_NAME,
.of_match_table = mvneta_match,
+   .pm = _pm_ops,
},
 };
 
-- 
1.9.1

[PATCH] net: mvneta: support suspend and resume

2017-03-15 Thread Jane Li 
Add basic support for handling suspend and resume.

Signed-off-by: Jane Li 
---
 drivers/net/ethernet/marvell/mvneta.c | 44 +++
 1 file changed, 44 insertions(+)

diff --git a/drivers/net/ethernet/marvell/mvneta.c 
b/drivers/net/ethernet/marvell/mvneta.c
index 61dd446..4f16342 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -4405,6 +4405,49 @@ static int mvneta_remove(struct platform_device *pdev)
return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int mvneta_suspend(struct device *device)
+{
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+
+   if (netif_running(dev))
+   mvneta_stop(dev);
+   netif_device_detach(dev);
+   clk_disable_unprepare(pp->clk_bus);
+   clk_disable_unprepare(pp->clk);
+   return 0;
+}
+
+static int mvneta_resume(struct device *device)
+{
+   struct net_device *dev = dev_get_drvdata(device);
+   struct mvneta_port *pp = netdev_priv(dev);
+   int err;
+
+   clk_prepare_enable(pp->clk);
+   clk_prepare_enable(pp->clk_bus);
+   mvneta_defaults_set(pp);
+   err = mvneta_port_power_up(pp, pp->phy_interface);
+   if (err < 0) {
+   dev_err(device, "can't power up port\n");
+   return err;
+   }
+
+   if (pp->use_inband_status)
+   mvneta_fixed_link_update(pp, dev->phydev);
+
+   netif_device_attach(dev);
+   if (netif_running(dev))
+   mvneta_open(dev);
+   return 0;
+}
+#endif
+
+static const struct dev_pm_ops mvneta_pm_ops = {
+   SET_LATE_SYSTEM_SLEEP_PM_OPS(mvneta_suspend, mvneta_resume)
+};
+
 static const struct of_device_id mvneta_match[] = {
{ .compatible = "marvell,armada-370-neta" },
{ .compatible = "marvell,armada-xp-neta" },
@@ -4419,6 +4462,7 @@ static int mvneta_remove(struct platform_device *pdev)
.driver = {
.name = MVNETA_DRIVER_NAME,
.of_match_table = mvneta_match,
+   .pm = _pm_ops,
},
 };
 
-- 
1.9.1

Re: [PATCH] printk: fix one circular lockdep warning about console_lock

2014-03-20 Thread Jane Li 
On 03/19/2014 06:00 PM, Jan Kara wrote:

> On Wed 19-03-14 11:08:08, Jane Li wrote:
>> On 02/12/2014 05:19 AM, Andrew Morton wrote:
>>> On Tue, 11 Feb 2014 14:50:00 +0800  wrote:
>   Umm, I disagree with the patch. What I proposed in my answer to your patch 
> is something like the patch below. Does it fix your problem?
>
>   Honza
>
> From 91497a88c403a7f22e78fee2f69d7413c6e8209f Mon Sep 17 00:00:00 2001
> From: Jan Kara 
> Date: Wed, 19 Mar 2014 10:56:12 +0100
> Subject: [PATCH] printk: Fixup lockdep annotation in console_suspend()
>
> Although console_suspend() releases console_sem, it doesn't tell lockdep 
> about it. That results in the lockdep warning about circular locking when 
> doing the following:
>   enter suspend ->  resume ->  plug-out CPUx (echo 0 > cpux/online)
>
> Fix the problem by telling lockdep we actually released the semaphore in
> console_suspend() and acquired it again in console_resume().
>
> Signed-off-by: Jan Kara 
> ---
>  kernel/printk/printk.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 
> 4dae9cbe9259..e6ada322782b 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1880,6 +1880,7 @@ void suspend_console(void)
>   console_lock();
>   console_suspended = 1;
>   up(_sem);
> + mutex_release(_lock_dep_map, 1, _RET_IP_);
>  }
>  
>  void resume_console(void)
> @@ -1887,6 +1888,7 @@ void resume_console(void)
>   if (!console_suspend_enabled)
>   return;
>   down(_sem);
> + mutex_acquire(_lock_dep_map, 0, 0, _RET_IP_);
>   console_suspended = 0;
>   console_unlock();
>  }
> --
> 1.8.1.4

Oh, yes, this new solution works well. If run same test, no circular lockdep 
warning occurs now. I will update patch v2.

Sorry for misunderstanding your answer before.

Thanks!

Best Regards,

Jane


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] printk: fix one circular lockdep warning about console_lock

2014-03-20 Thread Jane Li 
On 03/19/2014 06:00 PM, Jan Kara wrote:

 On Wed 19-03-14 11:08:08, Jane Li wrote:
 On 02/12/2014 05:19 AM, Andrew Morton wrote:
 On Tue, 11 Feb 2014 14:50:00 +0800j...@marvell.com  wrote:
   Umm, I disagree with the patch. What I proposed in my answer to your patch 
 is something like the patch below. Does it fix your problem?

   Honza

 From 91497a88c403a7f22e78fee2f69d7413c6e8209f Mon Sep 17 00:00:00 2001
 From: Jan Kara j...@suse.cz
 Date: Wed, 19 Mar 2014 10:56:12 +0100
 Subject: [PATCH] printk: Fixup lockdep annotation in console_suspend()

 Although console_suspend() releases console_sem, it doesn't tell lockdep 
 about it. That results in the lockdep warning about circular locking when 
 doing the following:
   enter suspend -  resume -  plug-out CPUx (echo 0  cpux/online)

 Fix the problem by telling lockdep we actually released the semaphore in
 console_suspend() and acquired it again in console_resume().

 Signed-off-by: Jan Kara j...@suse.cz
 ---
  kernel/printk/printk.c | 2 ++
  1 file changed, 2 insertions(+)

 diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 
 4dae9cbe9259..e6ada322782b 100644
 --- a/kernel/printk/printk.c
 +++ b/kernel/printk/printk.c
 @@ -1880,6 +1880,7 @@ void suspend_console(void)
   console_lock();
   console_suspended = 1;
   up(console_sem);
 + mutex_release(console_lock_dep_map, 1, _RET_IP_);
  }
  
  void resume_console(void)
 @@ -1887,6 +1888,7 @@ void resume_console(void)
   if (!console_suspend_enabled)
   return;
   down(console_sem);
 + mutex_acquire(console_lock_dep_map, 0, 0, _RET_IP_);
   console_suspended = 0;
   console_unlock();
  }
 --
 1.8.1.4

Oh, yes, this new solution works well. If run same test, no circular lockdep 
warning occurs now. I will update patch v2.

Sorry for misunderstanding your answer before.

Thanks!

Best Regards,

Jane


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] printk: fix one circular lockdep warning about console_lock

2014-03-18 Thread Jane Li 

On 02/12/2014 05:19 AM, Andrew Morton wrote:


On Tue, 11 Feb 2014 14:50:00 +0800  wrote:


From: Jane Li

This patch tries to fix a warning about possible circular locking
dependency.

If do in following sequence:
 enter suspend ->  resume ->  plug-out CPUx (echo 0 > cpux/online)
lockdep will show warning as following:

==
[ INFO: possible circular locking dependency detected ]
3.10.0 #2 Tainted: G   O
---
sh/1271 is trying to acquire lock:
(console_lock){+.+.+.}, at: [] console_cpu_notify+0x20/0x2c
but task is already holding lock:
(cpu_hotplug.lock){+.+.+.}, at: [] cpu_hotplug_begin+0x2c/0x58
which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:
-> #2 (cpu_hotplug.lock){+.+.+.}:
[] lock_acquire+0x98/0x12c
[] mutex_lock_nested+0x50/0x3d8
[] cpu_hotplug_begin+0x2c/0x58
[] _cpu_up+0x24/0x154
[] cpu_up+0x64/0x84
[] smp_init+0x9c/0xd4
[] kernel_init_freeable+0x78/0x1c8
[] kernel_init+0x8/0xe4
[] ret_from_fork+0x14/0x2c

-> #1 (cpu_add_remove_lock){+.+.+.}:
[] lock_acquire+0x98/0x12c
[] mutex_lock_nested+0x50/0x3d8
[] disable_nonboot_cpus+0x8/0xe8
[] suspend_devices_and_enter+0x214/0x448
[] pm_suspend+0x1e4/0x284
[] try_to_suspend+0xa4/0xbc
[] process_one_work+0x1c4/0x4fc
[] worker_thread+0x138/0x37c
[] kthread+0xa4/0xb0
[] ret_from_fork+0x14/0x2c

-> #0 (console_lock){+.+.+.}:
[] __lock_acquire+0x1b38/0x1b80
[] lock_acquire+0x98/0x12c
[] console_lock+0x54/0x68
[] console_cpu_notify+0x20/0x2c
[] notifier_call_chain+0x44/0x84
[] __cpu_notify+0x2c/0x48
[] cpu_notify_nofail+0x8/0x14
[] _cpu_down+0xf4/0x258
[] cpu_down+0x24/0x40
[] store_online+0x30/0x74
[] dev_attr_store+0x18/0x24
[] sysfs_write_file+0x16c/0x19c
[] vfs_write+0xb4/0x190
[] SyS_write+0x3c/0x70
[] ret_fast_syscall+0x0/0x48

Chain exists of:
console_lock --> cpu_add_remove_lock --> cpu_hotplug.lock

Possible unsafe locking scenario:
CPU0CPU1

lock(cpu_hotplug.lock);
lock(cpu_add_remove_lock);
lock(cpu_hotplug.lock);
lock(console_lock);
   *** DEADLOCK ***

These traces hurt my brain.


There are three locks involved in two sequence:
a) pm suspend:
console_lock (@suspend_console())
cpu_add_remove_lock (@disable_nonboot_cpus())
cpu_hotplug.lock (@_cpu_down())

But but but.  suspend_console() releases console_sem again.  So the
sequence is actually

down(_sem) (@suspend_console())
up(_sem) (@suspend_console())
cpu_add_remove_lock (@disable_nonboot_cpus())
cpu_hotplug.lock (@_cpu_down())

So console_sem *doesn't* nest outside cpu_add_remove_lock and
cpu_hotplug.lock.


 Jan Kara and Jane have answered this question in other emails.


b) Plug-out CPUx:
cpu_add_remove_lock (@(cpu_down())
cpu_hotplug.lock (@_cpu_down())
console_lock (@console_cpu_notify()) => Lockdeps prints warning log.

There should be not real deadlock, as flag of console_suspended can
protect this.

console_lock() does down(_sem) *before* testing
console_suspended, so I don't understand this sentence - a more
detailed description would help.


Jane has answered this question in another email.


Printk registers cpu hotplug notify function. When CPUx is plug-out/in,
always execute console_lock() and console_unlock(). This patch
modifies that with console_trylock() and console_unlock(). Then use
that instead of the unconditional console_lock/unlock pair to avoid the
warning.

...

--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1893,6 +1893,20 @@ void resume_console(void)
  }
  
  /**

+ * console_flush - flush dmesg if console isn't suspended
+ *
+ * console_unlock always flushes the dmesg buffer, so just try to
+ * grab the console lock. If that fails we know that the current
+ * holder will eventually drop the console lock and so flush the dmesg
+ * buffers at the earliest possible time.
+ */

The comment should describe why we added this code, please: talk about
cpu_hotplug.lock and console_lock.


Daniel has answered this question in another email.


+void console_flush(void)
+{
+   if (console_trylock())
+   console_unlock();
+}
+
+/**
   * console_cpu_notify - print deferred console messages after CPU hotplug
   * @self: notifier struct
   * @action: CPU hotplug event
@@ -1911,8 +1925,7 @@ static int console_cpu_notify(struct notifier_block *self,
case CPU_DEAD:
case CPU_DOWN_FAILED:
case CPU_UP_CANCELED:
-   console_lock();
-   console_unlock();
+   console_flush();
}
return NOTIFY_OK;

Well, this is a bit hacky and makes the already-far-too-complex code
even more complex.  If it is indeed the case that the deadlock cannot
really occur then let's try to find a way of suppressing

Re: [PATCH] printk: fix one circular lockdep warning about console_lock

2014-03-18 Thread Jane Li 

On 02/12/2014 05:19 AM, Andrew Morton wrote:


On Tue, 11 Feb 2014 14:50:00 +0800j...@marvell.com  wrote:


From: Jane Lij...@marvell.com

This patch tries to fix a warning about possible circular locking
dependency.

If do in following sequence:
 enter suspend -  resume -  plug-out CPUx (echo 0  cpux/online)
lockdep will show warning as following:

==
[ INFO: possible circular locking dependency detected ]
3.10.0 #2 Tainted: G   O
---
sh/1271 is trying to acquire lock:
(console_lock){+.+.+.}, at: [c06ebf7c] console_cpu_notify+0x20/0x2c
but task is already holding lock:
(cpu_hotplug.lock){+.+.+.}, at: [c012b4e8] cpu_hotplug_begin+0x2c/0x58
which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:
- #2 (cpu_hotplug.lock){+.+.+.}:
[c017bb7c] lock_acquire+0x98/0x12c
[c06f5014] mutex_lock_nested+0x50/0x3d8
[c012b4e8] cpu_hotplug_begin+0x2c/0x58
[c06ebfac] _cpu_up+0x24/0x154
[c06ec140] cpu_up+0x64/0x84
[c0981834] smp_init+0x9c/0xd4
[c0973880] kernel_init_freeable+0x78/0x1c8
[c06e7f40] kernel_init+0x8/0xe4
[c010eec8] ret_from_fork+0x14/0x2c

- #1 (cpu_add_remove_lock){+.+.+.}:
[c017bb7c] lock_acquire+0x98/0x12c
[c06f5014] mutex_lock_nested+0x50/0x3d8
[c012b758] disable_nonboot_cpus+0x8/0xe8
[c016b83c] suspend_devices_and_enter+0x214/0x448
[c016bc54] pm_suspend+0x1e4/0x284
[c016bdcc] try_to_suspend+0xa4/0xbc
[c0143848] process_one_work+0x1c4/0x4fc
[c0143f80] worker_thread+0x138/0x37c
[c014aaf8] kthread+0xa4/0xb0
[c010eec8] ret_from_fork+0x14/0x2c

- #0 (console_lock){+.+.+.}:
[c017b5d0] __lock_acquire+0x1b38/0x1b80
[c017bb7c] lock_acquire+0x98/0x12c
[c01288c4] console_lock+0x54/0x68
[c06ebf7c] console_cpu_notify+0x20/0x2c
[c01501d4] notifier_call_chain+0x44/0x84
[c012b448] __cpu_notify+0x2c/0x48
[c012b5b0] cpu_notify_nofail+0x8/0x14
[c06e81bc] _cpu_down+0xf4/0x258
[c06e8344] cpu_down+0x24/0x40
[c06e921c] store_online+0x30/0x74
[c03b7298] dev_attr_store+0x18/0x24
[c025fc5c] sysfs_write_file+0x16c/0x19c
[c0207a98] vfs_write+0xb4/0x190
[c0207e58] SyS_write+0x3c/0x70
[c010ee00] ret_fast_syscall+0x0/0x48

Chain exists of:
console_lock -- cpu_add_remove_lock -- cpu_hotplug.lock

Possible unsafe locking scenario:
CPU0CPU1

lock(cpu_hotplug.lock);
lock(cpu_add_remove_lock);
lock(cpu_hotplug.lock);
lock(console_lock);
   *** DEADLOCK ***

These traces hurt my brain.


There are three locks involved in two sequence:
a) pm suspend:
console_lock (@suspend_console())
cpu_add_remove_lock (@disable_nonboot_cpus())
cpu_hotplug.lock (@_cpu_down())

But but but.  suspend_console() releases console_sem again.  So the
sequence is actually

down(console_sem) (@suspend_console())
up(console_sem) (@suspend_console())
cpu_add_remove_lock (@disable_nonboot_cpus())
cpu_hotplug.lock (@_cpu_down())

So console_sem *doesn't* nest outside cpu_add_remove_lock and
cpu_hotplug.lock.


 Jan Kara and Jane have answered this question in other emails.


b) Plug-out CPUx:
cpu_add_remove_lock (@(cpu_down())
cpu_hotplug.lock (@_cpu_down())
console_lock (@console_cpu_notify()) = Lockdeps prints warning log.

There should be not real deadlock, as flag of console_suspended can
protect this.

console_lock() does down(console_sem) *before* testing
console_suspended, so I don't understand this sentence - a more
detailed description would help.


Jane has answered this question in another email.


Printk registers cpu hotplug notify function. When CPUx is plug-out/in,
always execute console_lock() and console_unlock(). This patch
modifies that with console_trylock() and console_unlock(). Then use
that instead of the unconditional console_lock/unlock pair to avoid the
warning.

...

--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1893,6 +1893,20 @@ void resume_console(void)
  }
  
  /**

+ * console_flush - flush dmesg if console isn't suspended
+ *
+ * console_unlock always flushes the dmesg buffer, so just try to
+ * grabdrop the console lock. If that fails we know that the current
+ * holder will eventually drop the console lock and so flush the dmesg
+ * buffers at the earliest possible time.
+ */

The comment should describe why we added this code, please: talk about
cpu_hotplug.lock and console_lock.


Daniel has answered this question in another email.


+void console_flush(void)
+{
+   if (console_trylock())
+   console_unlock();
+}
+
+/**
   * console_cpu_notify - print deferred console messages after CPU hotplug
   * @self: notifier struct
   * @action: CPU hotplug event
@@ -1911,8 +1925,7 @@ static int console_cpu_notify(struct notifier_block *self,
case CPU_DEAD:
case CPU_DOWN_FAILED:
case CPU_UP_CANCELED:
-

Re: [PATCH] printk: fix one circular lockdep warning about console_lock

2014-02-13 Thread Jane Li 

On 02/12/2014 05:19 AM, Andrew Morton wrote:


There are three locks involved in two sequence:
a) pm suspend:
console_lock (@suspend_console())
cpu_add_remove_lock (@disable_nonboot_cpus())
cpu_hotplug.lock (@_cpu_down())

But but but.  suspend_console() releases console_sem again.


Console_lock does not refer to console_sem but console_lock_dep_map. Its name 
is console_lock. Suspend_console() does not release console_lock_dep_map.


So the
sequence is actually

down(_sem) (@suspend_console())


   acquire(_lock_dep_map) (_console())


up(_sem) (@suspend_console())
cpu_add_remove_lock (@disable_nonboot_cpus())
cpu_hotplug.lock (@_cpu_down())

So console_sem *doesn't* nest outside cpu_add_remove_lock and
cpu_hotplug.lock.


Add console_lock in the sequence.




b) Plug-out CPUx:
cpu_add_remove_lock (@(cpu_down())
cpu_hotplug.lock (@_cpu_down())
console_lock (@console_cpu_notify()) => Lockdeps prints warning log.

There should be not real deadlock, as flag of console_suspended can
protect this.

console_lock() does down(_sem) *before* testing
console_suspended, so I don't understand this sentence - a more
detailed description would help.


After suspend_console(), console_sem is unlocked, but console_lock_dep_map has 
been acquired.


Best Regards,
Jane

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] printk: fix one circular lockdep warning about console_lock

2014-02-13 Thread Jane Li 

On 02/12/2014 05:19 AM, Andrew Morton wrote:


There are three locks involved in two sequence:
a) pm suspend:
console_lock (@suspend_console())
cpu_add_remove_lock (@disable_nonboot_cpus())
cpu_hotplug.lock (@_cpu_down())

But but but.  suspend_console() releases console_sem again.


Console_lock does not refer to console_sem but console_lock_dep_map. Its name 
is console_lock. Suspend_console() does not release console_lock_dep_map.


So the
sequence is actually

down(console_sem) (@suspend_console())


   acquire(console_lock_dep_map) (suspend_console())


up(console_sem) (@suspend_console())
cpu_add_remove_lock (@disable_nonboot_cpus())
cpu_hotplug.lock (@_cpu_down())

So console_sem *doesn't* nest outside cpu_add_remove_lock and
cpu_hotplug.lock.


Add console_lock in the sequence.




b) Plug-out CPUx:
cpu_add_remove_lock (@(cpu_down())
cpu_hotplug.lock (@_cpu_down())
console_lock (@console_cpu_notify()) = Lockdeps prints warning log.

There should be not real deadlock, as flag of console_suspended can
protect this.

console_lock() does down(console_sem) *before* testing
console_suspended, so I don't understand this sentence - a more
detailed description would help.


After suspend_console(), console_sem is unlocked, but console_lock_dep_map has 
been acquired.


Best Regards,
Jane

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Question about console_lock lockdep after involving console_lock_dep_map

2014-02-10 Thread Jane Li 


On 02/09/2014 11:45 PM, Daniel Vetter wrote:

Adding many more people and lkml to the cc list. Please don't poke people
in private, but always cc a relevant mailing list.

On Sat, Feb 8, 2014 at 6:24 AM, Jane Li  wrote:

Hi Danial Vetter,

I found you had added console_lock_dep_map in commit daee7797 (console:
implement lockdep support for console_lock). I encounter another circular
lock warning related to it.

Sequence:

 enter suspend ->  resume ->  plug-out CPUx (echo 0 > cpux/online)

Then, lockdep will show warning as following:

==
[ INFO: possible circular locking dependency detected ]
3.10.0 #2 Tainted: G   O
---
sh/1271 is trying to acquire lock:
  (console_lock){+.+.+.}, at: [] console_cpu_notify+0x20/0x2c
but task is already holding lock:
(cpu_hotplug.lock){+.+.+.}, at: [] cpu_hotplug_begin+0x2c/0x58
which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:
-> #2 (cpu_hotplug.lock){+.+.+.}:
[] lock_acquire+0x98/0x12c
[] mutex_lock_nested+0x50/0x3d8
[] cpu_hotplug_begin+0x2c/0x58
[] _cpu_up+0x24/0x154
[] cpu_up+0x64/0x84
[] smp_init+0x9c/0xd4
[] kernel_init_freeable+0x78/0x1c8
[] kernel_init+0x8/0xe4
[] ret_from_fork+0x14/0x2c

-> #1 (cpu_add_remove_lock){+.+.+.}:
[] lock_acquire+0x98/0x12c
[] mutex_lock_nested+0x50/0x3d8
[] disable_nonboot_cpus+0x8/0xe8
[] suspend_devices_and_enter+0x214/0x448
[] pm_suspend+0x1e4/0x284
[] try_to_suspend+0xa4/0xbc
[] process_one_work+0x1c4/0x4fc
[] worker_thread+0x138/0x37c
[] kthread+0xa4/0xb0
[] ret_from_fork+0x14/0x2c

-> #0 (console_lock){+.+.+.}:
[] __lock_acquire+0x1b38/0x1b80
[] lock_acquire+0x98/0x12c
[] console_lock+0x54/0x68
[] console_cpu_notify+0x20/0x2c
[] notifier_call_chain+0x44/0x84
[] __cpu_notify+0x2c/0x48
[] cpu_notify_nofail+0x8/0x14
[] _cpu_down+0xf4/0x258
[] cpu_down+0x24/0x40
[] store_online+0x30/0x74
[] dev_attr_store+0x18/0x24
[] sysfs_write_file+0x16c/0x19c
[] vfs_write+0xb4/0x190
[] SyS_write+0x3c/0x70
[] ret_fastChain exists of:
   console_lock --> cpu_add_remove_lock --> cpu_hotplug.lock

Possible unsafe locking scenario:
   CPU0CPU1
   
lock(cpu_hotplug.lock);
   lock(cpu_add_remove_lock);
   lock(cpu_hotplug.lock);
lock(console_lock);
  *** DEADLOCK ***



Analyze this information, there are three locks involved in two sequence:

 pm suspend: console_lock (@suspend_console()) -> cpu_add_remove_lock
(@disable_nonboot_cpus()) -> cpu_hotplug.lock (@_cpu_down())

 Plug-out CPUx: cpu_add_remove_lock (@(cpu_down()) ->
cpu_hotplug.lock (@_cpu_down()) -> console_lock (@console_cpu_notify()) =>
Lockdeps prints warning log.


I check code and there should be not real deadlock, as flag of
console_suspended can protect this.

Do you know how to avoid this warning?

I think the right approach here is to add a new function to do the console
flushing:

/**
  * console_flush - flush dmesg if console isn't suspended
  *
  * console_unlock always flushes the dmesg buffer, so just try to
  * grab the console lock. If that fails we know that the current
  * holder will eventually drop the console lock and so flush the dmesg
  * buffers at the earliest possible time.
  */
void console_flush(void)
{
if (console_trylock())
console_unlock();
}

Then use that instead of the unconditional console_lock/unlock pair int
the console_cpu_notitifier. Since that's practically the patch already
feel free to smash a Signed-off-by: Daniel Vetter 
on top if it works.

Cheers, Daniel


Do same test as you suggested, there is no warning now.

I have updated the patch named "printk: fix one circular lockdep warning about 
console_lock".

Thanks!

Best Regards,
Jane

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Question about console_lock lockdep after involving console_lock_dep_map

2014-02-10 Thread Jane Li 


On 02/09/2014 11:45 PM, Daniel Vetter wrote:

Adding many more people and lkml to the cc list. Please don't poke people
in private, but always cc a relevant mailing list.

On Sat, Feb 8, 2014 at 6:24 AM, Jane Li j...@marvell.com wrote:

Hi Danial Vetter,

I found you had added console_lock_dep_map in commit daee7797 (console:
implement lockdep support for console_lock). I encounter another circular
lock warning related to it.

Sequence:

 enter suspend -  resume -  plug-out CPUx (echo 0  cpux/online)

Then, lockdep will show warning as following:

==
[ INFO: possible circular locking dependency detected ]
3.10.0 #2 Tainted: G   O
---
sh/1271 is trying to acquire lock:
  (console_lock){+.+.+.}, at: [c06ebf7c] console_cpu_notify+0x20/0x2c
but task is already holding lock:
(cpu_hotplug.lock){+.+.+.}, at: [c012b4e8] cpu_hotplug_begin+0x2c/0x58
which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:
- #2 (cpu_hotplug.lock){+.+.+.}:
[c017bb7c] lock_acquire+0x98/0x12c
[c06f5014] mutex_lock_nested+0x50/0x3d8
[c012b4e8] cpu_hotplug_begin+0x2c/0x58
[c06ebfac] _cpu_up+0x24/0x154
[c06ec140] cpu_up+0x64/0x84
[c0981834] smp_init+0x9c/0xd4
[c0973880] kernel_init_freeable+0x78/0x1c8
[c06e7f40] kernel_init+0x8/0xe4
[c010eec8] ret_from_fork+0x14/0x2c

- #1 (cpu_add_remove_lock){+.+.+.}:
[c017bb7c] lock_acquire+0x98/0x12c
[c06f5014] mutex_lock_nested+0x50/0x3d8
[c012b758] disable_nonboot_cpus+0x8/0xe8
[c016b83c] suspend_devices_and_enter+0x214/0x448
[c016bc54] pm_suspend+0x1e4/0x284
[c016bdcc] try_to_suspend+0xa4/0xbc
[c0143848] process_one_work+0x1c4/0x4fc
[c0143f80] worker_thread+0x138/0x37c
[c014aaf8] kthread+0xa4/0xb0
[c010eec8] ret_from_fork+0x14/0x2c

- #0 (console_lock){+.+.+.}:
[c017b5d0] __lock_acquire+0x1b38/0x1b80
[c017bb7c] lock_acquire+0x98/0x12c
[c01288c4] console_lock+0x54/0x68
[c06ebf7c] console_cpu_notify+0x20/0x2c
[c01501d4] notifier_call_chain+0x44/0x84
[c012b448] __cpu_notify+0x2c/0x48
[c012b5b0] cpu_notify_nofail+0x8/0x14
[c06e81bc] _cpu_down+0xf4/0x258
[c06e8344] cpu_down+0x24/0x40
[c06e921c] store_online+0x30/0x74
[c03b7298] dev_attr_store+0x18/0x24
[c025fc5c] sysfs_write_file+0x16c/0x19c
[c0207a98] vfs_write+0xb4/0x190
[c0207e58] SyS_write+0x3c/0x70
[c010ee00] ret_fastChain exists of:
   console_lock -- cpu_add_remove_lock -- cpu_hotplug.lock

Possible unsafe locking scenario:
   CPU0CPU1
   
lock(cpu_hotplug.lock);
   lock(cpu_add_remove_lock);
   lock(cpu_hotplug.lock);
lock(console_lock);
  *** DEADLOCK ***



Analyze this information, there are three locks involved in two sequence:

 pm suspend: console_lock (@suspend_console()) - cpu_add_remove_lock
(@disable_nonboot_cpus()) - cpu_hotplug.lock (@_cpu_down())

 Plug-out CPUx: cpu_add_remove_lock (@(cpu_down()) -
cpu_hotplug.lock (@_cpu_down()) - console_lock (@console_cpu_notify()) =
Lockdeps prints warning log.


I check code and there should be not real deadlock, as flag of
console_suspended can protect this.

Do you know how to avoid this warning?

I think the right approach here is to add a new function to do the console
flushing:

/**
  * console_flush - flush dmesg if console isn't suspended
  *
  * console_unlock always flushes the dmesg buffer, so just try to
  * grabdrop the console lock. If that fails we know that the current
  * holder will eventually drop the console lock and so flush the dmesg
  * buffers at the earliest possible time.
  */
void console_flush(void)
{
if (console_trylock())
console_unlock();
}

Then use that instead of the unconditional console_lock/unlock pair int
the console_cpu_notitifier. Since that's practically the patch already
feel free to smash a Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
on top if it works.

Cheers, Daniel


Do same test as you suggested, there is no warning now.

I have updated the patch named printk: fix one circular lockdep warning about 
console_lock.

Thanks!

Best Regards,
Jane

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] cpufreq: Fix timer/workqueue corruption by protecting reading governor_enabled

2014-01-03 Thread Jane Li 


On 01/03/2014 02:50 PM, Viresh Kumar wrote:

On 3 January 2014 12:14,   wrote:

diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index dc196bb..15c62df 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -389,6 +389,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
unsigned int relation);
  int cpufreq_register_governor(struct cpufreq_governor *governor);
  void cpufreq_unregister_governor(struct cpufreq_governor *governor);
+extern struct mutex cpufreq_governor_lock;

  /* CPUFREQ DEFAULT GOVERNOR */
  /*

Move this to cpufreq_governor.h instead. I don't want this to be available
for everybody to use it.


OK. Have pushed PATCH v4. Please review again.

Thanks!

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] cpufreq: Fix timer/workqueue corruption by protecting reading governor_enabled

2014-01-03 Thread Jane Li 


On 01/03/2014 02:50 PM, Viresh Kumar wrote:

On 3 January 2014 12:14,  j...@marvell.com wrote:

diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index dc196bb..15c62df 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -389,6 +389,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
unsigned int relation);
  int cpufreq_register_governor(struct cpufreq_governor *governor);
  void cpufreq_unregister_governor(struct cpufreq_governor *governor);
+extern struct mutex cpufreq_governor_lock;

  /* CPUFREQ DEFAULT GOVERNOR */
  /*

Move this to cpufreq_governor.h instead. I don't want this to be available
for everybody to use it.


OK. Have pushed PATCH v4. Please review again.

Thanks!

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] cpufreq: Fix timer/workqueue corruption by protecting reading governor_enabled

2014-01-02 Thread Jane Li 


On 01/03/2014 07:26 AM, Dmitry Torokhov wrote

Unlocking in different branches is not the best practice IMO, I'd
recommend doing:

mutex_lock(_governor_lock);

if (!policy->governor_enabled)
goto out_unlock;

...

out_unlock:
mutex_unlock(_governor_lock);

Thanks!


OK. I have pushed PATCH v3. Please review again.

Besides, I use checkpatch.pl to check this patch, and find there is warning. 
PATCH v3 also move cpufreq_governor_lock declaration to cpufreq.h.

WARNING: externs should be avoided in .c files
#106: FILE: drivers/cpufreq/cpufreq_governor.c:25:
+extern struct mutex cpufreq_governor_lock;

Thanks!

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] cpufreq: Fix timer/workqueue corruption by protecting reading governor_enabled

2014-01-02 Thread Jane Li 


On 01/03/2014 07:26 AM, Dmitry Torokhov wrote

Unlocking in different branches is not the best practice IMO, I'd
recommend doing:

mutex_lock(cpufreq_governor_lock);

if (!policy-governor_enabled)
goto out_unlock;

...

out_unlock:
mutex_unlock(cpufreq_governor_lock);

Thanks!


OK. I have pushed PATCH v3. Please review again.

Besides, I use checkpatch.pl to check this patch, and find there is warning. 
PATCH v3 also move cpufreq_governor_lock declaration to cpufreq.h.

WARNING: externs should be avoided in .c files
#106: FILE: drivers/cpufreq/cpufreq_governor.c:25:
+extern struct mutex cpufreq_governor_lock;

Thanks!

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] cpufreq: Fix timer/workqueue corruption by protecting reading governor_enabled

2014-01-01 Thread Jane Li 

Yes, I test it. After adding cpufreq_governor_lock in gov_queue_work() and
running same test, there is no debugobjects warning.

But it really can't work at all.. There should be a separate copy of lock
in every file that includes cpufreq.h.. And so this shouldn't have worked.


Oh.. I understand what you mean now. My patch is not right and cannot fix this 
issue.

By default, the debugobjects warning sometimes occurs after five minutes, and 
sometimes occurs after twenty hours. With this patch, I test more than fifty 
hours, and warning did not occurs. It shows that my test time is not long 
enough and miss the right one.


I have updated PATCH v2, please review again. Thanks.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] cpufreq: Fix timer/workqueue corruption by protecting reading governor_enabled

2014-01-01 Thread Jane Li 

Yes, I test it. After adding cpufreq_governor_lock in gov_queue_work() and
running same test, there is no debugobjects warning.

But it really can't work at all.. There should be a separate copy of lock
in every file that includes cpufreq.h.. And so this shouldn't have worked.


Oh.. I understand what you mean now. My patch is not right and cannot fix this 
issue.

By default, the debugobjects warning sometimes occurs after five minutes, and 
sometimes occurs after twenty hours. With this patch, I test more than fifty 
hours, and warning did not occurs. It shows that my test time is not long 
enough and miss the right one.


I have updated PATCH v2, please review again. Thanks.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] cpufreq: Fix timer/workqueue corruption by protecting reading governor_enabled

2013-12-29 Thread Jane Li 


On 12/27/2013 05:40 PM, Viresh Kumar wrote:

On 27 December 2013 15:00,   wrote:

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
-static DEFINE_MUTEX(cpufreq_governor_lock);
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index dc196bb..4faafe7 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -254,6 +254,7 @@ struct cpufreq_driver {

  int cpufreq_register_driver(struct cpufreq_driver *driver_data);
  int cpufreq_unregister_driver(struct cpufreq_driver *driver_data);
+static DEFINE_MUTEX(cpufreq_governor_lock);

No way, this would never work. This would create separate locks
in each file that includes cpufreq.h. And so the locks you are talking
about wouldn't protect governor.


I checked my code, and found I lost following part.
My local code base is not exactly same as open source, and has included this.


diff --git a/drivers/cpufreq/cpufreq_governor.c 
b/drivers/cpufreq/cpufreq_governor.c
index e6be635..c4d0ee6 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -19,6 +19,7 @@
 #include 
 #include 
 #include 
+#include 

 #include "cpufreq_governor.h"


Have you actually tested this code? If this fixes the breakage you
saw? If this fixes it then you need to do better investigation of your
problem.


Yes, I test it. After adding cpufreq_governor_lock in gov_queue_work() and 
running same test, there is no debugobjects warning.


you actually need to remove the static keyword from cpufreq.c file.
Nothing else.


Modify cpufreq_governor_lock definition. Do you think it is OK?

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 16d7b4a..185c9f5 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -39,7 +39,7 @@ static struct cpufreq_driver *cpufreq_driver;
 static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
 static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data_fallback);
 static DEFINE_RWLOCK(cpufreq_driver_lock);
-static DEFINE_MUTEX(cpufreq_governor_lock);
+DEFINE_MUTEX(cpufreq_governor_lock);
 static LIST_HEAD(cpufreq_policy_list);

 #ifdef CONFIG_HOTPLUG_CPU
diff --git a/drivers/cpufreq/cpufreq_governor.c 
b/drivers/cpufreq/cpufreq_governor.c
index e6be635..485ee0d 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -22,6 +22,8 @@

 #include "cpufreq_governor.h"

+extern struct mutex cpufreq_governor_lock;
+
 static struct attribute_group *get_sysfs_attr(struct dbs_data *dbs_data)
 {
if (have_governor_per_policy())

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] cpufreq: Fix timer/workqueue corruption by protecting reading governor_enabled

2013-12-29 Thread Jane Li 


On 12/27/2013 05:40 PM, Viresh Kumar wrote:

On 27 December 2013 15:00,  j...@marvell.com wrote:

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
-static DEFINE_MUTEX(cpufreq_governor_lock);
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index dc196bb..4faafe7 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -254,6 +254,7 @@ struct cpufreq_driver {

  int cpufreq_register_driver(struct cpufreq_driver *driver_data);
  int cpufreq_unregister_driver(struct cpufreq_driver *driver_data);
+static DEFINE_MUTEX(cpufreq_governor_lock);

No way, this would never work. This would create separate locks
in each file that includes cpufreq.h. And so the locks you are talking
about wouldn't protect governor.


I checked my code, and found I lost following part.
My local code base is not exactly same as open source, and has included this.


diff --git a/drivers/cpufreq/cpufreq_governor.c 
b/drivers/cpufreq/cpufreq_governor.c
index e6be635..c4d0ee6 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -19,6 +19,7 @@
 #include linux/export.h
 #include linux/kernel_stat.h
 #include linux/slab.h
+#include linux/cpufreq.h

 #include cpufreq_governor.h


Have you actually tested this code? If this fixes the breakage you
saw? If this fixes it then you need to do better investigation of your
problem.


Yes, I test it. After adding cpufreq_governor_lock in gov_queue_work() and 
running same test, there is no debugobjects warning.


you actually need to remove the static keyword from cpufreq.c file.
Nothing else.


Modify cpufreq_governor_lock definition. Do you think it is OK?

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 16d7b4a..185c9f5 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -39,7 +39,7 @@ static struct cpufreq_driver *cpufreq_driver;
 static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
 static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data_fallback);
 static DEFINE_RWLOCK(cpufreq_driver_lock);
-static DEFINE_MUTEX(cpufreq_governor_lock);
+DEFINE_MUTEX(cpufreq_governor_lock);
 static LIST_HEAD(cpufreq_policy_list);

 #ifdef CONFIG_HOTPLUG_CPU
diff --git a/drivers/cpufreq/cpufreq_governor.c 
b/drivers/cpufreq/cpufreq_governor.c
index e6be635..485ee0d 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -22,6 +22,8 @@

 #include cpufreq_governor.h

+extern struct mutex cpufreq_governor_lock;
+
 static struct attribute_group *get_sysfs_attr(struct dbs_data *dbs_data)
 {
if (have_governor_per_policy())

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/