From: Sunil Muthuswamy <sunilmut@xxxxxxxxxxxxx> Sent: Tuesday, January 4, 2022 11:24 AM
>
> On Mon, 27 Dec 2021 17:38:07 +0000,
> "Michael Kelley (LINUX)" <mikelley@xxxxxxxxxxxxx> wrote:
> >
> > From: Sunil Muthuswamy <sunilmut@xxxxxxxxxxxxxxxxxxx> Sent: Friday,
> > December 17, 2021 10:52 AM
> > >
[snip]
> > > +
> > > +static int hv_pci_vec_irq_domain_alloc(struct irq_domain *domain,
> > > + unsigned int virq, unsigned int nr_irqs,
> > > + void *args)
> > > +{
> > > + irq_hw_number_t hwirq;
> > > + unsigned int i;
> > > + int ret;
> > > +
> > > + ret = hv_pci_vec_alloc_device_irq(domain, nr_irqs, &hwirq);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + for (i = 0; i < nr_irqs; i++) {
> > > + ret = hv_pci_vec_irq_gic_domain_alloc(domain, virq + i,
> > > + hwirq + i);
> > > + if (ret)
> > > + goto free_irq;
> > > +
> > > + ret = irq_domain_set_hwirq_and_chip(domain, virq + i,
> > > + hwirq + i,
> > > + &hv_arm64_msi_irq_chip,
> > > + domain->host_data);
> > > + if (ret)
> > > + goto free_irq;
> >
> > This error path doesn't clean up correctly. While parent IRQs allocated
> > in previous iterations of the loop is cleaned up, the parent IRQ
> > allocated in the current iteration is not.
> >
>
> 'irq_domain_set_hwirq_and_chip' really shouldn't fail. If you still feel
> that we should address this, I can.
>
My view is that the code should be logically consistent. If the
error path should never happen, then we can ignore the return value
and not try to do any cleanup. But if we are going to treat the error
as possible and have cleanup code, then the cleanup code should be
correct. It looks like the GIC irqchip drivers follow your approach and
assume irq_domain_set_hwirq_and_chip() never fails, so they don't
check the return value. I would be OK with that approach here.
Michael
