[PATCH - 0/2] Identify native drivers and ACPI subsystem accessing the same resources

[PATCH - 0/2] Identify native drivers and ACPI subsystem accessing the same resources

[Thomas Renninger]

Hi,

(after review and some discussion) this is meant for -mm.

In the recent past more and more cases appeared where (mainly
hwmon/sensors) drivers interfered with ACPI subsystem.

Such interference can show up in all kind of bugs (messed up
temperature, thermal management which is rather sever,..), that
are hard to identify -> Remove potential drivers and wait ...

This patch should:
a) Identify machines where potentially ACPI interference can happen and
   tell the user which legacy drivers are affected.
b) Identify drivers/HW where we might need an ACPI driver in future
c) If it works out (if not too much important drivers are affected)
   enforce that native, non-ACPI drivers using the same IO/System
   memory addresses as declared in ACPI namespace fail to load.


There are two kind of IO/System memory declarations in ACPI ASL
language: general variables and device specific resource
declarations.
The latter is already handled by pnpacpi and resources get requested 
from pnpacpi already. These resources can (with this patch) partly
already be requested by the general ACPI declarations and this patch
should handle this gracefully.

ACPI drivers (e.g. pnpacpi and others) are allowed to request these
resources (marked IORESOURCE_SHARED) through acpi_request_region().
Depending on strict, lax, no(default) others (drivers not using ACPI)
requesting ACPI variable resources will:
  strict: fail to request the resource -> driver normally will not load
  lax:    you get a syslog entry which resource might conflict with a
          "shared resource" and a "System might run unstable.." warning.
  no:     same behaviour as without the patch


strict and no params should work as described.
Unfortunately there is one unresolved problem with the lax option:

Here two examples of not nice things that could happen with lax option:

 a) legacy drivers (in this case arch/x86_64/setup.c) request resources
    before ACPI variables (shared resources) are requested:
(/proc/ioports):
0080-008f : dma page reg
  0080-0080 : ACPI DEB0*
    In this case it's working as "dma page reg" includes the ACPI 
    SystemIO variable's space. If the ACPI variable is bigger, I expect
    the ACPI variable would not get inserted...

 b) A legacy driver requests resources bigger than the ACPI SystemIO 
    variable, but the ACPI resource variable lies within the requested
    space. Example:
(/proc/ioports):
0070-0071 : rtc0
0072-0073 : ACPI KSB0*
(syslog):
IO region [rtc] conflicts with [ACPI KSB0].  Ignoring.., system might
run unstable.
rtc: I/O resource 70 is not free.

    The rtc driver seems to request the whole rtc space (0x70-0x77)
    which fails because 0x72-0x73 has already been requested SHARED by
    an ACPI SystemIO variable. At least parts of the rtc space got
    requested (rtc0), I haven't checked whether the device was working
    correctly...
    This is the case where lax would not behave as expected, which is
    kind of sad, because lax was my favorite default option.
    I have no idea how this issue could get addressed cleanly..., maybe
    someone has an idea...


Another issue that needs fix up is that the motherboad driver is not
requesting it's resources anymore (which it still did with 2.6.18 - I
didn't dig here anymore, maybe someone can comment why this has changed)
Example:
2.6.22.1:
(syslog with lax option):
IO region [amd756_smbus] conflicts with [ACPI PMIO].  Ignoring.., system
might run unstable.
(/proc/ioports)
5000-50fe : ACPI PMIO*
  5000-5003 : ACPI PM1a_EVT_BLK
  5004-5005 : ACPI PM1a_CNT_BLK
  5008-500b : ACPI PM_TMR
  5020-5023 : ACPI GPE0_BLK
  50b0-50b7 : ACPI GPE1_BLK
  50e0-50ef : amd756_smbus       # with strict this one must not be here

This is because the non-acpi driver amd756_smbus tries to request from
an ACPI reserved region -> this is what we try to find and eliminate.
In 2.6.18 a motherboard ACPI driver reserved space via acpipnp which
would look like this then (which is what we want):
5000-50fe : ACPI PMIO*          # ACPI SystemIO variable
..
  50e0-50ff : motherboard       # PNP0C02 motherboard driver reserves region via acpipnp shared
    50e0-50ef : amd756_smbus    # amd756_smbus allowed to get resources from motherboard driver
..


One example DSDT and bugreport of ACPI interfering with hwmon driver is
here (with strict there the it87 module, trying to access 0x29[56] would
not load. We also would get a pointer that on this one an ACPI driver is
needed to control this device -> there are some vendor/BIOS specific
ACPI functions providing functionality for this device like: SFAN, FON,
FOFF, RTMP, STHY, STOS, SCFG):
https://bugzilla.novell.com/show_bug.cgi?id=259992


Summary:
  - IMO good enough for -mm, no functional change with default param
    -> Would be nice if more people test, this one is very machine
       specific, especially that if not activated (no param or
       acpi_enforce_resources=no) nothing changes.
  - motherboard driver not reserving _CRS resources currently, needs
    to be addressed so that the patch does not throw false positive
    interference on motherboard subdrivers.
  - Not fixable (maybe someone has an idea): If ACPI IO region declares
    a smaller IO part than the later native driver (e.g. example above
    with rtc driver), the partially overlapping resource cannot be
    registered and the native driver fails to load with strict and lax
    option.

Thanks,

   Thomas

Re: [PATCH - 0/2] Identify native drivers and ACPI subsystem accessing the same resources

[Bjorn Helgaas]

On Friday 13 July 2007 06:58:12 am Thomas Renninger wrote:
> This patch should:
> a) Identify machines where potentially ACPI interference can happen and
>    tell the user which legacy drivers are affected.
> b) Identify drivers/HW where we might need an ACPI driver in future
> c) If it works out (if not too much important drivers are affected)
>    enforce that native, non-ACPI drivers using the same IO/System
>    memory addresses as declared in ACPI namespace fail to load.

I like some of the ideas here, but as you say, there are some
issues to work out.

> There are two kind of IO/System memory declarations in ACPI ASL
> language: general variables and device specific resource
> declarations.
> The latter is already handled by pnpacpi and resources get requested 
> from pnpacpi already. These resources can (with this patch) partly
> already be requested by the general ACPI declarations and this patch
> should handle this gracefully.

The PNP core does not request resources for devices that are active
at boot.  Those resources currently don't get requested until a driver
claims the device.  I think this is a bug that we should fix.

> ACPI drivers (e.g. pnpacpi and others) are allowed to request these
> resources (marked IORESOURCE_SHARED) through acpi_request_region().

I'm not a fan of IORESOURCE_SHARED.  I think that will make people
think it's acceptable to share a device between several drivers.  It
would be better to keep the ugliness of forcibly taking over a resource,
just as an incentive to clean up the drivers.

> Here two examples of not nice things that could happen with lax option:
> 
>  a) legacy drivers (in this case arch/x86_64/setup.c) request resources
>     before ACPI variables (shared resources) are requested:
> (/proc/ioports):
> 0080-008f : dma page reg
>   0080-0080 : ACPI DEB0*
>     In this case it's working as "dma page reg" includes the ACPI 
>     SystemIO variable's space. If the ACPI variable is bigger, I expect
>     the ACPI variable would not get inserted...

I think the best thing would be to reserve the PNP (including ACPI)
resources first, then the legacy things from setup.c.  I haven't looked
into that to see whether it's feasible.  Then you might have something
like this:

  0080-008f : 00:07 PNP0200
    0080-008f : dma page reg

>  b) A legacy driver requests resources bigger than the ACPI SystemIO 
>     variable, but the ACPI resource variable lies within the requested
>     space. Example:
> (/proc/ioports):
> 0070-0071 : rtc0
> 0072-0073 : ACPI KSB0*
> (syslog):
> IO region [rtc] conflicts with [ACPI KSB0].  Ignoring.., system might
> run unstable.
> rtc: I/O resource 70 is not free.
> 
>     The rtc driver seems to request the whole rtc space (0x70-0x77)
>     which fails because 0x72-0x73 has already been requested SHARED by
>     an ACPI SystemIO variable. At least parts of the rtc space got
>     requested (rtc0), I haven't checked whether the device was working
>     correctly...

I tripped over a couple of these when I changed the PNP core to request
resources for active devices.  The RTC is one; often BIOS says the RTC
is at 0x70-0x71 and 0x72-0x73, but the rtc drivers like to claim 0x8
(RTC_IO_EXTENT) ports.  However, I think the driver only *uses* two
ports, so we should change the driver to only request what it is
using.

Another case is the "dma1" region from e820.c.  Currently this is
0x0-0x1f, but most PNP0c02 devices only respond from 0x10-0x1f.  So
I think we should start by splitting "dma1" into 0x0-0xf and 0x10-0x1f.

> Another issue that needs fix up is that the motherboad driver is not
> requesting it's resources anymore (which it still did with 2.6.18 - I
> didn't dig here anymore, maybe someone can comment why this has changed)

I don't know what's up here; I haven't looked at this recently.

> Summary:
> ...
>   - Not fixable (maybe someone has an idea): If ACPI IO region declares
>     a smaller IO part than the later native driver (e.g. example above
>     with rtc driver), the partially overlapping resource cannot be
>     registered and the native driver fails to load with strict and lax
>     option.

Assuming the BIOS describes the region correctly, I'd say a driver
that claims a larger region is buggy and should be fixed.

Bjorn

Re: [PATCH - 0/2] Identify native drivers and ACPI subsystem accessing the same resources

[Thomas Renninger]

On Fri, 2007-07-13 at 10:56 -0600, Bjorn Helgaas wrote:
> On Friday 13 July 2007 06:58:12 am Thomas Renninger wrote:
> > This patch should:
> > a) Identify machines where potentially ACPI interference can happen and
> >    tell the user which legacy drivers are affected.
> > b) Identify drivers/HW where we might need an ACPI driver in future
> > c) If it works out (if not too much important drivers are affected)
> >    enforce that native, non-ACPI drivers using the same IO/System
> >    memory addresses as declared in ACPI namespace fail to load.
> 
> I like some of the ideas here, but as you say, there are some
> issues to work out.
That sounds promising...

> > There are two kind of IO/System memory declarations in ACPI ASL
> > language: general variables and device specific resource
> > declarations.
> > The latter is already handled by pnpacpi and resources get requested 
> > from pnpacpi already. These resources can (with this patch) partly
> > already be requested by the general ACPI declarations and this patch
> > should handle this gracefully.
> 
> The PNP core does not request resources for devices that are active
> at boot.  Those resources currently don't get requested until a driver
> claims the device.  I think this is a bug that we should fix.
Yep, this is also a problem for the ACPI variables (in fact not really,
as long as not overlapping like the rtc resource) and probably the
reason why pnpacpi ignores addresses below 0x100?
> 
> > ACPI drivers (e.g. pnpacpi and others) are allowed to request these
> > resources (marked IORESOURCE_SHARED) through acpi_request_region().
> 
> I'm not a fan of IORESOURCE_SHARED.  I think that will make people
> think it's acceptable to share a device between several drivers.  It
> would be better to keep the ugliness of forcibly taking over a resource,
> just as an incentive to clean up the drivers.
It's only purpose is to implement the lax option (insure everything
works as before, but show offending drivers which is, as I said, my
favorite default).
Your suggestion is to just enforce the drivers getting cleaned up or
they won't be functional, just offering
acpi_enforce_resources=[no,strict]?
I fear then this patch will be in mm for ever

IMO:
  - Making sure ACPI claimed regions do not interfere with native
    drivers is very important and will get much more important in near 
    future
  - We need a step by step transition to fix up driver specific things
    smoothly. What about an ugly implementation (the lax option with
    IORESOURCE_SHARED code in kernel/resources.c), that gets reverted
    some kernel iterations later when we can be sure nothing sever
    breaks?

> > Here two examples of not nice things that could happen with lax option:
> > 
> >  a) legacy drivers (in this case arch/x86_64/setup.c) request resources
> >     before ACPI variables (shared resources) are requested:
> > (/proc/ioports):
> > 0080-008f : dma page reg
> >   0080-0080 : ACPI DEB0*
> >     In this case it's working as "dma page reg" includes the ACPI 
> >     SystemIO variable's space. If the ACPI variable is bigger, I expect
> >     the ACPI variable would not get inserted...
> 
> I think the best thing would be to reserve the PNP (including ACPI)
> resources first, then the legacy things from setup.c.  I haven't looked
> into that to see whether it's feasible.  Then you might have something
> like this:
> 
>   0080-008f : 00:07 PNP0200
>     0080-008f : dma page reg
> 
> >  b) A legacy driver requests resources bigger than the ACPI SystemIO 
> >     variable, but the ACPI resource variable lies within the requested
> >     space. Example:
> > (/proc/ioports):
> > 0070-0071 : rtc0
> > 0072-0073 : ACPI KSB0*
> > (syslog):
> > IO region [rtc] conflicts with [ACPI KSB0].  Ignoring.., system might
> > run unstable.
> > rtc: I/O resource 70 is not free.
> > 
> >     The rtc driver seems to request the whole rtc space (0x70-0x77)
> >     which fails because 0x72-0x73 has already been requested SHARED by
> >     an ACPI SystemIO variable. At least parts of the rtc space got
> >     requested (rtc0), I haven't checked whether the device was working
> >     correctly...
> 
> I tripped over a couple of these when I changed the PNP core to request
> resources for active devices.  The RTC is one; often BIOS says the RTC
> is at 0x70-0x71 and 0x72-0x73, but the rtc drivers like to claim 0x8
> (RTC_IO_EXTENT) ports.  However, I think the driver only *uses* two
> ports, so we should change the driver to only request what it is
> using.
But there could be more?
I have an idea how to implement that so that lax option still works as
expected, but this would be even more ugly than IORESOURCE_SHARED (in
fact extend the IORESOURCE_SHARED code in kernel/resource.c):
If e.g. rtc requests ports already requested by an ACPI variable (marked
IORESOURCE_SHARED or whatever), the marked one gets released and rtc
driver is allowed to claim them (with the message: "IO region [rtc]
conflicts with [ACPI KSB0].  Ignoring.., system might run unstable.")

acpi_request_resource() must check for vanishing entries then (should do
so anyways, forgot that...).

I need to think some more about this, maybe I give it a proof-of-concept
and very-ugly implementation try...

> Another case is the "dma1" region from e820.c.  Currently this is
> 0x0-0x1f, but most PNP0c02 devices only respond from 0x10-0x1f.  So
> I think we should start by splitting "dma1" into 0x0-0xf and 0x10-0x1f.
> 
> > Another issue that needs fix up is that the motherboad driver is not
> > requesting it's resources anymore (which it still did with 2.6.18 - I
> > didn't dig here anymore, maybe someone can comment why this has changed)
> 
> I don't know what's up here; I haven't looked at this recently.
Ok, that was because of too low PNP_MAX_PORT value...
Now I get:
5000-50fe : ACPI PMIO
  5000-50bf : pnp 00:07
    5000-5003 : ACPI PM1a_EVT_BLK
    5004-5005 : ACPI PM1a_CNT_BLK
    5008-500b : ACPI PM_TMR
    5020-5023 : ACPI GPE0_BLK
    50b0-50b7 : ACPI GPE1_BLK
  50c0-50df : pnp 00:07
  50e0-50ef : amd756_smbus

Argggggh, normally it should be:
5000-50fe : ACPI PMIO
...
  50c0-50df : pnp 00:07
  50e0-50ff : pnp 00:07       # this one is missing because
                              # it's bigger than the parent
    50e0-50ef : amd756_smbus


this is because:
AML:
    Name (SMBS, 0x50E0)
    Name (SMBL, 0x20)
    ...
    IO (Decode16,             // values get filled below
          0x0000,             // Range Minimum
          0x0000,             // Range Maximum
          0x00,               // Alignment
          0x00,               // Length
    _Y0D)
    ...
    If (SMBS)
    {
           CreateWordField (CRS, \_SB.PCI0.SBRG.RMSC._Y0E._MIN, GP10)
           CreateWordField (CRS, \_SB.PCI0.SBRG.RMSC._Y0E._MAX, GP11)
           CreateByteField (CRS, \_SB.PCI0.SBRG.RMSC._Y0E._LEN, GP1L)
           Store (SMBS, GP10)
           Store (SMBS, GP11)
           Store (SMBL, GP1L)
    }
that means this pnp region (filled with SMBS and SMBL values)
(0x50e0-0x50ff, starting at 0x50e0 and length of 0x20) inside the pnp
device is one byte longer than the one claimed by the ACPI variable PMIO
(0x5000-0x50fe) -> partially overlapping -> cannot be added as a child.

If this should work at all it must be possible to add pnpacpi devices to
an ACPI variable, even it exceeds the variable's border...
(Or the first machine I used has a buggy DSDT table :) ).

If this patch gets extended to allow the overlapping of ACPI SystemIO
Operation Regions and ACPI resource declarations (which would make the
code even more ugly, but I think this should be done), it still had the
functionality to identify native driver interference and all could work
as intended... But, if there is an ACPI kernel driver serving the ACPI
device (and it should be allowed to access its resources...), it's not
possible to ensure this one does not access the same resources than some
arbitrary executed AML code at the same time (this can only be ensured
if ACPI spec forbids that device resources and OperationRegion
declarations can overlap...).

Thanks,

    Thomas

> > Summary:
> > ...
> >   - Not fixable (maybe someone has an idea): If ACPI IO region declares
> >     a smaller IO part than the later native driver (e.g. example above
> >     with rtc driver), the partially overlapping resource cannot be
> >     registered and the native driver fails to load with strict and lax
> >     option.
> 
> Assuming the BIOS describes the region correctly, I'd say a driver
> that claims a larger region is buggy and should be fixed.

Yep, but a temporary solution where everything just works fine and a
message: "This driver needs fixing" is needed IMO (if the code gets
accepted... It's possible, but ...).

Re: [PATCH - 0/2] Identify native drivers and ACPI subsystem accessing the same resources

[Bjorn Helgaas]

On Wednesday 18 July 2007 11:50:38 am Thomas Renninger wrote:
> On Fri, 2007-07-13 at 10:56 -0600, Bjorn Helgaas wrote:
> > The PNP core does not request resources for devices that are active
> > at boot.  Those resources currently don't get requested until a driver
> > claims the device.  I think this is a bug that we should fix.
> Yep, this is also a problem for the ACPI variables (in fact not really,
> as long as not overlapping like the rtc resource) and probably the
> reason why pnpacpi ignores addresses below 0x100?

By "ACPI variables," do you mean PM1a_EVT_BLK and the like?  Those
should be subsets of the _CRS resources for some device.

> IMO:
>   - Making sure ACPI claimed regions do not interfere with native
>     drivers is very important and will get much more important in near 
>     future

I agree with this.  I know some of these native drivers (lm-sensors, etc)
are pretty essential right now.  But they just aren't safe because they
use resources that ACPI thinks it owns.  I think the drivers should be
changed to explicitly override (with appropriate KERN_WARN messages)
any prior ACPI resource reservations.

> > >     The rtc driver seems to request the whole rtc space (0x70-0x77)
> > >     which fails because 0x72-0x73 has already been requested SHARED by
> > >     an ACPI SystemIO variable. At least parts of the rtc space got
> > >     requested (rtc0), I haven't checked whether the device was working
> > >     correctly...
> > 
> > I tripped over a couple of these when I changed the PNP core to request
> > resources for active devices.  The RTC is one; often BIOS says the RTC
> > is at 0x70-0x71 and 0x72-0x73, but the rtc drivers like to claim 0x8
> > (RTC_IO_EXTENT) ports.  However, I think the driver only *uses* two
> > ports, so we should change the driver to only request what it is
> > using.

> But there could be more?

I think the PNP core should request all the ports the device responds
to (as reported by the BIOS).  The driver should request only the ports
it uses.  If the rtc driver only uses two ports, there's no reason for
it to request all eight.

> Argggggh, normally it should be:
> 5000-50fe : ACPI PMIO
> ...
>   50c0-50df : pnp 00:07
>   50e0-50ff : pnp 00:07       # this one is missing because
>                               # it's bigger than the parent
>     50e0-50ef : amd756_smbus

IMO, this is backwards.  "pnp 00:07" should be the parent and should
be reserved by the PNP core.  A side-effect of this is that
drivers/pnp/system.c would not be needed at all.

> > > Summary:
> > > ...
> > >   - Not fixable (maybe someone has an idea): If ACPI IO region declares
> > >     a smaller IO part than the later native driver (e.g. example above
> > >     with rtc driver), the partially overlapping resource cannot be
> > >     registered and the native driver fails to load with strict and lax
> > >     option.
> > 
> > Assuming the BIOS describes the region correctly, I'd say a driver
> > that claims a larger region is buggy and should be fixed.
> 
> Yep, but a temporary solution where everything just works fine and a
> message: "This driver needs fixing" is needed IMO (if the code gets
> accepted... It's possible, but ...).

How about something like this:  We fix all the native drivers we know
about.  We make the PNP and ACPI cores request resources for all active
devices by default, but add a flag like "pnp=no_reservations" that turns
this off.  Then native drivers that request conflicting resources
will fail, but the user can limp along by using the boot-time option.

Bjorn

Re: [PATCH - 0/2] Identify native drivers and ACPI subsystem accessing the same resources

[Thomas Renninger]

On Wed, 2007-07-18 at 16:10 -0600, Bjorn Helgaas wrote:
> On Wednesday 18 July 2007 11:50:38 am Thomas Renninger wrote:
> > On Fri, 2007-07-13 at 10:56 -0600, Bjorn Helgaas wrote:
> > > The PNP core does not request resources for devices that are active
> > > at boot.  Those resources currently don't get requested until a driver
> > > claims the device.  I think this is a bug that we should fix.
> > Yep, this is also a problem for the ACPI variables (in fact not really,
> > as long as not overlapping like the rtc resource) and probably the
> > reason why pnpacpi ignores addresses below 0x100?
> 
> By "ACPI variables," do you mean PM1a_EVT_BLK and the like?  Those
> should be subsets of the _CRS resources for some device.
No, I mean SystemIO OperationRegion declarations, like:
OperationRegion (IOID, SystemIO, 0x2E, 0x02)
Field (IOID, ByteAcc, NoLock, Preserve)
{
     INDX,   8, 
     DATA,   8
}
It may happen that you see e.g.:
0080-008f : dma page reg     # statically requested early in setup.c
  0080-0080 : ACPI DEB0      # ACPI SystemIO OperationRegion
                             # requested at ACPI table parse time

It's not nice..., but also does not hurt, as long as those do not
overlap. Even if these overlap the ACPI region/variable just won't be
able to reserve the region, not perfect, but better than before when
ACPI regions/variables were simply ignored.

> > IMO:
> >   - Making sure ACPI claimed regions do not interfere with native
> >     drivers is very important and will get much more important in near 
> >     future
> 
> I agree with this.  I know some of these native drivers (lm-sensors, etc)
> are pretty essential right now.  But they just aren't safe because they
> use resources that ACPI thinks it owns.  I think the drivers should be
> changed to explicitly override (with appropriate KERN_WARN messages)
> any prior ACPI resource reservations.
> 
> > > >     The rtc driver seems to request the whole rtc space (0x70-0x77)
> > > >     which fails because 0x72-0x73 has already been requested SHARED by
> > > >     an ACPI SystemIO variable. At least parts of the rtc space got
> > > >     requested (rtc0), I haven't checked whether the device was working
> > > >     correctly...
> > > 
> > > I tripped over a couple of these when I changed the PNP core to request
> > > resources for active devices.  The RTC is one; often BIOS says the RTC
> > > is at 0x70-0x71 and 0x72-0x73, but the rtc drivers like to claim 0x8
> > > (RTC_IO_EXTENT) ports.  However, I think the driver only *uses* two
> > > ports, so we should change the driver to only request what it is
> > > using.
> 
> > But there could be more?
> 
> I think the PNP core should request all the ports the device responds
> to (as reported by the BIOS).  The driver should request only the ports
> it uses.  If the rtc driver only uses two ports, there's no reason for
> it to request all eight.
> 
> > Argggggh, normally it should be:
> > 5000-50fe : ACPI PMIO
> > ...
> >   50c0-50df : pnp 00:07
> >   50e0-50ff : pnp 00:07       # this one is missing because
> >                               # it's bigger than the parent
> >     50e0-50ef : amd756_smbus
> 
> IMO, this is backwards.  "pnp 00:07" should be the parent and should
> be reserved by the PNP core.  A side-effect of this is that
> drivers/pnp/system.c would not be needed at all.

This patch is about to reserve the ACPI regions/variables (see the AML
descriptions at the beginning).
If I interpreted the code and your comment right, only PNP0c02 and
PNP0c01 is served (reserved) by pnpacpi layer currently (didn't see this
first). And you want to let pnpacpi reserve all ACPI device specific
resources (_CRS)? This should be done by removing system.c and the
interface to identify specific PNP devices (probe, device_id,..), but
just check for a _CRS function for all ACPI devices in
drivers/pnp/pnpacpi/core.c and reserve the resources.
IMO yes, this should be done and would be the first step.
Also reserve ACPI variables/OperationRegions, this is one is for, would
be the second step (and needs some more rework anyway).

I am off for a week, but will read and try out a bit more.
The next iteration will take some days (please stop me if you already
see some undoable hurdles that I may have overseen)...

Thanks a lot Bjorn, for the detailed review and all your input.

    Thomas

> > > > Summary:
> > > > ...
> > > >   - Not fixable (maybe someone has an idea): If ACPI IO region declares
> > > >     a smaller IO part than the later native driver (e.g. example above
> > > >     with rtc driver), the partially overlapping resource cannot be
> > > >     registered and the native driver fails to load with strict and lax
> > > >     option.
> > > 
> > > Assuming the BIOS describes the region correctly, I'd say a driver
> > > that claims a larger region is buggy and should be fixed.
> > 
> > Yep, but a temporary solution where everything just works fine and a
> > message: "This driver needs fixing" is needed IMO (if the code gets
> > accepted... It's possible, but ...).
> 
> How about something like this:  We fix all the native drivers we know
> about.  We make the PNP and ACPI cores request resources for all active
> devices by default, but add a flag like "pnp=no_reservations" that turns
> this off.  Then native drivers that request conflicting resources
> will fail, but the user can limp along by using the boot-time option.

Yes sounds good.
What I wanted to do is to also reserve the OperationRegions. I didn't
realize that pnp layer only reserves resources for specific devices and
this should be done first (reserve all).
I am pretty sure arbitrary AML code using variables from
OperationRegions can still be outside device specific resources and we
need to reserve those too (or at least have a boot option to identify
possible interference) to make sure ACPI interpreted code does not clash
with native drivers or at least identify which drivers potentially
interfere.
However, this needs an ugly implementation (extension of
kernel/resources.c) because those could partly overlap with device
specific ACPI resources (the pnp ones):

5000-50fe : ACPI PMIO   # ACPI variable/OperationRegion -> for ASL 
                        # example definition, see beginning of the mail
 ...
  50c0-50df : pnp 00:07  # These two are device specific ACPI resources
  50e0-50ff : pnp 00:07  # This one's end address overlaps by one byte
                         # with its parent -> must get accepted
     50e0-50ef : amd756_smbus

Then you'd have the pnp (pnpacpi only) and ACPI
variables/OperationRegions that should always be the parents (where the
variables are the parents of the pnpacpi resources) and those are
allowed to overlap and represent/reserve all ACPI resources.

Re: [PATCH - 0/2] Identify native drivers and ACPI subsystem accessing the same resources

[Thomas Renninger]

On Fri, 2007-07-20 at 15:54 +0200, Thomas Renninger wrote:
> On Wed, 2007-07-18 at 16:10 -0600, Bjorn Helgaas wrote:
> > On Wednesday 18 July 2007 11:50:38 am Thomas Renninger wrote:
> > > On Fri, 2007-07-13 at 10:56 -0600, Bjorn Helgaas wrote:
> > > > The PNP core does not request resources for devices that are active
> > > > at boot.  Those resources currently don't get requested until a driver
> > > > claims the device.  I think this is a bug that we should fix.
> > > Yep, this is also a problem for the ACPI variables (in fact not really,
> > > as long as not overlapping like the rtc resource) and probably the
> > > reason why pnpacpi ignores addresses below 0x100?
> > 
> > By "ACPI variables," do you mean PM1a_EVT_BLK and the like?  Those
> > should be subsets of the _CRS resources for some device.
> No, I mean SystemIO OperationRegion declarations, like:
> OperationRegion (IOID, SystemIO, 0x2E, 0x02)
> Field (IOID, ByteAcc, NoLock, Preserve)
> {
>      INDX,   8, 
>      DATA,   8
> }
> It may happen that you see e.g.:
> 0080-008f : dma page reg     # statically requested early in setup.c
>   0080-0080 : ACPI DEB0      # ACPI SystemIO OperationRegion
>                              # requested at ACPI table parse time
> 
> It's not nice..., but also does not hurt, as long as those do not
> overlap. Even if these overlap the ACPI region/variable just won't be
> able to reserve the region, not perfect, but better than before when
> ACPI regions/variables were simply ignored.
> 
> > > IMO:
> > >   - Making sure ACPI claimed regions do not interfere with native
> > >     drivers is very important and will get much more important in near 
> > >     future
> > 
> > I agree with this.  I know some of these native drivers (lm-sensors, etc)
> > are pretty essential right now.  But they just aren't safe because they
> > use resources that ACPI thinks it owns.  I think the drivers should be
> > changed to explicitly override (with appropriate KERN_WARN messages)
> > any prior ACPI resource reservations.
> > 
> > > > >     The rtc driver seems to request the whole rtc space (0x70-0x77)
> > > > >     which fails because 0x72-0x73 has already been requested SHARED by
> > > > >     an ACPI SystemIO variable. At least parts of the rtc space got
> > > > >     requested (rtc0), I haven't checked whether the device was working
> > > > >     correctly...
> > > > 
> > > > I tripped over a couple of these when I changed the PNP core to request
> > > > resources for active devices.  The RTC is one; often BIOS says the RTC
> > > > is at 0x70-0x71 and 0x72-0x73, but the rtc drivers like to claim 0x8
> > > > (RTC_IO_EXTENT) ports.  However, I think the driver only *uses* two
> > > > ports, so we should change the driver to only request what it is
> > > > using.
> > 
> > > But there could be more?
> > 
> > I think the PNP core should request all the ports the device responds
> > to (as reported by the BIOS).  The driver should request only the ports
> > it uses.  If the rtc driver only uses two ports, there's no reason for
> > it to request all eight.
> > 
> > > Argggggh, normally it should be:
> > > 5000-50fe : ACPI PMIO
> > > ...
> > >   50c0-50df : pnp 00:07
> > >   50e0-50ff : pnp 00:07       # this one is missing because
> > >                               # it's bigger than the parent
> > >     50e0-50ef : amd756_smbus
> > 
> > IMO, this is backwards.  "pnp 00:07" should be the parent and should
> > be reserved by the PNP core.  A side-effect of this is that
> > drivers/pnp/system.c would not be needed at all.
> 
> This patch is about to reserve the ACPI regions/variables (see the AML
> descriptions at the beginning).
> If I interpreted the code and your comment right, only PNP0c02 and
> PNP0c01 is served (reserved) by pnpacpi layer currently (didn't see this
> first). And you want to let pnpacpi reserve all ACPI device specific
> resources (_CRS)? This should be done by removing system.c and the
> interface to identify specific PNP devices (probe, device_id,..), but
> just check for a _CRS function for all ACPI devices in
> drivers/pnp/pnpacpi/core.c and reserve the resources.
> IMO yes, this should be done and would be the first step.
> Also reserve ACPI variables/OperationRegions, this is one is for, would
> be the second step (and needs some more rework anyway).
> 
> I am off for a week, but will read and try out a bit more.
> The next iteration will take some days (please stop me if you already
> see some undoable hurdles that I may have overseen)...
> 
> Thanks a lot Bjorn, for the detailed review and all your input.
> 
>     Thomas
> 
> > > > > Summary:
> > > > > ...
> > > > >   - Not fixable (maybe someone has an idea): If ACPI IO region declares
> > > > >     a smaller IO part than the later native driver (e.g. example above
> > > > >     with rtc driver), the partially overlapping resource cannot be
> > > > >     registered and the native driver fails to load with strict and lax
> > > > >     option.
> > > > 
> > > > Assuming the BIOS describes the region correctly, I'd say a driver
> > > > that claims a larger region is buggy and should be fixed.
> > > 
> > > Yep, but a temporary solution where everything just works fine and a
> > > message: "This driver needs fixing" is needed IMO (if the code gets
> > > accepted... It's possible, but ...).
> > 
> > How about something like this:  We fix all the native drivers we know
> > about.  We make the PNP and ACPI cores request resources for all active
> > devices by default, but add a flag like "pnp=no_reservations" that turns
> > this off.  Then native drivers that request conflicting resources
> > will fail, but the user can limp along by using the boot-time option.
> 
> Yes sounds good.
> What I wanted to do is to also reserve the OperationRegions. I didn't
> realize that pnp layer only reserves resources for specific devices and
> this should be done first (reserve all).
> I am pretty sure arbitrary AML code using variables from
> OperationRegions can still be outside device specific resources and we
> need to reserve those too (or at least have a boot option to identify
> possible interference) to make sure ACPI interpreted code does not clash
> with native drivers or at least identify which drivers potentially
> interfere.
> However, this needs an ugly implementation (extension of
> kernel/resources.c) because those could partly overlap with device
> specific ACPI resources (the pnp ones):
> 
> 5000-50fe : ACPI PMIO   # ACPI variable/OperationRegion -> for ASL 
>                         # example definition, see beginning of the mail
>  ...
>   50c0-50df : pnp 00:07  # These two are device specific ACPI resources
>   50e0-50ff : pnp 00:07  # This one's end address overlaps by one byte
>                          # with its parent -> must get accepted
>      50e0-50ef : amd756_smbus
> 
> Then you'd have the pnp (pnpacpi only) and ACPI
> variables/OperationRegions that should always be the parents (where the
> variables are the parents of the pnpacpi resources) and those are
> allowed to overlap and represent/reserve all ACPI resources.

Another idea just came to my mind:
Not reserving the ACPI variables/OperationRegions at all in
kernel/resources.c. Instead store these in a list in drivers/acpi/osl.c
as I've already done and add something at the beginning of:
kernel/resources.c:request_region()
like (pseudo-code):
#ifdef ACPI
    err = check_for_acpi_operation_region_clashes(struct *resource)
    if (err)
	...
#endif
this should be the most unintrusive way, (nearly) not polluting
kernel/resources.c
and in drivers/acpi/osl.c:
check_for_acpi_operation_region_clashes(struct *resource){
   ...
   if (clash)
      if (acpi_enforce_resources == LAX)
          pr_info ("Resource %s might interfere with ACPI"
                   " Operation region %s\n", ...);
          return no_error;
      else {
          printk (KERN_ERR "Resource %s interferes with ACPI"
                   " Operation region %s\n", ...);
          return error;
      }
 ...
}

Will try a bit more..., I will still read mails next week...

Thanks,

    Thomas