Messages in this thread | | | From | Mark-PK Tsai <> | Subject | Re: [PATCH 1/2] irqchip: irq-mst: Add MStar interrupt controller support | Date | Wed, 19 Aug 2020 22:55:26 +0800 |
| |
From: Marc Zyngier <maz@kernel.org>
>On 2020-08-19 14:31, Mark-PK Tsai wrote: >> From: Marc Zyngier <maz@kernel.org> >> >>> > + >>> > +static int mst_intc_domain_alloc(struct irq_domain *domain, unsigned >>> > int virq, >>> > + unsigned int nr_irqs, void *data) >>> > +{ >>> > + int i; >>> > + irq_hw_number_t hwirq; >>> > + struct irq_fwspec parent_fwspec, *fwspec = data; >>> > + struct mst_intc_chip_data *cd = (struct mst_intc_chip_data >>> > *)domain->host_data; >>> >>> No cast necessary here. >>> >>> > + >>> > + /* Not GIC compliant */ >>> > + if (fwspec->param_count != 3) >>> > + return -EINVAL; >>> > + >>> > + /* No PPI should point to this domain */ >>> > + if (fwspec->param[0]) >>> > + return -EINVAL; >>> > + >>> > + if (fwspec->param[1] >= cd->nr_irqs) >>> >>> This condition is bogus, as it doesn't take into account the nr_irqs >>> parameter. >>> >> >> >> The hwirq number need to be in the irq map range. (property: >> mstar,irqs-map-range) >> If it's not, it must be incorrect configuration. > >I agree. And since you are checking whether the configuration is >correct, >it'd better be completely correct. > >> So how about use the condition as following? >> >> if (hwirq >= cd->nr_irqs) >> return -EINVAL; > >Again, this says nothing of the validity of (hwirq + nr_irqs - 1)... >
How about move this to mst_intc_domain_translate? Then all the irq_fwspec point to domain mst_intc should be valid.
The mst_intc_domain_translate will be as following:
static int mst_intc_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type) { struct mst_intc_chip_data *cd = d->host_data;
if (is_of_node(fwspec->fwnode)) { if (fwspec->param_count != 3) return -EINVAL;
/* No PPI should point to this domain */ if (fwspec->param[0] != 0) return -EINVAL;
if (fwspec->param[1] >= cd->nr_irqs) return -EINVAL;
*hwirq = fwspec->param[1]; *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; return 0; }
return -EINVAL; }
>> >>> > + return -EINVAL; >>> > + >>> > + hwirq = fwspec->param[1]; >>> > + for (i = 0; i < nr_irqs; i++) >>> > + irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i, >>> > + &mst_intc_chip, >>> > + domain->host_data); | |