Re: [Bugfix] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716

["Rafael J. Wysocki" <rjw@rjwysocki.net>]

On Tuesday, March 24, 2015 08:18:35 AM Bjorn Helgaas wrote:
> On Mon, Mar 23, 2015 at 9:59 PM, Jiang Liu <jiang.liu@linux.intel.com> wrote:
> > On 2015/3/24 10:42, Bjorn Helgaas wrote:
> >> On Mon, Mar 23, 2015 at 9:22 PM, Jiang Liu <jiang.liu@linux.intel.com> wrote:
> >>> On 2015/3/24 0:48, Bjorn Helgaas wrote:
> >>>> On Mon, Mar 23, 2015 at 03:22:14PM +0800, Jiang Liu wrote:
> >>>>> Commit 63f1789ec716("Ignore resources consumed by host bridge itself")
> >>>>> tries to ignore resources consumed by PCI host bridge itself by
> >>>>> checking IORESOURCE_WINDOW flag, which causes regression on some
> >>>>> platforms.
> >>>>
> >>>> "Do.  Or do not.  There is no try."
> >>>> [http://www.starwars.com/video/do-or-do-not]
> >>>>
> >>>> That commit doesn't *try* to do something.  It *does* something.  Just
> >>>> explain what it does and what's wrong with what it does.
> >>>>
> >>>>> For example, PC Engines APU.1C platform defines PCI MMIO resources with
> >>>>> ACPI Memory32Fixed operator as below:
> >>>>> Name (CRES, ResourceTemplate ()
> >>>>> {
> >>>>>     ...
> >>>>>     WordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode,
> >>>>>         0x0000,             // Granularity
> >>>>>         0x0D00,             // Range Minimum
> >>>>>         0xFFFF,             // Range Maximum
> >>>>>         0x0000,             // Translation Offset
> >>>>>         0xF300,             // Length
> >>>>>         ,, , TypeStatic)
> >>>>>     Memory32Fixed (ReadOnly,
> >>>>>         0x000A0000,         // Address Base
> >>>>>         0x00020000,         // Address Length
> >>>>>         )
> >>>>>     Memory32Fixed (ReadOnly,
> >>>>>         0x00000000,         // Address Base
> >>>>>         0x00000000,         // Address Length
> >>>>>         _Y00)
> >>>>> })
> >>>>>
> >>>>> Memory32Fixed operator doesn't support concept of "producer/consumer"
> >>>>> and it will be treated as "consumer" by the ACPI resource parsing
> >>>>> interface, thus cause regression. So the fix is only to check
> >>>>> "producer/consumer" flag for resources having "producer/consumer" flag.
> >>>>
> >>>> Apparently the problem is with the Memory32Fixed resources above; it sounds
> >>>> like we ignore them after 63f1789ec716?  I don't quite understand how this
> >>>> fix works.  acpi_dev_filter_resource_type() has cases for both
> >>>> ACPI_RESOURCE_TYPE_FIXED_MEMORY32 and ACPI_RESOURCE_TYPE_ADDRESSxx, but
> >>>> this patch only touches the latter, not the
> >>>> ACPI_RESOURCE_TYPE_FIXED_MEMORY32 case.
> >>> The idea is:
> >>> 1) caller specifies IORESOURCE_WINDOW to query resources provided
> >>>    by the device, otherwise it's querying resources consumed by
> >>>    the device.
> >>> 2) For resource descriptors having producer/consumer flag, such as
> >>>     ACPI_RESOURCE_TYPE_ADDRESSxx, we check the producer/consumer flag.
> >>> 3) For resource descriptors not having producer/consumer flag, such
> >>>    as ACPI_RESOURCE_TYPE_FIXED_MEMORY32, we skip checking the
> >>>    producer/consumer flag.
> >>
> >> I figured out that much by reading the code.  But I think the code is
> >> very hard to read, and I still don't really understand how it works.
> >>
> >> Before this fix, we ignore Memory32Fixed resources.  After this fix,
> >> we use Memory32Fixed as a window.  I think that's an incorrect
> >> interpretation of Memory32Fixed.
> > Hi Bjorn,
> >         I think it's illegal to use Memory32Fixed for PCI host
> > bridge resource window too. The fix is to solve the regression
> > by relaxing constraints, but that may not be the right solution.
> 
> Relaxing the constraint for all platforms is only a solution if we're
> confident that it works for all platforms.  I'm not confident because
> I don't know what effect this patch has on systems that use
> Memory32Fixed correctly.
> 
> It's possible that you can analyze the behavior and explain that if
> you relax the constraint, the Memory32Fixed handling (both for bridge
> and non-bridge devices) will be the same as it was before
> 63f1789ec716.  If you can do that, I think it would be reasonable
> because you'd be preserving the previous, known-working behavior.
> 
> If you go that route, I'd like to see a patch that explicitly touches
> the Memory32Fixed handling.  The current patch claims to change the
> way we handle Memory32Fixed, but nothing in the code change is
> directly related to Memory32Fixed, so it's very confusing.
> 
> >         How about waiting for a while to see whether there are more
> > bug reports related to this. If only limited platform affected,
> > we could treat it as BIOS bugs and use quirk to handle it.
> > Otherwise we may need to relax the constraint.
> 
> I don't think waiting is a good strategy.  We know we have a
> regression, and I think we need to fix that ASAP without waiting for
> more failure reports.

Agreed on all points.

Rafael