Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Wed, 19 Aug 2020 16:24:17 +0100
From:   Marc Zyngier <maz@kernel.org>
To:     Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Cc:     alix.wu@mediatek.com, daniel@0x0f.com, devicetree@vger.kernel.org,
        jason@lakedaemon.net, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
        matthias.bgg@gmail.com, robh+dt@kernel.org, tglx@linutronix.de,
        yj.chiang@mediatek.com
Subject: Re: [PATCH 1/2] irqchip: irq-mst: Add MStar interrupt controller
 support
In-Reply-To: <20200819145525.26315-1-mark-pk.tsai@mediatek.com>
References: <f593f5e395c8558657a3f265b7038ec3@kernel.org>
 <20200819145525.26315-1-mark-pk.tsai@mediatek.com>
User-Agent: Roundcube Webmail/1.4.7
Message-ID: <85749ec80ad0b9c85e3984c808d02b9c@kernel.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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