| Message ID | 20190408151125.32080-1-alexandru.elisei@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests] lib: arm: Use correct halt() prototype from smp.h | expand |
On Mon, Apr 08, 2019 at 04:11:25PM +0100, Alexandru Elisei wrote: > The prototype for the halt() function is incorrect, because halt() doesn't > take any arguments. Fix it by using the prototype from smp.h. > > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > --- Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
On Mon, Apr 08, 2019 at 04:11:25PM +0100, Alexandru Elisei wrote: > The prototype for the halt() function is incorrect, because halt() doesn't > take any arguments. Fix it by using the prototype from smp.h. > > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > --- > lib/arm/io.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/lib/arm/io.c b/lib/arm/io.c > index 8226b765bdc5..6d3d7afed002 100644 > --- a/lib/arm/io.c > +++ b/lib/arm/io.c > @@ -15,11 +15,10 @@ > #include <asm/psci.h> > #include <asm/spinlock.h> > #include <asm/io.h> > +#include <asm/smp.h> > > #include "io.h" > > -extern void halt(int code); > - > static struct spinlock uart_lock; > /* > * Use this guess for the uart base in order to make an attempt at > @@ -93,6 +92,6 @@ void exit(int code) > { > chr_testdev_exit(code); > psci_system_off(); > - halt(code); > + halt(); > __builtin_unreachable(); > } > -- > 2.17.0 > I don't mind this change, because per the code it is the "correct" thing to do. However, I was being a bit tricky here when I wrote it. By changing the prototype to take 'code' as argument we guarantee that 'code' will be in x0/r0 when we halt, giving us a last chance to see it when inspecting the halted unit test state. Anyway, like I said, I'm fine with the cleanup, but the prototype abuse does serve a purpose - maybe just not a good enough purpose to justify the weirdness. Thanks, drew
On 4/9/19 8:40 AM, Andrew Jones wrote: > On Mon, Apr 08, 2019 at 04:11:25PM +0100, Alexandru Elisei wrote: >> The prototype for the halt() function is incorrect, because halt() doesn't >> take any arguments. Fix it by using the prototype from smp.h. >> >> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> >> --- >> lib/arm/io.c | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/lib/arm/io.c b/lib/arm/io.c >> index 8226b765bdc5..6d3d7afed002 100644 >> --- a/lib/arm/io.c >> +++ b/lib/arm/io.c >> @@ -15,11 +15,10 @@ >> #include <asm/psci.h> >> #include <asm/spinlock.h> >> #include <asm/io.h> >> +#include <asm/smp.h> >> >> #include "io.h" >> >> -extern void halt(int code); >> - >> static struct spinlock uart_lock; >> /* >> * Use this guess for the uart base in order to make an attempt at >> @@ -93,6 +92,6 @@ void exit(int code) >> { >> chr_testdev_exit(code); >> psci_system_off(); >> - halt(code); >> + halt(); >> __builtin_unreachable(); >> } >> -- >> 2.17.0 >> > I don't mind this change, because per the code it is the "correct" > thing to do. However, I was being a bit tricky here when I wrote it. > By changing the prototype to take 'code' as argument we guarantee > that 'code' will be in x0/r0 when we halt, giving us a last chance > to see it when inspecting the halted unit test state. > > Anyway, like I said, I'm fine with the cleanup, but the prototype > abuse does serve a purpose - maybe just not a good enough purpose > to justify the weirdness. Now it makes sense. I didn't think it was intentional, but now that you have mentioned it, the same pattern is used by powerpc. Perhaps a comment explaining that having different prototypes was on purpose would be the best solution? > > Thanks, > drew
On Tue, Apr 09, 2019 at 10:15:59AM +0100, Alexandru Elisei wrote: > On 4/9/19 8:40 AM, Andrew Jones wrote: > > On Mon, Apr 08, 2019 at 04:11:25PM +0100, Alexandru Elisei wrote: > >> The prototype for the halt() function is incorrect, because halt() doesn't > >> take any arguments. Fix it by using the prototype from smp.h. > >> > >> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > >> --- > >> lib/arm/io.c | 5 ++--- > >> 1 file changed, 2 insertions(+), 3 deletions(-) > >> > >> diff --git a/lib/arm/io.c b/lib/arm/io.c > >> index 8226b765bdc5..6d3d7afed002 100644 > >> --- a/lib/arm/io.c > >> +++ b/lib/arm/io.c > >> @@ -15,11 +15,10 @@ > >> #include <asm/psci.h> > >> #include <asm/spinlock.h> > >> #include <asm/io.h> > >> +#include <asm/smp.h> > >> > >> #include "io.h" > >> > >> -extern void halt(int code); > >> - > >> static struct spinlock uart_lock; > >> /* > >> * Use this guess for the uart base in order to make an attempt at > >> @@ -93,6 +92,6 @@ void exit(int code) > >> { > >> chr_testdev_exit(code); > >> psci_system_off(); > >> - halt(code); > >> + halt(); > >> __builtin_unreachable(); > >> } > >> -- > >> 2.17.0 > >> > > I don't mind this change, because per the code it is the "correct" > > thing to do. However, I was being a bit tricky here when I wrote it. > > By changing the prototype to take 'code' as argument we guarantee > > that 'code' will be in x0/r0 when we halt, giving us a last chance > > to see it when inspecting the halted unit test state. > > > > Anyway, like I said, I'm fine with the cleanup, but the prototype > > abuse does serve a purpose - maybe just not a good enough purpose > > to justify the weirdness. > > Now it makes sense. I didn't think it was intentional, but now that you have > mentioned it, the same pattern is used by powerpc. No surprise there. I wrote that too, based on the arm code :) > > Perhaps a comment explaining that having different prototypes was on purpose > would be the best solution? Yes, either a comment explaining the weirdness or your patch to remove it would be a good idea in order to avoid future head scratching. I'll send a patch that adds comments to both arm and powerpc. Thanks, drew
diff --git a/lib/arm/io.c b/lib/arm/io.c index 8226b765bdc5..6d3d7afed002 100644 --- a/lib/arm/io.c +++ b/lib/arm/io.c @@ -15,11 +15,10 @@ #include <asm/psci.h> #include <asm/spinlock.h> #include <asm/io.h> +#include <asm/smp.h> #include "io.h" -extern void halt(int code); - static struct spinlock uart_lock; /* * Use this guess for the uart base in order to make an attempt at @@ -93,6 +92,6 @@ void exit(int code) { chr_testdev_exit(code); psci_system_off(); - halt(code); + halt(); __builtin_unreachable(); }
The prototype for the halt() function is incorrect, because halt() doesn't take any arguments. Fix it by using the prototype from smp.h. Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> --- lib/arm/io.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)