Re: [PATCH net] can: rockchip_canfd: stop bus-off restart before closing the device

[Cunhao Lu]

Hi,

I checked all four findings against the current branch.

> - [Medium] The patch now has rkcanfd_set_mode() return -ENETDOWN when it
>   sees priv->closing.
> - [Medium] rkcanfd_stop() publishes priv->closing = true (line 776)
>   before it masks the controller interrupts with…

The two issues involving closing were addressed in v2.

> [Severity: High]
> This isn't a bug introduced by this patch, but this patch hardens the
> restart path, so is it safe to reinitialize the timestamp delayed_work
> here?
> ...
> Would it work to call INIT_DELAYED_WORK() once at probe time, and to call
> cancel_delayed_work_sync(&priv->timestamp) in the process-context restart
> path before the timecounter is reinitialized?

Agreed. Excluding the IRQ handler does not stop the timestamp worker.
I will initialize the delayed_work once at probe time and synchronously
cancel it in the process-context restart path, after excluding the IRQ
handler and before rkcanfd_chip_start() reinitializes the timestamp
state. The bus-off IRQ path will keep its non-synchronous cancellation.


> [Severity: High]
> This is a pre-existing issue, but this hunk touches the request_irq()
> path in rkcanfd_open(). What happens to NAPI if request_irq() fails?
> ...
> Should can_rx_offload_disable(&priv->offload) be added to this error path?

Yes. I will add it to the request_irq() failure path, before stopping the
controller, to balance can_rx_offload_enable(). I will address this and
the timestamp work issue in separate patches.

I will fix these bugs in v3.

Regards,
Cunhao Lu