| Message ID | 20201119145434.28065-1-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/irq: Propagate the error from init_one_desc_irq() in init_irq_data() | expand |
Hi, > On 19 Nov 2020, at 14:54, Julien Grall <julien@xen.org> wrote: > > From: Julien Grall <jgrall@amazon.com> > > init_one_desc_irq() can return an error if it is unable to allocate > memory. While this is unlikely to happen during boot (called from > init_irq_data()), it is better to harden the code by propagting the > return value. > > Spotted by coverity. > > CID: 106529 > > Signed-off-by: Julien Grall <jgrall@amazon.com> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Cheers Bertrand > --- > xen/arch/arm/irq.c | 7 ++++++- > xen/arch/x86/irq.c | 7 ++++++- > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c > index 3877657a5277..279d221a2b85 100644 > --- a/xen/arch/arm/irq.c > +++ b/xen/arch/arm/irq.c > @@ -88,7 +88,12 @@ static int __init init_irq_data(void) > for ( irq = NR_LOCAL_IRQS; irq < NR_IRQS; irq++ ) > { > struct irq_desc *desc = irq_to_desc(irq); > - init_one_irq_desc(desc); > + int rc; > + > + rc = init_one_irq_desc(desc); > + if ( rc ) > + return rc; > + > desc->irq = irq; > desc->action = NULL; > } > diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c > index 45966947919e..3ebd684415ac 100644 > --- a/xen/arch/x86/irq.c > +++ b/xen/arch/x86/irq.c > @@ -428,9 +428,14 @@ int __init init_irq_data(void) > > for ( irq = 0; irq < nr_irqs_gsi; irq++ ) > { > + int rc; > + > desc = irq_to_desc(irq); > desc->irq = irq; > - init_one_irq_desc(desc); > + > + rc = init_one_irq_desc(desc); > + if ( rc ) > + return rc; > } > for ( ; irq < nr_irqs; irq++ ) > irq_to_desc(irq)->irq = irq; > -- > 2.17.1 > >
On Thu, Nov 19, 2020 at 02:54:34PM +0000, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > init_one_desc_irq() can return an error if it is unable to allocate > memory. While this is unlikely to happen during boot (called from > init_irq_data()), it is better to harden the code by propagting the > return value. > > Spotted by coverity. > > CID: 106529 > > Signed-off-by: Julien Grall <jgrall@amazon.com> > --- > xen/arch/x86/irq.c | 7 ++++++- Fox x86: Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c > index 3877657a5277..279d221a2b85 100644 > --- a/xen/arch/arm/irq.c > +++ b/xen/arch/arm/irq.c > @@ -88,7 +88,12 @@ static int __init init_irq_data(void) > for ( irq = NR_LOCAL_IRQS; irq < NR_IRQS; irq++ ) > { > struct irq_desc *desc = irq_to_desc(irq); > - init_one_irq_desc(desc); > + int rc; > + > + rc = init_one_irq_desc(desc); You could init rc at definition. Thanks, Roger.
On Thu, 19 Nov 2020, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > init_one_desc_irq() can return an error if it is unable to allocate > memory. While this is unlikely to happen during boot (called from > init_irq_data()), it is better to harden the code by propagting the > return value. > > Spotted by coverity. > > CID: 106529 > > Signed-off-by: Julien Grall <jgrall@amazon.com> Hi Julien, Thanks for the patch. I was about to commit it when I realized there is one more caller: xen/arch/arm/irq.c:init_local_irq_data Should we change that too to check for the return error? > --- > xen/arch/arm/irq.c | 7 ++++++- > xen/arch/x86/irq.c | 7 ++++++- > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c > index 3877657a5277..279d221a2b85 100644 > --- a/xen/arch/arm/irq.c > +++ b/xen/arch/arm/irq.c > @@ -88,7 +88,12 @@ static int __init init_irq_data(void) > for ( irq = NR_LOCAL_IRQS; irq < NR_IRQS; irq++ ) > { > struct irq_desc *desc = irq_to_desc(irq); > - init_one_irq_desc(desc); > + int rc; > + > + rc = init_one_irq_desc(desc); > + if ( rc ) > + return rc; > + > desc->irq = irq; > desc->action = NULL; > } > diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c > index 45966947919e..3ebd684415ac 100644 > --- a/xen/arch/x86/irq.c > +++ b/xen/arch/x86/irq.c > @@ -428,9 +428,14 @@ int __init init_irq_data(void) > > for ( irq = 0; irq < nr_irqs_gsi; irq++ ) > { > + int rc; > + > desc = irq_to_desc(irq); > desc->irq = irq; > - init_one_irq_desc(desc); > + > + rc = init_one_irq_desc(desc); > + if ( rc ) > + return rc; > } > for ( ; irq < nr_irqs; irq++ ) > irq_to_desc(irq)->irq = irq; > -- > 2.17.1 >
On 19/11/2020 23:44, Stefano Stabellini wrote: > On Thu, 19 Nov 2020, Julien Grall wrote: >> From: Julien Grall <jgrall@amazon.com> >> >> init_one_desc_irq() can return an error if it is unable to allocate >> memory. While this is unlikely to happen during boot (called from >> init_irq_data()), it is better to harden the code by propagting the >> return value. >> >> Spotted by coverity. >> >> CID: 106529 >> >> Signed-off-by: Julien Grall <jgrall@amazon.com> > > Hi Julien, Hi Stefano, > Thanks for the patch. I was about to commit it when I realized there is > one more caller: xen/arch/arm/irq.c:init_local_irq_data > > Should we change that too to check for the return error? We should change too. I will send a new version. Cheers,
Hi Roger, On 19/11/2020 15:11, Roger Pau Monné wrote: > On Thu, Nov 19, 2020 at 02:54:34PM +0000, Julien Grall wrote: >> From: Julien Grall <jgrall@amazon.com> >> >> init_one_desc_irq() can return an error if it is unable to allocate >> memory. While this is unlikely to happen during boot (called from >> init_irq_data()), it is better to harden the code by propagting the >> return value. >> >> Spotted by coverity. >> >> CID: 106529 >> >> Signed-off-by: Julien Grall <jgrall@amazon.com> > >> --- >> xen/arch/x86/irq.c | 7 ++++++- > > Fox x86: > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thank you! > >> 2 files changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c >> index 3877657a5277..279d221a2b85 100644 >> --- a/xen/arch/arm/irq.c >> +++ b/xen/arch/arm/irq.c >> @@ -88,7 +88,12 @@ static int __init init_irq_data(void) >> for ( irq = NR_LOCAL_IRQS; irq < NR_IRQS; irq++ ) >> { >> struct irq_desc *desc = irq_to_desc(irq); >> - init_one_irq_desc(desc); >> + int rc; >> + >> + rc = init_one_irq_desc(desc); > > You could init rc at definition. I need to send a new version, so I have merged the two line together. Cheers,
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c index 3877657a5277..279d221a2b85 100644 --- a/xen/arch/arm/irq.c +++ b/xen/arch/arm/irq.c @@ -88,7 +88,12 @@ static int __init init_irq_data(void) for ( irq = NR_LOCAL_IRQS; irq < NR_IRQS; irq++ ) { struct irq_desc *desc = irq_to_desc(irq); - init_one_irq_desc(desc); + int rc; + + rc = init_one_irq_desc(desc); + if ( rc ) + return rc; + desc->irq = irq; desc->action = NULL; } diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c index 45966947919e..3ebd684415ac 100644 --- a/xen/arch/x86/irq.c +++ b/xen/arch/x86/irq.c @@ -428,9 +428,14 @@ int __init init_irq_data(void) for ( irq = 0; irq < nr_irqs_gsi; irq++ ) { + int rc; + desc = irq_to_desc(irq); desc->irq = irq; - init_one_irq_desc(desc); + + rc = init_one_irq_desc(desc); + if ( rc ) + return rc; } for ( ; irq < nr_irqs; irq++ ) irq_to_desc(irq)->irq = irq;