| Message ID | 20230220-rtas-queue-for-6-4-v1-5-010e4416f13f@linux.ibm.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | RTAS changes for 6.4 | expand |
On Mon, 2023-03-06 at 15:33 -0600, Nathan Lynch via B4 Relay wrote: > From: Nathan Lynch <nathanl@linux.ibm.com> > > The function name va_rtas_call_unlocked() is confusing: it may be > called with or without rtas_lock held. Rename it to va_rtas_call(). > > Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com> Not a huge fan of the name, the va_ suggests that the only difference between this function and rtas_call() is the varargs handling. Perhaps something like __rtas_call()? > --- > arch/powerpc/kernel/rtas.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c > index c29c38b1a55a..96a10a0abe3a 100644 > --- a/arch/powerpc/kernel/rtas.c > +++ b/arch/powerpc/kernel/rtas.c > @@ -996,9 +996,8 @@ static void __init init_error_log_max(void) {} > #endif > > > -static void > -va_rtas_call_unlocked(struct rtas_args *args, int token, int nargs, > int nret, > - va_list list) > +static void va_rtas_call(struct rtas_args *args, int token, int > nargs, int nret, > + va_list list) > { > int i; > > @@ -1038,7 +1037,7 @@ void rtas_call_unlocked(struct rtas_args *args, > int token, int nargs, int nret, > va_list list; > > va_start(list, nret); > - va_rtas_call_unlocked(args, token, nargs, nret, list); > + va_rtas_call(args, token, nargs, nret, list); > va_end(list); > } > > @@ -1138,7 +1137,7 @@ int rtas_call(int token, int nargs, int nret, > int *outputs, ...) > args = &rtas_args; > > va_start(list, outputs); > - va_rtas_call_unlocked(args, token, nargs, nret, list); > + va_rtas_call(args, token, nargs, nret, list); > va_end(list); > > /* A -1 return code indicates that the last command couldn't >
Andrew Donnellan <ajd@linux.ibm.com> writes: > On Mon, 2023-03-06 at 15:33 -0600, Nathan Lynch via B4 Relay wrote: >> From: Nathan Lynch <nathanl@linux.ibm.com> >> >> The function name va_rtas_call_unlocked() is confusing: it may be >> called with or without rtas_lock held. Rename it to va_rtas_call(). >> >> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com> > > Not a huge fan of the name, the va_ suggests that the only difference > between this function and rtas_call() is the varargs handling. Perhaps > something like __rtas_call()? I would be more inclined to agree if va_rtas_call() were a public API, like rtas_call(). But it's not, so the convention you're appealing to shouldn't inform the expectations of external users of the rtas_* APIs, at least. __rtas_call() conveys strictly less information than va_rtas_call() IMO. Most functions in the kernel that take a va_list have a "v" worked into their name somehow.
Nathan Lynch via B4 Relay <devnull+nathanl.linux.ibm.com@kernel.org> writes: > From: Nathan Lynch <nathanl@linux.ibm.com> > > The function name va_rtas_call_unlocked() is confusing: it may be > called with or without rtas_lock held. Rename it to va_rtas_call(). I'm not sure about this one. The "unlocked" is meant to convey that it doesn't do any locking. The caller has to be OK with that, or do its own locking. Andrew is right that the common naming pattern is foo() that takes the lock and __foo() that doesn't - but I agree that's not very pretty. Can we just leave it as-is? cheers
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c index c29c38b1a55a..96a10a0abe3a 100644 --- a/arch/powerpc/kernel/rtas.c +++ b/arch/powerpc/kernel/rtas.c @@ -996,9 +996,8 @@ static void __init init_error_log_max(void) {} #endif -static void -va_rtas_call_unlocked(struct rtas_args *args, int token, int nargs, int nret, - va_list list) +static void va_rtas_call(struct rtas_args *args, int token, int nargs, int nret, + va_list list) { int i; @@ -1038,7 +1037,7 @@ void rtas_call_unlocked(struct rtas_args *args, int token, int nargs, int nret, va_list list; va_start(list, nret); - va_rtas_call_unlocked(args, token, nargs, nret, list); + va_rtas_call(args, token, nargs, nret, list); va_end(list); } @@ -1138,7 +1137,7 @@ int rtas_call(int token, int nargs, int nret, int *outputs, ...) args = &rtas_args; va_start(list, outputs); - va_rtas_call_unlocked(args, token, nargs, nret, list); + va_rtas_call(args, token, nargs, nret, list); va_end(list); /* A -1 return code indicates that the last command couldn't