* [PATCH -v2 0/2] ACPI, APEI, use general HEST table parsing in AER firmware_first setup (resend) @ 2010-03-11 2:14 Huang Ying 2010-03-11 2:14 ` [PATCH -v2 1/2] ACPI, APEI, Make APEI core configurable built-in instead of module Huang Ying 0 siblings, 1 reply; 6+ messages in thread From: Huang Ying @ 2010-03-11 2:14 UTC (permalink / raw) To: Len Brown, Jesse Barnes; +Cc: linux-kernel, Andi Kleen, linux-acpi, linux-pci Changelog: v2: - Setup firmware_first for every PCIE end point instead of just root point. - Wrap firmware accessing in a function instead of raw hardware, because HEST parsing involved. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH -v2 1/2] ACPI, APEI, Make APEI core configurable built-in instead of module 2010-03-11 2:14 [PATCH -v2 0/2] ACPI, APEI, use general HEST table parsing in AER firmware_first setup (resend) Huang Ying @ 2010-03-11 2:14 ` Huang Ying 2010-03-11 2:14 ` [PATCH -v2 2/2] ACPI, APEI, PCIE AER, use general HEST table parsing in AER firmware_first setup Huang Ying 0 siblings, 1 reply; 6+ messages in thread From: Huang Ying @ 2010-03-11 2:14 UTC (permalink / raw) To: Len Brown, Jesse Barnes Cc: linux-kernel, Andi Kleen, linux-acpi, linux-pci, Huang Ying PCIE AER optionally depends on APEI HEST tabling parsing. If AER is built-in, HEST should be built-in or not configured at all instead of module. It is hard to express this elegantly in Kconfig. It is better to make APEI code part configurable built-in instead of module. On the other hand, APEI core code is used for hardware error processing. It may run in very bad condition. It is reasonable to keep it built-in if enabled. Signed-off-by: Huang Ying <ying.huang@intel.com> --- Documentation/kernel-parameters.txt | 10 ++++---- drivers/acpi/apei/Kconfig | 2 - drivers/acpi/apei/apei-base.c | 42 +++++------------------------------- drivers/acpi/apei/apei-internal.h | 4 --- drivers/acpi/apei/einj.c | 2 - drivers/acpi/apei/hest.c | 34 ++++++++++++++++++++++++----- 6 files changed, 42 insertions(+), 52 deletions(-) --- a/Documentation/kernel-parameters.txt +++ b/Documentation/kernel-parameters.txt @@ -351,11 +351,6 @@ and is between 256 and 4096 characters. not play well with APC CPU idle - disable it if you have APC and your system crashes randomly. - apei.hest_disable= [ACPI] - Disable Hardware Error Source Table (HEST) support, - corresponding firmware-first mode error processing - logic will be disabled. - apic= [APIC,X86-32] Advanced Programmable Interrupt Controller Change the output verbosity whilst booting Format: { quiet (default) | verbose | debug } @@ -857,6 +852,11 @@ and is between 256 and 4096 characters. hd= [EIDE] (E)IDE hard drive subsystem geometry Format: <cyl>,<head>,<sect> + hest_disable [ACPI] + Disable Hardware Error Source Table (HEST) support, + corresponding firmware-first mode error processing + logic will be disabled. + highmem=nn[KMG] [KNL,BOOT] forces the highmem zone to have an exact size of <nn>. This works even on boxes that have no highmem otherwise. This also works to reduce highmem --- a/drivers/acpi/apei/Kconfig +++ b/drivers/acpi/apei/Kconfig @@ -1,5 +1,5 @@ config ACPI_APEI - tristate "ACPI Platform Error Interface (APEI)" + bool "ACPI Platform Error Interface (APEI)" depends on X86 help APEI allows to report errors (for example from the chipset) --- a/drivers/acpi/apei/apei-base.c +++ b/drivers/acpi/apei/apei-base.c @@ -45,12 +45,6 @@ #define APEI_PFX "APEI: " -struct dentry *apei_debug_dir; -EXPORT_SYMBOL_GPL(apei_debug_dir); - -int hest_disable; -EXPORT_SYMBOL(hest_disable); - /* * APEI ERST (Error Record Serialization Table) and EINJ (Error * INJection) interpreter framework. @@ -553,37 +547,13 @@ int apei_exec_collect_resources(struct a } EXPORT_SYMBOL_GPL(apei_exec_collect_resources); -static int __init apei_init(void) +struct dentry *apei_get_debugfs_dir(void) { - int rc; - - apei_debug_dir = debugfs_create_dir("apei", NULL); - if (!apei_debug_dir) - return -ENOMEM; - if (!hest_disable) { - rc = hest_init(); - if (rc) { - hest_disable = 1; - if (rc != -ENODEV) - pr_err( - "ACPI: APEI: Failed to initialize Hardware " - "Error Source Table (HEST) subsystem\n"); - } - } + static struct dentry *dapei; - return 0; -} + if (!dapei) + dapei = debugfs_create_dir("apei", NULL); -static void __exit apei_exit(void) -{ - debugfs_remove_recursive(apei_debug_dir); + return dapei; } - -module_init(apei_init); -module_exit(apei_exit); - -module_param(hest_disable, int, 0444); - -MODULE_AUTHOR("Huang Ying"); -MODULE_DESCRIPTION("ACPI Platform Error Interface support"); -MODULE_LICENSE("GPL"); +EXPORT_SYMBOL_GPL(apei_get_debugfs_dir); --- a/drivers/acpi/apei/apei-internal.h +++ b/drivers/acpi/apei/apei-internal.h @@ -6,8 +6,6 @@ #ifndef APEI_INTERNAL_H #define APEI_INTERNAL_H -int hest_init(void); - struct apei_exec_context; typedef int (*apei_exec_ins_func_t)(struct apei_exec_context *ctx, @@ -93,5 +91,5 @@ int apei_exec_collect_resources(struct a struct apei_resources *resources); struct dentry; -extern struct dentry *apei_debug_dir; +struct dentry *apei_get_debugfs_dir(void); #endif --- a/drivers/acpi/apei/einj.c +++ b/drivers/acpi/apei/einj.c @@ -409,7 +409,7 @@ static int __init einj_init(void) } rc = -ENOMEM; - einj_debug_dir = debugfs_create_dir("einj", apei_debug_dir); + einj_debug_dir = debugfs_create_dir("einj", apei_get_debugfs_dir()); if (!einj_debug_dir) goto err_cleanup; fentry = debugfs_create_file("available_error_type", S_IRUSR, --- a/drivers/acpi/apei/hest.c +++ b/drivers/acpi/apei/hest.c @@ -40,6 +40,9 @@ #define HEST_PFX "HEST: " +int hest_disable; +EXPORT_SYMBOL_GPL(hest_disable); + /* HEST table parsing */ static struct acpi_table_hest *hest_tab; @@ -118,30 +121,49 @@ int apei_hest_parse(apei_hest_func_t fun } EXPORT_SYMBOL_GPL(apei_hest_parse); -int __init hest_init(void) +static int __init setup_hest_disable(char *str) +{ + hest_disable = 1; + return 0; +} + +__setup("hest_disable", setup_hest_disable); + +static int __init hest_init(void) { acpi_status status; - int rc; + int rc = 0; if (acpi_disabled) - return -ENODEV; + goto err; + + if (hest_disable) { + pr_info(HEST_PFX "HEST tabling parsing is disabled.\n"); + goto err; + } status = acpi_get_table(ACPI_SIG_HEST, 0, (struct acpi_table_header **)&hest_tab); if (status == AE_NOT_FOUND) { pr_info(HEST_PFX "Table is not found!\n"); - return -ENODEV; + goto err; } else if (ACPI_FAILURE(status)) { const char *msg = acpi_format_exception(status); pr_info(HEST_PFX "Failed to get table, %s\n", msg); - return -EINVAL; + rc = -EINVAL; + goto err; } rc = apei_hest_parse(hest_void_parse, NULL); if (rc) - return rc; + goto err; pr_info(HEST_PFX "HEST table parsing is initialized.\n"); return 0; +err: + hest_disable = 1; + return rc; } + +subsys_initcall(hest_init); ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH -v2 2/2] ACPI, APEI, PCIE AER, use general HEST table parsing in AER firmware_first setup 2010-03-11 2:14 ` [PATCH -v2 1/2] ACPI, APEI, Make APEI core configurable built-in instead of module Huang Ying @ 2010-03-11 2:14 ` Huang Ying 2010-03-12 3:43 ` Hidetoshi Seto 0 siblings, 1 reply; 6+ messages in thread From: Huang Ying @ 2010-03-11 2:14 UTC (permalink / raw) To: Len Brown, Jesse Barnes Cc: linux-kernel, Andi Kleen, linux-acpi, linux-pci, Huang Ying Now, a dedicated HEST tabling parsing code is used for PCIE AER firmware_first setup. It is rebased on general HEST tabling parsing code of APEI. The firmware_first setup code is moved from PCI core to AER driver too, because it is only AER related. Signed-off-by: Huang Ying <ying.huang@intel.com> --- drivers/acpi/Makefile | 1 drivers/acpi/hest.c | 135 ------------------------------------- drivers/pci/pcie/aer/aerdrv.h | 9 ++ drivers/pci/pcie/aer/aerdrv_acpi.c | 70 +++++++++++++++++++ drivers/pci/pcie/aer/aerdrv_core.c | 15 +++- drivers/pci/probe.c | 8 -- include/acpi/acpi_hest.h | 12 --- include/linux/pci.h | 3 8 files changed, 92 insertions(+), 161 deletions(-) delete mode 100644 drivers/acpi/hest.c delete mode 100644 include/acpi/acpi_hest.h --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -20,7 +20,6 @@ obj-y += acpi.o \ # All the builtin files are in the "acpi." module_param namespace. acpi-y += osl.o utils.o reboot.o acpi-y += atomicio.o -acpi-y += hest.o # sleep related files acpi-y += wakeup.o --- a/drivers/acpi/hest.c +++ /dev/null @@ -1,135 +0,0 @@ -#include <linux/acpi.h> -#include <linux/pci.h> - -#define PREFIX "ACPI: " - -static inline unsigned long parse_acpi_hest_ia_machine_check(struct acpi_hest_ia_machine_check *p) -{ - return sizeof(*p) + - (sizeof(struct acpi_hest_ia_error_bank) * p->num_hardware_banks); -} - -static inline unsigned long parse_acpi_hest_ia_corrected(struct acpi_hest_ia_corrected *p) -{ - return sizeof(*p) + - (sizeof(struct acpi_hest_ia_error_bank) * p->num_hardware_banks); -} - -static inline unsigned long parse_acpi_hest_ia_nmi(struct acpi_hest_ia_nmi *p) -{ - return sizeof(*p); -} - -static inline unsigned long parse_acpi_hest_generic(struct acpi_hest_generic *p) -{ - return sizeof(*p); -} - -static inline unsigned int hest_match_pci(struct acpi_hest_aer_common *p, struct pci_dev *pci) -{ - return (0 == pci_domain_nr(pci->bus) && - p->bus == pci->bus->number && - p->device == PCI_SLOT(pci->devfn) && - p->function == PCI_FUNC(pci->devfn)); -} - -static unsigned long parse_acpi_hest_aer(void *hdr, int type, struct pci_dev *pci, int *firmware_first) -{ - struct acpi_hest_aer_common *p = hdr + sizeof(struct acpi_hest_header); - unsigned long rc=0; - u8 pcie_type = 0; - u8 bridge = 0; - switch (type) { - case ACPI_HEST_TYPE_AER_ROOT_PORT: - rc = sizeof(struct acpi_hest_aer_root); - pcie_type = PCI_EXP_TYPE_ROOT_PORT; - break; - case ACPI_HEST_TYPE_AER_ENDPOINT: - rc = sizeof(struct acpi_hest_aer); - pcie_type = PCI_EXP_TYPE_ENDPOINT; - break; - case ACPI_HEST_TYPE_AER_BRIDGE: - rc = sizeof(struct acpi_hest_aer_bridge); - if ((pci->class >> 16) == PCI_BASE_CLASS_BRIDGE) - bridge = 1; - break; - } - - if (p->flags & ACPI_HEST_GLOBAL) { - if ((pci->is_pcie && (pci->pcie_type == pcie_type)) || bridge) - *firmware_first = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST); - } - else - if (hest_match_pci(p, pci)) - *firmware_first = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST); - return rc; -} - -static int acpi_hest_firmware_first(struct acpi_table_header *stdheader, struct pci_dev *pci) -{ - struct acpi_table_hest *hest = (struct acpi_table_hest *)stdheader; - void *p = (void *)hest + sizeof(*hest); /* defined by the ACPI 4.0 spec */ - struct acpi_hest_header *hdr = p; - - int i; - int firmware_first = 0; - static unsigned char printed_unused = 0; - static unsigned char printed_reserved = 0; - - for (i=0, hdr=p; p < (((void *)hest) + hest->header.length) && i < hest->error_source_count; i++) { - switch (hdr->type) { - case ACPI_HEST_TYPE_IA32_CHECK: - p += parse_acpi_hest_ia_machine_check(p); - break; - case ACPI_HEST_TYPE_IA32_CORRECTED_CHECK: - p += parse_acpi_hest_ia_corrected(p); - break; - case ACPI_HEST_TYPE_IA32_NMI: - p += parse_acpi_hest_ia_nmi(p); - break; - /* These three should never appear */ - case ACPI_HEST_TYPE_NOT_USED3: - case ACPI_HEST_TYPE_NOT_USED4: - case ACPI_HEST_TYPE_NOT_USED5: - if (!printed_unused) { - printk(KERN_DEBUG PREFIX - "HEST Error Source list contains an obsolete type (%d).\n", hdr->type); - printed_unused = 1; - } - break; - case ACPI_HEST_TYPE_AER_ROOT_PORT: - case ACPI_HEST_TYPE_AER_ENDPOINT: - case ACPI_HEST_TYPE_AER_BRIDGE: - p += parse_acpi_hest_aer(p, hdr->type, pci, &firmware_first); - break; - case ACPI_HEST_TYPE_GENERIC_ERROR: - p += parse_acpi_hest_generic(p); - break; - /* These should never appear either */ - case ACPI_HEST_TYPE_RESERVED: - default: - if (!printed_reserved) { - printk(KERN_DEBUG PREFIX - "HEST Error Source list contains a reserved type (%d).\n", hdr->type); - printed_reserved = 1; - } - break; - } - } - return firmware_first; -} - -int acpi_hest_firmware_first_pci(struct pci_dev *pci) -{ - acpi_status status = AE_NOT_FOUND; - struct acpi_table_header *hest = NULL; - status = acpi_get_table(ACPI_SIG_HEST, 1, &hest); - - if (ACPI_SUCCESS(status)) { - if (acpi_hest_firmware_first(hest, pci)) { - return 1; - } - } - return 0; -} -EXPORT_SYMBOL_GPL(acpi_hest_firmware_first_pci); --- a/drivers/pci/pcie/aer/aerdrv.h +++ b/drivers/pci/pcie/aer/aerdrv.h @@ -134,4 +134,13 @@ static inline int aer_osc_setup(struct p } #endif +#ifdef CONFIG_ACPI_APEI +extern void aer_set_firmware_first(struct pci_dev *pci_dev); +#else +static inline void aer_set_firmware_first(struct pci_dev *pci_dev) +{ + pci_dev->__aer_firmware_first_valid = 1; +} +#endif + #endif /* _AERDRV_H_ */ --- a/drivers/pci/pcie/aer/aerdrv_acpi.c +++ b/drivers/pci/pcie/aer/aerdrv_acpi.c @@ -16,6 +16,7 @@ #include <linux/acpi.h> #include <linux/pci-acpi.h> #include <linux/delay.h> +#include <acpi/apei.h> #include "aerdrv.h" /** @@ -53,3 +54,72 @@ int aer_osc_setup(struct pcie_device *pc return 0; } + +#ifdef CONFIG_ACPI_APEI +static inline int hest_match_pci(struct acpi_hest_aer_common *p, struct pci_dev *pci) +{ + return (0 == pci_domain_nr(pci->bus) && + p->bus == pci->bus->number && + p->device == PCI_SLOT(pci->devfn) && + p->function == PCI_FUNC(pci->devfn)); +} + +struct aer_hest_parse_info +{ + struct pci_dev *pci_dev; + int firmware_first; +}; + +static int aer_hest_parse(struct acpi_hest_header *hest_hdr, void *data) +{ + struct aer_hest_parse_info *info = data; + struct acpi_hest_aer_common *p; + u8 pcie_type = 0; + u8 bridge = 0; + int ff = 0; + + switch (hest_hdr->type) { + case ACPI_HEST_TYPE_AER_ROOT_PORT: + pcie_type = PCI_EXP_TYPE_ROOT_PORT; + break; + case ACPI_HEST_TYPE_AER_ENDPOINT: + pcie_type = PCI_EXP_TYPE_ENDPOINT; + break; + case ACPI_HEST_TYPE_AER_BRIDGE: + if ((info->pci_dev->class >> 16) == PCI_BASE_CLASS_BRIDGE) + bridge = 1; + break; + default: + return 0; + } + + p = (struct acpi_hest_aer_common *)(hest_hdr + 1); + if (p->flags & ACPI_HEST_GLOBAL) { + if ((info->pci_dev->is_pcie && + info->pci_dev->pcie_type == pcie_type) || bridge) + ff = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST); + } else + if (hest_match_pci(p, info->pci_dev)) + ff = !!(p->flags & ACPI_HEST_FIRMWARE_FIRST); + info->firmware_first = ff; + + return 0; +} + +void aer_set_firmware_first(struct pci_dev *pci_dev) +{ + int rc; + struct aer_hest_parse_info info = { + .pci_dev = pci_dev, + .firmware_first = 0, + }; + + rc = apei_hest_parse(aer_hest_parse, &info); + + if (rc) + pci_dev->__aer_firmware_first = 0; + else + pci_dev->__aer_firmware_first = info.firmware_first; + pci_dev->__aer_firmware_first_valid = 1; +} +#endif --- a/drivers/pci/pcie/aer/aerdrv_core.c +++ b/drivers/pci/pcie/aer/aerdrv_core.c @@ -30,12 +30,19 @@ static int nosourceid; module_param(forceload, bool, 0); module_param(nosourceid, bool, 0); +static int pcie_aer_get_firmware_first(struct pci_dev *dev) +{ + if (!dev->__aer_firmware_first_valid) + aer_set_firmware_first(dev); + return dev->__aer_firmware_first; +} + int pci_enable_pcie_error_reporting(struct pci_dev *dev) { u16 reg16 = 0; int pos; - if (dev->aer_firmware_first) + if (pcie_aer_get_firmware_first(dev)) return -EIO; pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR); @@ -63,7 +70,7 @@ int pci_disable_pcie_error_reporting(str u16 reg16 = 0; int pos; - if (dev->aer_firmware_first) + if (pcie_aer_get_firmware_first(dev)) return -EIO; pos = pci_pcie_cap(dev); @@ -858,7 +865,7 @@ void aer_delete_rootport(struct aer_rpc */ int aer_init(struct pcie_device *dev) { - if (dev->port->aer_firmware_first) { + if (pcie_aer_get_firmware_first(dev->port)) { dev_printk(KERN_DEBUG, &dev->device, "PCIe errors handled by platform firmware.\n"); goto out; @@ -872,7 +879,7 @@ out: if (forceload) { dev_printk(KERN_DEBUG, &dev->device, "aerdrv forceload requested.\n"); - dev->port->aer_firmware_first = 0; + dev->port->__aer_firmware_first = 0; return 0; } return -ENXIO; --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -10,7 +10,6 @@ #include <linux/module.h> #include <linux/cpumask.h> #include <linux/pci-aspm.h> -#include <acpi/acpi_hest.h> #include "pci.h" #define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */ @@ -900,12 +899,6 @@ void set_pcie_hotplug_bridge(struct pci_ pdev->is_hotplug_bridge = 1; } -static void set_pci_aer_firmware_first(struct pci_dev *pdev) -{ - if (acpi_hest_firmware_first_pci(pdev)) - pdev->aer_firmware_first = 1; -} - #define LEGACY_IO_RESOURCE (IORESOURCE_IO | IORESOURCE_PCI_FIXED) /** @@ -935,7 +928,6 @@ int pci_setup_device(struct pci_dev *dev dev->multifunction = !!(hdr_type & 0x80); dev->error_state = pci_channel_io_normal; set_pcie_port_type(dev); - set_pci_aer_firmware_first(dev); list_for_each_entry(slot, &dev->bus->slots, list) if (PCI_SLOT(dev->devfn) == slot->number) --- a/include/acpi/acpi_hest.h +++ /dev/null @@ -1,12 +0,0 @@ -#ifndef __ACPI_HEST_H -#define __ACPI_HEST_H - -#include <linux/pci.h> - -#ifdef CONFIG_ACPI -extern int acpi_hest_firmware_first_pci(struct pci_dev *pci); -#else -static inline int acpi_hest_firmware_first_pci(struct pci_dev *pci) { return 0; } -#endif - -#endif --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -311,7 +311,8 @@ struct pci_dev { unsigned int is_virtfn:1; unsigned int reset_fn:1; unsigned int is_hotplug_bridge:1; - unsigned int aer_firmware_first:1; + unsigned int __aer_firmware_first_valid:1; + unsigned int __aer_firmware_first:1; pci_dev_flags_t dev_flags; atomic_t enable_cnt; /* pci_enable_device has been called */ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -v2 2/2] ACPI, APEI, PCIE AER, use general HEST table parsing in AER firmware_first setup 2010-03-11 2:14 ` [PATCH -v2 2/2] ACPI, APEI, PCIE AER, use general HEST table parsing in AER firmware_first setup Huang Ying @ 2010-03-12 3:43 ` Hidetoshi Seto 2010-03-12 7:59 ` Huang Ying 0 siblings, 1 reply; 6+ messages in thread From: Hidetoshi Seto @ 2010-03-12 3:43 UTC (permalink / raw) To: Huang Ying Cc: Len Brown, Jesse Barnes, linux-kernel, Andi Kleen, linux-acpi, linux-pci (2010/03/11 11:14), Huang Ying wrote: > -EXPORT_SYMBOL_GPL(acpi_hest_firmware_first_pci); > --- a/drivers/pci/pcie/aer/aerdrv.h > +++ b/drivers/pci/pcie/aer/aerdrv.h > @@ -134,4 +134,13 @@ static inline int aer_osc_setup(struct p > } > #endif > > +#ifdef CONFIG_ACPI_APEI > +extern void aer_set_firmware_first(struct pci_dev *pci_dev); > +#else > +static inline void aer_set_firmware_first(struct pci_dev *pci_dev) > +{ > + pci_dev->__aer_firmware_first_valid = 1; > +} > +#endif > + > #endif /* _AERDRV_H_ */ How about having aer_get_firmware_first() in this header too? > --- a/drivers/pci/pcie/aer/aerdrv_acpi.c > +++ b/drivers/pci/pcie/aer/aerdrv_acpi.c : > +#ifdef CONFIG_ACPI_APEI : > +void aer_set_firmware_first(struct pci_dev *pci_dev) > +{ : > +} > +#endif > --- a/drivers/pci/pcie/aer/aerdrv_core.c > +++ b/drivers/pci/pcie/aer/aerdrv_core.c > @@ -30,12 +30,19 @@ static int nosourceid; > module_param(forceload, bool, 0); > module_param(nosourceid, bool, 0); > > +static int pcie_aer_get_firmware_first(struct pci_dev *dev) > +{ > + if (!dev->__aer_firmware_first_valid) > + aer_set_firmware_first(dev); > + return dev->__aer_firmware_first; > +} > + Then you can put pcie_aer_get_firmware_first() to next to pcie_aer_set_firmware_first() in aerdrv_acpi.c, which is the best file for these functions I think. > @@ -872,7 +879,7 @@ out: > if (forceload) { > dev_printk(KERN_DEBUG, &dev->device, > "aerdrv forceload requested.\n"); > - dev->port->aer_firmware_first = 0; > + dev->port->__aer_firmware_first = 0; > return 0; > } > return -ENXIO; I'd like to see a purpose-oriented method here, something like pcie_aer_force_firmware_first_to(dev, forcedvalue). Or, it would be more better, change pcie_aer_set_firmware_first() (= currently used by APEI only) to static with better name. E.g. Before: After: # get a flag that tells @dev is firmware first or not pcie_aer_get_firmware_first(dev) => pcie_aer_get_firmware_first(dev) # no change # set a flag that tells @dev is firmware first or not dev->port->__aer_firmware_first = X => pcie_aer_set_firmware_first(dev, X) # parse hest to know @dev is firmware first or not pcie_aer_set_firmware_first(dev) => aer_parse_hest_for(dev) > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -311,7 +311,8 @@ struct pci_dev { > unsigned int is_virtfn:1; > unsigned int reset_fn:1; > unsigned int is_hotplug_bridge:1; > - unsigned int aer_firmware_first:1; > + unsigned int __aer_firmware_first_valid:1; > + unsigned int __aer_firmware_first:1; > pci_dev_flags_t dev_flags; > atomic_t enable_cnt; /* pci_enable_device has been called */ BTW, what the prefix "__" for? I recommend you to separate this 2/2 patch into 2 pieces, one for PCI addressed to Jesse, and another is for ACPI. i.e.: [PATCH] PCIE, AER: arrange pcie_aer_{get,set}_firmware_first [PATCH 1/2] ACPI, APEI: Make APEI core configurable built-in [PATCH 2/2] ACPI, APEI: use general HEST table parsing in AER You can make 3rd patch to contain only *acpi* changes, remove of acpi/hest.c and modify of pcie/aer/aerdrv_acpi.c. Then it will be easy-to-pull for Len, right? Thanks, H.Seto ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -v2 2/2] ACPI, APEI, PCIE AER, use general HEST table parsing in AER firmware_first setup 2010-03-12 3:43 ` Hidetoshi Seto @ 2010-03-12 7:59 ` Huang Ying 2010-03-13 1:23 ` Jesse Barnes 0 siblings, 1 reply; 6+ messages in thread From: Huang Ying @ 2010-03-12 7:59 UTC (permalink / raw) To: Hidetoshi Seto, Len Brown, Jesse Barnes Cc: linux-kernel@vger.kernel.org, Andi Kleen, linux-acpi@vger.kernel.org, linux-pci@vger.kernel.org On Fri, 2010-03-12 at 11:43 +0800, Hidetoshi Seto wrote: > (2010/03/11 11:14), Huang Ying wrote: > > -EXPORT_SYMBOL_GPL(acpi_hest_firmware_first_pci); > > --- a/drivers/pci/pcie/aer/aerdrv.h > > +++ b/drivers/pci/pcie/aer/aerdrv.h > > @@ -134,4 +134,13 @@ static inline int aer_osc_setup(struct p > > } > > #endif > > > > +#ifdef CONFIG_ACPI_APEI > > +extern void aer_set_firmware_first(struct pci_dev *pci_dev); > > +#else > > +static inline void aer_set_firmware_first(struct pci_dev *pci_dev) > > +{ > > + pci_dev->__aer_firmware_first_valid = 1; > > +} > > +#endif > > + > > #endif /* _AERDRV_H_ */ > > How about having aer_get_firmware_first() in this header too? > > > --- a/drivers/pci/pcie/aer/aerdrv_acpi.c > > +++ b/drivers/pci/pcie/aer/aerdrv_acpi.c > : > > +#ifdef CONFIG_ACPI_APEI > : > > +void aer_set_firmware_first(struct pci_dev *pci_dev) > > +{ > : > > +} > > +#endif > > --- a/drivers/pci/pcie/aer/aerdrv_core.c > > +++ b/drivers/pci/pcie/aer/aerdrv_core.c > > @@ -30,12 +30,19 @@ static int nosourceid; > > module_param(forceload, bool, 0); > > module_param(nosourceid, bool, 0); > > > > +static int pcie_aer_get_firmware_first(struct pci_dev *dev) > > +{ > > + if (!dev->__aer_firmware_first_valid) > > + aer_set_firmware_first(dev); > > + return dev->__aer_firmware_first; > > +} > > + > > Then you can put pcie_aer_get_firmware_first() to next to > pcie_aer_set_firmware_first() in aerdrv_acpi.c, which is > the best file for these functions I think. > > > @@ -872,7 +879,7 @@ out: > > if (forceload) { > > dev_printk(KERN_DEBUG, &dev->device, > > "aerdrv forceload requested.\n"); > > - dev->port->aer_firmware_first = 0; > > + dev->port->__aer_firmware_first = 0; > > return 0; > > } > > return -ENXIO; > > I'd like to see a purpose-oriented method here, something like > pcie_aer_force_firmware_first_to(dev, forcedvalue). > > Or, it would be more better, change pcie_aer_set_firmware_first() > (= currently used by APEI only) to static with better name. > > E.g. > Before: After: > > # get a flag that tells @dev is firmware first or not > pcie_aer_get_firmware_first(dev) > => pcie_aer_get_firmware_first(dev) # no change > > # set a flag that tells @dev is firmware first or not > dev->port->__aer_firmware_first = X > => pcie_aer_set_firmware_first(dev, X) > > # parse hest to know @dev is firmware first or not > pcie_aer_set_firmware_first(dev) > => aer_parse_hest_for(dev) Sounds reasonable. After putting current pcie_aer_set_firmware_first implementation into aerdrv_core.c, we can make it static. And we can add pcie_aer_force_firmware_first for forced setting. > > --- a/include/linux/pci.h > > +++ b/include/linux/pci.h > > @@ -311,7 +311,8 @@ struct pci_dev { > > unsigned int is_virtfn:1; > > unsigned int reset_fn:1; > > unsigned int is_hotplug_bridge:1; > > - unsigned int aer_firmware_first:1; > > + unsigned int __aer_firmware_first_valid:1; > > + unsigned int __aer_firmware_first:1; > > pci_dev_flags_t dev_flags; > > atomic_t enable_cnt; /* pci_enable_device has been called */ > > BTW, what the prefix "__" for? I want to warn users that this is a internal interface, don't use it directly. > I recommend you to separate this 2/2 patch into 2 pieces, > one for PCI addressed to Jesse, and another is for ACPI. > i.e.: > > [PATCH] PCIE, AER: arrange pcie_aer_{get,set}_firmware_first > [PATCH 1/2] ACPI, APEI: Make APEI core configurable built-in > [PATCH 2/2] ACPI, APEI: use general HEST table parsing in AER > > You can make 3rd patch to contain only *acpi* changes, remove of > acpi/hest.c and modify of pcie/aer/aerdrv_acpi.c. > Then it will be easy-to-pull for Len, right? Cross maintainers merging is painful after all. Even I split the patches as you proposed, I may need to wait another 2 merge windows to merge the code. To make it goes more smoothly, I suggest to merge all the code in Len's tree after getting ack from Jesse. Hi, Len and Jesse, how about my suggestion? Best Regards, Huang Ying ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -v2 2/2] ACPI, APEI, PCIE AER, use general HEST table parsing in AER firmware_first setup 2010-03-12 7:59 ` Huang Ying @ 2010-03-13 1:23 ` Jesse Barnes 0 siblings, 0 replies; 6+ messages in thread From: Jesse Barnes @ 2010-03-13 1:23 UTC (permalink / raw) To: Huang Ying Cc: Hidetoshi Seto, Len Brown, linux-kernel@vger.kernel.org, Andi Kleen, linux-acpi@vger.kernel.org, linux-pci@vger.kernel.org On Fri, 12 Mar 2010 15:59:40 +0800 Huang Ying <ying.huang@intel.com> wrote: > On Fri, 2010-03-12 at 11:43 +0800, Hidetoshi Seto wrote: > > (2010/03/11 11:14), Huang Ying wrote: > > > -EXPORT_SYMBOL_GPL(acpi_hest_firmware_first_pci); > > > --- a/drivers/pci/pcie/aer/aerdrv.h > > > +++ b/drivers/pci/pcie/aer/aerdrv.h > > > @@ -134,4 +134,13 @@ static inline int aer_osc_setup(struct p > > > } > > > #endif > > > > > > +#ifdef CONFIG_ACPI_APEI > > > +extern void aer_set_firmware_first(struct pci_dev *pci_dev); > > > +#else > > > +static inline void aer_set_firmware_first(struct pci_dev *pci_dev) > > > +{ > > > + pci_dev->__aer_firmware_first_valid = 1; > > > +} > > > +#endif > > > + > > > #endif /* _AERDRV_H_ */ > > > > How about having aer_get_firmware_first() in this header too? > > > > > --- a/drivers/pci/pcie/aer/aerdrv_acpi.c > > > +++ b/drivers/pci/pcie/aer/aerdrv_acpi.c > > : > > > +#ifdef CONFIG_ACPI_APEI > > : > > > +void aer_set_firmware_first(struct pci_dev *pci_dev) > > > +{ > > : > > > +} > > > +#endif > > > --- a/drivers/pci/pcie/aer/aerdrv_core.c > > > +++ b/drivers/pci/pcie/aer/aerdrv_core.c > > > @@ -30,12 +30,19 @@ static int nosourceid; > > > module_param(forceload, bool, 0); > > > module_param(nosourceid, bool, 0); > > > > > > +static int pcie_aer_get_firmware_first(struct pci_dev *dev) > > > +{ > > > + if (!dev->__aer_firmware_first_valid) > > > + aer_set_firmware_first(dev); > > > + return dev->__aer_firmware_first; > > > +} > > > + > > > > Then you can put pcie_aer_get_firmware_first() to next to > > pcie_aer_set_firmware_first() in aerdrv_acpi.c, which is > > the best file for these functions I think. > > > > > @@ -872,7 +879,7 @@ out: > > > if (forceload) { > > > dev_printk(KERN_DEBUG, &dev->device, > > > "aerdrv forceload requested.\n"); > > > - dev->port->aer_firmware_first = 0; > > > + dev->port->__aer_firmware_first = 0; > > > return 0; > > > } > > > return -ENXIO; > > > > I'd like to see a purpose-oriented method here, something like > > pcie_aer_force_firmware_first_to(dev, forcedvalue). > > > > Or, it would be more better, change pcie_aer_set_firmware_first() > > (= currently used by APEI only) to static with better name. > > > > E.g. > > Before: After: > > > > # get a flag that tells @dev is firmware first or not > > pcie_aer_get_firmware_first(dev) > > => pcie_aer_get_firmware_first(dev) # no change > > > > # set a flag that tells @dev is firmware first or not > > dev->port->__aer_firmware_first = X > > => pcie_aer_set_firmware_first(dev, X) > > > > # parse hest to know @dev is firmware first or not > > pcie_aer_set_firmware_first(dev) > > => aer_parse_hest_for(dev) > > Sounds reasonable. After putting current pcie_aer_set_firmware_first > implementation into aerdrv_core.c, we can make it static. And we can add > pcie_aer_force_firmware_first for forced setting. > > > > --- a/include/linux/pci.h > > > +++ b/include/linux/pci.h > > > @@ -311,7 +311,8 @@ struct pci_dev { > > > unsigned int is_virtfn:1; > > > unsigned int reset_fn:1; > > > unsigned int is_hotplug_bridge:1; > > > - unsigned int aer_firmware_first:1; > > > + unsigned int __aer_firmware_first_valid:1; > > > + unsigned int __aer_firmware_first:1; > > > pci_dev_flags_t dev_flags; > > > atomic_t enable_cnt; /* pci_enable_device has been called */ > > > > BTW, what the prefix "__" for? > > I want to warn users that this is a internal interface, don't use it > directly. > > > I recommend you to separate this 2/2 patch into 2 pieces, > > one for PCI addressed to Jesse, and another is for ACPI. > > i.e.: > > > > [PATCH] PCIE, AER: arrange pcie_aer_{get,set}_firmware_first > > [PATCH 1/2] ACPI, APEI: Make APEI core configurable built-in > > [PATCH 2/2] ACPI, APEI: use general HEST table parsing in AER > > > > You can make 3rd patch to contain only *acpi* changes, remove of > > acpi/hest.c and modify of pcie/aer/aerdrv_acpi.c. > > Then it will be easy-to-pull for Len, right? > > Cross maintainers merging is painful after all. Even I split the patches > as you proposed, I may need to wait another 2 merge windows to merge the > code. To make it goes more smoothly, I suggest to merge all the code in > Len's tree after getting ack from Jesse. > > Hi, Len and Jesse, how about my suggestion? Yeah, it seems like the bulk of it is ACPI related, so going through Len's tree is fine with me. The PCI portion seems reasonable assuming you include Hidetoshi-san's suggestions, you can add my Ack for now (I'll yell if I find something I don't like in your next post). Thanks, -- Jesse Barnes, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-03-13 1:23 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-03-11 2:14 [PATCH -v2 0/2] ACPI, APEI, use general HEST table parsing in AER firmware_first setup (resend) Huang Ying 2010-03-11 2:14 ` [PATCH -v2 1/2] ACPI, APEI, Make APEI core configurable built-in instead of module Huang Ying 2010-03-11 2:14 ` [PATCH -v2 2/2] ACPI, APEI, PCIE AER, use general HEST table parsing in AER firmware_first setup Huang Ying 2010-03-12 3:43 ` Hidetoshi Seto 2010-03-12 7:59 ` Huang Ying 2010-03-13 1:23 ` Jesse Barnes
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox