linux-kernel - Re: [RFC PATCH 1/2] irqchip: ti-tsir: Add support for Timesync Interrupt Router

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87lduin4o5.ffs@tglx>
Date: Thu, 06 Feb 2025 22:28:58 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: Chintan Vankar <c-vankar@...com>, Jason Reeder <jreeder@...com>,
 vigneshr@...com, nm@...com, Chintan
 Vankar <c-vankar@...com>, Paolo Abeni <pabeni@...hat.com>, Jakub Kicinski
 <kuba@...nel.org>, Eric Dumazet <edumazet@...gle.com>, "David S. Miller"
 <davem@...emloft.net>, Andrew Lunn <andrew+netdev@...n.ch>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org, srk@...com,
 s-vadapalli@...com, danishanwar@...com, m-malladi@...com
Subject: Re: [RFC PATCH 1/2] irqchip: ti-tsir: Add support for Timesync
 Interrupt Router

On Wed, Feb 05 2025 at 21:31, Chintan Vankar wrote:
> +++ b/drivers/irqchip/ti-timesync-intr.c
> @@ -0,0 +1,109 @@
> +// SPDX-License-Identifier: GPL

That's not a valid license identifier

> +static struct irq_chip ts_intr_irq_chip = {
> +	.name			= "TIMESYNC_INTRTR",
> +};

How is this interrupt chip supposed to work? All it implements is a
name.

> +static int ts_intr_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> +				    unsigned int nr_irqs, void *arg)
> +{
> +	unsigned int output_line, input_line, output_line_offset;
> +	struct irq_fwspec *fwspec = (struct irq_fwspec *)arg;
> +	int ret;
> +
> +	irq_domain_set_hwirq_and_chip(domain, virq, output_line,
> +				      &ts_intr_irq_chip,
> +				      NULL);

You set the interrupt chip and data before validating that the input
argument is valid. That does not make any sense.

> +	/* Check for two input parameters: output line and corresponding input line */
> +	if (fwspec->param_count != 2)
> +		return -EINVAL;
> +
> +	output_line = fwspec->param[0];
> +
> +	/* Timesync Interrupt Router's mux-controller register starts at offset 4 from base
> +	 * address and each output line are at offset in multiple of 4s in Timesync INTR's
> +	 * register space, calculate the register offset from provided output line.
> +	 */

Please use proper kernel comment style as documented:

  https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#comment-style

This is not networking code.

> +	output_line_offset = 4 * output_line + 0x4;

Magic hardcoded numbers '4' and '0x4' without any explanation of the logic.

> +	output_line_to_virq[output_line] = virq;
> +	input_line = fwspec->param[1] & TIMESYNC_INTRTR_ENABLE;
> +
> +	/* Map output line corresponding to input line */
> +	writel(input_line, tsr_data.tsr_base + output_line_offset);
> +
> +	/* When interrupt enable bit is set for Timesync Interrupt Router it maps the output
> +	 * line with the existing input line, hence enable interrupt line after we set bits for
> +	 * output line.

I have no idea what this comment is trying to tell me.

> +	 */
> +	input_line |= TIMESYNC_INTRTR_INT_ENABLE;
> +	writel(input_line, tsr_data.tsr_base + output_line_offset);
> +
> +	return 0;
> +}
> +
> +static void ts_intr_irq_domain_free(struct irq_domain *domain, unsigned int virq,
> +				    unsigned int nr_irqs)
> +{
> +	struct output_line_to_virq *node, *n;
> +	unsigned int output_line_offset;
> +	int i;
> +
> +	for (i = 0; i < TIMESYNC_INTRTR_MAX_OUTPUT_LINES; i++) {
> +		if (output_line_to_virq[i] == virq) {
> +			/* Calculate the register offset value from provided output line */

Can you please implement a properly commented helper function which
explains how this offset calculation is supposed to work?

> +			output_line_offset = 4 * i + 0x4;
> +			writel(~TIMESYNC_INTRTR_INT_ENABLE, tsr_data.tsr_base + output_line_offset);
> +		}
> +	}
> +}
> +
> +static const struct irq_domain_ops ts_intr_irq_domain_ops = {
> +	.alloc		= ts_intr_irq_domain_alloc,
> +	.free		= ts_intr_irq_domain_free,
> +};
> +
> +static int tsr_init(struct device_node *node)
> +{
> +	tsr_data.tsr_base = of_iomap(node, 0);
> +	if (IS_ERR(tsr_data.tsr_base)) {
> +		pr_err("Unable to get reg\n");
> +		return PTR_ERR(tsr_data.tsr_base);
> +	}
> +
> +	tsr_data.domain = irq_domain_create_tree(&node->fwnode, &ts_intr_irq_domain_ops, &tsr_data);

So this instantiates a interrupt domain which is completely disconnected
from the rest of the world.

How is the output side of this supposed to handle an interrupt which is
routed to it?

Thanks,

        tglx