Re: [PATCH 3/3] irqchip: RISC-V Local Interrupt Controller Driver

[Thomas Gleixner]

On Fri, 22 Jun 2018, Palmer Dabbelt wrote:
> +struct riscv_irq_data {
> +	struct irq_chip		chip;
> +	struct irq_domain	*domain;
> +	int			hart;
> +	char			name[20];
> +};
> +DEFINE_PER_CPU(struct riscv_irq_data, riscv_irq_data);

static ?

> +static void riscv_intc_irq(struct pt_regs *regs)
> +{
> +	struct pt_regs *old_regs = set_irq_regs(regs);
> +	struct irq_domain *domain;
> +	unsigned long cause = csr_read(scause);
> +
> +	/*
> +	 * The high order bit of the trap cause register is always set for
> +	 * interrupts, which allows us to differentiate them from exceptions
> +	 * quickly.  The INTERRUPT_CAUSE_* macros don't contain that bit, so we
> +	 * need to mask it off here.
> +	 */
> +	WARN_ON((cause & (1UL << (PTR_BITS - 1))) == 0);

So what's the point of continuing here? 


> +	cause = cause & ~(1UL << (PTR_BITS - 1));

Please define a proper CAUSE_MASK or such as this is really hard to read.

> +/*
> + * On RISC-V systems local interrupts are masked or unmasked by writing the SIE
> + * (Supervisor Interrupt Enable) CSR.  As CSRs can only be written on the local
> + * hart, these functions can only be called on the hart that corresponds to the
> + * IRQ chip.  They are only called internally to this module, so they BUG_ON if
> + * this condition is violated rather than attempting to handle the error by
> + * forwarding to the target hart, as that's already expected to have been done.
> + */
> +static void riscv_irq_mask(struct irq_data *d)
> +{
> +	struct riscv_irq_data *data = irq_data_get_irq_chip_data(d);
> +
> +	BUG_ON(smp_processor_id() != data->hart);
> +	csr_clear(sie, 1 << (long)d->hwirq);

What's the point of this type cast? Whether you shift by unsigned long or by
long does not really matter, right? I'd rather expected to see 1U << d->hwirq

> +static int __init riscv_intc_init(struct device_node *node, struct device_node *parent)
> +{
> +	int hart;
> +	struct riscv_irq_data *data;

Nit. Reverse fir tree ordering please

	struct riscv_irq_data *data;
	int hart;

Simpler to parse.

> +
> +	if (parent)
> +		return 0;

Hmm, that wants a comment because it's not clear why this is done for the
casual reader.

> +
> +	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, PTR_BITS,
> +					     &riscv_irqdomain_ops, data);
> +	if (!data->domain)
> +		goto error_add_linear;
> +
> +	set_handle_irq(&riscv_intc_irq);
> +
> +	pr_info("%s: %lu local interrupts mapped\n", data->name, PTR_BITS);
> +	return 0;
> +
> +error_add_linear:
> +	pr_warning("%s: unable to add IRQ domain\n",
> +		   data->name);

One line please.

> +	return -ENXIO;

Thanks,

	tglx