Message ID | 20240222-rtas-fix-ibm-reset-pe-dma-window-v1-1-7aaf235ac63c@linux.ibm.com (mailing list archive) |
---|---|
State | Accepted |
Commit | fad87dbd48156ab940538f052f1820f4b6ed2819 |
Headers | show |
Series | powerpc/rtas: use correct function name for resetting TCE tables | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 23 jobs. |
Nathan Lynch via B4 Relay <devnull+nathanl.linux.ibm.com@kernel.org> writes: > From: Nathan Lynch <nathanl@linux.ibm.com> > > The PAPR spec spells the function name as > > "ibm,reset-pe-dma-windows" > > but in practice firmware uses the singular form: Just to be clear, you're talking about IBM firmware on PowerVM machines. > "ibm,reset-pe-dma-window" > > in the device tree. Since we have the wrong spelling in the RTAS > function table, reverse lookups (token -> name) fail and warn: > > unexpected failed lookup for token 86 > WARNING: CPU: 1 PID: 545 at arch/powerpc/kernel/rtas.c:659 __do_enter_rtas_trace+0x2a4/0x2b4 ... > diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c > index 7e793b503e29..8064d9c3de86 100644 > --- a/arch/powerpc/kernel/rtas.c > +++ b/arch/powerpc/kernel/rtas.c > @@ -375,8 +375,13 @@ static struct rtas_function rtas_function_table[] __ro_after_init = { > [RTAS_FNIDX__IBM_REMOVE_PE_DMA_WINDOW] = { > .name = "ibm,remove-pe-dma-window", > }, > - [RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOWS] = { > - .name = "ibm,reset-pe-dma-windows", > + [RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOW] = { > + /* > + * Note: PAPR+ v2.13 7.3.31.4.1 spells this as > + * "ibm,reset-pe-dma-windows" (plural), but RTAS > + * implementations use the singular form in practice. > + */ > + .name = "ibm,reset-pe-dma-window", Qemu also spells it that way: $ grep -C 12 ibm,reset-pe-dma-window hw/ppc/spapr_rtas_ddw.c static void spapr_rtas_ddw_init(void) { spapr_rtas_register(RTAS_IBM_QUERY_PE_DMA_WINDOW, "ibm,query-pe-dma-window", rtas_ibm_query_pe_dma_window); spapr_rtas_register(RTAS_IBM_CREATE_PE_DMA_WINDOW, "ibm,create-pe-dma-window", rtas_ibm_create_pe_dma_window); spapr_rtas_register(RTAS_IBM_REMOVE_PE_DMA_WINDOW, "ibm,remove-pe-dma-window", rtas_ibm_remove_pe_dma_window); spapr_rtas_register(RTAS_IBM_RESET_PE_DMA_WINDOW, "ibm,reset-pe-dma-window", rtas_ibm_reset_pe_dma_window); } There's no version in SLOF, it delegates to Qemu. The old platforms that use RTAS won't implement this call at all, so there's no issue with the naming there. So LGTM. cheers
Michael Ellerman <mpe@ellerman.id.au> writes: > Nathan Lynch via B4 Relay <devnull+nathanl.linux.ibm.com@kernel.org> > writes: >> From: Nathan Lynch <nathanl@linux.ibm.com> >> >> The PAPR spec spells the function name as >> >> "ibm,reset-pe-dma-windows" >> >> but in practice firmware uses the singular form: > > Just to be clear, you're talking about IBM firmware on PowerVM > machines. Yes, although I did check Qemu too. >> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c >> index 7e793b503e29..8064d9c3de86 100644 >> --- a/arch/powerpc/kernel/rtas.c >> +++ b/arch/powerpc/kernel/rtas.c >> @@ -375,8 +375,13 @@ static struct rtas_function rtas_function_table[] __ro_after_init = { >> [RTAS_FNIDX__IBM_REMOVE_PE_DMA_WINDOW] = { >> .name = "ibm,remove-pe-dma-window", >> }, >> - [RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOWS] = { >> - .name = "ibm,reset-pe-dma-windows", >> + [RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOW] = { >> + /* >> + * Note: PAPR+ v2.13 7.3.31.4.1 spells this as >> + * "ibm,reset-pe-dma-windows" (plural), but RTAS >> + * implementations use the singular form in practice. >> + */ >> + .name = "ibm,reset-pe-dma-window", > > Qemu also spells it that way: > > $ grep -C 12 ibm,reset-pe-dma-window hw/ppc/spapr_rtas_ddw.c > static void spapr_rtas_ddw_init(void) > { > spapr_rtas_register(RTAS_IBM_QUERY_PE_DMA_WINDOW, > "ibm,query-pe-dma-window", > rtas_ibm_query_pe_dma_window); > spapr_rtas_register(RTAS_IBM_CREATE_PE_DMA_WINDOW, > "ibm,create-pe-dma-window", > rtas_ibm_create_pe_dma_window); > spapr_rtas_register(RTAS_IBM_REMOVE_PE_DMA_WINDOW, > "ibm,remove-pe-dma-window", > rtas_ibm_remove_pe_dma_window); > spapr_rtas_register(RTAS_IBM_RESET_PE_DMA_WINDOW, > "ibm,reset-pe-dma-window", > rtas_ibm_reset_pe_dma_window); > } > > There's no version in SLOF, it delegates to Qemu. > > The old platforms that use RTAS won't implement this call at all, so > there's no issue with the naming there. Thanks for checking. I've added SLOF to my checklist for things like this.
On Thu, 22 Feb 2024 16:19:14 -0600, Nathan Lynch wrote: > The PAPR spec spells the function name as > > "ibm,reset-pe-dma-windows" > > but in practice firmware uses the singular form: > > "ibm,reset-pe-dma-window" > > [...] Applied to powerpc/fixes. [1/1] powerpc/rtas: use correct function name for resetting TCE tables https://git.kernel.org/powerpc/c/fad87dbd48156ab940538f052f1820f4b6ed2819 cheers
diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h index 9bb2210c8d44..065ffd1b2f8a 100644 --- a/arch/powerpc/include/asm/rtas.h +++ b/arch/powerpc/include/asm/rtas.h @@ -69,7 +69,7 @@ enum rtas_function_index { RTAS_FNIDX__IBM_READ_SLOT_RESET_STATE, RTAS_FNIDX__IBM_READ_SLOT_RESET_STATE2, RTAS_FNIDX__IBM_REMOVE_PE_DMA_WINDOW, - RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOWS, + RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOW, RTAS_FNIDX__IBM_SCAN_LOG_DUMP, RTAS_FNIDX__IBM_SET_DYNAMIC_INDICATOR, RTAS_FNIDX__IBM_SET_EEH_OPTION, @@ -164,7 +164,7 @@ typedef struct { #define RTAS_FN_IBM_READ_SLOT_RESET_STATE rtas_fn_handle(RTAS_FNIDX__IBM_READ_SLOT_RESET_STATE) #define RTAS_FN_IBM_READ_SLOT_RESET_STATE2 rtas_fn_handle(RTAS_FNIDX__IBM_READ_SLOT_RESET_STATE2) #define RTAS_FN_IBM_REMOVE_PE_DMA_WINDOW rtas_fn_handle(RTAS_FNIDX__IBM_REMOVE_PE_DMA_WINDOW) -#define RTAS_FN_IBM_RESET_PE_DMA_WINDOWS rtas_fn_handle(RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOWS) +#define RTAS_FN_IBM_RESET_PE_DMA_WINDOW rtas_fn_handle(RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOW) #define RTAS_FN_IBM_SCAN_LOG_DUMP rtas_fn_handle(RTAS_FNIDX__IBM_SCAN_LOG_DUMP) #define RTAS_FN_IBM_SET_DYNAMIC_INDICATOR rtas_fn_handle(RTAS_FNIDX__IBM_SET_DYNAMIC_INDICATOR) #define RTAS_FN_IBM_SET_EEH_OPTION rtas_fn_handle(RTAS_FNIDX__IBM_SET_EEH_OPTION) diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c index 7e793b503e29..8064d9c3de86 100644 --- a/arch/powerpc/kernel/rtas.c +++ b/arch/powerpc/kernel/rtas.c @@ -375,8 +375,13 @@ static struct rtas_function rtas_function_table[] __ro_after_init = { [RTAS_FNIDX__IBM_REMOVE_PE_DMA_WINDOW] = { .name = "ibm,remove-pe-dma-window", }, - [RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOWS] = { - .name = "ibm,reset-pe-dma-windows", + [RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOW] = { + /* + * Note: PAPR+ v2.13 7.3.31.4.1 spells this as + * "ibm,reset-pe-dma-windows" (plural), but RTAS + * implementations use the singular form in practice. + */ + .name = "ibm,reset-pe-dma-window", }, [RTAS_FNIDX__IBM_SCAN_LOG_DUMP] = { .name = "ibm,scan-log-dump",