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

Marc Zyngier
2020-08-19T15:24:28Z

On 2020-08-19 15:55, Mark-PK Tsai wrote:
> 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;
> }

It would make more sense.

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