Message-ID: <1338398390.16730.483.camel@misato.fc.hp.com>
Date:	Wed, 30 May 2012 11:19:50 -0600
From:	Toshi Kani <toshi.kani@...com>
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 Wed, 2012-05-30 at 09:24 -0600, Shuah Khan wrote:
> 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.

This means this server is already working fine without Linux's _OST
support or with some private patch.

> 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.

This can be incremental effort if we indeed need _OST support for legacy
ACPI hotplug.  Jiang and I had also agreed on this.

> 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.

The OS-FW interface of docking / undocking operations is fairly
well-established with unique _DCK method.  It is not clear to me if
there is any need to modify the current interface with _OST.  There
might be such need in future, but I do not want to mess up with this
procedure with my speculation.  

> 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.

I agree with your concerns.  However, this patchset is the first step,
not the final step.  This first step is targeted to support the _OST
use-cases defined in the DIG64 HPPF spec.  We can continue to enhance it
as we find more needs.  I am willing to help anyone who has plan to
implement _OST for other use-cases.

Thanks,
-Toshi 


> -- Shuah
> 
> 


--
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