[PATCH net 0/3] Few mvneta fixes

[PATCH net 0/3] Few mvneta fixes

[Gregory CLEMENT]

Hi David,

here is a patch set of few fixes. Without the first one, a kernel
configured with debug features ended to hang when the driver is built
as a module and is removed. This is quite is annoying for debugging!

The second patch fix a forgotten flag at the initial submission of the
driver.

The third patch is only really a cosmetic one so I have no problem to
not apply it for 4.5 and wait for 4.6.

I really would like to see the first one applied for 4.5 and for the
second I let you judge if it something needed for now or that should
wait the next release.

Thanks,

Gregory

Dmitri Epshtein (2):
  net: mvneta: enable change MAC address when interface is up
  net: mvneta: fix error messages in mvneta_port_down function

Gregory CLEMENT (1):
  net: mvneta: Fix spinlock usage

 drivers/net/ethernet/marvell/mvneta.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

-- 
2.5.0

[PATCH net 1/3] net: mvneta: Fix spinlock usage

[Gregory CLEMENT]

In the previous patch, the spinlock was not initialized. While it didn't
cause any trouble yet it could be a problem to use it uninitialized.

The most annoying part was the critical section protected by the spinlock
in mvneta_stop(). Some of the functions could sleep as pointed when
activated CONFIG_DEBUG_ATOMIC_SLEEP. Actually, in mvneta_stop() we only
need to protect the is_stopped flagged, indeed the code of the notifier
for CPU online is protected by the same spinlock, so when we get the
lock, the notifer work is done.

Reported-by: Patrick Uiterwijk <patrick@puiterwijk.org>
Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index b0ae69f84493..8dc7df2edff6 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -3070,17 +3070,17 @@ static int mvneta_stop(struct net_device *dev)
 	struct mvneta_port *pp = netdev_priv(dev);
 
 	/* Inform that we are stopping so we don't want to setup the
-	 * driver for new CPUs in the notifiers
+	 * driver for new CPUs in the notifiers. The code of the
+	 * notifier for CPU online is protected by the same spinlock,
+	 * so when we get the lock, the notifer work is done.
 	 */
 	spin_lock(&pp->lock);
 	pp->is_stopped = true;
+	spin_unlock(&pp->lock);
+
 	mvneta_stop_dev(pp);
 	mvneta_mdio_remove(pp);
 	unregister_cpu_notifier(&pp->cpu_notifier);
-	/* Now that the notifier are unregistered, we can release le
-	 * lock
-	 */
-	spin_unlock(&pp->lock);
 	on_each_cpu(mvneta_percpu_disable, pp, true);
 	free_percpu_irq(dev->irq, pp->ports);
 	mvneta_cleanup_rxqs(pp);
@@ -3612,6 +3612,7 @@ static int mvneta_probe(struct platform_device *pdev)
 	dev->ethtool_ops = &mvneta_eth_tool_ops;
 
 	pp = netdev_priv(dev);
+	spin_lock_init(&pp->lock);
 	pp->phy_node = phy_node;
 	pp->phy_interface = phy_mode;
 
-- 
2.5.0

Re: [PATCH net 1/3] net: mvneta: Fix spinlock usage

[Jisheng Zhang]

Dear Gregory,

On Tue, 8 Mar 2016 13:57:04 +0100 Gregory CLEMENT wrote:

> In the previous patch, the spinlock was not initialized. While it didn't
> cause any trouble yet it could be a problem to use it uninitialized.
> 
> The most annoying part was the critical section protected by the spinlock
> in mvneta_stop(). Some of the functions could sleep as pointed when
> activated CONFIG_DEBUG_ATOMIC_SLEEP. Actually, in mvneta_stop() we only
> need to protect the is_stopped flagged, indeed the code of the notifier
> for CPU online is protected by the same spinlock, so when we get the
> lock, the notifer work is done.
> 
> Reported-by: Patrick Uiterwijk <patrick@puiterwijk.org>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  drivers/net/ethernet/marvell/mvneta.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index b0ae69f84493..8dc7df2edff6 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -3070,17 +3070,17 @@ static int mvneta_stop(struct net_device *dev)
>  	struct mvneta_port *pp = netdev_priv(dev);
>  
>  	/* Inform that we are stopping so we don't want to setup the
> -	 * driver for new CPUs in the notifiers
> +	 * driver for new CPUs in the notifiers. The code of the
> +	 * notifier for CPU online is protected by the same spinlock,
> +	 * so when we get the lock, the notifer work is done.
>  	 */
>  	spin_lock(&pp->lock);
>  	pp->is_stopped = true;
> +	spin_unlock(&pp->lock);

This fix sleep in atomic issue. But
I see race here. Let's assume is_stopped is false.

cpu0:                           	cpu1:
mvneta_percpu_notifier():		mvneta_stop():

if (pp->is_stopped) {
	spin_unlock(&pp->lock);
	break;
}

					pp->is_stopped = true;
					spin_unlock(&pp->lock);


netif_tx_stop_all_queues(pp->dev);
for_each_online_cpu(other_cpu) {
....

Thanks,
Jisheng

> +
>  	mvneta_stop_dev(pp);
>  	mvneta_mdio_remove(pp);
>  	unregister_cpu_notifier(&pp->cpu_notifier);
> -	/* Now that the notifier are unregistered, we can release le
> -	 * lock
> -	 */
> -	spin_unlock(&pp->lock);
>  	on_each_cpu(mvneta_percpu_disable, pp, true);
>  	free_percpu_irq(dev->irq, pp->ports);
>  	mvneta_cleanup_rxqs(pp);
> @@ -3612,6 +3612,7 @@ static int mvneta_probe(struct platform_device *pdev)
>  	dev->ethtool_ops = &mvneta_eth_tool_ops;
>  
>  	pp = netdev_priv(dev);
> +	spin_lock_init(&pp->lock);
>  	pp->phy_node = phy_node;
>  	pp->phy_interface = phy_mode;
>  

Re: [PATCH net 1/3] net: mvneta: Fix spinlock usage

[Gregory CLEMENT]

Hi Jisheng,
 
 On mer., mars 09 2016, Jisheng Zhang <jszhang@marvell.com> wrote:

> Dear Gregory,
>
> On Tue, 8 Mar 2016 13:57:04 +0100 Gregory CLEMENT wrote:
>
>> In the previous patch, the spinlock was not initialized. While it didn't
>> cause any trouble yet it could be a problem to use it uninitialized.
>> 
>> The most annoying part was the critical section protected by the spinlock
>> in mvneta_stop(). Some of the functions could sleep as pointed when
>> activated CONFIG_DEBUG_ATOMIC_SLEEP. Actually, in mvneta_stop() we only
>> need to protect the is_stopped flagged, indeed the code of the notifier
>> for CPU online is protected by the same spinlock, so when we get the
>> lock, the notifer work is done.
>> 
>> Reported-by: Patrick Uiterwijk <patrick@puiterwijk.org>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> ---
>>  drivers/net/ethernet/marvell/mvneta.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
>> index b0ae69f84493..8dc7df2edff6 100644
>> --- a/drivers/net/ethernet/marvell/mvneta.c
>> +++ b/drivers/net/ethernet/marvell/mvneta.c
>> @@ -3070,17 +3070,17 @@ static int mvneta_stop(struct net_device *dev)
>>  	struct mvneta_port *pp = netdev_priv(dev);
>>  
>>  	/* Inform that we are stopping so we don't want to setup the
>> -	 * driver for new CPUs in the notifiers
>> +	 * driver for new CPUs in the notifiers. The code of the
>> +	 * notifier for CPU online is protected by the same spinlock,
>> +	 * so when we get the lock, the notifer work is done.
>>  	 */
>>  	spin_lock(&pp->lock);
>>  	pp->is_stopped = true;
>> +	spin_unlock(&pp->lock);
>
> This fix sleep in atomic issue. But
> I see race here. Let's assume is_stopped is false.

You forgot that the lock was hold in the mvneta_percpu_notifier so your
scenario can't happen.

>
> cpu0:                           	cpu1:
> mvneta_percpu_notifier():		mvneta_stop():
>

spin_lock(&pp->lock);

> if (pp->is_stopped) {
> 	spin_unlock(&pp->lock);
> 	break;
> }
>

                                      the lock is hold in
                                      mvneta_percpu_notifier(), so as
                                      said in the comment this cpu is
                                      waiting for on the following
                                      line:
                                      spin_lock(&pp->lock);

                                      This code will be executed only
                                      when the lock will be released
> 					pp->is_stopped = true;
> 					spin_unlock(&pp->lock);
>
>
> netif_tx_stop_all_queues(pp->dev);
> for_each_online_cpu(other_cpu) {
> ....
>
So what will happen is:
cpu0:                           	cpu1:
mvneta_percpu_notifier():		mvneta_stop():

spin_lock(&pp->lock);
if (pp->is_stopped) {
	spin_unlock(&pp->lock);
	break;
}
                                        spin_lock(&pp->lock);

netif_tx_stop_all_queues(pp->dev);
for_each_online_cpu(other_cpu) {
....
spin_unlock(&pp->lock);
					pp->is_stopped = true;
					spin_unlock(&pp->lock);


Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH net 1/3] net: mvneta: Fix spinlock usage

[Jisheng Zhang]

Dear Gregory,

On Wed, 9 Mar 2016 08:49:40 +0100 Gregory CLEMENT wrote:

> Hi Jisheng,
>  
>  On mer., mars 09 2016, Jisheng Zhang <jszhang@marvell.com> wrote:
> 
> > Dear Gregory,
> >
> > On Tue, 8 Mar 2016 13:57:04 +0100 Gregory CLEMENT wrote:
> >  
> >> In the previous patch, the spinlock was not initialized. While it didn't
> >> cause any trouble yet it could be a problem to use it uninitialized.
> >> 
> >> The most annoying part was the critical section protected by the spinlock
> >> in mvneta_stop(). Some of the functions could sleep as pointed when
> >> activated CONFIG_DEBUG_ATOMIC_SLEEP. Actually, in mvneta_stop() we only
> >> need to protect the is_stopped flagged, indeed the code of the notifier
> >> for CPU online is protected by the same spinlock, so when we get the
> >> lock, the notifer work is done.
> >> 
> >> Reported-by: Patrick Uiterwijk <patrick@puiterwijk.org>
> >> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> >> ---
> >>  drivers/net/ethernet/marvell/mvneta.c | 11 ++++++-----
> >>  1 file changed, 6 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> >> index b0ae69f84493..8dc7df2edff6 100644
> >> --- a/drivers/net/ethernet/marvell/mvneta.c
> >> +++ b/drivers/net/ethernet/marvell/mvneta.c
> >> @@ -3070,17 +3070,17 @@ static int mvneta_stop(struct net_device *dev)
> >>  	struct mvneta_port *pp = netdev_priv(dev);
> >>  
> >>  	/* Inform that we are stopping so we don't want to setup the
> >> -	 * driver for new CPUs in the notifiers
> >> +	 * driver for new CPUs in the notifiers. The code of the
> >> +	 * notifier for CPU online is protected by the same spinlock,
> >> +	 * so when we get the lock, the notifer work is done.
> >>  	 */
> >>  	spin_lock(&pp->lock);
> >>  	pp->is_stopped = true;
> >> +	spin_unlock(&pp->lock);  
> >
> > This fix sleep in atomic issue. But
> > I see race here. Let's assume is_stopped is false.  
> 
> You forgot that the lock was hold in the mvneta_percpu_notifier so your
> scenario can't happen.
> 
> >
> > cpu0:                           	cpu1:
> > mvneta_percpu_notifier():		mvneta_stop():
> >  
> 
> spin_lock(&pp->lock);
> 
> > if (pp->is_stopped) {
> > 	spin_unlock(&pp->lock);
> > 	break;
> > }

OOPS, I misread the code here as "the lock will be unlocked" ;)
Sorry for noise.

If you want, feel free to add

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

> >  
> 
>                                       the lock is hold in
>                                       mvneta_percpu_notifier(), so as
>                                       said in the comment this cpu is
>                                       waiting for on the following
>                                       line:
>                                       spin_lock(&pp->lock);
> 
>                                       This code will be executed only
>                                       when the lock will be released
> > 					pp->is_stopped = true;
> > 					spin_unlock(&pp->lock);
> >
> >
> > netif_tx_stop_all_queues(pp->dev);
> > for_each_online_cpu(other_cpu) {
> > ....
> >  
> So what will happen is:
> cpu0:                           	cpu1:
> mvneta_percpu_notifier():		mvneta_stop():
> 
> spin_lock(&pp->lock);
> if (pp->is_stopped) {
> 	spin_unlock(&pp->lock);
> 	break;
> }
>                                         spin_lock(&pp->lock);
> 
> netif_tx_stop_all_queues(pp->dev);
> for_each_online_cpu(other_cpu) {
> ....
> spin_unlock(&pp->lock);
> 					pp->is_stopped = true;
> 					spin_unlock(&pp->lock);
> 
> 
> Gregory
> 

[PATCH net 2/3] net: mvneta: enable change MAC address when interface is up

[Gregory CLEMENT]

From: Dmitri Epshtein <dima@marvell.com>

Function eth_prepare_mac_addr_change() is called as part of MAC
address change. This function check if interface is running.
To enable change MAC address when interface is running:
IFF_LIVE_ADDR_CHANGE flag must be set to dev->priv_flags field

Fixes: c5aff18204da ("net: mvneta: driver for Marvell Armada 370/XP
network unit")
Cc: stable@vger.kernel.org
Signed-off-by: Dmitri Epshtein <dima@marvell.com>
Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 8dc7df2edff6..2ee05cebea75 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -3721,7 +3721,7 @@ static int mvneta_probe(struct platform_device *pdev)
 	dev->features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO;
 	dev->hw_features |= dev->features;
 	dev->vlan_features |= dev->features;
-	dev->priv_flags |= IFF_UNICAST_FLT;
+	dev->priv_flags |= IFF_UNICAST_FLT | IFF_LIVE_ADDR_CHANGE;
 	dev->gso_max_segs = MVNETA_MAX_TSO_SEGS;
 
 	err = register_netdev(dev);
-- 
2.5.0

[PATCH net 3/3] net: mvneta: fix error messages in mvneta_port_down function

[Gregory CLEMENT]

From: Dmitri Epshtein <dima@marvell.com>

This commit corrects error printing when shutting down the port. Also
magic numbers are replaced by existing macros.

Signed-off-by: Dmitri Epshtein <dima@marvell.com>
Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 2ee05cebea75..225d933259c0 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -873,14 +873,14 @@ static void mvneta_port_down(struct mvneta_port *pp)
 	do {
 		if (count++ >= MVNETA_RX_DISABLE_TIMEOUT_MSEC) {
 			netdev_warn(pp->dev,
-				    "TIMEOUT for RX stopped ! rx_queue_cmd: 0x08%x\n",
+				    "TIMEOUT for RX stopped ! rx_queue_cmd: 0x%08x\n",
 				    val);
 			break;
 		}
 		mdelay(1);
 
 		val = mvreg_read(pp, MVNETA_RXQ_CMD);
-	} while (val & 0xff);
+	} while (val & MVNETA_RXQ_ENABLE_MASK);
 
 	/* Stop Tx port activity. Check port Tx activity. Issue stop
 	 * command for active channels only
@@ -905,14 +905,14 @@ static void mvneta_port_down(struct mvneta_port *pp)
 		/* Check TX Command reg that all Txqs are stopped */
 		val = mvreg_read(pp, MVNETA_TXQ_CMD);
 
-	} while (val & 0xff);
+	} while (val & MVNETA_TXQ_ENABLE_MASK);
 
 	/* Double check to verify that TX FIFO is empty */
 	count = 0;
 	do {
 		if (count++ >= MVNETA_TX_FIFO_EMPTY_TIMEOUT) {
 			netdev_warn(pp->dev,
-				    "TX FIFO empty timeout status=0x08%x\n",
+				    "TX FIFO empty timeout status=0x%08x\n",
 				    val);
 			break;
 		}
-- 
2.5.0

Re: [PATCH net 3/3] net: mvneta: fix error messages in mvneta_port_down function

[David Miller]

From: Gregory CLEMENT <gregory.clement@free-electrons.com>
Date: Tue,  8 Mar 2016 13:57:06 +0100

> From: Dmitri Epshtein <dima@marvell.com>
> 
> This commit corrects error printing when shutting down the port. Also
> magic numbers are replaced by existing macros.
> 
> Signed-off-by: Dmitri Epshtein <dima@marvell.com>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Individual patches should do one thing, rather than several at once.

Please split this up into two changes, thanks.