RE: [PATCH] Stall semantics

RE: [PATCH] Stall semantics

[Moore, Robert]

I'm questioning whether it is appropriate to *ever* call sleep since
this behavior seems to violate the spec.


-----Original Message-----
From: Nate Lawson [mailto:nate-Y6VGUYTwhu0@public.gmane.org] 
Sent: Tuesday, September 30, 2003 2:45 PM
To: Moore, Robert
Cc: acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
Subject: RE: [ACPI] [PATCH] Stall semantics

On Tue, 30 Sep 2003, Moore, Robert wrote:
> The ACPI spec says this:
>
> 16.2.3.4.1.15  Stall (Stall for a Short Time)

Yes, this is what I was referring to.

> StallTerm	:= Stall(
> 	MicroSecs	//TermArg=>Integer
> )
> The Stall term is used to implement short-term timing requirements.
> Execution is delayed for at least the required number of microseconds.
> The implementation of Stall is OS-specific, but must not relinquish
> control of the processor. Because of this, delays longer than 100
> microseconds must use Sleep instead of Stall.
-----
>
> This seems to imply that sleep() cannot be used to implement long
> "stalls" because it can relinquish the cpu.

Please read the patch and the last sentence you quoted from the spec.
Your code already calls Sleep instead of Stall for values > 1000 us.  I
simply made it stick to the spec of > 100 us.

-Nate


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

RE: [PATCH] Stall semantics

[Nate Lawson]

On Tue, 30 Sep 2003, Moore, Robert wrote:
> I'm questioning whether it is appropriate to *ever* call sleep since
> this behavior seems to violate the spec.

I believe it is correct, logic is below.

> -----Original Message-----
> From: Nate Lawson [mailto:nate-Y6VGUYTwhu0@public.gmane.org]
> Sent: Tuesday, September 30, 2003 2:45 PM
> To: Moore, Robert
> Cc: acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> Subject: RE: [ACPI] [PATCH] Stall semantics
>
> On Tue, 30 Sep 2003, Moore, Robert wrote:
> > The ACPI spec says this:
> >
> > 16.2.3.4.1.15  Stall (Stall for a Short Time)
>
> Yes, this is what I was referring to.
>
> > StallTerm	:= Stall(
> > 	MicroSecs	//TermArg=>Integer
> > )
> > The Stall term is used to implement short-term timing requirements.
> > Execution is delayed for at least the required number of microseconds.
> > The implementation of Stall is OS-specific, but must not relinquish
> > control of the processor. Because of this, delays longer than 100
> > microseconds must use Sleep instead of Stall.
> -----
> >
> > This seems to imply that sleep() cannot be used to implement long
> > "stalls" because it can relinquish the cpu.
>
> Please read the patch and the last sentence you quoted from the spec.
> Your code already calls Sleep instead of Stall for values > 1000 us.  I
> simply made it stick to the spec of > 100 us.

Let me quote that line again:
> Because of this, delays longer than 100 microseconds must use Sleep
> instead of Stall.

The current code is incorrect.  It uses Stall (i.e. a delay loop) for
waits of up to 1000 us, not 100 us like the standard.  My patch makes the
code match the standard.

Your point is whether Sleep should automatically be called by OSPM when
the AML specifies a Stall of more than a certain amount (let's stick to
100 us for this example).  I believe OSPM can and should automatically
call Sleep when the time requested exceeds the maximum for Stall.  Look at
that line again.  The key word is "use".  If OSPM automatically translates
the call, delays longer than 100 us will indeed _use_ Sleep instead of
Stall.  This gives correct, standard behavior even when the AML didn't
originally match the standard.

The fact of the matter is, there is plenty of AML out there which has this
problem.  And, the correct, intended behavior can still be produced when
parsing such AML without violating the spec.  This is the perfect example
of DWIM ("Do What I Mean").  The AML author intended to call Sleep for
such long stalls but made a small mistake.  In fact, the Intel author of
this section of ACPICA made the same mistake of not knowing the line at
which a Stall becomes a Sleep (i.e. 100 vs. 1000 us).

This is a very small change and not worth arguing about.  Let's move
forward with fixing more pressing problems like the "Utallocate 0 length"
errors that keep popping up.

-Nate


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: [PATCH] Stall semantics

[Matthew Wilcox]

On Wed, Oct 01, 2003 at 09:59:49AM -0700, Nate Lawson wrote:
> Let me quote that line again:
> > Because of this, delays longer than 100 microseconds must use Sleep
> > instead of Stall.
> 
> The current code is incorrect.  It uses Stall (i.e. a delay loop) for
> waits of up to 1000 us, not 100 us like the standard.  My patch makes the
> code match the standard.
> 
> Your point is whether Sleep should automatically be called by OSPM when
> the AML specifies a Stall of more than a certain amount (let's stick to
> 100 us for this example).  I believe OSPM can and should automatically
> call Sleep when the time requested exceeds the maximum for Stall.  Look at
> that line again.  The key word is "use".  If OSPM automatically translates
> the call, delays longer than 100 us will indeed _use_ Sleep instead of
> Stall.  This gives correct, standard behavior even when the AML didn't
> originally match the standard.

Hang on though.  We've got two contradictory requirements from the
combination of the AML and the Specification.  You've chosen one
requirement over the other without discussion.  I think an equally valid
interpretation is "This code must not relinquish the CPU", in which case,
it would conform to the specification to Stall for only 100us and then
return to the AML.

Fundamentally, we're outside the specification and into nasal demon
territory.  We can do whatever we like.  The question is: what to do that
will break the least number of machines.  Sleeping unexpectedly can easily
break code -- this is why you can't schedule while holding a spinlock,
for example.  Of course, Stalling for too long is also a bad idea.

I think the current ACPI behaviour is probably adequate -- accommodate
incopetent BIOS writers up to a point, then sleep, and damn them anyway.

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: [PATCH] Stall semantics

[Nate Lawson]

On Wed, 1 Oct 2003, Matthew Wilcox wrote:
> On Wed, Oct 01, 2003 at 09:59:49AM -0700, Nate Lawson wrote:
> > Let me quote that line again:
> > > Because of this, delays longer than 100 microseconds must use Sleep
> > > instead of Stall.
> >
> > The current code is incorrect.  It uses Stall (i.e. a delay loop) for
> > waits of up to 1000 us, not 100 us like the standard.  My patch makes the
> > code match the standard.
> >
> > Your point is whether Sleep should automatically be called by OSPM when
> > the AML specifies a Stall of more than a certain amount (let's stick to
> > 100 us for this example).  I believe OSPM can and should automatically
> > call Sleep when the time requested exceeds the maximum for Stall.  Look at
> > that line again.  The key word is "use".  If OSPM automatically translates
> > the call, delays longer than 100 us will indeed _use_ Sleep instead of
> > Stall.  This gives correct, standard behavior even when the AML didn't
> > originally match the standard.
>
> Fundamentally, we're outside the specification and into nasal demon
> territory.  We can do whatever we like.  The question is: what to do that
> will break the least number of machines.  Sleeping unexpectedly can easily
> break code -- this is why you can't schedule while holding a spinlock,
> for example.  Of course, Stalling for too long is also a bad idea.

You're assuming the AML mistake is made both ways.  If it is uniformly
made one way, it's easy to work around correctly.

> I think the current ACPI behaviour is probably adequate -- accommodate
> incopetent BIOS writers up to a point, then sleep, and damn them anyway.

It damns us OS vendors a different way in that we get unacceptably long
idle spin loops on some machines.  Spinning for 1000 us is not correct any
way you interpret the standard.

If we can't reach agreement, it's better to leave things the same.  The
other part of my patch is still useful in reducing redundant code.

-Nate


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: [PATCH] Stall semantics

[Matthew Wilcox]

On Wed, Oct 01, 2003 at 10:27:04AM -0700, Nate Lawson wrote:
> You're assuming the AML mistake is made both ways.  If it is uniformly
> made one way, it's easy to work around correctly.

Actually, I assume that AML writers are idiots who can't read the spec.
They almost certainly want to stall for 2 seconds.  The fact that they
can't doesn't stop them trying.

> > I think the current ACPI behaviour is probably adequate -- accommodate
> > incopetent BIOS writers up to a point, then sleep, and damn them anyway.
> 
> It damns us OS vendors a different way in that we get unacceptably long
> idle spin loops on some machines.  Spinning for 1000 us is not correct any
> way you interpret the standard.

You have a machine which isn't able to stall for 1 millisecond?
Many drivers do stall for that much, and more.  Grepping the linux-2.6
tree shows a lot of drivers calling mdelay() -- tulip, e1000, symbios,
aic7xxx, radeon, mach64, ohci, uhci, ide, ...

One gem..
./drivers/ide/pci/pdc202xx_new.c:       mdelay(2000);   /* 2 seconds ?! */

I don't think a 1ms stall is exceptional, or even remarkable.

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: [PATCH] Stall semantics

[Nate Lawson]

On Wed, 1 Oct 2003, Matthew Wilcox wrote:
> > > I think the current ACPI behaviour is probably adequate -- accommodate
> > > incopetent BIOS writers up to a point, then sleep, and damn them anyway.
> >
> > It damns us OS vendors a different way in that we get unacceptably long
> > idle spin loops on some machines.  Spinning for 1000 us is not correct any
> > way you interpret the standard.
>
> You have a machine which isn't able to stall for 1 millisecond?

The performance loss becomes more noticable when run repeatedly.  A single
1 ms stall is not horrible.  In any case, my latter statement is the
point.

-Nate


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

RE: [PATCH] Stall semantics

[Moore, Robert]

Here's what the MS interpreter does for Stall(n);

1) If n <= 255 (MAX_BYTE), perform the stall.
2) If n > 255, throw an error and abort the control method.

In this case, I suggest that we simply duplicate the MS behavior, it
seems like a good compromise and also provides ACPI CA with exact
compatibility.

Bob


-----Original Message-----
From: Nate Lawson [mailto:nate-Y6VGUYTwhu0@public.gmane.org] 
Sent: Wednesday, October 01, 2003 10:27 AM
To: Matthew Wilcox
Cc: Moore, Robert; acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
Subject: Re: [ACPI] [PATCH] Stall semantics

On Wed, 1 Oct 2003, Matthew Wilcox wrote:
> On Wed, Oct 01, 2003 at 09:59:49AM -0700, Nate Lawson wrote:
> > Let me quote that line again:
> > > Because of this, delays longer than 100 microseconds must use
Sleep
> > > instead of Stall.
> >
> > The current code is incorrect.  It uses Stall (i.e. a delay loop)
for
> > waits of up to 1000 us, not 100 us like the standard.  My patch
makes the
> > code match the standard.
> >
> > Your point is whether Sleep should automatically be called by OSPM
when
> > the AML specifies a Stall of more than a certain amount (let's stick
to
> > 100 us for this example).  I believe OSPM can and should
automatically
> > call Sleep when the time requested exceeds the maximum for Stall.
Look at
> > that line again.  The key word is "use".  If OSPM automatically
translates
> > the call, delays longer than 100 us will indeed _use_ Sleep instead
of
> > Stall.  This gives correct, standard behavior even when the AML
didn't
> > originally match the standard.
>
> Fundamentally, we're outside the specification and into nasal demon
> territory.  We can do whatever we like.  The question is: what to do
that
> will break the least number of machines.  Sleeping unexpectedly can
easily
> break code -- this is why you can't schedule while holding a spinlock,
> for example.  Of course, Stalling for too long is also a bad idea.

You're assuming the AML mistake is made both ways.  If it is uniformly
made one way, it's easy to work around correctly.

> I think the current ACPI behaviour is probably adequate -- accommodate
> incopetent BIOS writers up to a point, then sleep, and damn them
anyway.

It damns us OS vendors a different way in that we get unacceptably long
idle spin loops on some machines.  Spinning for 1000 us is not correct
any
way you interpret the standard.

If we can't reach agreement, it's better to leave things the same.  The
other part of my patch is still useful in reducing redundant code.

-Nate


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: [PATCH] Stall semantics

[Matthew Wilcox]

On Fri, Oct 03, 2003 at 01:23:29PM -0700, Moore, Robert wrote:
> 
> Here's what the MS interpreter does for Stall(n);
> 
> 1) If n <= 255 (MAX_BYTE), perform the stall.
> 2) If n > 255, throw an error and abort the control method.
> 
> In this case, I suggest that we simply duplicate the MS behavior, it
> seems like a good compromise and also provides ACPI CA with exact
> compatibility.

Sounds good to me (though personally, I'd trim it down to 200ms).

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

RE: [PATCH] Stall semantics

[Nate Lawson]

I'm ok with this.  The longest Stall I've seen in ASL is 200 us (VAIO
PCG-F) so I believe their limit is the ad-hoc standard.

-Nate

On Fri, 3 Oct 2003, Moore, Robert wrote:
> Here's what the MS interpreter does for Stall(n);
>
> 1) If n <= 255 (MAX_BYTE), perform the stall.
> 2) If n > 255, throw an error and abort the control method.
>
> In this case, I suggest that we simply duplicate the MS behavior, it
> seems like a good compromise and also provides ACPI CA with exact
> compatibility.
>
> Bob


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

RE: [PATCH] Stall semantics

[Moore, Robert]

Nate,

The ACPI spec says this:

16.2.3.4.1.15  Stall (Stall for a Short Time)

StallTerm	:= Stall(
	MicroSecs	//TermArg=>Integer
)
The Stall term is used to implement short-term timing requirements.
Execution is delayed for at least the required number of microseconds.
The implementation of Stall is OS-specific, but must not relinquish
control of the processor. Because of this, delays longer than 100
microseconds must use Sleep instead of Stall.



This seems to imply that sleep() cannot be used to implement long
"stalls" because it can relinquish the cpu.

Bob


-----Original Message-----
From: acpi-devel-admin-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
[mailto:acpi-devel-admin-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org] On Behalf Of Nate Lawson
Sent: Tuesday, September 30, 2003 12:38 PM
To: acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
Subject: [ACPI] [PATCH] Stall semantics

A simple and low priority patch for sticking to the specified 100 us max
stall delay.

http://bugzilla.kernel.org/show_bug.cgi?id=1299

-Nate


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
Acpi-devel mailing list
Acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/acpi-devel


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

RE: [PATCH] Stall semantics

[Nate Lawson]

On Tue, 30 Sep 2003, Moore, Robert wrote:
> The ACPI spec says this:
>
> 16.2.3.4.1.15  Stall (Stall for a Short Time)

Yes, this is what I was referring to.

> StallTerm	:= Stall(
> 	MicroSecs	//TermArg=>Integer
> )
> The Stall term is used to implement short-term timing requirements.
> Execution is delayed for at least the required number of microseconds.
> The implementation of Stall is OS-specific, but must not relinquish
> control of the processor. Because of this, delays longer than 100
> microseconds must use Sleep instead of Stall.
-----
>
> This seems to imply that sleep() cannot be used to implement long
> "stalls" because it can relinquish the cpu.

Please read the patch and the last sentence you quoted from the spec.
Your code already calls Sleep instead of Stall for values > 1000 us.  I
simply made it stick to the spec of > 100 us.

-Nate


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

[PATCH] Stall semantics

[Nate Lawson]

A simple and low priority patch for sticking to the specified 100 us max
stall delay.

http://bugzilla.kernel.org/show_bug.cgi?id=1299

-Nate


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf