[PATCH] lock: Add doc comments for spin_lock_irq()

[PATCH] lock: Add doc comments for spin_lock_irq()

[Daroc Alden]

The commonly used spin_lock_irq(), spin_lock_irqsave(),
spin_unlock_irq(), and spin_unlock_irqrestore() functions do not
currently have any documentation; this commit adds kerneldoc comments
to these four functions describing when their behavior and when they are
appropriate to use.

Signed-off-by: Daroc Alden <daroc@lwn.net>
---
 include/linux/spinlock.h | 53 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index d3561c4a080e..35bd55605319 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -371,11 +371,47 @@ do {									\
 	raw_spin_lock_nest_lock(spinlock_check(lock), nest_lock);	\
 } while (0)
 
+/**
+ * spin_lock_irq() - Lock a spinlock while disabling interrupts.
+ * @lock: The spinlock that will be locked.
+ *
+ * When a spinlock is shared by code running in interrupt context and process
+ * context, it is important to ensure that interrupts are disabled while the
+ * lock is held. Otherwise, an interrupt handler might attempt to take the lock
+ * while it is already held, leading to a deadlock.
+ *
+ * This function unconditionally disables interrupts on the local CPU, and then
+ * locks the provided spinlock. It is suitable for use in contexts where
+ * interrupts are known to be enabled — because the corresponding unlock
+ * function, spin_unlock_irq(), unconditionally enables interrupts.
+ *
+ * When code can be called with interrupts either enabled or disabled, prefer
+ * spin_lock_irqsave(), which preserves the current state so that it can be
+ * restored when the spinlock is released.
+ */
 static __always_inline void spin_lock_irq(spinlock_t *lock)
 {
 	raw_spin_lock_irq(&lock->rlock);
 }
 
+/**
+ * spin_lock_irqsave() - Lock a lock, disable interrupts, and save current state.
+ * @lock: The spinlock that will be locked.
+ * @flags: An unsigned long to store the current interrupt state.
+ *
+ * When a spinlock is shared by code running in interrupt context and process
+ * context, it is important to ensure that interrupts are disabled while the
+ * lock is held. Otherwise, an interrupt handler might attempt to take the lock
+ * while it is already held, leading to a deadlock.
+ *
+ * This function disables interrupts on the local CPU if they are enabled, and
+ * then locks the provided spinlock. The previous state of interrupts (enabled
+ * or disabled) is saved in the @flags argument so that it can be restored by
+ * the corresponding call to spin_unlock_irqrestore().
+ *
+ * When code will only be run with interrupts enabled, using spin_lock_irq() can
+ * avoid the need to create a local variable to save the state.
+ */
 #define spin_lock_irqsave(lock, flags)				\
 do {								\
 	raw_spin_lock_irqsave(spinlock_check(lock), flags);	\
@@ -396,11 +432,28 @@ static __always_inline void spin_unlock_bh(spinlock_t *lock)
 	raw_spin_unlock_bh(&lock->rlock);
 }
 
+/**
+ * spin_unlock_irq() - Unlock a spinlock and enable interrupts.
+ * @lock: The spinlock that will be unlocked.
+ *
+ * This function unlocks the provided lock, and then unconditionally enables
+ * interrupts on the current CPU. It should typically correspond to a previous
+ * call to spin_lock_irq().
+ */
 static __always_inline void spin_unlock_irq(spinlock_t *lock)
 {
 	raw_spin_unlock_irq(&lock->rlock);
 }
 
+/**
+ * spin_unlock_irqrestore() - Unlock a spinlock and restore interrupt state.
+ * @lock: The spinlock that will be unlocked.
+ * @flags: The previously saved interrupt state to restore.
+ *
+ * This function unlocks the provided lock, and then restores interrupts to
+ * whichever state (enabled or disabled) is indicated by @flags. @flags should
+ * come from a previous call to spin_lock_irqsave().
+ */
 static __always_inline void spin_unlock_irqrestore(spinlock_t *lock, unsigned long flags)
 {
 	raw_spin_unlock_irqrestore(&lock->rlock, flags);
-- 
2.51.0

Re: [PATCH] lock: Add doc comments for spin_lock_irq()

[Waiman Long]

On 10/10/25 5:53 PM, Daroc Alden wrote:
> The commonly used spin_lock_irq(), spin_lock_irqsave(),
> spin_unlock_irq(), and spin_unlock_irqrestore() functions do not
> currently have any documentation; this commit adds kerneldoc comments
> to these four functions describing when their behavior and when they are
> appropriate to use.
>
> Signed-off-by: Daroc Alden <daroc@lwn.net>

This patch looks fine. Just wonder why just 
spin_lock_irq()/spin_lock_irqsave() and not spin_lock()/spin_lock_bh() 
as these functions also don't have kerneldoc comments. Also 
spin_lock_irqsave() is a macro and not actually a function, maybe we 
should mention that in the comment.

Cheers,
Longman

> ---
>   include/linux/spinlock.h | 53 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 53 insertions(+)
>
> diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
> index d3561c4a080e..35bd55605319 100644
> --- a/include/linux/spinlock.h
> +++ b/include/linux/spinlock.h
> @@ -371,11 +371,47 @@ do {									\
>   	raw_spin_lock_nest_lock(spinlock_check(lock), nest_lock);	\
>   } while (0)
>   
> +/**
> + * spin_lock_irq() - Lock a spinlock while disabling interrupts.
> + * @lock: The spinlock that will be locked.
> + *
> + * When a spinlock is shared by code running in interrupt context and process
> + * context, it is important to ensure that interrupts are disabled while the
> + * lock is held. Otherwise, an interrupt handler might attempt to take the lock
> + * while it is already held, leading to a deadlock.
> + *
> + * This function unconditionally disables interrupts on the local CPU, and then
> + * locks the provided spinlock. It is suitable for use in contexts where
> + * interrupts are known to be enabled — because the corresponding unlock
> + * function, spin_unlock_irq(), unconditionally enables interrupts.
> + *
> + * When code can be called with interrupts either enabled or disabled, prefer
> + * spin_lock_irqsave(), which preserves the current state so that it can be
> + * restored when the spinlock is released.
> + */
>   static __always_inline void spin_lock_irq(spinlock_t *lock)
>   {
>   	raw_spin_lock_irq(&lock->rlock);
>   }
>   
> +/**
> + * spin_lock_irqsave() - Lock a lock, disable interrupts, and save current state.
> + * @lock: The spinlock that will be locked.
> + * @flags: An unsigned long to store the current interrupt state.
> + *
> + * When a spinlock is shared by code running in interrupt context and process
> + * context, it is important to ensure that interrupts are disabled while the
> + * lock is held. Otherwise, an interrupt handler might attempt to take the lock
> + * while it is already held, leading to a deadlock.
> + *
> + * This function disables interrupts on the local CPU if they are enabled, and
> + * then locks the provided spinlock. The previous state of interrupts (enabled
> + * or disabled) is saved in the @flags argument so that it can be restored by
> + * the corresponding call to spin_unlock_irqrestore().
> + *
> + * When code will only be run with interrupts enabled, using spin_lock_irq() can
> + * avoid the need to create a local variable to save the state.
> + */
>   #define spin_lock_irqsave(lock, flags)				\
>   do {								\
>   	raw_spin_lock_irqsave(spinlock_check(lock), flags);	\
> @@ -396,11 +432,28 @@ static __always_inline void spin_unlock_bh(spinlock_t *lock)
>   	raw_spin_unlock_bh(&lock->rlock);
>   }
>   
> +/**
> + * spin_unlock_irq() - Unlock a spinlock and enable interrupts.
> + * @lock: The spinlock that will be unlocked.
> + *
> + * This function unlocks the provided lock, and then unconditionally enables
> + * interrupts on the current CPU. It should typically correspond to a previous
> + * call to spin_lock_irq().
> + */
>   static __always_inline void spin_unlock_irq(spinlock_t *lock)
>   {
>   	raw_spin_unlock_irq(&lock->rlock);
>   }
>   
> +/**
> + * spin_unlock_irqrestore() - Unlock a spinlock and restore interrupt state.
> + * @lock: The spinlock that will be unlocked.
> + * @flags: The previously saved interrupt state to restore.
> + *
> + * This function unlocks the provided lock, and then restores interrupts to
> + * whichever state (enabled or disabled) is indicated by @flags. @flags should
> + * come from a previous call to spin_lock_irqsave().
> + */
>   static __always_inline void spin_unlock_irqrestore(spinlock_t *lock, unsigned long flags)
>   {
>   	raw_spin_unlock_irqrestore(&lock->rlock, flags);

Re: [PATCH] lock: Add doc comments for spin_lock_irq()

[Daroc Alden]

On Fri, 10 Oct 2025 23:15:50 -0400
Waiman Long <llong@redhat.com> wrote:

> On 10/10/25 5:53 PM, Daroc Alden wrote:
> > The commonly used spin_lock_irq(), spin_lock_irqsave(),
> > spin_unlock_irq(), and spin_unlock_irqrestore() functions do not
> > currently have any documentation; this commit adds kerneldoc
> > comments to these four functions describing when their behavior and
> > when they are appropriate to use.
> >
> > Signed-off-by: Daroc Alden <daroc@lwn.net>  
> 
> This patch looks fine. Just wonder why just 
> spin_lock_irq()/spin_lock_irqsave() and not
> spin_lock()/spin_lock_bh() as these functions also don't have
> kerneldoc comments. Also spin_lock_irqsave() is a macro and not
> actually a function, maybe we should mention that in the comment.
> 

Because I had to research spin_lock_irq()/spin_lock_irqsave() for a
recent article, and therefore felt confident that I understood how they
behaved and what should go in the doc comment.

If you — as a more experienced kernel person — can describe how/why the
_bh() variants are used, I'm happy to add doc comments for them as
well. My current understanding is that they interact with whatever is
left of the "big kernel lock". Is that right?

-- 
Daroc Alden (they/them)
Editor, LWN | https://lwn.net

Re: [PATCH] lock: Add doc comments for spin_lock_irq()

[Waiman Long]

On 10/11/25 2:28 PM, Daroc Alden wrote:
> On Fri, 10 Oct 2025 23:15:50 -0400
> Waiman Long <llong@redhat.com> wrote:
>
>> On 10/10/25 5:53 PM, Daroc Alden wrote:
>>> The commonly used spin_lock_irq(), spin_lock_irqsave(),
>>> spin_unlock_irq(), and spin_unlock_irqrestore() functions do not
>>> currently have any documentation; this commit adds kerneldoc
>>> comments to these four functions describing when their behavior and
>>> when they are appropriate to use.
>>>
>>> Signed-off-by: Daroc Alden <daroc@lwn.net>
>> This patch looks fine. Just wonder why just
>> spin_lock_irq()/spin_lock_irqsave() and not
>> spin_lock()/spin_lock_bh() as these functions also don't have
>> kerneldoc comments. Also spin_lock_irqsave() is a macro and not
>> actually a function, maybe we should mention that in the comment.
>>
> Because I had to research spin_lock_irq()/spin_lock_irqsave() for a
> recent article, and therefore felt confident that I understood how they
> behaved and what should go in the doc comment.
>
> If you — as a more experienced kernel person — can describe how/why the
> _bh() variants are used, I'm happy to add doc comments for them as
> well. My current understanding is that they interact with whatever is
> left of the "big kernel lock". Is that right?

"bh" in spin_lock_bh() stands for bottom half which is essentially what 
what is being done in the softIRQ context. So spin_lock_bh() just 
prevents the softIRQ code from being executed. This is my understanding, 
but I may have missed other use cases of spin_lock_bh(). Others can 
chime in if there is more to say. Anyway, I am fine with adding more 
comments to spinlock code.

Cheers, Longman

Re: [PATCH] lock: Add doc comments for spin_lock_irq()

[Daroc Alden]

On Sat, 11 Oct 2025 22:31:17 -0400
Waiman Long <llong@redhat.com> wrote:

> On 10/11/25 2:28 PM, Daroc Alden wrote:
> > On Fri, 10 Oct 2025 23:15:50 -0400
> > Waiman Long <llong@redhat.com> wrote:
> >  
> >> On 10/10/25 5:53 PM, Daroc Alden wrote:  
> >>> The commonly used spin_lock_irq(), spin_lock_irqsave(),
> >>> spin_unlock_irq(), and spin_unlock_irqrestore() functions do not
> >>> currently have any documentation; this commit adds kerneldoc
> >>> comments to these four functions describing when their behavior
> >>> and when they are appropriate to use.
> >>>
> >>> Signed-off-by: Daroc Alden <daroc@lwn.net>  
> >> This patch looks fine. Just wonder why just
> >> spin_lock_irq()/spin_lock_irqsave() and not
> >> spin_lock()/spin_lock_bh() as these functions also don't have
> >> kerneldoc comments. Also spin_lock_irqsave() is a macro and not
> >> actually a function, maybe we should mention that in the comment.
> >>  
> > Because I had to research spin_lock_irq()/spin_lock_irqsave() for a
> > recent article, and therefore felt confident that I understood how
> > they behaved and what should go in the doc comment.
> >
> > If you — as a more experienced kernel person — can describe how/why
> > the _bh() variants are used, I'm happy to add doc comments for them
> > as well. My current understanding is that they interact with
> > whatever is left of the "big kernel lock". Is that right?  
> 
> "bh" in spin_lock_bh() stands for bottom half which is essentially
> what what is being done in the softIRQ context. So spin_lock_bh()
> just prevents the softIRQ code from being executed. This is my
> understanding, but I may have missed other use cases of
> spin_lock_bh(). Others can chime in if there is more to say. Anyway,
> I am fine with adding more comments to spinlock code.
> 

Ah, okay!

I went and read some of the existing locking documentation with that
context in mind, and I think I understand. I think the doc comment
should look something like this:

/**
 * spin_lock_bh() - Disable softIRQs and take the provided spinlock.
 * @lock: The spinlock to acquire.
 *
 * When data is shared between code that can run in process context and
 * code that can run in a softIRQ, if the softIRQ tries to acquire a
 * spinlock that is already held, the system could deadlock. This
 * function disables softIRQs before taking the provided spinlock. It
 * should typically be paired with a call to spin_unlock_bh() in order
 * to reenable softIRQs when the lock is released.
 *
 * If the interrupt code can run as a hard interrupt instead of a soft
 * interrupt, this is the wrong function: use spin_lock_irqsave(). If in
 * doubt, using spin_lock_irqsave() instead of spin_lock_bh() is always
 * permissible, since the former is a superset of the latter.
 *
 * If synchronizing between a tasklet or timer and a softIRQ, the plain
 * spin_lock() function can be used, because these are not interrupted
 * by softIRQs on the same CPU.
 */

If nobody chimes in to correct me, I'll add that (and a similar
comment for the plain spin_lock()) and post a v2 in a day or two.


Thanks for the help!

-- 
Daroc Alden (they/them)
Editor, LWN | https://lwn.net

Re: [PATCH] lock: Add doc comments for spin_lock_irq()

[Waiman Long]

On 10/12/25 9:48 AM, Daroc Alden wrote:
> On Sat, 11 Oct 2025 22:31:17 -0400
> Waiman Long <llong@redhat.com> wrote:
>
>> On 10/11/25 2:28 PM, Daroc Alden wrote:
>>> On Fri, 10 Oct 2025 23:15:50 -0400
>>> Waiman Long <llong@redhat.com> wrote:
>>>   
>>>> On 10/10/25 5:53 PM, Daroc Alden wrote:
>>>>> The commonly used spin_lock_irq(), spin_lock_irqsave(),
>>>>> spin_unlock_irq(), and spin_unlock_irqrestore() functions do not
>>>>> currently have any documentation; this commit adds kerneldoc
>>>>> comments to these four functions describing when their behavior
>>>>> and when they are appropriate to use.
>>>>>
>>>>> Signed-off-by: Daroc Alden <daroc@lwn.net>
>>>> This patch looks fine. Just wonder why just
>>>> spin_lock_irq()/spin_lock_irqsave() and not
>>>> spin_lock()/spin_lock_bh() as these functions also don't have
>>>> kerneldoc comments. Also spin_lock_irqsave() is a macro and not
>>>> actually a function, maybe we should mention that in the comment.
>>>>   
>>> Because I had to research spin_lock_irq()/spin_lock_irqsave() for a
>>> recent article, and therefore felt confident that I understood how
>>> they behaved and what should go in the doc comment.
>>>
>>> If you — as a more experienced kernel person — can describe how/why
>>> the _bh() variants are used, I'm happy to add doc comments for them
>>> as well. My current understanding is that they interact with
>>> whatever is left of the "big kernel lock". Is that right?
>> "bh" in spin_lock_bh() stands for bottom half which is essentially
>> what what is being done in the softIRQ context. So spin_lock_bh()
>> just prevents the softIRQ code from being executed. This is my
>> understanding, but I may have missed other use cases of
>> spin_lock_bh(). Others can chime in if there is more to say. Anyway,
>> I am fine with adding more comments to spinlock code.
>>
> Ah, okay!
>
> I went and read some of the existing locking documentation with that
> context in mind, and I think I understand. I think the doc comment
> should look something like this:
>
> /**
>   * spin_lock_bh() - Disable softIRQs and take the provided spinlock.
>   * @lock: The spinlock to acquire.
>   *
>   * When data is shared between code that can run in process context and
>   * code that can run in a softIRQ, if the softIRQ tries to acquire a
>   * spinlock that is already held, the system could deadlock. This
>   * function disables softIRQs before taking the provided spinlock. It
>   * should typically be paired with a call to spin_unlock_bh() in order
>   * to reenable softIRQs when the lock is released.
>   *
>   * If the interrupt code can run as a hard interrupt instead of a soft
>   * interrupt, this is the wrong function: use spin_lock_irqsave(). If in
>   * doubt, using spin_lock_irqsave() instead of spin_lock_bh() is always
>   * permissible, since the former is a superset of the latter.
I believe the above description is correct.
>   *
>   * If synchronizing between a tasklet or timer and a softIRQ, the plain
>   * spin_lock() function can be used, because these are not interrupted
>   * by softIRQs on the same CPU.
>   */

Tasklets and timer handling are run in softIRQ context.  SoftIRQs are 
sub-divided into a number of priority levels (see 
include/linux/interrupt.h) from high to low, they are not going to 
interrupt each other.

Cheers,
Longman