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

[Palmer Dabbelt]

On Mon, 03 Jul 2017 04:13:34 PDT (-0700), tglx@xxxxxxxxxxxxx wrote:
> 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.

OK, we'll do it that way.  I'll submit another patch set soon.

Thanks!