From: Huang Ying on
On Tue, 2010-03-02 at 16:09 +0800, Hidetoshi Seto wrote:
> (2010/03/02 10:55), Huang Ying wrote:
> > ... The firmware_first setup code is moved from PCI core to
> > AER driver too, because it is only AER related.
> (snip)
> > diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
> > index c843a79..cc527c1 100644
> > --- a/drivers/pci/pcie/aer/aerdrv_core.c
> > +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> > @@ -858,6 +858,8 @@ void aer_delete_rootport(struct aer_rpc *rpc)
> > */
> > int aer_init(struct pcie_device *dev)
> > {
> > + aer_set_firmware_first(dev);
> > +
> > if (dev->port->aer_firmware_first) {
> > dev_printk(KERN_DEBUG, &dev->device,
> > "PCIe errors handled by platform firmware.\n");
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 2a94309..ccfaf19 100644
> (snip)
> > @@ -935,7 +928,6 @@ int pci_setup_device(struct pci_dev *dev)
> > dev->multifunction = !!(hdr_type & 0x80);
> > dev->error_state = pci_channel_io_normal;
> > set_pcie_port_type(dev);
> > - set_pci_aer_firmware_first(dev);
> >
> > list_for_each_entry(slot, &dev->bus->slots, list)
> > if (PCI_SLOT(dev->devfn) == slot->number)
>
> The aer_init() will be called for root ports, but not for end point
> devices or so on. So please remain the firmware_first setup code in
> PCI core. Otherwise endpoint drivers will get success on call of
> pci_enable_pcie_error_reporting() regardless of the firmware first.

Or we can call firmware_first setup code in
pci_enable_pcie_error_reporting(), because

1. I think AER related code should be put in drivers/pci/pcie/aer
instead of PCI core or drivers/acpi, if it is possible.

2. pci_setup_device is called so early, so that it is hard to do some
HEST related initialization (such as checking bad format) before it.

Best Regards,
Huang Ying


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Huang Ying on
On Tue, 2010-03-02 at 19:04 +0800, Hidetoshi Seto wrote:
> (2010/03/02 18:13), Huang Ying wrote:
> > On Tue, 2010-03-02 at 16:09 +0800, Hidetoshi Seto wrote:
> >> The aer_init() will be called for root ports, but not for end point
> >> devices or so on. So please remain the firmware_first setup code in
> >> PCI core. Otherwise endpoint drivers will get success on call of
> >> pci_enable_pcie_error_reporting() regardless of the firmware first.
> >
> > Or we can call firmware_first setup code in
> > pci_enable_pcie_error_reporting(), because
> >
> > 1. I think AER related code should be put in drivers/pci/pcie/aer
> > instead of PCI core or drivers/acpi, if it is possible.
> >
> > 2. pci_setup_device is called so early, so that it is hard to do some
> > HEST related initialization (such as checking bad format) before it.
>
> I understands the feeling, but before agreeing with your
> proposal, I'd like to have an answer of a question:
>
> - Is it necessary to setup the firmware_first flag
> for an endpoint even if the endpoint's driver never
> call pci_enable_pcie_error_reporting()?
>
> According to the current implementation, there are no
> driver referring the firmware_first flag other than that
> it owns. However I guess that the flag will be necessary
> for AER driver (i.e. aerdrv_core) in near future, because
> we can use the flag to determine whether the AER driver
> can check the device or not, when it is required to walk
> pci bus hierarchy to find an erroneous device.
>
> For example, assume that there are 2 endpoints under a same
> root port. One is (likely on-board) "firmware first" endpoint,
> with driver which does not call pci_enable_pcie_error_reporting()
> (because of no interest in AER, or just not implemented yet,
> anyway). The other is (likely card seated on a slot) not
> firmware first, with better driver which can handle it's AER.
> If my understanding is correct and if everything goes well,
> errors on one should be reported via APEI while the other should
> be reported via AER driver.

Yes. I think this should be supported. How about something as follow?

struct pci_dev {
...
unsigned int __firmware_first:2;
...
};

int pcie_aer_get_firmware_first(struct pci_dev *dev)
{
if (!dev->__firmware_first)
aer_set_firmware_first(dev);
return dev->__firmware_first & 0x1;
}

Then we use pcie_aer_get_firmware_first() instead of dev->firmware_first
directly.

Best Regards,
Huang Ying


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Hidetoshi Seto on
(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-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Huang Ying on
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(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jesse Barnes on
On Fri, 12 Mar 2010 15:59:40 +0800
Huang Ying <ying.huang(a)intel.com> wrote:

> 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?

Yeah, it seems like the bulk of it is ACPI related, so going through
Len's tree is fine with me. The PCI portion seems reasonable assuming
you include Hidetoshi-san's suggestions, you can add my Ack for now
(I'll yell if I find something I don't like in your next post).

Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/