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

[Thomas Gleixner]

On Thu, 29 Jun 2017, Palmer Dabbelt wrote:
> On Wed, 28 Jun 2017 13:47:40 PDT (-0700), tglx@xxxxxxxxxxxxx wrote:
> In this case the software interrupt is to handle IPIs, so it doesn't really
> make any sense to handle one without SMP.  I'm OK with just warning, though, as
> the IPIs are just for TLB shootdowns so skipping one on a non-SMP system seems
> safe.

Indeed.

> >> +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);
> >
> > Crashing the machine is the last resort if there is no chance to handle a
> > situation gracefully. Something like
> >
> > 	if (WARN_ON_ONCE(smp_processor_id() != data->hart))
> > 		do_something_sensible();
> > 	else
> > 		.....
> >
> > might at least keep the machine halfways functional for debugging.
> 
> In this case I think BUG_ON is the only sane thing to do.  I've gone and added
> a comment that explains what's going on here
> 
>   diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
>   index 7e55fe57e95f..3dd421ade128 100644
>   --- a/drivers/irqchip/irq-riscv-intc.c
>   +++ b/drivers/irqchip/irq-riscv-intc.c
>   @@ -97,6 +97,14 @@ static const struct irq_domain_ops riscv_irqdomain_ops = {
>           .xlate  = irq_domain_xlate_onecell,
>    };
> 
>   +/*
>   + * 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 cooresponds 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);
> 
> I think there's three options here:
> 
>  * BUG_ON like we do.
>  * Attempt to jump to the correct hart to set the CSR, but since we just did
>    that I don't really see why it would work the second time.
>  * Provide a warning and then ignore {un,}masking the IRQ, but that seems
>    dangerous.
> 
> I can change it to a warning if you think it's better.

With a coherent explanation why a BUG_ON() is the right thing to do, the
BUG_ON can stay.

>   diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
>   index 3dd421ade128..b2643f7131ff 100644
>   --- a/drivers/irqchip/irq-riscv-intc.c
>   +++ b/drivers/irqchip/irq-riscv-intc.c
>   @@ -137,9 +137,13 @@ static void riscv_irq_enable(struct irq_data *d)
>           struct riscv_irq_data *data = irq_data_get_irq_chip_data(d);
> 
>           /*
>   -        * There are two phases to setting up an interrupt: first we set a bit
>   -        * in this bookkeeping structure, which is used by trap_init to
>   -        * initialize SIE for each hart as it comes up.
>   +        * When booting a RISC-V system, procesors can come online at any time.

Not so much at any time. The kernel orchestrates that.

>   +        * Interrupts can only be enabled or disabled by writing a CSR on the
>   +        * hart that cooresponds to that interrupt controller, but CSRs can
>   +        * only be written locally.  In order to avoid waiting a long time for
>   +        * a hart to boot, we instead collect the interrupts to be enabled upon
>   +        * booting a hart in this bookkeeping structure, which is used by
>   +        * trap_init to initialize SIE for each hart as it comes up.
>            */
>           atomic_long_or((1 << (long)d->hwirq),
>                          &per_cpu(riscv_early_sie, data->hart));

That still does not answer the question WHY an interrupt for a not
available CPU would be enabled in the first place.
 
> >> +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) {
> >> +		/* If a hart is disabled, create a no-op irq domain.  Devices
> >> +		 * may still have interrupts connected to those harts.  This is
> >> +		 * not wrong... unless they actually load a driver that needs
> >> +		 * it!
> >> +		 */
> >> +		irq_domain_add_linear(
> >> +			node,
> >> +			8*sizeof(uintptr_t),
> >> +			&riscv_irqdomain_ops_noop,
> >> +			node->parent);
> >> +		return 0;
> >
> > I have a hard time to understand the logic here. Why do you need an
> > interrupt domain for something which does not exist? That does not make any
> > sense.
> 
> I think this is best explained as a comment, which I've added
> 
>   diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
>   index a62a1f04198e..52db58a94bdc 100644
>   --- a/drivers/irqchip/irq-riscv-intc.c
>   +++ b/drivers/irqchip/irq-riscv-intc.c
>   @@ -178,6 +178,32 @@ static void riscv_irq_disable(struct irq_data *d)
>                                            true);
>    }
> 
>   +/*
>   + * This "no op" interrupt handler deals with harts that for some reason exist
>   + * but can't actually run Linux.  Examples of these sorts of harts include:
>   + * - Harts that don't report an "okay" status, presumably because of a hardware
>   + *   fault.
>   + * - Harts with an ID larger than NR_CPUS.
>   + * - Harts with an ISA we don't understand.
>   + *
>   + * Interrupts targeted to these harts won't even actually be seen by Linux, as
>   + * there's no code running to ever receive the interrupt.  If this is an
>   + * invalid system configuration then there's nothing we can really do about it,
>   + * but we expect these configurations to usually be valid.

But what would enable such interrupts in the first place? The device whose
interrupt is routed into nirwana is going to be non-functional. See below.

>   + * For example, we generally allow the PLIC to route interrupts to every hart
>   + * in the system via the local interrupt controller on every hart.

That's fine, but you really want to control that via the interrupt affinity
mechanism and not by blindly routing every interrupt to every possible CPU
in the system. That mechanism is there for a reason.

>   + * When Linux
>   + * is built without CONFIG_SMP this still allows for a working system, as the
>   + * single enabled hart can handle every device interrupt in the system.

Well, that's how it should be. But that still does not require to handle
anything which is not usable.

>   + * It is
>   + * possible to build a PLIC that doesn't allow interrupts to be routed to the
>   + * first hart to boot.  If for some reason the PLIC was instantiated in this
>   + * manner then whatever devices could not be directed to the first hart to boot
>   + * would be unusable on non-CONFIG_SMP kernels, but as we can't change the PLIC
>   + * hardware there's nothing we can do about that.
>   + *
>   + * In order to avoid the complexity of having unmapped IRQ domains, we instead
>   + * just install the noop IRQ handler in domains we can't handle.

What's complex about that? If a device driver tries to request an unmapped
irq then the request will fail and the driver can act accordingly.

Silently pretending success and then letting the user bang his head against
the wall is definitely the worst thing you can do.

Thanks,

	tglx