Date:	Tue, 29 May 2012 22:27:04 +0000
From:	"Moore, Robert" <robert.moore@...el.com>
To:	Toshi Kani <toshi.kani@...com>,
	"shuahkhan@...il.com" <shuahkhan@...il.com>
CC:	"lenb@...nel.org" <lenb@...nel.org>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	"bhelgaas@...gle.com" <bhelgaas@...gle.com>,
	"liuj97@...il.com" <liuj97@...il.com>,
	"andi@...stfloor.org" <andi@...stfloor.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v4 0/6] ACPI: Add _OST support for ACPI hotplug

> > 2. Calling acpi_get_handle() on _OST prior to executing the method.
> > This will ensure that this method only gets run if it is present
> under
> > the device in question. Coupled with what is already outlined in #1
> > above, now _OST gets executed only when it is defined under the
> device object.
> > Example case in the existing code, please see
> acpi_processor_ppc_ost()
> > implementation.
> 
> Yes, I did look at acpi_processor_ppc_ost() when I implemented the
> function.  I believe calling acpi_get_handle() is redundant since
> acpi_ns_get_node() is called within acpi_evaluate_object() as well.
> acpi_evaluate_object() simply returns with AE_NOT_FOUND when _OST
> method does not exist.
>

This is correct. If _OST does not exist, AE_NOT_FOUND will be returned from evaluate_object.




> -----Original Message-----
> From: linux-acpi-owner@...r.kernel.org [mailto:linux-acpi-
> owner@...r.kernel.org] On Behalf Of Toshi Kani
> Sent: Thursday, May 24, 2012 12:49 PM
> To: shuahkhan@...il.com
> Cc: lenb@...nel.org; linux-acpi@...r.kernel.org; bhelgaas@...gle.com;
> liuj97@...il.com; andi@...stfloor.org; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH v4 0/6] ACPI: Add _OST support for ACPI hotplug
> 
> 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.
> 
> If user defines ACPI_HOTPLUG_OST at compile time, it forces the _OST to
> be supported. It works fine as long as the kernel hotplug capabilities
> match with the platform.
> 
> > _OST is a platform capability and once OS tells firmware it can
> > support it, isn't it expected to call all _OST method if present
> under
> > any device object that is hotplug capable? What are the implications
> > and issues if OS doesn't evaluate _OST on PCI for example, once it
> > tells the firmware it supports _OST as a platform capability when it
> > evaluates _OSC? The question is, is it safe? This goes back to the
> > first goal on exposure.
> 
> Yes, and therefore, this _OST support is enabled when all relevant
> hotplug operations are enabled.
> 
> > Also, when _OSC is evaluated, the _OST code in this patch set, tells
> > firmware it supports _OST, however it doesn't check whether or not
> the
> > firmware actually acked the capability or not. Hence, it doesn't make
> > sure firmware has support for it, before it enables the _OST.
> 
> This step is not necessary because FW does not implement _OST method
> when it does not support it.
> 
> > I think you will get closer to implementing a solution that achieves
> > the stated objectives by changing the design some as outlined below:
> (
> > I am sure there are other ideas )
> >
> > 1. implement this similar to the way APEI support
> > (OSC_SB_APEI_SUPPORT) is implemented by checking the firmware ack and
> > enabling the apei support using a bool osc_sb_apei_support_acked
> which
> > is only set when firmware acknowledges back saying it can also
> support
> > _OST. This ensures that the feature is enabled only when both OS and
> firmware support it.
> > It will also get rid of the compile time restrictions.
> 
> APEI needs the OS to configure FW mode, so this acknowledge is
> necessary. This step is not necessary for _OST.
> 
> > 2. Calling acpi_get_handle() on _OST prior to executing the method.
> > This will ensure that this method only gets run if it is present
> under
> > the device in question. Coupled with what is already outlined in #1
> > above, now _OST gets executed only when it is defined under the
> device object.
> > Example case in the existing code, please see
> acpi_processor_ppc_ost()
> > implementation.
> 
> Yes, I did look at acpi_processor_ppc_ost() when I implemented the
> function.  I believe calling acpi_get_handle() is redundant since
> acpi_ns_get_node() is called within acpi_evaluate_object() as well.
> acpi_evaluate_object() simply returns with AE_NOT_FOUND when _OST
> method does not exist.
> 
> > Please see other examples of _OST implementation in the kernel that
> > implement _OST for PAD and _PPC. These two examples will help you
> > understand my premise for these review comments.
> 
> I think acpi_processor_ppc_ost() has a bug since it calls _OST with 2
> parameters. _OST is defined to have 3 parameters. When I called my _OST
> method with 2 parameters for testing, it generated a warning message.
> 
> 
> Thanks,
> -Toshi
> 
> 
> > Thanks,
> > -- Shuah
> >
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi"
> in the body of a message to majordomo@...r.kernel.org More majordomo
> info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists