linux-kernel - Re: [PATCH v7 04/15] irqchip: RISC-V Local Interrupt Controller Driver

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 for Android: free password hash cracker in your pocket

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

Date:   Wed, 16 Aug 2017 17:12:18 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Palmer Dabbelt <palmer@...belt.com>
cc:     peterz@...radead.org, jason@...edaemon.net, marc.zyngier@....com,
        Arnd Bergmann <arnd@...db.de>, yamada.masahiro@...ionext.com,
        mmarek@...e.com, albert@...ive.com, will.deacon@....com,
        boqun.feng@...il.com, oleg@...hat.com, mingo@...hat.com,
        daniel.lezcano@...aro.org, gregkh@...uxfoundation.org,
        jslaby@...e.com, davem@...emloft.net, mchehab@...nel.org,
        hverkuil@...all.nl, rdunlap@...radead.org, viro@...iv.linux.org.uk,
        mhiramat@...nel.org, fweisbec@...il.com, mcgrof@...nel.org,
        dledford@...hat.com, bart.vanassche@...disk.com,
        sstabellini@...nel.org, mpe@...erman.id.au,
        rmk+kernel@...linux.org.uk, paul.gortmaker@...driver.com,
        nicolas.dichtel@...nd.com, linux@...ck-us.net,
        heiko.carstens@...ibm.com, schwidefsky@...ibm.com,
        geert@...ux-m68k.org, akpm@...ux-foundation.org,
        andriy.shevchenko@...ux.intel.com, jiri@...lanox.com,
        vgupta@...opsys.com, airlied@...hat.com, jk@...abs.org,
        chris@...is-wilson.co.uk, Jason@...c4.com,
        paulmck@...ux.vnet.ibm.com, ncardwell@...gle.com,
        linux-kernel@...r.kernel.org, linux-kbuild@...r.kernel.org,
        patches@...ups.riscv.org
Subject: Re: [PATCH v7 04/15] irqchip: RISC-V Local Interrupt Controller
 Driver

On Mon, 31 Jul 2017, Palmer Dabbelt wrote:
> +static void riscv_software_interrupt(void)
> +{
> +#ifdef CONFIG_SMP
> +	irqreturn_t ret;
> +
> +	ret = handle_ipi();
> +
> +	WARN_ON(ret == IRQ_NONE);

  	WARN_ON(handle_ipi() == IRQ_NONE);

perhaps?

> +#else
> +	/*
> +	 * We currently only use software interrupts to pass inter-processor
> +	 * interrupts, so if a non-SMP system gets a software interrupt then we
> +	 * don't know what to do.
> +	 */
> +	pr_warning("Software Interrupt without CONFIG_SMP\n");
> +#endif
> +}


> +static void riscv_irq_enable(struct irq_data *d)
> +{
> +	struct riscv_irq_data *data = irq_data_get_irq_chip_data(d);
> +
> +	/*
> +	 * It's only possible to write SIE on the current hart.  This jumps
> +	 * over to the target hart if it's not the current one.  It's invalid
> +	 * to write SIE on a hart that's not currently running.
> +	 */
> +	if (data->hart == smp_processor_id())
> +		riscv_irq_unmask(d);
> +	else if (cpu_online(data->hart))
> +		smp_call_function_single(data->hart,
> +					 riscv_irq_enable_helper,
> +					 d,
> +					 true);
> +	else
> +		WARN_ON_ONCE(1);

If you write a small helper:

static void riscv_remote_ctrl(unsigned int cpu, void (*fn)(void *d),
       	    		      struct irq_data *data)
{
	smp_call_function_single(cpu, cb, data, true);
}

Then both riscv_irq_enable() and riscv_irq_disable() become readable
functions.

	if (data->hart == smp_processor_id())
		riscv_irq_unmask(d);
	else if (cpu_online(data->hart))
	     	riscv_remote_ctrl(data->hart, riscv_irq_enable_helper, d);
	else
		WARN_ON_ONCE(1);

Hmm?

> +static int riscv_intc_init(struct device_node *node, struct device_node *parent)
> +{
> +	int hart;
> +	struct riscv_irq_data *data;
> +
> +	if (parent)
> +		return 0;
> +
> +	hart = riscv_of_processor_hart(node->parent);
> +	if (hart < 0)
> +		return -EIO;
> +
> +	data = &per_cpu(riscv_irq_data, hart);
> +	snprintf(data->name, sizeof(data->name), "riscv,cpu_intc,%d", hart);
> +	data->hart = hart;
> +	data->chip.name = data->name;
> +	data->chip.irq_mask = riscv_irq_mask;
> +	data->chip.irq_unmask = riscv_irq_unmask;
> +	data->chip.irq_enable = riscv_irq_enable;
> +	data->chip.irq_disable = riscv_irq_disable;
> +	data->domain = irq_domain_add_linear(
> +		node,
> +		8*sizeof(uintptr_t),
> +		&riscv_irqdomain_ops,
> +		data);

This is really horrible to read. What's wrong with using the full 80 chars?

	data->domain = irq_domain_add_linear(node, 8 * sizeof(uintptr_t),
					     &riscv_irqdomain_ops, data);

> +	if (!data->domain)
> +		goto error_add_linear;
> +	pr_info("%s: %d local interrupts mapped\n",
> +	        data->name, 8*(int)sizeof(uintptr_t));

Can we please make that '8 * sizeof()' a constant and use it in both
places? Which makes the pr_info also fit into a single line.

> +	return 0;
> +
> +error_add_linear:
> +	pr_warning("%s: unable to add IRQ domain\n",
> +		   data->name);

Single line please. Enough room.

> +	return -(ENXIO);

No braces.

Thanks,

	tglx