| Message ID | 1384529257-7590-3-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Fri, 15 Nov 2013, Julien Grall wrote: > The caller of xen_init_time, start_xen, doesn't check the return value > of the function. Xen will silently ignore the error and continue. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > xen/arch/arm/time.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c > index a30d422..938995d 100644 > --- a/xen/arch/arm/time.c > +++ b/xen/arch/arm/time.c > @@ -132,7 +132,7 @@ int __init init_xen_time(void) > > res = platform_init_time(); > if ( res ) > - return res; > + panic("Timer: Cannot initialize platform timer\n"); > > /* Check that this CPU supports the Generic Timer interface */ > if ( !cpu_has_gentimer ) > -- > 1.8.3.1 >
On Fri, 2013-11-15 at 15:58 +0000, Stefano Stabellini wrote: > On Fri, 15 Nov 2013, Julien Grall wrote: > > The caller of xen_init_time, start_xen, doesn't check the return value > > of the function. Xen will silently ignore the error and continue. > > > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> Acked-by: Ian Campbell <ian.campbell@citrix.com> I did wonder if it might be worth pushing the panic down into platform_time_init, so that the message could be more specific. But that seems like it relies on platform code to be more aware of when to panic and to use consistent wording etc. So all in all I think it is better here. > > > > xen/arch/arm/time.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c > > index a30d422..938995d 100644 > > --- a/xen/arch/arm/time.c > > +++ b/xen/arch/arm/time.c > > @@ -132,7 +132,7 @@ int __init init_xen_time(void) > > > > res = platform_init_time(); > > if ( res ) > > - return res; > > + panic("Timer: Cannot initialize platform timer\n"); > > > > /* Check that this CPU supports the Generic Timer interface */ > > if ( !cpu_has_gentimer ) > > -- > > 1.8.3.1 > >
On 11/19/2013 02:47 PM, Ian Campbell wrote: > On Fri, 2013-11-15 at 15:58 +0000, Stefano Stabellini wrote: >> On Fri, 15 Nov 2013, Julien Grall wrote: >>> The caller of xen_init_time, start_xen, doesn't check the return value >>> of the function. Xen will silently ignore the error and continue. >>> >>> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> >> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > Acked-by: Ian Campbell <ian.campbell@citrix.com> > > I did wonder if it might be worth pushing the panic down into > platform_time_init, so that the message could be more specific. But that > seems like it relies on platform code to be more aware of when to panic > and to use consistent wording etc. So all in all I think it is better > here. I thought about this solution. I didn't find good argument for one of these solutions. So, by default, I choose to panic in init_xen_time.
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c index a30d422..938995d 100644 --- a/xen/arch/arm/time.c +++ b/xen/arch/arm/time.c @@ -132,7 +132,7 @@ int __init init_xen_time(void) res = platform_init_time(); if ( res ) - return res; + panic("Timer: Cannot initialize platform timer\n"); /* Check that this CPU supports the Generic Timer interface */ if ( !cpu_has_gentimer )
The caller of xen_init_time, start_xen, doesn't check the return value of the function. Xen will silently ignore the error and continue. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/arch/arm/time.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)