lkml.org 
[lkml]   [2017]   [Jun]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patches in this message
/
Date
From
SubjectRe: [PATCH 1/2] irqchip: RISC-V Local Interrupt Controller Driver
On Wed, 28 Jun 2017 13:47:40 PDT (-0700), tglx@linutronix.de wrote:
> On Mon, 26 Jun 2017, Palmer Dabbelt wrote:
>> +DEFINE_PER_CPU(struct riscv_irq_data, riscv_irq_data);
>> +DEFINE_PER_CPU(atomic_long_t, riscv_early_sie);
>> +
>> +static void riscv_software_interrupt(void)
>> +{
>> +#ifdef CONFIG_SMP
>> + irqreturn_t ret;
>> +
>> + ret = handle_ipi();
>> + if (ret != IRQ_NONE)
>> + return;
>> +#endif
>> +
>> + BUG();
>
> Are you sure you want to crash the system just because a spurious interrupt
> happened?

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.

How does this look?

>> +}
>> +
>> +asmlinkage void __irq_entry do_IRQ(unsigned int cause, struct pt_regs *regs)
>> +{
>> + struct pt_regs *old_regs = set_irq_regs(regs);
>> + struct irq_domain *domain;
>> +
>> + irq_enter();
>> +
>> + /* There are three classes of interrupt: timer, software, and
>
> Please use proper multiline comment formatting:

Sorry about that. I went and fixed all the comments in here, I'll do it in the
PLIC driver (patch 2) as well.

> /*
> * bla.....
> * foo.....
> */
>
>> + * external devices. We dispatch between them here. External
>> + * device interrupts use the generic IRQ mechanisms.
>> + */
>> + switch (cause) {
>> + case INTERRUPT_CAUSE_TIMER:
>> + riscv_timer_interrupt();
>> + break;
>> + case INTERRUPT_CAUSE_SOFTWARE:
>> + riscv_software_interrupt();
>> + break;
>> + default:
>> + domain = per_cpu(riscv_irq_data, smp_processor_id()).domain;
>> + generic_handle_irq(irq_find_mapping(domain, cause));
>> + break;
>> + }
>> +
>> + irq_exit();
>> + set_irq_regs(old_regs);
>> +}
>> +
>> +static int riscv_irqdomain_map(struct irq_domain *d, unsigned int irq,
>> + irq_hw_number_t hwirq)
>> +{
>> + struct riscv_irq_data *data = d->host_data;
>> +
>> + irq_set_chip_and_handler(irq, &data->chip, handle_simple_irq);
>> + irq_set_chip_data(irq, data);
>> + irq_set_noprobe(irq);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct irq_domain_ops riscv_irqdomain_ops = {
>> + .map = riscv_irqdomain_map,
>> + .xlate = irq_domain_xlate_onecell,
>> +};
>> +
>> +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.

>> + csr_clear(sie, 1 << (long)d->hwirq);
>> +}
>> +
>> +static void riscv_irq_unmask(struct irq_data *d)
>> +{
>> + struct riscv_irq_data *data = irq_data_get_irq_chip_data(d);
>> +
>> + BUG_ON(smp_processor_id() != data->hart);
>> + csr_set(sie, 1 << (long)d->hwirq);
>> +}
>> +
>> +static void riscv_irq_enable_helper(void *d)
>> +{
>> + riscv_irq_unmask(d);
>> +}
>> +
>> +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.
>
> And what exactly has this to do with irq_enable()? Why would you call that
> for an interrupt which solely goes to a offline cpu?
>
>> +static void riscv_irq_disable(struct irq_data *d)
>> +{
>> + struct riscv_irq_data *data = irq_data_get_irq_chip_data(d);
>> +
>> + /* This is the mirror of riscv_irq_enable. */
>> + atomic_long_and(~(1 << (long)d->hwirq),
>> + &per_cpu(riscv_early_sie, data->hart));
>> + if (data->hart == smp_processor_id())
>> + riscv_irq_mask(d);
>> + else if (cpu_online(data->hart))
>> + smp_call_function_single(data->hart,
>> + riscv_irq_disable_helper,
>> + d,
>> + true);
>
> Same question as above.

I've attempted to improve the comment that describes why this is necessary.

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.
+ * 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));
I'm not sure how to get around this.

>> +}
>> +
>> +static void riscv_irq_mask_noop(struct irq_data *d) { }
>> +
>> +static void riscv_irq_unmask_noop(struct irq_data *d) { }
>>
>> +static void riscv_irq_enable_noop(struct irq_data *d)
>> +{
>> + struct device_node *data = irq_data_get_irq_chip_data(d);
>> + u32 hart;
>> +
>> + if (!of_property_read_u32(data, "reg", &hart))
>> + printk(
>> + KERN_WARNING "enabled interrupt %d for missing hart %d (this interrupt has no handler)\n",
>
> Has no handler? I really have a hard time to understand the logic here.
>
>> + (int)d->hwirq, hart);
>> +}
>> +
>> +static struct irq_chip riscv_noop_chip = {
>> + .name = "riscv,cpu-intc,noop",
>> + .irq_mask = riscv_irq_mask_noop,
>> + .irq_unmask = riscv_irq_unmask_noop,
>> + .irq_enable = riscv_irq_enable_noop,
>
> Please write that in tabular fashion:
>
> .name = "riscv,cpu-intc,noop",
> .irq_mask = riscv_irq_mask_noop,
>
>> +};
>> +
>> +static int riscv_irqdomain_map_noop(struct irq_domain *d, unsigned int irq,
>> + irq_hw_number_t hwirq)
>> +{
>> + struct device_node *data = d->host_data;
>> +
>> + irq_set_chip_and_handler(irq, &riscv_noop_chip, handle_simple_irq);
>> + irq_set_chip_data(irq, data);
>> + return 0;
>> +}
>> +
>> +static const struct irq_domain_ops riscv_irqdomain_ops_noop = {
>> + .map = riscv_irqdomain_map_noop,
>> + .xlate = irq_domain_xlate_onecell,
>> +};
>> +
>> +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.
+ *
+ * For example, we generally allow the PLIC to route interrupts to every hart
+ * in the system via the local interrupt controller on every hart. 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. 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.
+ */
static void riscv_irq_mask_noop(struct irq_data *d) { }
static void riscv_irq_unmask_noop(struct irq_data *d) { }
@@ -189,7 +215,7 @@ static void riscv_irq_enable_noop(struct irq_data *d)

if (!of_property_read_u32(data, "reg", &hart))
printk(
- KERN_WARNING "enabled interrupt %d for missing hart %d (this interrupt has no handler)\n",
+ KERN_WARNING "enabled no-op handler for interrupt %d on missing hart %d\n",
(int)d->hwirq, hart);
}
@@ -229,7 +255,7 @@ static int riscv_intc_init(struct device_node *node, struct device_node *parent)
* 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!
+ * it! See the comment above for more details.
*/
irq_domain_add_linear(
node,

Sorry if that's an odd way to do things -- if there's a better way to do this
then I'm all ears!

>> + }
>> +
>> + 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);
>> + WARN_ON(!data->domain);
>
> That warn_on is great. You warn and then continue as if nothing
> happened. Machine will crash later dereferencing a NULL pointer.

OK, makes sense. I checked another driver and they're returning ENXIO in this
case, so I did that.

diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
index 52db58a94bdc..11d8084a0ab1 100644
--- a/drivers/irqchip/irq-riscv-intc.c
+++ b/drivers/irqchip/irq-riscv-intc.c
@@ -245,6 +245,7 @@ static int riscv_intc_init(struct device_node *node, struct device_node *parent)
{
int hart;
struct riscv_irq_data *data;
+ struct irq_domain *domain;
if (parent)
return 0;
@@ -257,11 +258,13 @@ static int riscv_intc_init(struct device_node *node, struct device_node *parent)
* not wrong... unless they actually load a driver that needs
* it! See the comment above for more details.
*/
- irq_domain_add_linear(
+ domain = irq_domain_add_linear(
node,
8*sizeof(uintptr_t),
&riscv_irqdomain_ops_noop,
node->parent);
+ if (!domain)
+ goto error_add_linear;
return 0;
}
@@ -278,10 +281,17 @@ static int riscv_intc_init(struct device_node *node, struct device_node *parent)
8*sizeof(uintptr_t),
&riscv_irqdomain_ops,
data);
- WARN_ON(!data->domain);
+ if (!data->domain)
+ goto error_add_linear;
printk(KERN_INFO "%s: %d local interrupts mapped\n",
data->name, 8*(int)sizeof(uintptr_t));
return 0;
+
+error_add_linear:
+ printk(KERN_WARNING "%s: unable to add IRQ domain\n",
+ data->name);
+ return -(ENXIO);
+
}
IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);

> Some more epxlanations about the inner workings of all of this would be
> appreciated.

Hopefully the comments I added help here.

Thanks!

\
 
 \ /
  Last update: 2017-06-29 18:30    [W:0.074 / U:0.880 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site