Message-Id: <1184941324.15328.222.camel@queen.suse.de>
Date:	Fri, 20 Jul 2007 16:22:04 +0200
From:	Thomas Renninger <trenn@...e.de>
To:	Bjorn Helgaas <bjorn.helgaas@...com>
Cc:	linux-kernel <linux-kernel@...r.kernel.org>,
	Bernhard Walle <bwalle@...e.de>,
	"Starikovskiy, Alexey Y" <aystarik@...il.com>,
	linux-acpi <linux-acpi@...r.kernel.org>,
	akpm <akpm@...ux-foundation.org>, lm-sensors@...sensors.org,
	Jean Delvare <khali@...ux-fr.org>
Subject: Re: [PATCH - 0/2] Identify native drivers and ACPI subsystem
	accessing the same resources

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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists