Re: [PATCH -v2 2/2] ACPI, APEI, PCIE AER, use general HEST table parsing in AER firmware_first setup

[Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>]

(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