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

[Huang Ying]

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/