linux-kernel - Re: [GIT pull] irq updates for 4.13

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]
Date:   Wed, 12 Jul 2017 00:51:13 +0200
From:   Sebastian Reichel <sebastian.reichel@...labora.co.uk>
To:     Thomas Gleixner <tglx@...x.de>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Tony Lindgren <tony@...mide.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Ingo Molnar <mingo@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>, Pavel Machek <pavel@....cz>,
        Linus Walleij <linus.walleij@...aro.org>,
        Grygorii Strashko <grygorii.strashko@...com>
Subject: Re: [GIT pull] irq updates for 4.13

Hi,

On Tue, Jul 11, 2017 at 11:41:52PM +0200, Thomas Gleixner wrote:
> [...]
>
> Here is a revised version of the previous patch with the conditional
> locking removed and a bunch of comments added.

That one also fixes Droid 4 boot.

Tested-by: Sebastian Reichel <sebastian.reichel@...labora.co.uk>

-- Sebastian

> 8<----------------------------
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1090,6 +1090,16 @@ setup_irq_thread(struct irqaction *new,
>  /*
>   * Internal function to register an irqaction - typically used to
>   * allocate special interrupts that are part of the architecture.
> + *
> + * Locking rules:
> + *
> + * desc->request_mutex	Provides serialization against a concurrent free_irq()
> + *   chip_bus_lock	Provides serialization for slow bus operations
> + *     desc->lock	Provides serialization against hard interrupts
> + *
> + * chip_bus_lock and desc->lock are sufficient for all other management and
> + * interrupt related functions. desc->request_mutex solely serializes
> + * request/free_irq().
>   */
>  static int
>  __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
> @@ -1167,20 +1177,35 @@ static int
>  	if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)
>  		new->flags &= ~IRQF_ONESHOT;
>  
> +	/*
> +	 * Protects against a concurrent __free_irq() call which might wait
> +	 * for synchronize_irq() to complete without holding the optional
> +	 * chip bus lock and desc->lock.
> +	 */
>  	mutex_lock(&desc->request_mutex);
> +
> +	/*
> +	 * Acquire bus lock as the irq_request_resources() callback below
> +	 * might rely on the serialization or the magic power management
> +	 * functions which are abusing the irq_bus_lock() callback,
> +	 */
> +	chip_bus_lock(desc);
> +
> +	/* First installed action requests resources. */
>  	if (!desc->action) {
>  		ret = irq_request_resources(desc);
>  		if (ret) {
>  			pr_err("Failed to request resources for %s (irq %d) on irqchip %s\n",
>  			       new->name, irq, desc->irq_data.chip->name);
> -			goto out_mutex;
> +			goto out_bus_unlock;
>  		}
>  	}
>  
> -	chip_bus_lock(desc);
> -
>  	/*
>  	 * The following block of code has to be executed atomically
> +	 * protected against a concurrent interrupt and any of the other
> +	 * management calls which are not serialized via
> +	 * desc->request_mutex or the optional bus lock.
>  	 */
>  	raw_spin_lock_irqsave(&desc->lock, flags);
>  	old_ptr = &desc->action;
> @@ -1286,10 +1311,8 @@ static int
>  			ret = __irq_set_trigger(desc,
>  						new->flags & IRQF_TRIGGER_MASK);
>  
> -			if (ret) {
> -				irq_release_resources(desc);
> +			if (ret)
>  				goto out_unlock;
> -			}
>  		}
>  
>  		desc->istate &= ~(IRQS_AUTODETECT | IRQS_SPURIOUS_DISABLED | \
> @@ -1385,12 +1408,10 @@ static int
>  out_unlock:
>  	raw_spin_unlock_irqrestore(&desc->lock, flags);
>  
> -	chip_bus_sync_unlock(desc);
> -
>  	if (!desc->action)
>  		irq_release_resources(desc);
> -
> -out_mutex:
> +out_bus_unlock:
> +	chip_bus_sync_unlock(desc);
>  	mutex_unlock(&desc->request_mutex);
>  
>  out_thread:
> @@ -1472,6 +1493,7 @@ static struct irqaction *__free_irq(unsi
>  			WARN(1, "Trying to free already-free IRQ %d\n", irq);
>  			raw_spin_unlock_irqrestore(&desc->lock, flags);
>  			chip_bus_sync_unlock(desc);
> +			mutex_unlock(&desc->request_mutex);
>  			return NULL;
>  		}
>  
> @@ -1498,6 +1520,20 @@ static struct irqaction *__free_irq(unsi
>  #endif
>  
>  	raw_spin_unlock_irqrestore(&desc->lock, flags);
> +	/*
> +	 * Drop bus_lock here so the changes which were done in the chip
> +	 * callbacks above are synced out to the irq chips which hang
> +	 * behind a slow bus (I2C, SPI) before calling synchronize_irq().
> +	 *
> +	 * Aside of that the bus_lock can also be taken from the threaded
> +	 * handler in irq_finalize_oneshot() which results in a deadlock
> +	 * because synchronize_irq() would wait forever for the thread to
> +	 * complete, which is blocked on the bus lock.
> +	 *
> +	 * The still held desc->request_mutex() protects against a
> +	 * concurrent request_irq() of this irq so the release of resources
> +	 * and timing data is properly serialized.
> +	 */
>  	chip_bus_sync_unlock(desc);
>  
>  	unregister_handler_proc(irq, action);
> @@ -1530,8 +1566,15 @@ static struct irqaction *__free_irq(unsi
>  		}
>  	}
>  
> +	/* Last action releases resources */
>  	if (!desc->action) {
> +		/*
> +		 * Reaquire bus lock as irq_release_resources() might
> +		 * require it to deallocate resources over the slow bus.
> +		 */
> +		chip_bus_lock(desc);
>  		irq_release_resources(desc);
> +		chip_bus_sync_unlock(desc);
>  		irq_remove_timings(desc);
>  	}
>  

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)