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

Mark-PK Tsai
2020-08-19T14:56:22Z

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