Re: [PATCH net v6 3/6] can: rockchip_canfd: cancel bus-off restart before closing the device

["Cunhao Lu" <1579567540@qq.com>]

Hi,

> [Severity: Medium]
> Does using priv->irq_suspended as a boolean gate flag between
> rkcanfd_set_mode() and rkcanfd_irq() leave a race window during controller
> restart and shared IRQ handling?

The flag works together with synchronize_irq() to keep the IRQ handler
from accessing the device during reinitialization. synchronize_irq()
waits for handlers that have already passed the flag check.

> Setting priv->irq_suspended on entry to rkcanfd_set_mode() and clearing it on
> exit acts as an ad-hoc lock that is invisible to lockdep and does not
> serialize against other concurrent readers or writers of shared state (such as
> rkcanfd_get_berr_counter()).

The BEC cache race is a pre-existing issue. I will leave it out of this
series for now. When I have time, I will send a separate patch to fix it.

> In addition, rkcanfd_chip_start() already puts the controller into
> RKCANFD_REG_MODE_WORK_MODE and sets priv->can.state = CAN_STATE_ERROR_ACTIVE
> before rkcanfd_set_mode() clears priv->irq_suspended:
>
> can_restart()
>   rkcanfd_set_mode(ndev, CAN_MODE_START)
>     rkcanfd_chip_start(priv)
>       rkcanfd_chip_set_work_mode(priv);
>       priv->can.state = CAN_STATE_ERROR_ACTIVE;
>     smp_store_release(&priv->irq_suspended, false);
>     rkcanfd_chip_interrupts_enable(priv);
>     netif_wake_queue(ndev);
>
> Because rkcanfd_irq() reads the raw unmasked interrupt status from
> RKCANFD_REG_INT and masks it in software with ~priv->reg_int_mask_default
> rather than consulting RKCANFD_REG_INT_MASK, a shared IRQ firing on the
> IRQF_SHARED line after smp_store_release(&priv->irq_suspended, false) (or an
> interrupt firing right as rkcanfd_chip_interrupts_enable() unmasks hardware
> interrupts) can enter rkcanfd_irq() and handle a new bus-off condition before
> rkcanfd_set_mode() finishes:
>
> rkcanfd_irq()
>   rkcanfd_handle_state_error_int()
>     rkcanfd_chip_stop(priv, CAN_STATE_BUS_OFF)
>     can_bus_off(ndev)
>
> When rkcanfd_set_mode() then executes rkcanfd_chip_interrupts_enable(priv) and
> netif_wake_queue(ndev), wouldn't it unmask interrupts on the stopped
> controller and wake the transmit queue while priv->can.state is
> CAN_STATE_BUS_OFF?

This is already handled by patch 5/6. rkcanfd_chip_start_finish() holds
tx_lock while clearing irq_suspended, enabling interrupts and waking
the queue. The bus-off path takes the same lock in rkcanfd_tx_disable()
before publishing BUS_OFF and stopping the controller. A new bus-off
therefore takes effect after startup finishes.

> Similarly, because rkcanfd_irq() only checks priv->irq_suspended (which is
> only set during rkcanfd_set_mode()) and masks RKCANFD_REG_INT with
> ~priv->reg_int_mask_default, shared IRQs arriving during rkcanfd_open()
> between request_irq() and rkcanfd_chip_interrupts_enable(), or while in
> CAN_STATE_BUS_OFF after rkcanfd_chip_stop() masks RKCANFD_REG_INT_MASK, can
> still evaluate pending bits in RKCANFD_REG_INT. Would a proper spinlock (or
> honoring the active mask state under a lock) avoid this race?

Patch 5/6 also sets irq_suspended in rkcanfd_open() before request_irq().
It is cleared in rkcanfd_chip_start_finish() under tx_lock.

The bus-off stop path already resets the hardware and clears WORK_MODE.
I have not observed a failure caused by pending bits remaining after
reset, so I do not think this should be considered a bug.

Regards,
Cunhao Lu