LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* Question about rtc_lock
@ 2001-10-06  3:49 Thomas Hood
  2001-10-06 13:01 ` Alan Cox
  0 siblings, 1 reply; 7+ messages in thread
From: Thomas Hood @ 2001-10-06  3:49 UTC (permalink / raw)
  To: linux-kernel

The file arch/i386/kernel/bootflag.c contains this:

-------------------------------------------------
static void __init sbf_write(u8 v)
{
        if(sbf_port != -1)
        {
                v &= ~(1<<7);
                if(!parity(v))
                        v|=1<<7;

                spin_lock(&rtc_lock);
                CMOS_WRITE(v, sbf_port);
                spin_unlock(&rtc_lock);
        }
}
--------------------------------------------------

Does this code run with irqs disabled, or should these
spinlocks be _irq ?

--
Thomas


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Question about rtc_lock
  2001-10-06  3:49 Question about rtc_lock Thomas Hood
@ 2001-10-06 13:01 ` Alan Cox
  2001-10-06 13:06   ` Thomas Hood
  0 siblings, 1 reply; 7+ messages in thread
From: Alan Cox @ 2001-10-06 13:01 UTC (permalink / raw)
  To: Thomas Hood; +Cc: linux-kernel

>                 spin_lock(&rtc_lock);
>                 CMOS_WRITE(v, sbf_port);
>                 spin_unlock(&rtc_lock);
> 
> Does this code run with irqs disabled, or should these
> spinlocks be _irq ?

The CMOS isnt accessed from IRQ handlers

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Question about rtc_lock
  2001-10-06 13:01 ` Alan Cox
@ 2001-10-06 13:06   ` Thomas Hood
  2001-10-06 13:13     ` Alan Cox
  0 siblings, 1 reply; 7+ messages in thread
From: Thomas Hood @ 2001-10-06 13:06 UTC (permalink / raw)
  To: Alan Cox; +Cc: linux-kernel

On Sat, 2001-10-06 at 09:01, Alan Cox wrote:
> >                 spin_lock(&rtc_lock);
> >                 CMOS_WRITE(v, sbf_port);
> >                 spin_unlock(&rtc_lock);
> > 
> > Does this code run with irqs disabled, or should these
> > spinlocks be _irq ?
> 
> The CMOS isnt accessed from IRQ handlers

No, but what if the rtc interrupts while the lock is held in this
bit of code?

Thomas



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Question about rtc_lock
  2001-10-06 13:06   ` Thomas Hood
@ 2001-10-06 13:13     ` Alan Cox
  2001-10-06 14:40       ` Thomas Hood
  2001-10-06 15:24       ` Jonathan Lundell
  0 siblings, 2 replies; 7+ messages in thread
From: Alan Cox @ 2001-10-06 13:13 UTC (permalink / raw)
  To: Thomas Hood; +Cc: Alan Cox, linux-kernel

> > The CMOS isnt accessed from IRQ handlers
> 
> No, but what if the rtc interrupts while the lock is held in this
> bit of code?

Thats fine. It wont take the lock


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Question about rtc_lock
  2001-10-06 13:13     ` Alan Cox
@ 2001-10-06 14:40       ` Thomas Hood
  2001-10-07  3:24         ` Thomas Hood
  2001-10-06 15:24       ` Jonathan Lundell
  1 sibling, 1 reply; 7+ messages in thread
From: Thomas Hood @ 2001-10-06 14:40 UTC (permalink / raw)
  Cc: linux-kernel

On Sat, 2001-10-06 at 09:13, Alan Cox wrote:
> > No, but what if the rtc interrupts while the lock is held in this
> > bit of code?
> 
> Thats fine. It wont take the lock

But the first line of irq_interrupt() is:
   spin_lock (&rtc_lock);

If one has a multi-processor machine, and CPUx is going through
the bootflag code, which takes the rtc_lock, and that CPU is
interrupted and enters rtc_interrupt(), which tries to take the
rtc_lock, won't it deadlock?

If not, then I'm missing some clue about how these spinlocks work.

Thomas


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Question about rtc_lock
  2001-10-06 13:13     ` Alan Cox
  2001-10-06 14:40       ` Thomas Hood
@ 2001-10-06 15:24       ` Jonathan Lundell
  1 sibling, 0 replies; 7+ messages in thread
From: Jonathan Lundell @ 2001-10-06 15:24 UTC (permalink / raw)
  To: Thomas Hood; +Cc: linux-kernel

At 10:40 AM -0400 2001-10-06, Thomas Hood wrote:
>On Sat, 2001-10-06 at 09:13, Alan Cox wrote:
>>  > No, but what if the rtc interrupts while the lock is held in this
>>  > bit of code?
>>
>>  Thats fine. It wont take the lock
>
>But the first line of irq_interrupt() is:
>    spin_lock (&rtc_lock);
>
>If one has a multi-processor machine, and CPUx is going through
>the bootflag code, which takes the rtc_lock, and that CPU is
>interrupted and enters rtc_interrupt(), which tries to take the
>rtc_lock, won't it deadlock?
>
>If not, then I'm missing some clue about how these spinlocks work.

rtc_interrupt(), you mean.

Even if there weren't current interrupt code doing CMOS accesses, it 
would seem prudent to assume that there might be eventually, the 
RTC/NVRAM being a multi-purpose shared resource.
-- 
/Jonathan Lundell.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Question about rtc_lock
  2001-10-06 14:40       ` Thomas Hood
@ 2001-10-07  3:24         ` Thomas Hood
  0 siblings, 0 replies; 7+ messages in thread
From: Thomas Hood @ 2001-10-07  3:24 UTC (permalink / raw)
  To: Jonathan Lundell; +Cc: linux-kernel

On Sat, 2001-10-06 at 11:24, Jonathan Lundell wrote:
> rtc_interrupt(), you mean.

Right.

> Even if there weren't current interrupt code doing CMOS accesses, it 
> would seem prudent to assume that there might be eventually, the 
> RTC/NVRAM being a multi-purpose shared resource.

I'm not concerned about an irq handler (present or future)
interfering with us as we write to the CMOS RAM.  What I'm
concerned about is getting a rtc interrupt while we hold rtc_lock,
with deadlock being the result (since rtc_interrupt will spin on
the lock).

Either (1) we need to change these spinlocks to _irq, or (2) we
need to know that this bit of code runs only with irqs disabled.
My question is: Is it (1) or (2)?

Or is it (3) Thomas Hood is failing to understand something here?

Assuming the answer is (1), I append a patch that changes the
spinlock calls to _irqsave versions.

Cheers,
Thomas

The patch:
--- linux-2.4.10-ac5-fix/arch/i386/kernel/bootflag.c_PREV	Fri Oct  5 23:20:43 2001
+++ linux-2.4.10-ac5-fix/arch/i386/kernel/bootflag.c	Sat Oct  6 23:15:33 2001
@@ -81,26 +81,30 @@
 
 static void __init sbf_write(u8 v)
 {
+	unsigned long flags;
+
 	if(sbf_port != -1)
 	{
 		v &= ~(1<<7);
 		if(!parity(v))
 			v|=1<<7;
 			
-		spin_lock(&rtc_lock);
+		spin_lock_irqsave(&rtc_lock, flags);
 		CMOS_WRITE(v, sbf_port);
-		spin_unlock(&rtc_lock);
+		spin_unlock_irqrestore(&rtc_lock, flags);
 	}
 }
 
 static u8 __init sbf_read(void)
 {
 	u8 v;
+	unsigned long flags;
+
 	if(sbf_port == -1)
 		return 0;
-	spin_lock(&rtc_lock);
+	spin_lock_irqsave(&rtc_lock, flags);
 	v = CMOS_READ(sbf_port);
-	spin_unlock(&rtc_lock);
+	spin_unlock_irqrestore(&rtc_lock, flags);
 	return v;
 }
 


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2001-10-07  3:24 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2001-10-06  3:49 Question about rtc_lock Thomas Hood
2001-10-06 13:01 ` Alan Cox
2001-10-06 13:06   ` Thomas Hood
2001-10-06 13:13     ` Alan Cox
2001-10-06 14:40       ` Thomas Hood
2001-10-07  3:24         ` Thomas Hood
2001-10-06 15:24       ` Jonathan Lundell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).