| Message ID | 1387961936-20451-1-git-send-email-shangw@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, 2013-12-25 at 16:58 +0800, Gavin Shan wrote: > After reset on the specific PE or PHB, we never configure AER > correctly on PowerNV platform. We needn't care it on pSeries > platform. The patch introduces additional EEH operation eeh_ops:: > restore_bars() so that we have chance to configure AER correctly > for PowerNV platform. Why call it "restore_bars" if it restores something else (in this case AER) ? I would call it "restore_config" instead... Also rather than adding the knowledge of what AER bit works or not for the device, would it make sense to instead have a FW call to re-init the device ? Otherwise, you introduce duplication between Linux and Firmware with the risk of getting out of sync... Cheers, Ben. > Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com> > --- > arch/powerpc/include/asm/eeh.h | 1 + > arch/powerpc/kernel/eeh_pe.c | 3 +++ > arch/powerpc/platforms/pseries/eeh_pseries.c | 4 +++- > 3 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h > index d3e5e9b..4b709bf 100644 > --- a/arch/powerpc/include/asm/eeh.h > +++ b/arch/powerpc/include/asm/eeh.h > @@ -157,6 +157,7 @@ struct eeh_ops { > int (*read_config)(struct device_node *dn, int where, int size, u32 *val); > int (*write_config)(struct device_node *dn, int where, int size, u32 val); > int (*next_error)(struct eeh_pe **pe); > + void (*restore_bars)(struct device_node *dn); > }; > > extern struct eeh_ops *eeh_ops; > diff --git a/arch/powerpc/kernel/eeh_pe.c b/arch/powerpc/kernel/eeh_pe.c > index f945053..19eb95a 100644 > --- a/arch/powerpc/kernel/eeh_pe.c > +++ b/arch/powerpc/kernel/eeh_pe.c > @@ -737,6 +737,9 @@ static void *eeh_restore_one_device_bars(void *data, void *flag) > else > eeh_restore_device_bars(edev, dn); > > + if (eeh_ops->restore_bars) > + eeh_ops->restore_bars(dn); > + > return NULL; > } > > diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c > index ccb633e..623adaf 100644 > --- a/arch/powerpc/platforms/pseries/eeh_pseries.c > +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c > @@ -689,7 +689,9 @@ static struct eeh_ops pseries_eeh_ops = { > .get_log = pseries_eeh_get_log, > .configure_bridge = pseries_eeh_configure_bridge, > .read_config = pseries_eeh_read_config, > - .write_config = pseries_eeh_write_config > + .write_config = pseries_eeh_write_config, > + .next_error = NULL, > + .restore_bars = NULL > }; > > /**
On Sat, Dec 28, 2013 at 09:20:04AM +1100, Benjamin Herrenschmidt wrote: >On Wed, 2013-12-25 at 16:58 +0800, Gavin Shan wrote: >> After reset on the specific PE or PHB, we never configure AER >> correctly on PowerNV platform. We needn't care it on pSeries >> platform. The patch introduces additional EEH operation eeh_ops:: >> restore_bars() so that we have chance to configure AER correctly >> for PowerNV platform. > >Why call it "restore_bars" if it restores something else (in this case >AER) ? > >I would call it "restore_config" instead... Also rather than adding >the knowledge of what AER bit works or not for the device, would it >make sense to instead have a FW call to re-init the device ? > >Otherwise, you introduce duplication between Linux and Firmware with >the risk of getting out of sync... > Thanks for your comments, Ben. It's reasonable to have name "restore_config" and I'll introduce a FW call in next revision as you suggested :-) Thanks, Gavin >> Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com> >> --- >> arch/powerpc/include/asm/eeh.h | 1 + >> arch/powerpc/kernel/eeh_pe.c | 3 +++ >> arch/powerpc/platforms/pseries/eeh_pseries.c | 4 +++- >> 3 files changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h >> index d3e5e9b..4b709bf 100644 >> --- a/arch/powerpc/include/asm/eeh.h >> +++ b/arch/powerpc/include/asm/eeh.h >> @@ -157,6 +157,7 @@ struct eeh_ops { >> int (*read_config)(struct device_node *dn, int where, int size, u32 *val); >> int (*write_config)(struct device_node *dn, int where, int size, u32 val); >> int (*next_error)(struct eeh_pe **pe); >> + void (*restore_bars)(struct device_node *dn); >> }; >> >> extern struct eeh_ops *eeh_ops; >> diff --git a/arch/powerpc/kernel/eeh_pe.c b/arch/powerpc/kernel/eeh_pe.c >> index f945053..19eb95a 100644 >> --- a/arch/powerpc/kernel/eeh_pe.c >> +++ b/arch/powerpc/kernel/eeh_pe.c >> @@ -737,6 +737,9 @@ static void *eeh_restore_one_device_bars(void *data, void *flag) >> else >> eeh_restore_device_bars(edev, dn); >> >> + if (eeh_ops->restore_bars) >> + eeh_ops->restore_bars(dn); >> + >> return NULL; >> } >> >> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c >> index ccb633e..623adaf 100644 >> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c >> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c >> @@ -689,7 +689,9 @@ static struct eeh_ops pseries_eeh_ops = { >> .get_log = pseries_eeh_get_log, >> .configure_bridge = pseries_eeh_configure_bridge, >> .read_config = pseries_eeh_read_config, >> - .write_config = pseries_eeh_write_config >> + .write_config = pseries_eeh_write_config, >> + .next_error = NULL, >> + .restore_bars = NULL >> }; >> >> /** > >
diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h index d3e5e9b..4b709bf 100644 --- a/arch/powerpc/include/asm/eeh.h +++ b/arch/powerpc/include/asm/eeh.h @@ -157,6 +157,7 @@ struct eeh_ops { int (*read_config)(struct device_node *dn, int where, int size, u32 *val); int (*write_config)(struct device_node *dn, int where, int size, u32 val); int (*next_error)(struct eeh_pe **pe); + void (*restore_bars)(struct device_node *dn); }; extern struct eeh_ops *eeh_ops; diff --git a/arch/powerpc/kernel/eeh_pe.c b/arch/powerpc/kernel/eeh_pe.c index f945053..19eb95a 100644 --- a/arch/powerpc/kernel/eeh_pe.c +++ b/arch/powerpc/kernel/eeh_pe.c @@ -737,6 +737,9 @@ static void *eeh_restore_one_device_bars(void *data, void *flag) else eeh_restore_device_bars(edev, dn); + if (eeh_ops->restore_bars) + eeh_ops->restore_bars(dn); + return NULL; } diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c index ccb633e..623adaf 100644 --- a/arch/powerpc/platforms/pseries/eeh_pseries.c +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c @@ -689,7 +689,9 @@ static struct eeh_ops pseries_eeh_ops = { .get_log = pseries_eeh_get_log, .configure_bridge = pseries_eeh_configure_bridge, .read_config = pseries_eeh_read_config, - .write_config = pseries_eeh_write_config + .write_config = pseries_eeh_write_config, + .next_error = NULL, + .restore_bars = NULL }; /**
After reset on the specific PE or PHB, we never configure AER correctly on PowerNV platform. We needn't care it on pSeries platform. The patch introduces additional EEH operation eeh_ops:: restore_bars() so that we have chance to configure AER correctly for PowerNV platform. Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com> --- arch/powerpc/include/asm/eeh.h | 1 + arch/powerpc/kernel/eeh_pe.c | 3 +++ arch/powerpc/platforms/pseries/eeh_pseries.c | 4 +++- 3 files changed, 7 insertions(+), 1 deletion(-)