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-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
