On Thu, 2012-05-24 at 13:48 -0600, Toshi Kani wrote: > On Thu, 2012-05-24 at 11:34 -0600, Shuah Khan wrote: > > On Wed, 2012-05-23 at 20:25 -0600, Toshi Kani wrote: > > > This patchset supports ACPI OSPM Status Indication (_OST) method for > > > ACPI CPU/memory/container hotplug operations and sysfs eject. After > > > an ACPI hotplug operation has completed, OSPM calls _OST to indicate > > > the result of the operation to the platform. If a platform does not > > > support _OST, this patchset has no effect on the platform. > > > > > > This _OST support is enabled when all relevant ACPI hotplug operations, > > > such as CPU, memory and container hotplug, are enabled. This assures > > > consistent behavior among the hotplug operations with regarding the > > > _OST support. > > > > > > Some platforms may require the OS to support _OST in order to support > > > ACPI hotplug operations. For example, if a platform has the management > > > console where user can request a hotplug operation from, this _OST > > > support would be required for the management console to show the result > > > of the hotplug request to user. > > > > > > The _OST definition can be found in section 6.3.5 of ACPI 5.0 spec. > > > The HPPF spec below also describes hotplug flows with _OST. > > > > > > DIG64 Hot-Plug & Partitioning Flow (HPPF) Specification R1.0 > > > http://www.dig64.org/home/DIG64_HPPF_R1_0.pdf > > > > > > The changes have been tested with simulated _OST methods. > > > > Toshi, > > > > First of all thanks for asking for my feedback. :) Having benefited from > > reviewing the previous versions of this patch set, my thoughts on the > > implementation have evolved. > > Thanks for reviewing! :) > > > I have some general comments first in the response, and please find code > > specific comments on individual patches. > > > > This patch set enables Insertion/Ejection _OST processing support which > > will be a good addition since OS already supports it for Processor > > Aggregator Device Support and _PPC. > > Right. > > > However, in this case it is enabled as a compile time option and would > > require a kernel build when firmware starts supporting _OST method in > > some cases. Reference: PATCH v4 1/6. > > Yes, it requires ACPI CPU, Memory and Container hotplug be enabled in the kernel. > > > It also restricts the support to be all or nothing. i.e _OST is > > supported only when all relevant hotplug operations are supported and > > these need to be specifically enabled using the config options that > > control it. For example, if a platform supports CPU_HOTPLUG and not > > MEMORY_HOTPLUG, _OST support will be disabled even when firmware > > supports it for cpus. Also the set of hotplug operations is limited as > > _OST could be present in other hotplug cases such as PCI and PCIe. > > > > I understand the spirit of this restriction that you are trying to limit > > the exposure and it is a good goal. However, it probably could be > > achieved in a way that doesn't shoehorn the implementation. > > This restriction is to assure that the OS is compliant with the ACPI > spec. When the OS calls _OSC with the hotplug _OST bit set, the OS needs > to support _OST for all relevant ACPI hotplug operations. Unfortunately, > this requires all relevant hotplug modules be enabled in the OS under > the current implementation. > > For example, when the platform supports ACPI memory hotplug, but > ACPI_HOTPLUG_MEMORY is undefined in the OS, the OS needs to call _OSC > with the hotplug _OST bit unset. This is because the OS cannot receive > an ACPI notification to a memory object when ACPI_HOTPLUG_MEMORY is > undefined. Without the notify handler, we cannot call _OST. > > A long term solution to address this issue is to have the system global > notify handler to receive all hotplug notifications, and call _OST > accordingly. However, it will require restructuring efforts which well > beyond the scope of this patchset. The email below describes this issue > and my thoughts on this. > http://marc.info/?l=linux-acpi&m=133546048929384&w=2 > > > I think here are the goals, > > > > 1. limit exposure so platforms that don't implement _OST are not > > penalized evaluation _OST unnecessarily. > > This goal is met since the OS cannot evaluate _OST unless it is > implemented. > > > 2. enable it when needed without requiring special compile time steps > > and not worrying about sorting through various config options. > > I agree, but as I explained above, this is required to be compliant with > ACPI spec at this point. We can remove this restriction by improving the > notify handler design, but it will take more steps to do so. > > > 3. don't require all hotplug variants to be enabled in config, before OS > > enables _OST support. > > I agree, but the same reason above. > > > I see that you are enabling _OST evaluation and set the cap bit > > OSC_SB_PPC_OST_SUPPORT only when ACPI_HOTPLUG_OST is defined. What > > happens on when a kernel is configured with the config options that > > enable ACPI_HOTPLUG_OST at compile time, and other hotplug options for > > example CONFIG_HOTPLUG_PCI_PCIE, and CONFIG_HOTPLUG_PCI. > > Non-ACPI hotplug operations like PCIe native hotplug are irrelevant to _OST. Yes I agree with your statement about PCIe native hot-plug operations. However, as Jiang Liu pointed out in one of the reviews of an earlier version of this patch set, _OST method has been defined in ACPI4.0 spec and there are some platforms that already implement the _OST method. For example, Quanta QSSC-S4R server implements _OST for hot-pluggable PCI slots. So, we do have one example of a server that implements it for hot-pluggable PCI slots. Even if APCI PCI hotplug becomes legacy only, it still needs to be supported. Based on my reading of the ACPI 5.0 Spec, _OST method as it is defined under the scope of Device Ejection/Insertion is applicable to not just memory, cpu, container, and PCI slots, it could also be applicable depending how a platform chooses implement it, "even in the cases of docking and undocking mobile platforms to and from a peripheral expansion dock." Reference: 6.3 of ACPI 5.0 Spec. So I think it is wrong and narrow scoped to assume _OST will be and is implemented only in the device ejection/insertion cases this patch set addresses. -- Shuah -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
