From 380bcabba83e28db1baeb2bf7ac088bbc4abd41c Mon Sep 17 00:00:00 2001 From: xucheng5 Date: Mon, 30 Jun 2025 21:02:15 +0800 Subject: [PATCH] sja1000: replace enter_critical_section with spinlock Replace enter_critical_section() with spinlock-based protection to avoid sleeping in atomic or interrupt contexts. Signed-off-by: xucheng5 --- drivers/can/sja1000.c | 73 +++++-------------------------------- include/nuttx/can/sja1000.h | 5 +-- 2 files changed, 11 insertions(+), 67 deletions(-) diff --git a/drivers/can/sja1000.c b/drivers/can/sja1000.c index 681151a05bb..86395180d4f 100644 --- a/drivers/can/sja1000.c +++ b/drivers/can/sja1000.c @@ -40,6 +40,7 @@ #include #include #include +#include #include #include @@ -227,11 +228,7 @@ static void sja1000_reset(struct can_dev_s *dev) caninfo("SJA1000 Device %" PRIu8 "\n", port); -#ifdef CONFIG_ARCH_HAVE_MULTICPU flags = spin_lock_irqsave(&priv->lock); -#else - flags = enter_critical_section(); -#endif /* CONFIG_ARCH_HAVE_MULTICPU */ /* Disable the SJA1000 and stop ongoing transmissions */ @@ -260,11 +257,6 @@ static void sja1000_reset(struct can_dev_s *dev) ret = sja1000_baud_rate(priv, config->bitrate, config->clk_freq, config->sjw, config->samplep, 0); - if (ret != OK) - { - canerr("ERROR: Failed to set bit timing: %d\n", ret); - } - /* Restart the SJA1000 */ if (config->loopback) @@ -287,11 +279,12 @@ static void sja1000_reset(struct can_dev_s *dev) sja1000_putreg(priv, SJA1000_CMD_REG, SJA1000_ABORT_TX_M | SJA1000_CLR_OVERRUN_M); -#ifdef CONFIG_ARCH_HAVE_MULTICPU spin_unlock_irqrestore(&priv->lock, flags); -#else - leave_critical_section(flags); -#endif /* CONFIG_ARCH_HAVE_MULTICPU */ + + if (ret != OK) + { + canerr("ERROR: sja1000_baud_rate Failed to set bit timing: %d\n", ret); + } } /**************************************************************************** @@ -320,11 +313,7 @@ static int sja1000_setup(struct can_dev_s *dev) caninfo("SJA1000 (%" PRIu8 ")\n", port); -#ifdef CONFIG_ARCH_HAVE_MULTICPU flags = spin_lock_irqsave(&priv->lock); -#else - flags = enter_critical_section(); -#endif /* CONFIG_ARCH_HAVE_MULTICPU */ sja1000_putreg(priv, SJA1000_INT_ENA_REG, SJA1000_DEFAULT_INTERRUPTS); @@ -336,17 +325,14 @@ static int sja1000_setup(struct can_dev_s *dev) ret = config->attach( config, (sja1000_handler_t)sja1000_interrupt, (FAR void *)dev); + + spin_unlock_irqrestore(&priv->lock, flags); + if (ret < 0) { canerr("ERROR: Failed to attach to IRQ Handler!\n"); } -#ifdef CONFIG_ARCH_HAVE_MULTICPU - spin_unlock_irqrestore(&priv->lock, flags); -#else - leave_critical_section(flags); -#endif /* CONFIG_ARCH_HAVE_MULTICPU */ - return ret; } @@ -412,11 +398,7 @@ static void sja1000_rxint(struct can_dev_s *dev, bool enable) * so we have to protect this code section. */ -#ifdef CONFIG_ARCH_HAVE_MULTICPU flags = spin_lock_irqsave(&priv->lock); -#else - flags = enter_critical_section(); -#endif /* CONFIG_ARCH_HAVE_MULTICPU */ regval = sja1000_getreg(priv, SJA1000_INT_ENA_REG); if (enable) @@ -429,11 +411,7 @@ static void sja1000_rxint(struct can_dev_s *dev, bool enable) } sja1000_putreg(priv, SJA1000_INT_ENA_REG, regval); -#ifdef CONFIG_ARCH_HAVE_MULTICPU spin_unlock_irqrestore(&priv->lock, flags); -#else - leave_critical_section(flags); -#endif /* CONFIG_ARCH_HAVE_MULTICPU */ } /**************************************************************************** @@ -472,22 +450,14 @@ static void sja1000_txint(struct can_dev_s *dev, bool enable) * have to protect this code section. */ -#ifdef CONFIG_ARCH_HAVE_MULTICPU flags = spin_lock_irqsave(&priv->lock); -#else - flags = enter_critical_section(); -#endif /* CONFIG_ARCH_HAVE_MULTICPU */ /* Disable all TX interrupts */ regval = sja1000_getreg(priv, SJA1000_INT_ENA_REG); regval &= ~(SJA1000_TX_INT_ENA_M); sja1000_putreg(priv, SJA1000_INT_ENA_REG, regval); -#ifdef CONFIG_ARCH_HAVE_MULTICPU spin_unlock_irqrestore(&priv->lock, flags); -#else - leave_critical_section(flags); -#endif /* CONFIG_ARCH_HAVE_MULTICPU */ } cantrace("Exiting.\n"); @@ -638,11 +608,7 @@ static int sja1000_send(struct can_dev_s *dev, struct can_msg_s *msg) frame_info |= (1 << 6); } -#ifdef CONFIG_ARCH_HAVE_MULTICPU flags = spin_lock_irqsave(&priv->lock); -#else - flags = enter_critical_section(); -#endif /* CONFIG_ARCH_HAVE_MULTICPU */ /* Make sure that TX interrupts are enabled BEFORE sending the * message. @@ -711,11 +677,7 @@ static int sja1000_send(struct can_dev_s *dev, struct can_msg_s *msg) sja1000_putreg(priv, SJA1000_CMD_REG, SJA1000_TX_REQ_M); } -#ifdef CONFIG_ARCH_HAVE_MULTICPU spin_unlock_irqrestore(&priv->lock, flags); -#else - leave_critical_section(flags); -#endif /* CONFIG_ARCH_HAVE_MULTICPU */ return ret; } @@ -1122,7 +1084,6 @@ FAR struct can_dev_s *sja1000_instantiate(FAR struct sja1000_dev_s *priv) { struct sja1000_config_s *config = priv->config; FAR struct can_dev_s *dev; - irqstate_t flags; DEBUGASSERT(dev); DEBUGASSERT(priv); @@ -1139,25 +1100,11 @@ FAR struct can_dev_s *sja1000_instantiate(FAR struct sja1000_dev_s *priv) return NULL; } -#ifdef CONFIG_ARCH_HAVE_MULTICPU - flags = spin_lock_irqsave(&priv->lock); -#else - flags = enter_critical_section(); -#endif /* CONFIG_ARCH_HAVE_MULTICPU */ - -#ifdef CONFIG_ARCH_HAVE_MULTICPU - priv->lock = SP_UNLOCKED; -#endif /* CONFIG_ARCH_HAVE_MULTICPU */ + spin_lock_init(&priv->lock); dev->cd_ops = &g_sja1000ops; dev->cd_priv = (FAR void *)priv; -#ifdef CONFIG_ARCH_HAVE_MULTICPU - spin_unlock_irqrestore(&priv->lock, flags); -#else - leave_critical_section(flags); -#endif /* CONFIG_ARCH_HAVE_MULTICPU */ - /* Reset chip */ sja1000_reset(dev); diff --git a/include/nuttx/can/sja1000.h b/include/nuttx/can/sja1000.h index e3f032dd467..0b5551d5b2d 100644 --- a/include/nuttx/can/sja1000.h +++ b/include/nuttx/can/sja1000.h @@ -112,10 +112,7 @@ struct sja1000_dev_s uint8_t filters; /* STD/EXT filter bit allocator. */ uint8_t nalloc; /* Number of allocated filters */ uint32_t base; /* SJA1000 register base address */ - -#ifdef CONFIG_ARCH_HAVE_MULTICPU - spinlock_t lock; /* Device specific lock */ -#endif /* CONFIG_ARCH_HAVE_MULTICPU */ + spinlock_t lock; /* Device specific lock */ /* Register read/write callbacks. These operations all hidden behind * callbacks to isolate the driver from differences in register read/write