| Message ID | 1409039779-392-2-git-send-email-gwshan@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Michael Ellerman |
| Headers | show |
On Tue, 2014-26-08 at 07:56:16 UTC, Gavin Shan wrote: > From: Mike Qiu <qiudayu@linux.vnet.ibm.com> > > The patch synchronizes firmware header file (opal.h) for PCI error > injection. > > diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h > index 4593a93..9113653 100644 > --- a/arch/powerpc/include/asm/opal.h > +++ b/arch/powerpc/include/asm/opal.h > @@ -200,6 +201,33 @@ enum OpalPciErrorSeverity { > OPAL_EEH_SEV_INF = 5 > }; > > +enum OpalErrinjctType { > + OpalErrinjctTypeIoaBusError = 0, > + OpalErrinjctTypeIoaBusError64 = 1, > + > + /* IoaBusError & IoaBusError64 */ > + OpalEjtIoaLoadMemAddr = 0, > + OpalEjtIoaLoadMemData = 1, > + OpalEjtIoaLoadIoAddr = 2, > + OpalEjtIoaLoadIoData = 3, > + OpalEjtIoaLoadConfigAddr = 4, > + OpalEjtIoaLoadConfigData = 5, > + OpalEjtIoaStoreMemAddr = 6, > + OpalEjtIoaStoreMemData = 7, > + OpalEjtIoaStoreIoAddr = 8, > + OpalEjtIoaStoreIoData = 9, > + OpalEjtIoaStoreConfigAddr = 10, > + OpalEjtIoaStoreConfigData = 11, > + OpalEjtIoaDmaReadMemAddr = 12, > + OpalEjtIoaDmaReadMemData = 13, > + OpalEjtIoaDmaReadMemMaster = 14, > + OpalEjtIoaDmaReadMemTarget = 15, > + OpalEjtIoaDmaWriteMemAddr = 16, > + OpalEjtIoaDmaWriteMemData = 17, > + OpalEjtIoaDmaWriteMemMaster = 18, > + OpalEjtIoaDmaWriteMemTarget = 19, > +}; I realise these come from the skiboot source, but they're just too ugly. Please use kernel style naming, like most of the rest of the file, eg: OPAL_ERR_INJECT_IOA_BUS_ERR Also this enum seems to contain two separate types, the first two values are the "type", and the rest are "functions". The only usage I see is: /* Sanity check on error type */ if (type < OpalErrinjctTypeIoaBusError || type > OpalErrinjctTypeIoaBusError64 || function < OpalEjtIoaLoadMemAddr || function > OpalEjtIoaDmaWriteMemTarget) { pr_warn("%s: Invalid error type %d-%d\n", __func__, type, function); return -ERANGE; } So we could also just do: # define OPAL_ERR_INJECT_TYPE_MIN 0 # define OPAL_ERR_INJECT_TYPE_MAX 1 # define OPAL_ERR_INJECT_FUNC_MIN 0 # define OPAL_ERR_INJECT_FUNC_MAX 19 if (type < OPAL_ERR_INJECT_TYPE_MIN || type > OPAL_ERR_INJECT_TYPE_MAX || function < OPAL_ERR_INJECT_FUNC_MIN || function > OPAL_ERR_INJECT_FUNC_MIN) { pr_warn("%s: Invalid error type %d-%d\n", __func__, type, function); return -ERANGE; } cheers
On Thu, Sep 25, 2014 at 02:27:47PM +1000, Michael Ellerman wrote: >On Tue, 2014-26-08 at 07:56:16 UTC, Gavin Shan wrote: >> From: Mike Qiu <qiudayu@linux.vnet.ibm.com> >> >> The patch synchronizes firmware header file (opal.h) for PCI error >> injection. >> >> diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h >> index 4593a93..9113653 100644 >> --- a/arch/powerpc/include/asm/opal.h >> +++ b/arch/powerpc/include/asm/opal.h >> @@ -200,6 +201,33 @@ enum OpalPciErrorSeverity { >> OPAL_EEH_SEV_INF = 5 >> }; >> >> +enum OpalErrinjctType { >> + OpalErrinjctTypeIoaBusError = 0, >> + OpalErrinjctTypeIoaBusError64 = 1, >> + >> + /* IoaBusError & IoaBusError64 */ >> + OpalEjtIoaLoadMemAddr = 0, >> + OpalEjtIoaLoadMemData = 1, >> + OpalEjtIoaLoadIoAddr = 2, >> + OpalEjtIoaLoadIoData = 3, >> + OpalEjtIoaLoadConfigAddr = 4, >> + OpalEjtIoaLoadConfigData = 5, >> + OpalEjtIoaStoreMemAddr = 6, >> + OpalEjtIoaStoreMemData = 7, >> + OpalEjtIoaStoreIoAddr = 8, >> + OpalEjtIoaStoreIoData = 9, >> + OpalEjtIoaStoreConfigAddr = 10, >> + OpalEjtIoaStoreConfigData = 11, >> + OpalEjtIoaDmaReadMemAddr = 12, >> + OpalEjtIoaDmaReadMemData = 13, >> + OpalEjtIoaDmaReadMemMaster = 14, >> + OpalEjtIoaDmaReadMemTarget = 15, >> + OpalEjtIoaDmaWriteMemAddr = 16, >> + OpalEjtIoaDmaWriteMemData = 17, >> + OpalEjtIoaDmaWriteMemMaster = 18, >> + OpalEjtIoaDmaWriteMemTarget = 19, >> +}; > >I realise these come from the skiboot source, but they're just too ugly. > >Please use kernel style naming, like most of the rest of the file, eg: > > OPAL_ERR_INJECT_IOA_BUS_ERR > >Also this enum seems to contain two separate types, the first two values are >the "type", and the rest are "functions". > Yes, two separate types: One is major error type, another one is specific error type in that domain. >The only usage I see is: > > /* Sanity check on error type */ > if (type < OpalErrinjctTypeIoaBusError || > type > OpalErrinjctTypeIoaBusError64 || > function < OpalEjtIoaLoadMemAddr || > function > OpalEjtIoaDmaWriteMemTarget) { > pr_warn("%s: Invalid error type %d-%d\n", > __func__, type, function); > return -ERANGE; > } > >So we could also just do: > ># define OPAL_ERR_INJECT_TYPE_MIN 0 ># define OPAL_ERR_INJECT_TYPE_MAX 1 > ># define OPAL_ERR_INJECT_FUNC_MIN 0 ># define OPAL_ERR_INJECT_FUNC_MAX 19 > > if (type < OPAL_ERR_INJECT_TYPE_MIN || > type > OPAL_ERR_INJECT_TYPE_MAX || > function < OPAL_ERR_INJECT_FUNC_MIN || > function > OPAL_ERR_INJECT_FUNC_MIN) > { > pr_warn("%s: Invalid error type %d-%d\n", __func__, type, function); > return -ERANGE; > } > Ok. I'll take this and put it into next revision. Thanks, Gavin
On Thu, Sep 25, 2014 at 02:27:47PM +1000, Michael Ellerman wrote: >On Tue, 2014-26-08 at 07:56:16 UTC, Gavin Shan wrote: >> From: Mike Qiu <qiudayu@linux.vnet.ibm.com> >> >> The patch synchronizes firmware header file (opal.h) for PCI error >> injection. >> >> diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h >> index 4593a93..9113653 100644 >> --- a/arch/powerpc/include/asm/opal.h >> +++ b/arch/powerpc/include/asm/opal.h >> @@ -200,6 +201,33 @@ enum OpalPciErrorSeverity { >> OPAL_EEH_SEV_INF = 5 >> }; >> >> +enum OpalErrinjctType { >> + OpalErrinjctTypeIoaBusError = 0, >> + OpalErrinjctTypeIoaBusError64 = 1, >> + >> + /* IoaBusError & IoaBusError64 */ >> + OpalEjtIoaLoadMemAddr = 0, >> + OpalEjtIoaLoadMemData = 1, >> + OpalEjtIoaLoadIoAddr = 2, >> + OpalEjtIoaLoadIoData = 3, >> + OpalEjtIoaLoadConfigAddr = 4, >> + OpalEjtIoaLoadConfigData = 5, >> + OpalEjtIoaStoreMemAddr = 6, >> + OpalEjtIoaStoreMemData = 7, >> + OpalEjtIoaStoreIoAddr = 8, >> + OpalEjtIoaStoreIoData = 9, >> + OpalEjtIoaStoreConfigAddr = 10, >> + OpalEjtIoaStoreConfigData = 11, >> + OpalEjtIoaDmaReadMemAddr = 12, >> + OpalEjtIoaDmaReadMemData = 13, >> + OpalEjtIoaDmaReadMemMaster = 14, >> + OpalEjtIoaDmaReadMemTarget = 15, >> + OpalEjtIoaDmaWriteMemAddr = 16, >> + OpalEjtIoaDmaWriteMemData = 17, >> + OpalEjtIoaDmaWriteMemMaster = 18, >> + OpalEjtIoaDmaWriteMemTarget = 19, >> +}; > >I realise these come from the skiboot source, but they're just too ugly. > >Please use kernel style naming, like most of the rest of the file, eg: > > OPAL_ERR_INJECT_IOA_BUS_ERR > After thinking it for more, I decided to take this way because the specfic function types will potentially be used when we're going to support error injection from userland (QEMU). The new revision has folded all your comments (for other patches as well). Michael, I'm not sure it's good way to send all my patches for 3.18, or I just need send revised ones? Thanks, Gavin >Also this enum seems to contain two separate types, the first two values are >the "type", and the rest are "functions". > >The only usage I see is: > > /* Sanity check on error type */ > if (type < OpalErrinjctTypeIoaBusError || > type > OpalErrinjctTypeIoaBusError64 || > function < OpalEjtIoaLoadMemAddr || > function > OpalEjtIoaDmaWriteMemTarget) { > pr_warn("%s: Invalid error type %d-%d\n", > __func__, type, function); > return -ERANGE; > } > >So we could also just do: > ># define OPAL_ERR_INJECT_TYPE_MIN 0 ># define OPAL_ERR_INJECT_TYPE_MAX 1 > ># define OPAL_ERR_INJECT_FUNC_MIN 0 ># define OPAL_ERR_INJECT_FUNC_MAX 19 > > if (type < OPAL_ERR_INJECT_TYPE_MIN || > type > OPAL_ERR_INJECT_TYPE_MAX || > function < OPAL_ERR_INJECT_FUNC_MIN || > function > OPAL_ERR_INJECT_FUNC_MIN) > { > pr_warn("%s: Invalid error type %d-%d\n", __func__, type, function); > return -ERANGE; > } >
Michael Ellerman <mpe@ellerman.id.au> writes: >> + OpalEjtIoaDmaWriteMemTarget = 19, >> +}; > > I realise these come from the skiboot source, but they're just too ugly. > > Please use kernel style naming, like most of the rest of the file, eg: > > OPAL_ERR_INJECT_IOA_BUS_ERR You know what, I think I'd feel better if we changed skiboot source to be like this too. Many of the other enums in skiboot are kernel style and not camel. So, I'm going to go do that, then kernel and firmware can match.
On Fri, Oct 03, 2014 at 03:30:31PM +1000, Stewart Smith wrote: >Michael Ellerman <mpe@ellerman.id.au> writes: >>> + OpalEjtIoaDmaWriteMemTarget = 19, >>> +}; >> >> I realise these come from the skiboot source, but they're just too ugly. >> >> Please use kernel style naming, like most of the rest of the file, eg: >> >> OPAL_ERR_INJECT_IOA_BUS_ERR > >You know what, I think I'd feel better if we changed skiboot source to >be like this too. Many of the other enums in skiboot are kernel style >and not camel. So, I'm going to go do that, then kernel and firmware can >match. Yes, for the PCI error types/functions, I sent one patch to make skiboot looks same to kernel yesterday. Thanks, Gavin
diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h index 4593a93..9113653 100644 --- a/arch/powerpc/include/asm/opal.h +++ b/arch/powerpc/include/asm/opal.h @@ -148,6 +148,7 @@ struct opal_sg_list { #define OPAL_SET_PARAM 90 #define OPAL_DUMP_RESEND 91 #define OPAL_DUMP_INFO2 94 +#define OPAL_PCI_ERR_INJCT 96 #define OPAL_PCI_EEH_FREEZE_SET 97 #define OPAL_HANDLE_HMI 98 #define OPAL_REGISTER_DUMP_REGION 101 @@ -200,6 +201,33 @@ enum OpalPciErrorSeverity { OPAL_EEH_SEV_INF = 5 }; +enum OpalErrinjctType { + OpalErrinjctTypeIoaBusError = 0, + OpalErrinjctTypeIoaBusError64 = 1, + + /* IoaBusError & IoaBusError64 */ + OpalEjtIoaLoadMemAddr = 0, + OpalEjtIoaLoadMemData = 1, + OpalEjtIoaLoadIoAddr = 2, + OpalEjtIoaLoadIoData = 3, + OpalEjtIoaLoadConfigAddr = 4, + OpalEjtIoaLoadConfigData = 5, + OpalEjtIoaStoreMemAddr = 6, + OpalEjtIoaStoreMemData = 7, + OpalEjtIoaStoreIoAddr = 8, + OpalEjtIoaStoreIoData = 9, + OpalEjtIoaStoreConfigAddr = 10, + OpalEjtIoaStoreConfigData = 11, + OpalEjtIoaDmaReadMemAddr = 12, + OpalEjtIoaDmaReadMemData = 13, + OpalEjtIoaDmaReadMemMaster = 14, + OpalEjtIoaDmaReadMemTarget = 15, + OpalEjtIoaDmaWriteMemAddr = 16, + OpalEjtIoaDmaWriteMemData = 17, + OpalEjtIoaDmaWriteMemMaster = 18, + OpalEjtIoaDmaWriteMemTarget = 19, +}; + enum OpalShpcAction { OPAL_SHPC_GET_LINK_STATE = 0, OPAL_SHPC_GET_SLOT_STATE = 1 @@ -825,6 +853,8 @@ int64_t opal_pci_eeh_freeze_clear(uint64_t phb_id, uint64_t pe_number, uint64_t eeh_action_token); int64_t opal_pci_eeh_freeze_set(uint64_t phb_id, uint64_t pe_number, uint64_t eeh_action_token); +int64_t opal_pci_err_inject(uint64_t phb_id, uint32_t pe_no, uint32_t type, + uint32_t function, uint64_t addr, uint64_t mask); int64_t opal_pci_shpc(uint64_t phb_id, uint64_t shpc_action, uint8_t *state); diff --git a/arch/powerpc/platforms/powernv/opal-wrappers.S b/arch/powerpc/platforms/powernv/opal-wrappers.S index 5718855..9c68501 100644 --- a/arch/powerpc/platforms/powernv/opal-wrappers.S +++ b/arch/powerpc/platforms/powernv/opal-wrappers.S @@ -184,6 +184,7 @@ OPAL_CALL(opal_register_exception_handler, OPAL_REGISTER_OPAL_EXCEPTION_HANDLER) OPAL_CALL(opal_pci_eeh_freeze_status, OPAL_PCI_EEH_FREEZE_STATUS); OPAL_CALL(opal_pci_eeh_freeze_clear, OPAL_PCI_EEH_FREEZE_CLEAR); OPAL_CALL(opal_pci_eeh_freeze_set, OPAL_PCI_EEH_FREEZE_SET); +OPAL_CALL(opal_pci_err_inject, OPAL_PCI_ERR_INJCT); OPAL_CALL(opal_pci_shpc, OPAL_PCI_SHPC); OPAL_CALL(opal_pci_phb_mmio_enable, OPAL_PCI_PHB_MMIO_ENABLE); OPAL_CALL(opal_pci_set_phb_mem_window, OPAL_PCI_SET_PHB_MEM_WINDOW);