lkml.org 
[lkml]   [2010]   [Mar]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH -v2 2/2] ACPI, APEI, PCIE AER, use general HEST table parsing in AER firmware_first setup
(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



\
 
 \ /
  Last update: 2010-03-12 04:47    [W:0.047 / U:1.348 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site