Re: [PATCH] ACPI: Fix osc flag setup ordering to allow pcie hotpluguse when available

[Neil Horman]

On Fri, Aug 23, 2013 at 04:04:59PM -0600, Bjorn Helgaas wrote:
> On Fri, Aug 23, 2013 at 3:40 PM, Rafael J. Wysocki <rjw@xxxxxxx> wrote:
> > On Friday, August 23, 2013 02:46:23 PM Bjorn Helgaas wrote:
> >> On Fri, Aug 23, 2013 at 2:53 PM, Rafael J. Wysocki <rjw@xxxxxxx> wrote:
> >> > On Friday, August 23, 2013 04:05:11 PM Neil Horman wrote:
> >> >> On Fri, Aug 23, 2013 at 09:38:18PM +0200, Rafael J. Wysocki wrote:
> >> >> > [CCs added]
> >> >> >
> >> >> > Please always send PCI-related material to linux-pci in the first place.
> >> >> >
> >> >> Sorry, I ran get_maintainers and it seemed to think linux-acpi was sufficient.
> >> >>
> >> >> > The change that broke things for you was actually intentional:
> >> >> >
> >> >> > commit b8178f130e25c1bdac1c33e0996f1ff6e20ec08e
> >> >> > Author: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> >> >> > Date:   Mon Apr 1 15:47:39 2013 -0600
> >> >> >
> >> >> >     Revert "PCI/ACPI: Request _OSC control before scanning PCI root bus"
> >> >> >
> >> >> >     This reverts commit 8c33f51df406e1a1f7fa4e9b244845b7ebd61fa6.
> >> >> >
> >> >> > so I think we'll need to clean up the ASMP initialization after all.
> >> >> >
> >> >> Crud.  Reading over the revert commit, it seems like the problem boils down to
> >> >> the odering of checking aspm_disabled.  It seems to me that the simple fix is to
> >> >> track the desire for acpi to disable aspm separately from the users desire to
> >> >> disable aspm (aspm_disabled).  Then we just turn the points where we check the
> >> >> aspm_disabled into the appropriate or of two variables, except for
> >> >> pcie_asmp_sanity_check, which remains sensitive to just the user disable option.
> >> >>
> >> >> Or is there more to this?
> >> >
> >> > No, I think you're right.
> >> >
> >> > Bjorn, what do you think?
> >>
> >> My opinion is that the _OSC/ASPM state management is ridiculously
> >> complicated already, and this would make it slightly more complicated.
> >>  That's why my preference would be to attempt a more radical cleanup
> >> and simplification instead of adding another wart.
> >
> > Well, do you have anything specific in mind?
> 
> If I had a specific fix in mind, I would just post it, but I've never
> had time to work through it all.  What I mean by complicated is that
> every time I have to look at ASPM, I have to start by trying to figure
> out the relationships between these variables:
> 
>     aspm_policy                 # default 0 (POLICY_DEFAULT)
>                                   or POLICY_PERFORMANCE
>                                   or POLICY_POWERSAVE depending on config
>     aspm_disabled               # default 0
>     aspm_force                  # default 0
>     aspm_support_enabled        # default true
> 
> plus the _OSC-related code in acpi_pci_root_add(), which honestly is a
> rat's nest.
> 
I agree, I've only looked at it for an hour, and it looks pretty hairy.

> It sounds like Neil's fix (while probably correct) would tangle that
> nest a little more.  But I guess it would make sense to see the actual
No argument, it would add complexity to something thats already very complex.
That said, I think this needs to be fixed.  Currently there are several systems
that are reporting conflicts between ACPI and PCIE hotplug.  While that means
theres lots of griping, theres several people willing to test, so I think we
have an opportunity to fix this.

> patch and the justification (a regression fix, I suppose, but the
> details weren't clear to me) before deciding.
> 
Agreed.  A larger cleanup would be preferable at this point, but I'm not
sufficiently versed in the code to do that right now, so I'll try write an
appropriate for this particular bug, and see what you think on monday.


Regards
Neil

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