On Mon, Jun 12, 2023 at 07:32:52AM +0000, Oliver Upton wrote:
> Hi Dan,
>
> Thanks for fixing this. Couple of small comments:
>
> On Mon, Jun 12, 2023 at 10:07:46AM +0300, Dan Carpenter wrote:
> > Smatch detected this bug:
> > arch/arm64/kvm/arch_timer.c:1425 kvm_timer_hyp_init()
> > warn: missing unwind goto?
> >
> > There are a couple error paths which do not release their resources
> > correctly. Fix them.
> >
> > Fixes: 9e01dc76be6a ("KVM: arm/arm64: arch_timer: Assign the phys timer on VHE systems")
> > Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > ---
> > arch/arm64/kvm/arch_timer.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> > index 05b022be885b..c2df8332d2bd 100644
> > --- a/arch/arm64/kvm/arch_timer.c
> > +++ b/arch/arm64/kvm/arch_timer.c
> > @@ -1422,7 +1422,7 @@ int __init kvm_timer_hyp_init(bool has_gic)
> > if (err) {
> > kvm_err("kvm_arch_timer: can't request ptimer interrupt %d (%d)\n",
> > host_ptimer_irq, err);
> > - return err;
> > + goto out_free_irq;
> > }
> >
> > if (has_gic) {
> > @@ -1430,7 +1430,7 @@ int __init kvm_timer_hyp_init(bool has_gic)
> > kvm_get_running_vcpus());
> > if (err) {
> > kvm_err("kvm_arch_timer: error setting vcpu affinity\n");
> > - goto out_free_irq;
> > + goto out_free_ptimer_irq;
> > }
> > }
> >
> > @@ -1443,6 +1443,10 @@ int __init kvm_timer_hyp_init(bool has_gic)
> > }
> >
> > return 0;
> > +
> > +out_free_ptimer_irq:
> > + if (info->physical_irq > 0)
> > + free_percpu_irq(host_ptimer_irq, kvm_get_running_vcpus());
>
> nit: we shouldn't even jump to this label in the first place if
> there was no ptimer irq to set up... Maybe just drop the condition?
>
The condition is not necessary but I added it deliberately for
readability and in case we ever add more allocations to this function.
I want to keep it.
> > out_free_irq:
>
> I'd prefer this label be renamed 'out_free_vtimer_irq' to make it
> unambiguous.
I would prefer this too, but I left it out because I don't like to
rename things unnecessarily. However, since we both prefer this, then
I will rename it.
regards,
dan carpenter
