Messages in this thread |  | | Date | Sat, 17 Dec 2022 11:02:30 +0100 | Subject | Re: [PATCH] docs: kernel-hacking: discourage from calling disable_irq() in atomic | From | Manfred Spraul <> |
| |
Hi Alexander,
On 12/12/22 17:37, A. Sverdlin wrote: > From: Alexander Sverdlin <alexander.sverdlin@siemens.com> > > Correct the example in documentation so that disable_irq() is not being > called in atomic context and remove the comment allowing to do so "with > care" from the function header itself. > > disable_irq() calls sleeping synchronize_irq(), it's not allowed to call > them in atomic context. > > Link: https://lore.kernel.org/lkml/87k02wbs2n.ffs@tglx/ > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
Reviewed-by: Manfred Spraul <manfred@colorfullife.com>
(but check below, I would prefer if the change to kernel/irq/manage.c is dropped.
> --- > Documentation/kernel-hacking/locking.rst | 4 ++-- > Documentation/translations/it_IT/kernel-hacking/locking.rst | 4 ++-- > kernel/irq/manage.c | 2 -- > 3 files changed, 4 insertions(+), 6 deletions(-) > > diff --git a/Documentation/kernel-hacking/locking.rst b/Documentation/kernel-hacking/locking.rst > index 6805ae6e86e65..95fd6e0900d92 100644 > --- a/Documentation/kernel-hacking/locking.rst > +++ b/Documentation/kernel-hacking/locking.rst > @@ -1274,11 +1274,11 @@ 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); > + mutex_lock(&lock); > disable_irq(irq); > ... > enable_irq(irq); > - spin_unlock(&lock); > + mutex_unlock(&lock); > > The disable_irq() prevents the irq handler from running > (and waits for it to finish if it's currently running on other CPUs). > diff --git a/Documentation/translations/it_IT/kernel-hacking/locking.rst b/Documentation/translations/it_IT/kernel-hacking/locking.rst > index 51af37f2d6210..bfbada56cf351 100644 > --- a/Documentation/translations/it_IT/kernel-hacking/locking.rst > +++ b/Documentation/translations/it_IT/kernel-hacking/locking.rst > @@ -1309,11 +1309,11 @@ se i dati vengono occasionalmente utilizzati da un contesto utente o > da un'interruzione software. Il gestore d'interruzione non utilizza alcun > *lock*, e tutti gli altri accessi verranno fatti così:: > > - spin_lock(&lock); > + mutex_lock(&lock); > disable_irq(irq); > ... > enable_irq(irq); > - spin_unlock(&lock); > + mutex_unlock(&lock); > > La funzione disable_irq() impedisce al gestore d'interruzioni > d'essere eseguito (e aspetta che finisca nel caso fosse in esecuzione su
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c > index 40fe7806cc8c9..2054de5bf3c53 100644 > --- a/kernel/irq/manage.c > +++ b/kernel/irq/manage.c > @@ -722,8 +722,6 @@ EXPORT_SYMBOL(disable_irq_nosync); > * 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(unsigned int irq) > {
Can you drop this part?
I haven't noticed that you added this change into the patch, and thus I created a seperate patch.
https://git.kernel.org/pub/scm/linux/kernel/git/akpm/25-new.git/tree/patches/kernel-irq-managec-disable_irq-might-sleep.patch
As core difference: I've added a might_sleep() into disable_irq().
--
Manfred
|  |