Skip to content

Conversation

@ycsin
Copy link
Member

@ycsin ycsin commented Oct 22, 2024

Instead of doing an irq_lock(), use per-instance spinlock instead.

@ycsin ycsin added the area: RISCV RISCV Architecture (32-bit & 64-bit) label Oct 22, 2024
@ycsin ycsin marked this pull request as ready for review October 22, 2024 08:20
@ycsin ycsin requested review from dcpleung and nashif as code owners October 22, 2024 08:20
@ycsin ycsin requested a review from con-pax October 23, 2024 08:27
fkokosinski
fkokosinski previously approved these changes Oct 25, 2024
Copy link
Member

@fkokosinski fkokosinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ycsin ycsin marked this pull request as draft November 7, 2024 10:32
@ycsin
Copy link
Member Author

ycsin commented Nov 8, 2024

V2 - Changes to prevent deadlock due to recursive spinlocks:

diff --git a/drivers/interrupt_controller/intc_plic.c b/drivers/interrupt_controller/intc_plic.c
index 9303b2a275f..c89161a7645 100644
--- a/drivers/interrupt_controller/intc_plic.c
+++ b/drivers/interrupt_controller/intc_plic.c
@@ -422,6 +422,7 @@ int riscv_plic_irq_set_affinity(uint32_t irq, uint32_t cpumask)
        struct plic_data *data = dev->data;
        __maybe_unused const struct plic_config *config = dev->config;
        const uint32_t local_irq = irq_from_level_2(irq);
+       k_spinlock_key_t key;
 
        if (local_irq >= config->nr_irqs) {
                __ASSERT(false, "overflow: irq %d, local_irq %d", irq, local_irq);
@@ -433,18 +434,16 @@ int riscv_plic_irq_set_affinity(uint32_t irq, uint32_t cpumask)
                return -EINVAL;
        }
 
-       k_spinlock_key_t key = k_spin_lock(&data->lock);
-
+       key = k_spin_lock(&data->lock);
        /* Updated irq_cpumask for next time setting plic enable register */
        data->irq_cpumask[local_irq] = (plic_cpumask_t)cpumask;
+       k_spin_unlock(&data->lock, key);
 
        /* If irq is enabled, apply the new irq affinity */
        if (riscv_plic_irq_is_enabled(irq)) {
                riscv_plic_irq_enable(irq);
        }
 
-       k_spin_unlock(&data->lock, key);
-
        return 0;
 }
 #endif /* CONFIG_PLIC_IRQ_AFFINITY */

@ycsin ycsin marked this pull request as ready for review November 8, 2024 09:31
@ycsin ycsin requested a review from fkokosinski November 8, 2024 09:31
@ycsin ycsin marked this pull request as draft November 8, 2024 09:41
Instead of doing an `irq_lock()`, use per-instance spinlock instead.

Refactored out an unlocked version of `local_irq_is_enabled`
from `riscv_plic_irq_is_enabled()` to achieve that.

Signed-off-by: Yong Cong Sin <[email protected]>
@ycsin
Copy link
Member Author

ycsin commented Nov 8, 2024

V3 - reduces the number of spinlock lock/unlock

(.venv) ycsin@ycsin-mba zephyr % git diff            
diff --git a/drivers/interrupt_controller/intc_plic.c b/drivers/interrupt_controller/intc_plic.c
index c89161a7645..06e0fa52dda 100644
--- a/drivers/interrupt_controller/intc_plic.c
+++ b/drivers/interrupt_controller/intc_plic.c
@@ -310,29 +310,16 @@ void riscv_plic_irq_disable(uint32_t irq)
        k_spin_unlock(&data->lock, key);
 }
 
-/**
- * @brief Check if a riscv PLIC-specific interrupt line is enabled
- *
- * This routine checks if a RISCV PLIC-specific interrupt line is enabled.
- * @param irq IRQ number to check
- *
- * @return 1 or 0
- */
-int riscv_plic_irq_is_enabled(uint32_t irq)
+static int local_irq_is_enabled(const struct device *dev, uint32_t local_irq)
 {
-       const struct device *dev = get_plic_dev_from_irq(irq);
-       struct plic_data *data = dev->data;
-       const uint32_t local_irq = irq_from_level_2(irq);
        uint32_t bit_position = local_irq & PLIC_REG_MASK;
-       uint32_t en_value;
        int is_enabled = IS_ENABLED(CONFIG_PLIC_IRQ_AFFINITY) ? 0 : 1;
-       k_spinlock_key_t key = k_spin_lock(&data->lock);
 
        for (uint32_t cpu_num = 0; cpu_num < arch_num_cpus(); cpu_num++) {
                mem_addr_t en_addr =
                        get_context_en_addr(dev, cpu_num) + local_irq_to_reg_offset(local_irq);
+               uint32_t en_value = sys_read32(en_addr);
 
-               en_value = sys_read32(en_addr);
                if (IS_ENABLED(CONFIG_PLIC_IRQ_AFFINITY)) {
                        is_enabled |= !!(en_value & BIT(bit_position));
                } else {
@@ -340,11 +327,31 @@ int riscv_plic_irq_is_enabled(uint32_t irq)
                }
        }
 
-       k_spin_unlock(&data->lock, key);
-
        return is_enabled;
 }
 
+/**
+ * @brief Check if a riscv PLIC-specific interrupt line is enabled
+ *
+ * This routine checks if a RISCV PLIC-specific interrupt line is enabled.
+ * @param irq IRQ number to check
+ *
+ * @return 1 or 0
+ */
+int riscv_plic_irq_is_enabled(uint32_t irq)
+{
+       const struct device *dev = get_plic_dev_from_irq(irq);
+       struct plic_data *data = dev->data;
+       const uint32_t local_irq = irq_from_level_2(irq);
+       int ret = 0;
+
+       K_SPINLOCK(&data->lock) {
+               ret = local_irq_is_enabled(dev, local_irq);
+       }
+
+       return ret;
+}
+
 /**
  * @brief Set priority of a riscv PLIC-specific interrupt line
  *
@@ -437,12 +444,12 @@ int riscv_plic_irq_set_affinity(uint32_t irq, uint32_t cpumask)
        key = k_spin_lock(&data->lock);
        /* Updated irq_cpumask for next time setting plic enable register */
        data->irq_cpumask[local_irq] = (plic_cpumask_t)cpumask;
-       k_spin_unlock(&data->lock, key);
 
        /* If irq is enabled, apply the new irq affinity */
-       if (riscv_plic_irq_is_enabled(irq)) {
-               riscv_plic_irq_enable(irq);
+       if (local_irq_is_enabled(dev, local_irq)) {
+               plic_irq_enable_set_state(irq, true);
        }
+       k_spin_unlock(&data->lock, key);
 
        return 0;
 }

@ycsin ycsin marked this pull request as ready for review November 9, 2024 08:00
@ycsin ycsin requested review from cfriedt and luchnikov November 19, 2024 02:44
@nashif nashif merged commit 6843240 into zephyrproject-rtos:main Nov 20, 2024
24 checks passed
@ycsin ycsin deleted the pr/plic-spinlock branch November 20, 2024 13:47
@fabiobaltieri fabiobaltieri mentioned this pull request Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Interrupt Controller area: RISCV RISCV Architecture (32-bit & 64-bit)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants