* [PATCH 0/2] net: mvneta: improve suspend/resume @ 2018-03-29 10:12 ` Jisheng Zhang 0 siblings, 0 replies; 16+ messages in thread From: Jisheng Zhang @ 2018-03-29 10:12 UTC (permalink / raw) To: linux-arm-kernel This series tries to optimize the mvneta's suspend/resume implementation by only taking necessary actions. Jisheng Zhang (2): net: mvneta: split rxq/txq init into SW and HW parts net: mvneta: improve suspend/resume drivers/net/ethernet/marvell/mvneta.c | 146 +++++++++++++++++++++++++++------- 1 file changed, 119 insertions(+), 27 deletions(-) -- 2.16.3 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 0/2] net: mvneta: improve suspend/resume @ 2018-03-29 10:12 ` Jisheng Zhang 0 siblings, 0 replies; 16+ messages in thread From: Jisheng Zhang @ 2018-03-29 10:12 UTC (permalink / raw) To: David Miller, Thomas Petazzoni; +Cc: netdev, linux-arm-kernel, linux-kernel This series tries to optimize the mvneta's suspend/resume implementation by only taking necessary actions. Jisheng Zhang (2): net: mvneta: split rxq/txq init into SW and HW parts net: mvneta: improve suspend/resume drivers/net/ethernet/marvell/mvneta.c | 146 +++++++++++++++++++++++++++------- 1 file changed, 119 insertions(+), 27 deletions(-) -- 2.16.3 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/2] net: mvneta: split rxq/txq init into SW and HW parts 2018-03-29 10:12 ` Jisheng Zhang @ 2018-03-29 10:13 ` Jisheng Zhang -1 siblings, 0 replies; 16+ messages in thread From: Jisheng Zhang @ 2018-03-29 10:13 UTC (permalink / raw) To: linux-arm-kernel This is to prepare the suspend/resume improvement in next patch. The SW parts can be optimized out during resume. Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> --- drivers/net/ethernet/marvell/mvneta.c | 70 ++++++++++++++++++++++++++--------- 1 file changed, 53 insertions(+), 17 deletions(-) diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c index 30aab9bf77cc..4ec69bbd1eb4 100644 --- a/drivers/net/ethernet/marvell/mvneta.c +++ b/drivers/net/ethernet/marvell/mvneta.c @@ -2796,10 +2796,8 @@ static void mvneta_rx_reset(struct mvneta_port *pp) /* Rx/Tx queue initialization/cleanup methods */ -/* Create a specified RX queue */ -static int mvneta_rxq_init(struct mvneta_port *pp, - struct mvneta_rx_queue *rxq) - +static int mvneta_rxq_sw_init(struct mvneta_port *pp, + struct mvneta_rx_queue *rxq) { rxq->size = pp->rx_ring_size; @@ -2812,6 +2810,12 @@ static int mvneta_rxq_init(struct mvneta_port *pp, rxq->last_desc = rxq->size - 1; + return 0; +} + +static void mvneta_rxq_hw_init(struct mvneta_port *pp, + struct mvneta_rx_queue *rxq) +{ /* Set Rx descriptors queue starting address */ mvreg_write(pp, MVNETA_RXQ_BASE_ADDR_REG(rxq->id), rxq->descs_phys); mvreg_write(pp, MVNETA_RXQ_SIZE_REG(rxq->id), rxq->size); @@ -2835,6 +2839,20 @@ static int mvneta_rxq_init(struct mvneta_port *pp, mvneta_rxq_short_pool_set(pp, rxq); mvneta_rxq_non_occup_desc_add(pp, rxq, rxq->size); } +} + +/* Create a specified RX queue */ +static int mvneta_rxq_init(struct mvneta_port *pp, + struct mvneta_rx_queue *rxq) + +{ + int ret; + + ret = mvneta_rxq_sw_init(pp, rxq); + if (ret) + return ret; + + mvneta_rxq_hw_init(pp, rxq); return 0; } @@ -2857,9 +2875,8 @@ static void mvneta_rxq_deinit(struct mvneta_port *pp, rxq->descs_phys = 0; } -/* Create and initialize a tx queue */ -static int mvneta_txq_init(struct mvneta_port *pp, - struct mvneta_tx_queue *txq) +static int mvneta_txq_sw_init(struct mvneta_port *pp, + struct mvneta_tx_queue *txq) { int cpu; @@ -2872,7 +2889,6 @@ static int mvneta_txq_init(struct mvneta_port *pp, txq->tx_stop_threshold = txq->size - MVNETA_MAX_SKB_DESCS; txq->tx_wake_threshold = txq->tx_stop_threshold / 2; - /* Allocate memory for TX descriptors */ txq->descs = dma_alloc_coherent(pp->dev->dev.parent, txq->size * MVNETA_DESC_ALIGNED_SIZE, @@ -2882,14 +2898,6 @@ static int mvneta_txq_init(struct mvneta_port *pp, txq->last_desc = txq->size - 1; - /* Set maximum bandwidth for enabled TXQs */ - mvreg_write(pp, MVETH_TXQ_TOKEN_CFG_REG(txq->id), 0x03ffffff); - mvreg_write(pp, MVETH_TXQ_TOKEN_COUNT_REG(txq->id), 0x3fffffff); - - /* Set Tx descriptors queue starting address */ - mvreg_write(pp, MVNETA_TXQ_BASE_ADDR_REG(txq->id), txq->descs_phys); - mvreg_write(pp, MVNETA_TXQ_SIZE_REG(txq->id), txq->size); - txq->tx_skb = kmalloc_array(txq->size, sizeof(*txq->tx_skb), GFP_KERNEL); if (!txq->tx_skb) { @@ -2910,7 +2918,6 @@ static int mvneta_txq_init(struct mvneta_port *pp, txq->descs, txq->descs_phys); return -ENOMEM; } - mvneta_tx_done_pkts_coal_set(pp, txq, txq->done_pkts_coal); /* Setup XPS mapping */ if (txq_number > 1) @@ -2923,6 +2930,35 @@ static int mvneta_txq_init(struct mvneta_port *pp, return 0; } +static void mvneta_txq_hw_init(struct mvneta_port *pp, + struct mvneta_tx_queue *txq) +{ + /* Set maximum bandwidth for enabled TXQs */ + mvreg_write(pp, MVETH_TXQ_TOKEN_CFG_REG(txq->id), 0x03ffffff); + mvreg_write(pp, MVETH_TXQ_TOKEN_COUNT_REG(txq->id), 0x3fffffff); + + /* Set Tx descriptors queue starting address */ + mvreg_write(pp, MVNETA_TXQ_BASE_ADDR_REG(txq->id), txq->descs_phys); + mvreg_write(pp, MVNETA_TXQ_SIZE_REG(txq->id), txq->size); + + mvneta_tx_done_pkts_coal_set(pp, txq, txq->done_pkts_coal); +} + +/* Create and initialize a tx queue */ +static int mvneta_txq_init(struct mvneta_port *pp, + struct mvneta_tx_queue *txq) +{ + int ret; + + ret = mvneta_txq_sw_init(pp, txq); + if (ret < 0) + return ret; + + mvneta_txq_hw_init(pp, txq); + + return 0; +} + /* Free allocated resources when mvneta_txq_init() fails to allocate memory*/ static void mvneta_txq_deinit(struct mvneta_port *pp, struct mvneta_tx_queue *txq) -- 2.16.3 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 1/2] net: mvneta: split rxq/txq init into SW and HW parts @ 2018-03-29 10:13 ` Jisheng Zhang 0 siblings, 0 replies; 16+ messages in thread From: Jisheng Zhang @ 2018-03-29 10:13 UTC (permalink / raw) To: David Miller, Thomas Petazzoni; +Cc: netdev, linux-arm-kernel, linux-kernel This is to prepare the suspend/resume improvement in next patch. The SW parts can be optimized out during resume. Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> --- drivers/net/ethernet/marvell/mvneta.c | 70 ++++++++++++++++++++++++++--------- 1 file changed, 53 insertions(+), 17 deletions(-) diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c index 30aab9bf77cc..4ec69bbd1eb4 100644 --- a/drivers/net/ethernet/marvell/mvneta.c +++ b/drivers/net/ethernet/marvell/mvneta.c @@ -2796,10 +2796,8 @@ static void mvneta_rx_reset(struct mvneta_port *pp) /* Rx/Tx queue initialization/cleanup methods */ -/* Create a specified RX queue */ -static int mvneta_rxq_init(struct mvneta_port *pp, - struct mvneta_rx_queue *rxq) - +static int mvneta_rxq_sw_init(struct mvneta_port *pp, + struct mvneta_rx_queue *rxq) { rxq->size = pp->rx_ring_size; @@ -2812,6 +2810,12 @@ static int mvneta_rxq_init(struct mvneta_port *pp, rxq->last_desc = rxq->size - 1; + return 0; +} + +static void mvneta_rxq_hw_init(struct mvneta_port *pp, + struct mvneta_rx_queue *rxq) +{ /* Set Rx descriptors queue starting address */ mvreg_write(pp, MVNETA_RXQ_BASE_ADDR_REG(rxq->id), rxq->descs_phys); mvreg_write(pp, MVNETA_RXQ_SIZE_REG(rxq->id), rxq->size); @@ -2835,6 +2839,20 @@ static int mvneta_rxq_init(struct mvneta_port *pp, mvneta_rxq_short_pool_set(pp, rxq); mvneta_rxq_non_occup_desc_add(pp, rxq, rxq->size); } +} + +/* Create a specified RX queue */ +static int mvneta_rxq_init(struct mvneta_port *pp, + struct mvneta_rx_queue *rxq) + +{ + int ret; + + ret = mvneta_rxq_sw_init(pp, rxq); + if (ret) + return ret; + + mvneta_rxq_hw_init(pp, rxq); return 0; } @@ -2857,9 +2875,8 @@ static void mvneta_rxq_deinit(struct mvneta_port *pp, rxq->descs_phys = 0; } -/* Create and initialize a tx queue */ -static int mvneta_txq_init(struct mvneta_port *pp, - struct mvneta_tx_queue *txq) +static int mvneta_txq_sw_init(struct mvneta_port *pp, + struct mvneta_tx_queue *txq) { int cpu; @@ -2872,7 +2889,6 @@ static int mvneta_txq_init(struct mvneta_port *pp, txq->tx_stop_threshold = txq->size - MVNETA_MAX_SKB_DESCS; txq->tx_wake_threshold = txq->tx_stop_threshold / 2; - /* Allocate memory for TX descriptors */ txq->descs = dma_alloc_coherent(pp->dev->dev.parent, txq->size * MVNETA_DESC_ALIGNED_SIZE, @@ -2882,14 +2898,6 @@ static int mvneta_txq_init(struct mvneta_port *pp, txq->last_desc = txq->size - 1; - /* Set maximum bandwidth for enabled TXQs */ - mvreg_write(pp, MVETH_TXQ_TOKEN_CFG_REG(txq->id), 0x03ffffff); - mvreg_write(pp, MVETH_TXQ_TOKEN_COUNT_REG(txq->id), 0x3fffffff); - - /* Set Tx descriptors queue starting address */ - mvreg_write(pp, MVNETA_TXQ_BASE_ADDR_REG(txq->id), txq->descs_phys); - mvreg_write(pp, MVNETA_TXQ_SIZE_REG(txq->id), txq->size); - txq->tx_skb = kmalloc_array(txq->size, sizeof(*txq->tx_skb), GFP_KERNEL); if (!txq->tx_skb) { @@ -2910,7 +2918,6 @@ static int mvneta_txq_init(struct mvneta_port *pp, txq->descs, txq->descs_phys); return -ENOMEM; } - mvneta_tx_done_pkts_coal_set(pp, txq, txq->done_pkts_coal); /* Setup XPS mapping */ if (txq_number > 1) @@ -2923,6 +2930,35 @@ static int mvneta_txq_init(struct mvneta_port *pp, return 0; } +static void mvneta_txq_hw_init(struct mvneta_port *pp, + struct mvneta_tx_queue *txq) +{ + /* Set maximum bandwidth for enabled TXQs */ + mvreg_write(pp, MVETH_TXQ_TOKEN_CFG_REG(txq->id), 0x03ffffff); + mvreg_write(pp, MVETH_TXQ_TOKEN_COUNT_REG(txq->id), 0x3fffffff); + + /* Set Tx descriptors queue starting address */ + mvreg_write(pp, MVNETA_TXQ_BASE_ADDR_REG(txq->id), txq->descs_phys); + mvreg_write(pp, MVNETA_TXQ_SIZE_REG(txq->id), txq->size); + + mvneta_tx_done_pkts_coal_set(pp, txq, txq->done_pkts_coal); +} + +/* Create and initialize a tx queue */ +static int mvneta_txq_init(struct mvneta_port *pp, + struct mvneta_tx_queue *txq) +{ + int ret; + + ret = mvneta_txq_sw_init(pp, txq); + if (ret < 0) + return ret; + + mvneta_txq_hw_init(pp, txq); + + return 0; +} + /* Free allocated resources when mvneta_txq_init() fails to allocate memory*/ static void mvneta_txq_deinit(struct mvneta_port *pp, struct mvneta_tx_queue *txq) -- 2.16.3 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 1/2] net: mvneta: split rxq/txq init into SW and HW parts 2018-03-29 10:13 ` Jisheng Zhang @ 2018-03-29 11:42 ` Thomas Petazzoni -1 siblings, 0 replies; 16+ messages in thread From: Thomas Petazzoni @ 2018-03-29 11:42 UTC (permalink / raw) To: linux-arm-kernel Hello, On Thu, 29 Mar 2018 18:13:56 +0800, Jisheng Zhang wrote: > This is to prepare the suspend/resume improvement in next patch. The > SW parts can be optimized out during resume. > > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> Thanks, I have two very minor nits below, but otherwise: Acked-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> > +/* Create a specified RX queue */ > +static int mvneta_rxq_init(struct mvneta_port *pp, > + struct mvneta_rx_queue *rxq) > + > +{ > + int ret; > + > + ret = mvneta_rxq_sw_init(pp, rxq); > + if (ret) Here you're testing if (ret), while in mvneta_txq_init(), in the same situation, you're doing if (ret < 0). I don't have a preference for one or the other, but having them consistent between the two lpaces would be nice. > -/* Create and initialize a tx queue */ > -static int mvneta_txq_init(struct mvneta_port *pp, > - struct mvneta_tx_queue *txq) > +static int mvneta_txq_sw_init(struct mvneta_port *pp, > + struct mvneta_tx_queue *txq) > { > int cpu; > > @@ -2872,7 +2889,6 @@ static int mvneta_txq_init(struct mvneta_port *pp, > txq->tx_stop_threshold = txq->size - MVNETA_MAX_SKB_DESCS; > txq->tx_wake_threshold = txq->tx_stop_threshold / 2; > > - Spurious change. Thanks! Thomas Petazzoni -- Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] net: mvneta: split rxq/txq init into SW and HW parts @ 2018-03-29 11:42 ` Thomas Petazzoni 0 siblings, 0 replies; 16+ messages in thread From: Thomas Petazzoni @ 2018-03-29 11:42 UTC (permalink / raw) To: Jisheng Zhang; +Cc: David Miller, netdev, linux-arm-kernel, linux-kernel Hello, On Thu, 29 Mar 2018 18:13:56 +0800, Jisheng Zhang wrote: > This is to prepare the suspend/resume improvement in next patch. The > SW parts can be optimized out during resume. > > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> Thanks, I have two very minor nits below, but otherwise: Acked-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> > +/* Create a specified RX queue */ > +static int mvneta_rxq_init(struct mvneta_port *pp, > + struct mvneta_rx_queue *rxq) > + > +{ > + int ret; > + > + ret = mvneta_rxq_sw_init(pp, rxq); > + if (ret) Here you're testing if (ret), while in mvneta_txq_init(), in the same situation, you're doing if (ret < 0). I don't have a preference for one or the other, but having them consistent between the two lpaces would be nice. > -/* Create and initialize a tx queue */ > -static int mvneta_txq_init(struct mvneta_port *pp, > - struct mvneta_tx_queue *txq) > +static int mvneta_txq_sw_init(struct mvneta_port *pp, > + struct mvneta_tx_queue *txq) > { > int cpu; > > @@ -2872,7 +2889,6 @@ static int mvneta_txq_init(struct mvneta_port *pp, > txq->tx_stop_threshold = txq->size - MVNETA_MAX_SKB_DESCS; > txq->tx_wake_threshold = txq->tx_stop_threshold / 2; > > - Spurious change. Thanks! Thomas Petazzoni -- Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/2] net: mvneta: split rxq/txq init into SW and HW parts 2018-03-29 11:42 ` Thomas Petazzoni @ 2018-03-30 9:04 ` Jisheng Zhang -1 siblings, 0 replies; 16+ messages in thread From: Jisheng Zhang @ 2018-03-30 9:04 UTC (permalink / raw) To: linux-arm-kernel Hi, On Thu, 29 Mar 2018 13:42:59 +0200 Thomas Petazzoni wrote: > Hello, > > On Thu, 29 Mar 2018 18:13:56 +0800, Jisheng Zhang wrote: > > This is to prepare the suspend/resume improvement in next patch. The > > SW parts can be optimized out during resume. > > > > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > > Thanks, I have two very minor nits below, but otherwise: > > Acked-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> Thanks for reviewing. > > > +/* Create a specified RX queue */ > > +static int mvneta_rxq_init(struct mvneta_port *pp, > > + struct mvneta_rx_queue *rxq) > > + > > +{ > > + int ret; > > + > > + ret = mvneta_rxq_sw_init(pp, rxq); > > + if (ret) > > Here you're testing if (ret), while in mvneta_txq_init(), in the same > situation, you're doing if (ret < 0). I don't have a preference for one > or the other, but having them consistent between the two lpaces would > be nice. updated in v2. > > > -/* Create and initialize a tx queue */ > > -static int mvneta_txq_init(struct mvneta_port *pp, > > - struct mvneta_tx_queue *txq) > > +static int mvneta_txq_sw_init(struct mvneta_port *pp, > > + struct mvneta_tx_queue *txq) > > { > > int cpu; > > > > @@ -2872,7 +2889,6 @@ static int mvneta_txq_init(struct mvneta_port *pp, > > txq->tx_stop_threshold = txq->size - MVNETA_MAX_SKB_DESCS; > > txq->tx_wake_threshold = txq->tx_stop_threshold / 2; > > > > - > > Spurious change. There's an extra blank line here, so I removed it ;) Thanks ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] net: mvneta: split rxq/txq init into SW and HW parts @ 2018-03-30 9:04 ` Jisheng Zhang 0 siblings, 0 replies; 16+ messages in thread From: Jisheng Zhang @ 2018-03-30 9:04 UTC (permalink / raw) To: Thomas Petazzoni; +Cc: David Miller, netdev, linux-arm-kernel, linux-kernel Hi, On Thu, 29 Mar 2018 13:42:59 +0200 Thomas Petazzoni wrote: > Hello, > > On Thu, 29 Mar 2018 18:13:56 +0800, Jisheng Zhang wrote: > > This is to prepare the suspend/resume improvement in next patch. The > > SW parts can be optimized out during resume. > > > > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > > Thanks, I have two very minor nits below, but otherwise: > > Acked-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> Thanks for reviewing. > > > +/* Create a specified RX queue */ > > +static int mvneta_rxq_init(struct mvneta_port *pp, > > + struct mvneta_rx_queue *rxq) > > + > > +{ > > + int ret; > > + > > + ret = mvneta_rxq_sw_init(pp, rxq); > > + if (ret) > > Here you're testing if (ret), while in mvneta_txq_init(), in the same > situation, you're doing if (ret < 0). I don't have a preference for one > or the other, but having them consistent between the two lpaces would > be nice. updated in v2. > > > -/* Create and initialize a tx queue */ > > -static int mvneta_txq_init(struct mvneta_port *pp, > > - struct mvneta_tx_queue *txq) > > +static int mvneta_txq_sw_init(struct mvneta_port *pp, > > + struct mvneta_tx_queue *txq) > > { > > int cpu; > > > > @@ -2872,7 +2889,6 @@ static int mvneta_txq_init(struct mvneta_port *pp, > > txq->tx_stop_threshold = txq->size - MVNETA_MAX_SKB_DESCS; > > txq->tx_wake_threshold = txq->tx_stop_threshold / 2; > > > > - > > Spurious change. There's an extra blank line here, so I removed it ;) Thanks ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/2] net: mvneta: improve suspend/resume 2018-03-29 10:12 ` Jisheng Zhang @ 2018-03-29 10:15 ` Jisheng Zhang -1 siblings, 0 replies; 16+ messages in thread From: Jisheng Zhang @ 2018-03-29 10:15 UTC (permalink / raw) To: linux-arm-kernel Current suspend/resume implementation reuses the mvneta_open() and mvneta_close(), but it could be optimized to take only necessary actions during suspend/resume. One obvious problem of current implementation is: after hundreds of system suspend/resume cycles, the resume of mvneta could fail due to fragmented dma coherent memory. After this patch, the non-necessary memory alloc/free is optimized out. Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> --- drivers/net/ethernet/marvell/mvneta.c | 76 ++++++++++++++++++++++++++++++----- 1 file changed, 66 insertions(+), 10 deletions(-) diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c index 4ec69bbd1eb4..1870f1dd7093 100644 --- a/drivers/net/ethernet/marvell/mvneta.c +++ b/drivers/net/ethernet/marvell/mvneta.c @@ -4575,14 +4575,46 @@ static int mvneta_remove(struct platform_device *pdev) #ifdef CONFIG_PM_SLEEP static int mvneta_suspend(struct device *device) { + int queue; struct net_device *dev = dev_get_drvdata(device); struct mvneta_port *pp = netdev_priv(dev); - rtnl_lock(); - if (netif_running(dev)) - mvneta_stop(dev); - rtnl_unlock(); + if (!netif_running(dev)) + return 0; + netif_device_detach(dev); + + mvneta_stop_dev(pp); + + if (!pp->neta_armada3700) { + spin_lock(&pp->lock); + pp->is_stopped = true; + spin_unlock(&pp->lock); + + cpuhp_state_remove_instance_nocalls(online_hpstate, + &pp->node_online); + cpuhp_state_remove_instance_nocalls(CPUHP_NET_MVNETA_DEAD, + &pp->node_dead); + } + + for (queue = 0; queue < rxq_number; queue++) { + struct mvneta_rx_queue *rxq = &pp->rxqs[queue]; + + mvneta_rxq_drop_pkts(pp, rxq); + } + + for (queue = 0; queue < txq_number; queue++) { + struct mvneta_tx_queue *txq = &pp->txqs[queue]; + + /* Set minimum bandwidth for disabled TXQs */ + mvreg_write(pp, MVETH_TXQ_TOKEN_CFG_REG(txq->id), 0); + mvreg_write(pp, MVETH_TXQ_TOKEN_COUNT_REG(txq->id), 0); + + /* Set Tx descriptors queue starting address and size */ + mvreg_write(pp, MVNETA_TXQ_BASE_ADDR_REG(txq->id), 0); + mvreg_write(pp, MVNETA_TXQ_SIZE_REG(txq->id), 0); + } + clk_disable_unprepare(pp->clk_bus); clk_disable_unprepare(pp->clk); return 0; @@ -4593,7 +4625,7 @@ 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; + int err, queue; clk_prepare_enable(pp->clk); if (!IS_ERR(pp->clk_bus)) @@ -4614,13 +4646,37 @@ static int mvneta_resume(struct device *device) return err; } + if (!netif_running(dev)) + return 0; + netif_device_attach(dev); - rtnl_lock(); - if (netif_running(dev)) { - mvneta_open(dev); - mvneta_set_rx_mode(dev); + + for (queue = 0; queue < rxq_number; queue++) { + struct mvneta_rx_queue *rxq = &pp->rxqs[queue]; + + rxq->next_desc_to_proc = 0; + mvneta_rxq_hw_init(pp, rxq); } - rtnl_unlock(); + + for (queue = 0; queue < txq_number; queue++) { + struct mvneta_tx_queue *txq = &pp->txqs[queue]; + + txq->next_desc_to_proc = 0; + mvneta_txq_hw_init(pp, txq); + } + + if (!pp->neta_armada3700) { + spin_lock(&pp->lock); + pp->is_stopped = false; + spin_unlock(&pp->lock); + cpuhp_state_add_instance_nocalls(online_hpstate, + &pp->node_online); + cpuhp_state_add_instance_nocalls(CPUHP_NET_MVNETA_DEAD, + &pp->node_dead); + } + + mvneta_set_rx_mode(dev); + mvneta_start_dev(pp); return 0; } -- 2.16.3 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/2] net: mvneta: improve suspend/resume @ 2018-03-29 10:15 ` Jisheng Zhang 0 siblings, 0 replies; 16+ messages in thread From: Jisheng Zhang @ 2018-03-29 10:15 UTC (permalink / raw) To: David Miller, Thomas Petazzoni; +Cc: netdev, linux-arm-kernel, linux-kernel Current suspend/resume implementation reuses the mvneta_open() and mvneta_close(), but it could be optimized to take only necessary actions during suspend/resume. One obvious problem of current implementation is: after hundreds of system suspend/resume cycles, the resume of mvneta could fail due to fragmented dma coherent memory. After this patch, the non-necessary memory alloc/free is optimized out. Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> --- drivers/net/ethernet/marvell/mvneta.c | 76 ++++++++++++++++++++++++++++++----- 1 file changed, 66 insertions(+), 10 deletions(-) diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c index 4ec69bbd1eb4..1870f1dd7093 100644 --- a/drivers/net/ethernet/marvell/mvneta.c +++ b/drivers/net/ethernet/marvell/mvneta.c @@ -4575,14 +4575,46 @@ static int mvneta_remove(struct platform_device *pdev) #ifdef CONFIG_PM_SLEEP static int mvneta_suspend(struct device *device) { + int queue; struct net_device *dev = dev_get_drvdata(device); struct mvneta_port *pp = netdev_priv(dev); - rtnl_lock(); - if (netif_running(dev)) - mvneta_stop(dev); - rtnl_unlock(); + if (!netif_running(dev)) + return 0; + netif_device_detach(dev); + + mvneta_stop_dev(pp); + + if (!pp->neta_armada3700) { + spin_lock(&pp->lock); + pp->is_stopped = true; + spin_unlock(&pp->lock); + + cpuhp_state_remove_instance_nocalls(online_hpstate, + &pp->node_online); + cpuhp_state_remove_instance_nocalls(CPUHP_NET_MVNETA_DEAD, + &pp->node_dead); + } + + for (queue = 0; queue < rxq_number; queue++) { + struct mvneta_rx_queue *rxq = &pp->rxqs[queue]; + + mvneta_rxq_drop_pkts(pp, rxq); + } + + for (queue = 0; queue < txq_number; queue++) { + struct mvneta_tx_queue *txq = &pp->txqs[queue]; + + /* Set minimum bandwidth for disabled TXQs */ + mvreg_write(pp, MVETH_TXQ_TOKEN_CFG_REG(txq->id), 0); + mvreg_write(pp, MVETH_TXQ_TOKEN_COUNT_REG(txq->id), 0); + + /* Set Tx descriptors queue starting address and size */ + mvreg_write(pp, MVNETA_TXQ_BASE_ADDR_REG(txq->id), 0); + mvreg_write(pp, MVNETA_TXQ_SIZE_REG(txq->id), 0); + } + clk_disable_unprepare(pp->clk_bus); clk_disable_unprepare(pp->clk); return 0; @@ -4593,7 +4625,7 @@ 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; + int err, queue; clk_prepare_enable(pp->clk); if (!IS_ERR(pp->clk_bus)) @@ -4614,13 +4646,37 @@ static int mvneta_resume(struct device *device) return err; } + if (!netif_running(dev)) + return 0; + netif_device_attach(dev); - rtnl_lock(); - if (netif_running(dev)) { - mvneta_open(dev); - mvneta_set_rx_mode(dev); + + for (queue = 0; queue < rxq_number; queue++) { + struct mvneta_rx_queue *rxq = &pp->rxqs[queue]; + + rxq->next_desc_to_proc = 0; + mvneta_rxq_hw_init(pp, rxq); } - rtnl_unlock(); + + for (queue = 0; queue < txq_number; queue++) { + struct mvneta_tx_queue *txq = &pp->txqs[queue]; + + txq->next_desc_to_proc = 0; + mvneta_txq_hw_init(pp, txq); + } + + if (!pp->neta_armada3700) { + spin_lock(&pp->lock); + pp->is_stopped = false; + spin_unlock(&pp->lock); + cpuhp_state_add_instance_nocalls(online_hpstate, + &pp->node_online); + cpuhp_state_add_instance_nocalls(CPUHP_NET_MVNETA_DEAD, + &pp->node_dead); + } + + mvneta_set_rx_mode(dev); + mvneta_start_dev(pp); return 0; } -- 2.16.3 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/2] net: mvneta: improve suspend/resume 2018-03-29 10:15 ` Jisheng Zhang @ 2018-03-29 11:54 ` Thomas Petazzoni -1 siblings, 0 replies; 16+ messages in thread From: Thomas Petazzoni @ 2018-03-29 11:54 UTC (permalink / raw) To: linux-arm-kernel Hello Jisheng, On Thu, 29 Mar 2018 18:15:36 +0800, Jisheng Zhang wrote: > Current suspend/resume implementation reuses the mvneta_open() and > mvneta_close(), but it could be optimized to take only necessary > actions during suspend/resume. > > One obvious problem of current implementation is: after hundreds of > system suspend/resume cycles, the resume of mvneta could fail due to > fragmented dma coherent memory. After this patch, the non-necessary > memory alloc/free is optimized out. Indeed, this needs to be fixed, you're totally right. > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > --- > drivers/net/ethernet/marvell/mvneta.c | 76 ++++++++++++++++++++++++++++++----- > 1 file changed, 66 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c > index 4ec69bbd1eb4..1870f1dd7093 100644 > --- a/drivers/net/ethernet/marvell/mvneta.c > +++ b/drivers/net/ethernet/marvell/mvneta.c > @@ -4575,14 +4575,46 @@ static int mvneta_remove(struct platform_device *pdev) > #ifdef CONFIG_PM_SLEEP > static int mvneta_suspend(struct device *device) > { > + int queue; > struct net_device *dev = dev_get_drvdata(device); > struct mvneta_port *pp = netdev_priv(dev); > > - rtnl_lock(); > - if (netif_running(dev)) > - mvneta_stop(dev); > - rtnl_unlock(); > + if (!netif_running(dev)) > + return 0; This is changing the behavior I believe. The current code is: rtnl_lock(); if (netif_running(dev)) mvneta_stop(dev); rtnl_unlock(); netif_device_detach(dev); clk_disable_unprepare(pp->clk_bus); clk_disable_unprepare(pp->clk); return 0; So, when netif_running(dev) is false, we're indeed not calling mvneta_stop(), but we're still doing netif_device_detach(), and disabling the clocks. With your change, we're no longer doing these steps. > + > netif_device_detach(dev); > + > + mvneta_stop_dev(pp); > + > + if (!pp->neta_armada3700) { > + spin_lock(&pp->lock); > + pp->is_stopped = true; > + spin_unlock(&pp->lock); Real question: is it OK to set pp->is_stopped *after* calling mvneta_stop_dev(), while it was set before calling mvneta_stop_dev() in the current code ? > + > + cpuhp_state_remove_instance_nocalls(online_hpstate, > + &pp->node_online); > + cpuhp_state_remove_instance_nocalls(CPUHP_NET_MVNETA_DEAD, > + &pp->node_dead); Do we need to remove/add those CPU notifiers when suspending/resuming ? > + } > + > + for (queue = 0; queue < rxq_number; queue++) { > + struct mvneta_rx_queue *rxq = &pp->rxqs[queue]; > + > + mvneta_rxq_drop_pkts(pp, rxq); > + } Wouldn't it make sense to have mvneta_rxq_sw_deinit/mvneta_rxq_hw_deinit(), like you did for the initialization ? > + > + for (queue = 0; queue < txq_number; queue++) { > + struct mvneta_tx_queue *txq = &pp->txqs[queue]; > + > + /* Set minimum bandwidth for disabled TXQs */ > + mvreg_write(pp, MVETH_TXQ_TOKEN_CFG_REG(txq->id), 0); > + mvreg_write(pp, MVETH_TXQ_TOKEN_COUNT_REG(txq->id), 0); > + > + /* Set Tx descriptors queue starting address and size */ > + mvreg_write(pp, MVNETA_TXQ_BASE_ADDR_REG(txq->id), 0); > + mvreg_write(pp, MVNETA_TXQ_SIZE_REG(txq->id), 0); > + } Same comment here: a mvneta_txq_sw_deinit()/mvneta_txq_hw_deinit() would be good, and would avoid duplicating this logic. > + > clk_disable_unprepare(pp->clk_bus); > clk_disable_unprepare(pp->clk); > return 0; > @@ -4593,7 +4625,7 @@ 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; > + int err, queue; > > clk_prepare_enable(pp->clk); > if (!IS_ERR(pp->clk_bus)) > @@ -4614,13 +4646,37 @@ static int mvneta_resume(struct device *device) > return err; > } > > + if (!netif_running(dev)) > + return 0; > + > netif_device_attach(dev); > - rtnl_lock(); > - if (netif_running(dev)) { > - mvneta_open(dev); > - mvneta_set_rx_mode(dev); > + > + for (queue = 0; queue < rxq_number; queue++) { > + struct mvneta_rx_queue *rxq = &pp->rxqs[queue]; > + > + rxq->next_desc_to_proc = 0; > + mvneta_rxq_hw_init(pp, rxq); > } > - rtnl_unlock(); > + > + for (queue = 0; queue < txq_number; queue++) { > + struct mvneta_tx_queue *txq = &pp->txqs[queue]; > + > + txq->next_desc_to_proc = 0; > + mvneta_txq_hw_init(pp, txq); > + } > + > + if (!pp->neta_armada3700) { > + spin_lock(&pp->lock); > + pp->is_stopped = false; > + spin_unlock(&pp->lock); > + cpuhp_state_add_instance_nocalls(online_hpstate, > + &pp->node_online); > + cpuhp_state_add_instance_nocalls(CPUHP_NET_MVNETA_DEAD, > + &pp->node_dead); > + } > + > + mvneta_set_rx_mode(dev); > + mvneta_start_dev(pp); Thanks! Thomas -- Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] net: mvneta: improve suspend/resume @ 2018-03-29 11:54 ` Thomas Petazzoni 0 siblings, 0 replies; 16+ messages in thread From: Thomas Petazzoni @ 2018-03-29 11:54 UTC (permalink / raw) To: Jisheng Zhang; +Cc: David Miller, netdev, linux-arm-kernel, linux-kernel Hello Jisheng, On Thu, 29 Mar 2018 18:15:36 +0800, Jisheng Zhang wrote: > Current suspend/resume implementation reuses the mvneta_open() and > mvneta_close(), but it could be optimized to take only necessary > actions during suspend/resume. > > One obvious problem of current implementation is: after hundreds of > system suspend/resume cycles, the resume of mvneta could fail due to > fragmented dma coherent memory. After this patch, the non-necessary > memory alloc/free is optimized out. Indeed, this needs to be fixed, you're totally right. > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > --- > drivers/net/ethernet/marvell/mvneta.c | 76 ++++++++++++++++++++++++++++++----- > 1 file changed, 66 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c > index 4ec69bbd1eb4..1870f1dd7093 100644 > --- a/drivers/net/ethernet/marvell/mvneta.c > +++ b/drivers/net/ethernet/marvell/mvneta.c > @@ -4575,14 +4575,46 @@ static int mvneta_remove(struct platform_device *pdev) > #ifdef CONFIG_PM_SLEEP > static int mvneta_suspend(struct device *device) > { > + int queue; > struct net_device *dev = dev_get_drvdata(device); > struct mvneta_port *pp = netdev_priv(dev); > > - rtnl_lock(); > - if (netif_running(dev)) > - mvneta_stop(dev); > - rtnl_unlock(); > + if (!netif_running(dev)) > + return 0; This is changing the behavior I believe. The current code is: rtnl_lock(); if (netif_running(dev)) mvneta_stop(dev); rtnl_unlock(); netif_device_detach(dev); clk_disable_unprepare(pp->clk_bus); clk_disable_unprepare(pp->clk); return 0; So, when netif_running(dev) is false, we're indeed not calling mvneta_stop(), but we're still doing netif_device_detach(), and disabling the clocks. With your change, we're no longer doing these steps. > + > netif_device_detach(dev); > + > + mvneta_stop_dev(pp); > + > + if (!pp->neta_armada3700) { > + spin_lock(&pp->lock); > + pp->is_stopped = true; > + spin_unlock(&pp->lock); Real question: is it OK to set pp->is_stopped *after* calling mvneta_stop_dev(), while it was set before calling mvneta_stop_dev() in the current code ? > + > + cpuhp_state_remove_instance_nocalls(online_hpstate, > + &pp->node_online); > + cpuhp_state_remove_instance_nocalls(CPUHP_NET_MVNETA_DEAD, > + &pp->node_dead); Do we need to remove/add those CPU notifiers when suspending/resuming ? > + } > + > + for (queue = 0; queue < rxq_number; queue++) { > + struct mvneta_rx_queue *rxq = &pp->rxqs[queue]; > + > + mvneta_rxq_drop_pkts(pp, rxq); > + } Wouldn't it make sense to have mvneta_rxq_sw_deinit/mvneta_rxq_hw_deinit(), like you did for the initialization ? > + > + for (queue = 0; queue < txq_number; queue++) { > + struct mvneta_tx_queue *txq = &pp->txqs[queue]; > + > + /* Set minimum bandwidth for disabled TXQs */ > + mvreg_write(pp, MVETH_TXQ_TOKEN_CFG_REG(txq->id), 0); > + mvreg_write(pp, MVETH_TXQ_TOKEN_COUNT_REG(txq->id), 0); > + > + /* Set Tx descriptors queue starting address and size */ > + mvreg_write(pp, MVNETA_TXQ_BASE_ADDR_REG(txq->id), 0); > + mvreg_write(pp, MVNETA_TXQ_SIZE_REG(txq->id), 0); > + } Same comment here: a mvneta_txq_sw_deinit()/mvneta_txq_hw_deinit() would be good, and would avoid duplicating this logic. > + > clk_disable_unprepare(pp->clk_bus); > clk_disable_unprepare(pp->clk); > return 0; > @@ -4593,7 +4625,7 @@ 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; > + int err, queue; > > clk_prepare_enable(pp->clk); > if (!IS_ERR(pp->clk_bus)) > @@ -4614,13 +4646,37 @@ static int mvneta_resume(struct device *device) > return err; > } > > + if (!netif_running(dev)) > + return 0; > + > netif_device_attach(dev); > - rtnl_lock(); > - if (netif_running(dev)) { > - mvneta_open(dev); > - mvneta_set_rx_mode(dev); > + > + for (queue = 0; queue < rxq_number; queue++) { > + struct mvneta_rx_queue *rxq = &pp->rxqs[queue]; > + > + rxq->next_desc_to_proc = 0; > + mvneta_rxq_hw_init(pp, rxq); > } > - rtnl_unlock(); > + > + for (queue = 0; queue < txq_number; queue++) { > + struct mvneta_tx_queue *txq = &pp->txqs[queue]; > + > + txq->next_desc_to_proc = 0; > + mvneta_txq_hw_init(pp, txq); > + } > + > + if (!pp->neta_armada3700) { > + spin_lock(&pp->lock); > + pp->is_stopped = false; > + spin_unlock(&pp->lock); > + cpuhp_state_add_instance_nocalls(online_hpstate, > + &pp->node_online); > + cpuhp_state_add_instance_nocalls(CPUHP_NET_MVNETA_DEAD, > + &pp->node_dead); > + } > + > + mvneta_set_rx_mode(dev); > + mvneta_start_dev(pp); Thanks! Thomas -- Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/2] net: mvneta: improve suspend/resume 2018-03-29 11:54 ` Thomas Petazzoni @ 2018-03-30 9:15 ` Jisheng Zhang -1 siblings, 0 replies; 16+ messages in thread From: Jisheng Zhang @ 2018-03-30 9:15 UTC (permalink / raw) To: linux-arm-kernel On Thu, 29 Mar 2018 13:54:32 +0200 Thomas Petazzoni wrote: > Hello Jisheng, Hi Thomas, > > On Thu, 29 Mar 2018 18:15:36 +0800, Jisheng Zhang wrote: > > Current suspend/resume implementation reuses the mvneta_open() and > > mvneta_close(), but it could be optimized to take only necessary > > actions during suspend/resume. > > > > One obvious problem of current implementation is: after hundreds of > > system suspend/resume cycles, the resume of mvneta could fail due to > > fragmented dma coherent memory. After this patch, the non-necessary > > memory alloc/free is optimized out. > > Indeed, this needs to be fixed, you're totally right. > > > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > > --- > > drivers/net/ethernet/marvell/mvneta.c | 76 ++++++++++++++++++++++++++++++----- > > 1 file changed, 66 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c > > index 4ec69bbd1eb4..1870f1dd7093 100644 > > --- a/drivers/net/ethernet/marvell/mvneta.c > > +++ b/drivers/net/ethernet/marvell/mvneta.c > > @@ -4575,14 +4575,46 @@ static int mvneta_remove(struct platform_device *pdev) > > #ifdef CONFIG_PM_SLEEP > > static int mvneta_suspend(struct device *device) > > { > > + int queue; > > struct net_device *dev = dev_get_drvdata(device); > > struct mvneta_port *pp = netdev_priv(dev); > > > > - rtnl_lock(); > > - if (netif_running(dev)) > > - mvneta_stop(dev); > > - rtnl_unlock(); > > + if (!netif_running(dev)) > > + return 0; > > This is changing the behavior I believe. The current code is: > > rtnl_lock(); > if (netif_running(dev)) > mvneta_stop(dev); > rtnl_unlock(); > netif_device_detach(dev); > clk_disable_unprepare(pp->clk_bus); > clk_disable_unprepare(pp->clk); > return 0; > > So, when netif_running(dev) is false, we're indeed not calling > mvneta_stop(), but we're still doing netif_device_detach(), and > disabling the clocks. With your change, we're no longer doing these > steps. Indeed, will try to keep the behavior in v2 > > > + > > netif_device_detach(dev); > > + > > + mvneta_stop_dev(pp); > > + > > + if (!pp->neta_armada3700) { > > + spin_lock(&pp->lock); > > + pp->is_stopped = true; > > + spin_unlock(&pp->lock); > > Real question: is it OK to set pp->is_stopped *after* calling > mvneta_stop_dev(), while it was set before calling mvneta_stop_dev() in > the current code ? oops, you are right. Fixed in v2 > > > + > > + cpuhp_state_remove_instance_nocalls(online_hpstate, > > + &pp->node_online); > > + cpuhp_state_remove_instance_nocalls(CPUHP_NET_MVNETA_DEAD, > > + &pp->node_dead); > > Do we need to remove/add those CPU notifiers when suspending/resuming ? Take mvneta_cpu_online() as an example, if we don't remove it during suspend, when system is resume back, it will touch mac when secondary cpu is ON, but at this point the mvneta isn't resumed, this is not safe. > > > + } > > + > > + for (queue = 0; queue < rxq_number; queue++) { > > + struct mvneta_rx_queue *rxq = &pp->rxqs[queue]; > > + > > + mvneta_rxq_drop_pkts(pp, rxq); > > + } > > Wouldn't it make sense to have > mvneta_rxq_sw_deinit/mvneta_rxq_hw_deinit(), like you did for the > initialization ? For rxq deinit, we'd like to drop rx pkts, this is both HW and SW operation. So we reuse mvneta_rxq_drop_pkts() here. > > > + > > + for (queue = 0; queue < txq_number; queue++) { > > + struct mvneta_tx_queue *txq = &pp->txqs[queue]; > > + > > + /* Set minimum bandwidth for disabled TXQs */ > > + mvreg_write(pp, MVETH_TXQ_TOKEN_CFG_REG(txq->id), 0); > > + mvreg_write(pp, MVETH_TXQ_TOKEN_COUNT_REG(txq->id), 0); > > + > > + /* Set Tx descriptors queue starting address and size */ > > + mvreg_write(pp, MVNETA_TXQ_BASE_ADDR_REG(txq->id), 0); > > + mvreg_write(pp, MVNETA_TXQ_SIZE_REG(txq->id), 0); > > + } > > Same comment here: a mvneta_txq_sw_deinit()/mvneta_txq_hw_deinit() > would be good, and would avoid duplicating this logic. yep, will do in v2. Thanks a lot for the kind review. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] net: mvneta: improve suspend/resume @ 2018-03-30 9:15 ` Jisheng Zhang 0 siblings, 0 replies; 16+ messages in thread From: Jisheng Zhang @ 2018-03-30 9:15 UTC (permalink / raw) To: Thomas Petazzoni; +Cc: David Miller, netdev, linux-arm-kernel, linux-kernel On Thu, 29 Mar 2018 13:54:32 +0200 Thomas Petazzoni wrote: > Hello Jisheng, Hi Thomas, > > On Thu, 29 Mar 2018 18:15:36 +0800, Jisheng Zhang wrote: > > Current suspend/resume implementation reuses the mvneta_open() and > > mvneta_close(), but it could be optimized to take only necessary > > actions during suspend/resume. > > > > One obvious problem of current implementation is: after hundreds of > > system suspend/resume cycles, the resume of mvneta could fail due to > > fragmented dma coherent memory. After this patch, the non-necessary > > memory alloc/free is optimized out. > > Indeed, this needs to be fixed, you're totally right. > > > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > > --- > > drivers/net/ethernet/marvell/mvneta.c | 76 ++++++++++++++++++++++++++++++----- > > 1 file changed, 66 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c > > index 4ec69bbd1eb4..1870f1dd7093 100644 > > --- a/drivers/net/ethernet/marvell/mvneta.c > > +++ b/drivers/net/ethernet/marvell/mvneta.c > > @@ -4575,14 +4575,46 @@ static int mvneta_remove(struct platform_device *pdev) > > #ifdef CONFIG_PM_SLEEP > > static int mvneta_suspend(struct device *device) > > { > > + int queue; > > struct net_device *dev = dev_get_drvdata(device); > > struct mvneta_port *pp = netdev_priv(dev); > > > > - rtnl_lock(); > > - if (netif_running(dev)) > > - mvneta_stop(dev); > > - rtnl_unlock(); > > + if (!netif_running(dev)) > > + return 0; > > This is changing the behavior I believe. The current code is: > > rtnl_lock(); > if (netif_running(dev)) > mvneta_stop(dev); > rtnl_unlock(); > netif_device_detach(dev); > clk_disable_unprepare(pp->clk_bus); > clk_disable_unprepare(pp->clk); > return 0; > > So, when netif_running(dev) is false, we're indeed not calling > mvneta_stop(), but we're still doing netif_device_detach(), and > disabling the clocks. With your change, we're no longer doing these > steps. Indeed, will try to keep the behavior in v2 > > > + > > netif_device_detach(dev); > > + > > + mvneta_stop_dev(pp); > > + > > + if (!pp->neta_armada3700) { > > + spin_lock(&pp->lock); > > + pp->is_stopped = true; > > + spin_unlock(&pp->lock); > > Real question: is it OK to set pp->is_stopped *after* calling > mvneta_stop_dev(), while it was set before calling mvneta_stop_dev() in > the current code ? oops, you are right. Fixed in v2 > > > + > > + cpuhp_state_remove_instance_nocalls(online_hpstate, > > + &pp->node_online); > > + cpuhp_state_remove_instance_nocalls(CPUHP_NET_MVNETA_DEAD, > > + &pp->node_dead); > > Do we need to remove/add those CPU notifiers when suspending/resuming ? Take mvneta_cpu_online() as an example, if we don't remove it during suspend, when system is resume back, it will touch mac when secondary cpu is ON, but at this point the mvneta isn't resumed, this is not safe. > > > + } > > + > > + for (queue = 0; queue < rxq_number; queue++) { > > + struct mvneta_rx_queue *rxq = &pp->rxqs[queue]; > > + > > + mvneta_rxq_drop_pkts(pp, rxq); > > + } > > Wouldn't it make sense to have > mvneta_rxq_sw_deinit/mvneta_rxq_hw_deinit(), like you did for the > initialization ? For rxq deinit, we'd like to drop rx pkts, this is both HW and SW operation. So we reuse mvneta_rxq_drop_pkts() here. > > > + > > + for (queue = 0; queue < txq_number; queue++) { > > + struct mvneta_tx_queue *txq = &pp->txqs[queue]; > > + > > + /* Set minimum bandwidth for disabled TXQs */ > > + mvreg_write(pp, MVETH_TXQ_TOKEN_CFG_REG(txq->id), 0); > > + mvreg_write(pp, MVETH_TXQ_TOKEN_COUNT_REG(txq->id), 0); > > + > > + /* Set Tx descriptors queue starting address and size */ > > + mvreg_write(pp, MVNETA_TXQ_BASE_ADDR_REG(txq->id), 0); > > + mvreg_write(pp, MVNETA_TXQ_SIZE_REG(txq->id), 0); > > + } > > Same comment here: a mvneta_txq_sw_deinit()/mvneta_txq_hw_deinit() > would be good, and would avoid duplicating this logic. yep, will do in v2. Thanks a lot for the kind review. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/2] net: mvneta: improve suspend/resume 2018-03-30 9:15 ` Jisheng Zhang @ 2018-03-30 9:49 ` Thomas Petazzoni -1 siblings, 0 replies; 16+ messages in thread From: Thomas Petazzoni @ 2018-03-30 9:49 UTC (permalink / raw) To: linux-arm-kernel Hello, On Fri, 30 Mar 2018 17:15:47 +0800, Jisheng Zhang wrote: > > > + cpuhp_state_remove_instance_nocalls(online_hpstate, > > > + &pp->node_online); > > > + cpuhp_state_remove_instance_nocalls(CPUHP_NET_MVNETA_DEAD, > > > + &pp->node_dead); > > > > Do we need to remove/add those CPU notifiers when suspending/resuming ? > > Take mvneta_cpu_online() as an example, if we don't remove it during > suspend, when system is resume back, it will touch mac when secondary > cpu is ON, but at this point the mvneta isn't resumed, this is not safe. Hm. I'm still a bit confused by this new CPU hotplug API. I understand the issue you have and indeed unregistering the CPU hotplug callbacks is a way to solve the problem, but I find it weird that we have to do this. Anyway, it's OK to do it, because it's anyway what was done so far. It is just annoying that there is a duplication of the logic between mvneta_suspend() and mvneta_stop() on one side, and duplication between mvneta_resume() and mvnete_start() on the other side. > > > + for (queue = 0; queue < rxq_number; queue++) { > > > + struct mvneta_rx_queue *rxq = &pp->rxqs[queue]; > > > + > > > + mvneta_rxq_drop_pkts(pp, rxq); > > > + } > > > > Wouldn't it make sense to have > > mvneta_rxq_sw_deinit/mvneta_rxq_hw_deinit(), like you did for the > > initialization ? > > For rxq deinit, we'd like to drop rx pkts, this is both HW and SW operation. > So we reuse mvneta_rxq_drop_pkts() here. Hum, OK, indeed. It would have been nicer to have something symmetric, with the hw/sw parts split in a similar way for the init and deinit of both txqs and rxqs, but I agree that dropping the RX packets before going into suspend involves both HW and SW operations. Thanks! Thomas Petazzoni -- Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] net: mvneta: improve suspend/resume @ 2018-03-30 9:49 ` Thomas Petazzoni 0 siblings, 0 replies; 16+ messages in thread From: Thomas Petazzoni @ 2018-03-30 9:49 UTC (permalink / raw) To: Jisheng Zhang; +Cc: David Miller, netdev, linux-arm-kernel, linux-kernel Hello, On Fri, 30 Mar 2018 17:15:47 +0800, Jisheng Zhang wrote: > > > + cpuhp_state_remove_instance_nocalls(online_hpstate, > > > + &pp->node_online); > > > + cpuhp_state_remove_instance_nocalls(CPUHP_NET_MVNETA_DEAD, > > > + &pp->node_dead); > > > > Do we need to remove/add those CPU notifiers when suspending/resuming ? > > Take mvneta_cpu_online() as an example, if we don't remove it during > suspend, when system is resume back, it will touch mac when secondary > cpu is ON, but at this point the mvneta isn't resumed, this is not safe. Hm. I'm still a bit confused by this new CPU hotplug API. I understand the issue you have and indeed unregistering the CPU hotplug callbacks is a way to solve the problem, but I find it weird that we have to do this. Anyway, it's OK to do it, because it's anyway what was done so far. It is just annoying that there is a duplication of the logic between mvneta_suspend() and mvneta_stop() on one side, and duplication between mvneta_resume() and mvnete_start() on the other side. > > > + for (queue = 0; queue < rxq_number; queue++) { > > > + struct mvneta_rx_queue *rxq = &pp->rxqs[queue]; > > > + > > > + mvneta_rxq_drop_pkts(pp, rxq); > > > + } > > > > Wouldn't it make sense to have > > mvneta_rxq_sw_deinit/mvneta_rxq_hw_deinit(), like you did for the > > initialization ? > > For rxq deinit, we'd like to drop rx pkts, this is both HW and SW operation. > So we reuse mvneta_rxq_drop_pkts() here. Hum, OK, indeed. It would have been nicer to have something symmetric, with the hw/sw parts split in a similar way for the init and deinit of both txqs and rxqs, but I agree that dropping the RX packets before going into suspend involves both HW and SW operations. Thanks! Thomas Petazzoni -- Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-03-30 9:49 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-03-29 10:12 [PATCH 0/2] net: mvneta: improve suspend/resume Jisheng Zhang 2018-03-29 10:12 ` Jisheng Zhang 2018-03-29 10:13 ` [PATCH 1/2] net: mvneta: split rxq/txq init into SW and HW parts Jisheng Zhang 2018-03-29 10:13 ` Jisheng Zhang 2018-03-29 11:42 ` Thomas Petazzoni 2018-03-29 11:42 ` Thomas Petazzoni 2018-03-30 9:04 ` Jisheng Zhang 2018-03-30 9:04 ` Jisheng Zhang 2018-03-29 10:15 ` [PATCH 2/2] net: mvneta: improve suspend/resume Jisheng Zhang 2018-03-29 10:15 ` Jisheng Zhang 2018-03-29 11:54 ` Thomas Petazzoni 2018-03-29 11:54 ` Thomas Petazzoni 2018-03-30 9:15 ` Jisheng Zhang 2018-03-30 9:15 ` Jisheng Zhang 2018-03-30 9:49 ` Thomas Petazzoni 2018-03-30 9:49 ` Thomas Petazzoni
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.