Re: can: j1939: unregister_netdevice: waiting for vcan0 to become free.

[Oleksij Rempel <o.rempel@pengutronix.de>]

Hi,

On Tue, Jan 13, 2026 at 12:46:30PM +0900, Tetsuo Handa wrote:
> Currently, the (session->last_cmd != 0) path in j1939_xtp_rx_rts_session_active() is
> preventing the (session->state == J1939_SESSION_WAITING_ABORT) path in j1939_tp_rxtimer()
>  from being called. This results in two j1939_priv refcounts leak (which in turn results in
> one net_device refcount leak) due to j1939_session_deactivate_activate_next() being not called.
> 
> This problem goes away if I do either
> 
> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -1689,16 +1692,18 @@ static int j1939_xtp_rx_rts_session_active(struct j1939_session *session,
> 
>         if (session->last_cmd != 0) {
>                 /* we received a second rts on the same connection */
> -               netdev_alert(priv->ndev, "%s: 0x%p: connection exists (%02x %02x). last cmd: %x\n",
> +               netdev_alert(priv->ndev, "%s (modified): 0x%p: connection exists (%02x %02x). last cmd: %x\n",
>                              __func__, session, skcb->addr.sa, skcb->addr.da,
>                              session->last_cmd);
> 
> +               /*
>                 j1939_session_timers_cancel(session);
>                 j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
>                 if (session->transmission)
>                         j1939_session_deactivate_activate_next(session);
> 
>                 return -EBUSY;
> +               */
>         }
> 
>         if (session->skcb.addr.sa != skcb->addr.sa ||
> 
> or
> 
> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -1697,6 +1700,11 @@ static int j1939_xtp_rx_rts_session_active(struct j1939_session *session,
>                 j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
>                 if (session->transmission)
>                         j1939_session_deactivate_activate_next(session);
> +               else if (session->state == J1939_SESSION_WAITING_ABORT) {

This way looks better for me. May be add a comment like this:
            /* Force deactivation for the receiver.
             * If we rely on the timer starting in j1939_session_cancel, 
             * a second RTS call here will cancel that timer and fail 
             * to restart it because the state is already WAITING_ABORT.
             */

> +                       netdev_alert(priv->ndev, "%s (modified): 0x%p: abort rx timeout. Force session deactivation\n",
> +                                    __func__, session);
> +                       j1939_session_deactivate_activate_next(session);
> +               }
> 
>                 return -EBUSY;
>         }
> 
> . But what is the correct approach?

The second one. Thank you for your work.

Best Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |