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

[Marc Zyngier]

On 2020-08-19 15:55, Mark-PK Tsai wrote:
> From: Marc Zyngier <maz@xxxxxxxxxx>
>
> > On 2020-08-19 14:31, Mark-PK Tsai wrote:
> > > From: Marc Zyngier <maz@xxxxxxxxxx>
> > >
> > > > > +
> > > > > +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...