net: m_can: missing mutexes in tx_work_queue and isr-handler

net: m_can: missing mutexes in tx_work_queue and isr-handler

[Prückl Thomas]

Hi Chandrasekar,

I'm using the m_can driver with the externals TCAN4550-Q1 can controller (SPI) on an IMX8MN controller.
After some time the driver stops working.

After reviewing the driver (m_can.c) I noticed that there are not mutex locks preventing concurrent access of 
m_can_isr and m_can_tx_work_queue.

After I added mutex_lock in these functions the driver was working fine.

Is the driver really missing the locking or is it done on another level?

Yours sincerely,
Thomas Prueckl

Re: net: m_can: missing mutexes in tx_work_queue and isr-handler

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 843 bytes --]

On 10.12.2024 09:02:37, Prückl Thomas wrote:
> Hi Chandrasekar,
> 
> I'm using the m_can driver with the externals TCAN4550-Q1 can controller (SPI) on an IMX8MN controller.
> After some time the driver stops working.
> 
> After reviewing the driver (m_can.c) I noticed that there are not mutex locks preventing concurrent access of 
> m_can_isr and m_can_tx_work_queue.
> 
> After I added mutex_lock in these functions the driver was working fine.

Can you show us the patch?

> Is the driver really missing the locking or is it done on another level?

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

AW: net: m_can: missing mutexes in tx_work_queue and isr-handler

[Prückl Thomas]

> Can you show us the patch?

My current patch currently just locks the isr and work queue.
I guess that additional locks are necessary too (e.g. in m_can_close)

Subject: [PATCH] can: m_can: added mutex to lock isr and tx work queue

---
 drivers/net/can/m_can/m_can.c | 17 ++++++++++++++++-
 drivers/net/can/m_can/m_can.h |  1 +
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 2395b1225cc8..34de6be2db76 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1085,7 +1085,7 @@ static int m_can_echo_tx_event(struct net_device *dev)
 	return err;
 }
 
-static irqreturn_t m_can_isr(int irq, void *dev_id)
+static irqreturn_t m_can_isr2(int irq, void *dev_id)
 {
 	struct net_device *dev = (struct net_device *)dev_id;
 	struct m_can_classdev *cdev = netdev_priv(dev);
@@ -1150,6 +1150,17 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t m_can_isr(int irq, void *dev_id)
+{
+	struct net_device *dev = (struct net_device *)dev_id;
+	struct m_can_classdev *cdev = netdev_priv(dev);
+	irqreturn_t ret;
+	mutex_lock(&cdev->lock);
+	ret = m_can_isr2(irq, dev_id);
+	mutex_unlock(&cdev->lock);
+	return ret;
+}
+
 static const struct can_bittiming_const m_can_bittiming_const_30X = {
 	.name = KBUILD_MODNAME,
 	.tseg1_min = 2,		/* Time segment 1 = prop_seg + phase_seg1 */
@@ -1771,7 +1782,9 @@ static void m_can_tx_work_queue(struct work_struct *ws)
 	struct m_can_classdev *cdev = container_of(ws, struct m_can_classdev,
 						   tx_work);
 
+	mutex_lock(&cdev->lock);
 	m_can_tx_handler(cdev);
+	mutex_unlock(&cdev->lock);
 }
 
 static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
@@ -1825,6 +1838,8 @@ static int m_can_open(struct net_device *dev)
 	struct m_can_classdev *cdev = netdev_priv(dev);
 	int err;
 
+	mutex_init(&cdev->lock);
+
 	err = phy_power_on(cdev->transceiver);
 	if (err)
 		return err;
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index 520e14277dff..9d7245108e60 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -91,6 +91,7 @@ struct m_can_classdev {
 
 	int pm_clock_support;
 	int is_peripheral;
+	struct mutex lock;
 
 	struct mram_cfg mcfg[MRAM_CFG_NUM];
 
-- 
2.34.1

Re: AW: net: m_can: missing mutexes in tx_work_queue and isr-handler

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 788 bytes --]

On 10.12.2024 15:04:29, Prückl Thomas wrote:
> > Can you show us the patch?
> 
> My current patch currently just locks the isr and work queue.
> I guess that additional locks are necessary too (e.g. in m_can_close)
> 
> Subject: [PATCH] can: m_can: added mutex to lock isr and tx work queue

Can you make the locking finer, I mean only lock the register access
which is critical? And keep in mind, that you cannot use mutexes from
the non-threaded IRQ handler, i.e. in non-peripheral mode.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]