Thread (17 messages) 17 messages, 4 authors, 2020-09-02

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

From: Marc Zyngier <maz@kernel.org>
Date: 2020-08-19 15:24:28
Also in: linux-devicetree, linux-mediatek, lkml

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help