Re: [PATCH 1/2] irqchip: irq-mst: Add MStar interrupt controller support

Marc Zyngier
2020-08-19T13:42:55Z

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)...

>
>> > + 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);

... which you are using here.

M.
--
Jazz is not dead. It just smells funny...