On Mon, 03 Jul 2017 04:13:34 PDT (-0700), t...@linutronix.de wrote:
> On Thu, 29 Jun 2017, Palmer Dabbelt wrote:
>> On Wed, 28 Jun 2017 13:47:40 PDT (-0700), t...@linutronix.de 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!
