Re: [PATCH net v5 5/6] can: rockchip_canfd: serialize TX with bus-off and restart

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

Hi,

> [Severity: Low]
> This lock stays held with hard IRQs disabled until the end of
> rkcanfd_start_xmit(). The locked region includes the echo skb handling:
> 
> drivers/net/can/rockchip/rockchip_canfd-tx.c:rkcanfd_start_xmit() {
>     ...
> frame_len = can_skb_get_frame_len(skb);
> err = can_put_echo_skb(skb, ndev, tx_head, frame_len);
> if (!err)
> netdev_sent_queue(priv->ndev, frame_len);
>     ...
> }
> 
> can_put_echo_skb()->can_create_echo_skb() clones the skb and then calls
> consume_skb() on the original. If the clone fails, it calls kfree_skb()
> instead. can_put_echo_skb() also calls kfree_skb() in three cases: IFF_ECHO
> is clear, the protocol check fails, or the echo slot is still occupied.
> 
> Freeing the original skb runs the socket destructor (sock_wfree()). If this
> is the last TX skb of a closed socket, that can continue into __sk_free().
> 
> The comment above dev_kfree_skb_irq() in include/linux/netdevice.h says
> kfree_skb() and consume_skb() must not be called with hardware interrupts
> disabled. Before this patch, ndo_start_xmit() ran with only BH disabled.

You are right. On non-PREEMPT_RT kernels, the new locking places the
original skb's release inside an IRQ-disabled section. I will fix this
in the next revision while preserving the synchronization with bus-off
and close.

> [Severity: Medium]
> This is a pre-existing issue that this patch did not introduce. What
> happens when can_put_echo_skb() fails earlier in this function?

Yes, the existing code still advances tx_head and submits the frame after
echo allocation fails. When tx_tail reaches the empty slot, completion
matching fails and tx_tail cannot advance, which can stall TX.

This predates the series. I have recorded it as a separate follow-up fix
and will keep it outside this restart/close series.

Regards,
Cunhao Lu

pw-bot: cr