netdev - Re: [PATCH] net: mvneta: Add missing hotplug notifier transition

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8737rx2tqw.fsf@free-electrons.com>
Date:	Fri, 11 Mar 2016 11:28:55 +0100
From:	Gregory CLEMENT <gregory.clement@...e-electrons.com>
To:	Anna-Maria Gleixner <anna-maria@...utronix.de>
Cc:	linux-kernel@...r.kernel.org, rt@...utronix.de,
	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
	netdev@...r.kernel.org
Subject: Re: [PATCH] net: mvneta: Add missing hotplug notifier transition

Hi Anna-Maria,
 
 On ven., mars 11 2016, Anna-Maria Gleixner <anna-maria@...utronix.de> wrote:

> The mvneta_percpu_notifier() hotplug callback lacks handling of the
> CPU_DOWN_FAILED case. That means, if CPU_DOWN_PREPARE failes, the
> driver is not well configured on the CPU.
>
> Add handling for CPU_DOWN_FAILED[_FROZEN] hotplug notifier transition
> to setup the driver.

I agree that we need to handle this case, however reusing CPU_ONLINE
case for it seems too much. Indeed in the case of a CPU down failure all
the napi are already synchronized thanks to the CPU_DOWN_PREPARE
case. Moreover, we didn't call yet mvneta_percpu_elect() so we don't have
to call it again. I would prefer something like that:

@@ -2987,6 +2987,23 @@ static int mvneta_percpu_notifier(struct notifier_block *nfb,
                                         pp, true);
 
                break;
+       case CPU_DOWN_FAILED:
+       case CPU_DOWN_FAILED_FROZEN:
+               /* Re-enable per-CPU interrupts on the CPU that failed
+                * to bring up.
+                */
+               smp_call_function_single(cpu, mvneta_percpu_enable,
+                                        pp, true);
+
+               napi_enable(&port->napi);
+               /* Unmask all ethernet port interrupts */
+               on_each_cpu(mvneta_percpu_unmask_interrupt, pp, true);
+               mvreg_write(pp, MVNETA_INTR_MISC_MASK,
+                       MVNETA_CAUSE_PHY_STATUS_CHANGE |
+                       MVNETA_CAUSE_LINK_CHANGE |
+                       MVNETA_CAUSE_PSC_SYNC_CHANGE);
+               netif_tx_start_all_queues(pp->dev);
+               break;
        case CPU_DEAD:
        case CPU_DEAD_FROZEN:
                /* Check if a new CPU must be elected now this on is down */


Also it would be nice to appy this fix on the stable kernel, the bug was
introduced with this commit: f86428854480 ("net: mvneta: Statically
assign queues to CPUs")

Thanks,

Gregory

>
> Cc: Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>
> Cc: netdev@...r.kernel.org
> Signed-off-by: Anna-Maria Gleixner <anna-maria@...utronix.de>
> ---
>  drivers/net/ethernet/marvell/mvneta.c |    2 ++
>  1 file changed, 2 insertions(+)
>
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -2920,6 +2920,8 @@ static int mvneta_percpu_notifier(struct
>  	switch (action) {
>  	case CPU_ONLINE:
>  	case CPU_ONLINE_FROZEN:
> +	case CPU_DOWN_FAILED:
> +	case CPU_DOWN_FAILED_FROZEN:
>  		spin_lock(&pp->lock);
>  		/* Configuring the driver for a new CPU while the
>  		 * driver is stopping is racy, so just avoid it.

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