(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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
