Possible race condition of the rockchip_canfd driver

Possible race condition of the rockchip_canfd driver

[Andrea Daoud]

Hi Marc,

I'm using the rockchip_canfd driver on an RK3568. When under high bus
load, I get
the following logs [1] in rkcanfd_tx_tail_is_eff, and the CAN bus is unable to
communicate properly under this condition. The exact cause is currently not
entirely clear, and it's not reliably reproducible.

In the logs we can spot some strange points:

1. Line 24, tx_head == tx_tail. This should have been rejected by the if
(!rkcanfd_get_tx_pending) clause.

2. Line 26, the last bit of priv->tx_tail (0x0185dbb3) is 1. This means that the
tx_tail should be 1, because rkcanfd_get_tx_tail is essentially mod the
priv->tx_tail by two. But the printed tx_tail is 0.

I believe these problems could mean that the code is suffering from some race
condition. It seems that, in the whole IRQ processing chain of the driver,
there's no lock protection. Maybe some IRQ happens within the execution of
rkcanfd_tx_tail_is_eff, and touches the state of the tx_head and tx_tail?

Could you please have a look at the code, and check if some locking is needed?

[1]: https://pastebin.com/R7uuEKEz

Re: Possible race condition of the rockchip_canfd driver

[Marc Kleine-Budde]

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

Hello,

On 18.09.2025 20:58:33, Andrea Daoud wrote:
> I'm using the rockchip_canfd driver on an RK3568. When under high bus
> load, I get
> the following logs [1] in rkcanfd_tx_tail_is_eff, and the CAN bus is unable to
> communicate properly under this condition. The exact cause is currently not
> entirely clear, and it's not reliably reproducible.

Our customer is using a v3 silicon revision of the chip, which doesn't
this workaround.

> In the logs we can spot some strange points:
> 
> 1. Line 24, tx_head == tx_tail. This should have been rejected by the if
> (!rkcanfd_get_tx_pending) clause.
> 
> 2. Line 26, the last bit of priv->tx_tail (0x0185dbb3) is 1. This means that the
> tx_tail should be 1, because rkcanfd_get_tx_tail is essentially mod the
> priv->tx_tail by two. But the printed tx_tail is 0.
> 
> I believe these problems could mean that the code is suffering from some race
> condition. It seems that, in the whole IRQ processing chain of the driver,
> there's no lock protection. Maybe some IRQ happens within the execution of
> rkcanfd_tx_tail_is_eff, and touches the state of the tx_head and tx_tail?
> 
> Could you please have a look at the code, and check if some locking is needed?

My time for community support is currently a bit limited. I think this
has to wait a bit, apologies :/

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 --]

Re: Possible race condition of the rockchip_canfd driver

[Andrea Daoud]

On Sat, Sep 20, 2025 at 3:04 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> Hello,

Thanks for your reply!

>
> On 18.09.2025 20:58:33, Andrea Daoud wrote:
> > I'm using the rockchip_canfd driver on an RK3568. When under high bus
> > load, I get
> > the following logs [1] in rkcanfd_tx_tail_is_eff, and the CAN bus is unable to
> > communicate properly under this condition. The exact cause is currently not
> > entirely clear, and it's not reliably reproducible.
>
> Our customer is using a v3 silicon revision of the chip, which doesn't
> this workaround.

Could you please let me know how to check whether my RK3568 is v2 or v3?

>
> > In the logs we can spot some strange points:
> >
> > 1. Line 24, tx_head == tx_tail. This should have been rejected by the if
> > (!rkcanfd_get_tx_pending) clause.
> >
> > 2. Line 26, the last bit of priv->tx_tail (0x0185dbb3) is 1. This means that the
> > tx_tail should be 1, because rkcanfd_get_tx_tail is essentially mod the
> > priv->tx_tail by two. But the printed tx_tail is 0.
> >
> > I believe these problems could mean that the code is suffering from some race
> > condition. It seems that, in the whole IRQ processing chain of the driver,
> > there's no lock protection. Maybe some IRQ happens within the execution of
> > rkcanfd_tx_tail_is_eff, and touches the state of the tx_head and tx_tail?
> >
> > Could you please have a look at the code, and check if some locking is needed?
>
> My time for community support is currently a bit limited. I think this
> has to wait a bit, apologies :/

No worries, I will debug myself, and hopefully send a PR if I found
something out.

>
> 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   |

Regards,
Andrea

Re: Possible race condition of the rockchip_canfd driver

[Marc Kleine-Budde]

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

On 20.09.2025 18:08:03, Andrea Daoud wrote:
> > On 18.09.2025 20:58:33, Andrea Daoud wrote:
> > > I'm using the rockchip_canfd driver on an RK3568. When under high bus
> > > load, I get
> > > the following logs [1] in rkcanfd_tx_tail_is_eff, and the CAN bus is unable to
> > > communicate properly under this condition. The exact cause is currently not
> > > entirely clear, and it's not reliably reproducible.
> >
> > Our customer is using a v3 silicon revision of the chip, which doesn't
> > this workaround.
> 
> Could you please let me know how to check whether my RK3568 is v2 or v3?

Alexander Shiyan (Cc'ed) reads the information from an nvmem cell:

| https://github.com/MacroGroup/barebox/blob/macro/arch/arm/boards/diasom-rk3568/board.c#L239-L257

The idea is to fixup the device tree in the bootloader depending on the
SoC revision, so that the CAN driver uses only the needed workarounds.

> > > In the logs we can spot some strange points:
> > >
> > > 1. Line 24, tx_head == tx_tail. This should have been rejected by the if
> > > (!rkcanfd_get_tx_pending) clause.
> > >
> > > 2. Line 26, the last bit of priv->tx_tail (0x0185dbb3) is 1. This means that the
> > > tx_tail should be 1, because rkcanfd_get_tx_tail is essentially mod the
> > > priv->tx_tail by two. But the printed tx_tail is 0.
> > >
> > > I believe these problems could mean that the code is suffering from some race
> > > condition. It seems that, in the whole IRQ processing chain of the driver,
> > > there's no lock protection. Maybe some IRQ happens within the execution of
> > > rkcanfd_tx_tail_is_eff, and touches the state of the tx_head and tx_tail?
> > >
> > > Could you please have a look at the code, and check if some locking is needed?
> >
> > My time for community support is currently a bit limited. I think this
> > has to wait a bit, apologies :/
> 
> No worries, I will debug myself, and hopefully send a PR if I found
> something out.

Great, I have a both a v2 and a v3 SoC here to test.

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 --]

Re: Possible race condition of the rockchip_canfd driver

[Andrea Daoud]

Hi Marc,

On Mon, Sep 22, 2025 at 4:50 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 20.09.2025 18:08:03, Andrea Daoud wrote:
> > > On 18.09.2025 20:58:33, Andrea Daoud wrote:
> > > > I'm using the rockchip_canfd driver on an RK3568. When under high bus
> > > > load, I get
> > > > the following logs [1] in rkcanfd_tx_tail_is_eff, and the CAN bus is unable to
> > > > communicate properly under this condition. The exact cause is currently not
> > > > entirely clear, and it's not reliably reproducible.
> > >
> > > Our customer is using a v3 silicon revision of the chip, which doesn't
> > > this workaround.
> >
> > Could you please let me know how to check whether my RK3568 is v2 or v3?
>
> Alexander Shiyan (Cc'ed) reads the information from an nvmem cell:
>
> | https://github.com/MacroGroup/barebox/blob/macro/arch/arm/boards/diasom-rk3568/board.c#L239-L257
>
> The idea is to fixup the device tree in the bootloader depending on the
> SoC revision, so that the CAN driver uses only the needed workarounds.
>

Thanks, it is not easy to correlate this because I am currently not using
barebox. I'll try this later.

> > > > In the logs we can spot some strange points:
> > > >
> > > > 1. Line 24, tx_head == tx_tail. This should have been rejected by the if
> > > > (!rkcanfd_get_tx_pending) clause.
> > > >
> > > > 2. Line 26, the last bit of priv->tx_tail (0x0185dbb3) is 1. This means that the
> > > > tx_tail should be 1, because rkcanfd_get_tx_tail is essentially mod the
> > > > priv->tx_tail by two. But the printed tx_tail is 0.
> > > >
> > > > I believe these problems could mean that the code is suffering from some race
> > > > condition. It seems that, in the whole IRQ processing chain of the driver,
> > > > there's no lock protection. Maybe some IRQ happens within the execution of
> > > > rkcanfd_tx_tail_is_eff, and touches the state of the tx_head and tx_tail?
> > > >
> > > > Could you please have a look at the code, and check if some locking is needed?
> > >
> > > My time for community support is currently a bit limited. I think this
> > > has to wait a bit, apologies :/
> >
> > No worries, I will debug myself, and hopefully send a PR if I found
> > something out.
>
> Great, I have a both a v2 and a v3 SoC here to test.

It turns out there are two issues:

1. The race condition (between TX interrupt and TX queue netif xmit) is indeed
an issue. Fixed by adding a spinlock around TX logic, and no warning
occurs after
adding the lock.

2. The CLK_CAN0 was clocked by GPLL, which makes it 148.5MHz. This frequency
will lead to various errors quickly when dealing with EFF IDs. Fixed
by constraining it
to use CPLL and 250MHz freq.

Regarding problem 1, I will send a patch.

Regarding problem 2, I suggest adding some extra sanity checks to
guard against this.

>
> 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   |


Regards,
Andrea

Re: Possible race condition of the rockchip_canfd driver

[Alexander Shiyan]

Hello.

> > > Could you please let me know how to check whether my RK3568 is v2 or v3?
> >
> > Alexander Shiyan (Cc'ed) reads the information from an nvmem cell:
> >
> > | https://github.com/MacroGroup/barebox/blob/macro/arch/arm/boards/diasom-rk3568/board.c#L239-L257
> >
> > The idea is to fixup the device tree in the bootloader depending on the
> > SoC revision, so that the CAN driver uses only the needed workarounds.
> >
>
> Thanks, it is not easy to correlate this because I am currently not using
> barebox. I'll try this later.

I think this can be done from the userspace.
Build the driver as a module, get the SoC version, then modprobe it
with an alias for the desired version.

Re: Possible race condition of the rockchip_canfd driver

[Marc Kleine-Budde]

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

On 28.09.2025 00:26:59, Andrea Daoud wrote:
> > Alexander Shiyan (Cc'ed) reads the information from an nvmem cell:
> >
> > | https://github.com/MacroGroup/barebox/blob/macro/arch/arm/boards/diasom-rk3568/board.c#L239-L257
> >
> > The idea is to fixup the device tree in the bootloader depending on the
> > SoC revision, so that the CAN driver uses only the needed workarounds.
> 
> Thanks, it is not easy to correlate this because I am currently not using
> barebox. I'll try this later.

The most important information in this code is where the SoC revision is
located: there is a separate nvmem cell for this. You can read out the
information once manually and change your device tree accordingly.

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 --]