Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <be5a4c10-3b69-1c2d-d413-62f79ccc178b@colorfullife.com>
Date:   Fri, 9 Dec 2022 19:47:52 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.0
Subject: Re: Invalid locking pattern in
 Documentation/kernel-hacking/locking.rst?
To:     "Sverdlin, Alexander" <alexander.sverdlin@siemens.com>,
        "corbet@lwn.net" <corbet@lwn.net>,
        "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
        thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@kernel.org>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        1vier1@web.de
References: <442ecdf402f8e726f2be4ab19c7299d272e27c0b.camel@siemens.com>
Content-Language: en-US
From:   Manfred Spraul <manfred@colorfullife.com>
In-Reply-To: <442ecdf402f8e726f2be4ab19c7299d272e27c0b.camel@siemens.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Alexander,

On 12/9/22 13:23, Sverdlin, Alexander wrote:
> Dear documentation maintainers,
>
> the latest version of locking.rst contains the following (since 2005):
>
> "Manfred Spraul points out that you can still do this, even if the data
> is very occasionally accessed in user context or softirqs/tasklets. The
> irq handler doesn't use a lock, and all other accesses are done as so::
>
>          spin_lock(&lock);
>          disable_irq(irq);
> "
>
> Isn't it "sleeping in atomic" actually because of the sleeping
> disable_irq()?

Good catch!

The documentation of disable_irq() claims that the function is safe to 
be called from IRQ context (for careful callers)

But it calls synchronize_irq(). And synchronize_irq() claims that the 
function can be called only from preemptible code.

The change was in 2009:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v6.1-rc8&id=3aa551c9b4c40018f0e261a178e3d25478dc04a9 


@Thomas/@Ingo: What do we want?

Declare disable_irq()&synchronize_irq() as safe from irq context only if 
no threaded irq handlers are used?

Or declare both function as preemptible context only?


The update for locking.rst would be to switch from spin_lock() to 
mutex_lock().


https://elixir.bootlin.com/linux/latest/source/kernel/irq/manage.c#L126

> /**
> * synchronize_irq - wait for pending IRQ handlers (on other CPUs)
> * @irq: interrupt number to wait for
> *
> * This function waits for any pending IRQ handlers for this interrupt
> * to complete before returning. If you use this function while
> * holding a resource the IRQ handler may need you will deadlock.
> *
> * Can only be called from preemptible code as it might sleep when
> * an interrupt thread is associated to @irq.
> *
> * It optionally makes sure (when the irq chip supports that method)
> * that the interrupt is not pending in any CPU and waiting for
> * service.
> */
> void  synchronize_irq 
> <https://elixir.bootlin.com/linux/latest/C/ident/synchronize_irq>(unsigned  int  irq)
> {
> 	struct  irq_desc <https://elixir.bootlin.com/linux/latest/C/ident/irq_desc>  *desc  =  irq_to_desc 
> <https://elixir.bootlin.com/linux/latest/C/ident/irq_to_desc>(irq);
>
> 	if  (desc)  {
> 		__synchronize_hardirq 
> <https://elixir.bootlin.com/linux/latest/C/ident/__synchronize_hardirq>(desc,  true <https://elixir.bootlin.com/linux/latest/C/ident/true>);
> 		/*
> * We made sure that no hardirq handler is
> * running. Now verify that no threaded handlers are
> * active.
> */
> 		wait_event 
> <https://elixir.bootlin.com/linux/latest/C/ident/wait_event>(desc->wait_for_threads 
> <https://elixir.bootlin.com/linux/latest/C/ident/wait_for_threads>,
> 			!atomic_read 
> <https://elixir.bootlin.com/linux/latest/C/ident/atomic_read>(&desc->threads_active 
> <https://elixir.bootlin.com/linux/latest/C/ident/threads_active>));
> 	}
> }
> EXPORT_SYMBOL 
> <https://elixir.bootlin.com/linux/latest/C/ident/EXPORT_SYMBOL>(synchronize_irq 
> <https://elixir.bootlin.com/linux/latest/C/ident/synchronize_irq>);

https://elixir.bootlin.com/linux/latest/source/kernel/irq/manage.c#L716

> /**
> * disable_irq - disable an irq and wait for completion
> * @irq: Interrupt to disable
> *
> * Disable the selected interrupt line. Enables and Disables are
> * nested.
> * This function waits for any pending IRQ handlers for this interrupt
> * to complete before returning. If you use this function while
> * holding a resource the IRQ handler may need you will deadlock.
> *
> * This function may be called - with care - from IRQ context.
> */
> void  disable_irq 
> <https://elixir.bootlin.com/linux/latest/C/ident/disable_irq>(unsigned  int  irq)
> {
> 	if  (!__disable_irq_nosync 
> <https://elixir.bootlin.com/linux/latest/C/ident/__disable_irq_nosync>(irq))
> 		synchronize_irq 
> <https://elixir.bootlin.com/linux/latest/C/ident/synchronize_irq>(irq);
> }
>
--

     Manfred