Re: [PATCH net v7 3/4] can: j1939: drop the message when the reassembled skb is gone

[Chaosheng Qu <quchaosheng000406@163.com>]

Hi Oleksij,

No, a stack trace adds nothing here, so netdev_err_once() it is.

The branch is defense in depth: with the EDPO bound in place it should be
unreachable, and if it ever does fire, what I need is the session state, not
the call stack. Whoever reads the log wants to know which offset missed and
what the queue looked like, and a backtrace of j1939_session_completed()
tells them neither. So I would put that state in the message:

	if (se_skb) {
		j1939_sk_recv(session->priv, se_skb);
		consume_skb(se_skb);
	} else {
		netdev_err_once(session->priv->ndev,
				"%s: 0x%p: no skb at dpo %u, total %u, rx %u, queue %u\n",
				__func__, session, session->pkt.dpo,
				session->pkt.total, session->pkt.rx,
				skb_queue_len(&session->skb_queue));
		session->err = EBADMSG;
	}

One correction to what I wrote yesterday, since it is the same line:
session->err = -EBADMSG has the wrong sign. __j1939_sk_errqueue() assigns
session->err straight into serr->ee.ee_errno, which is a __u32 that user
space reads as a positive errno; the TX_ACK case right above it stores
ENOMSG, and j1939_xtp_abort_to_errno() returns positive values as well. A
negative value would reach user space as 4294967222 instead of EBADMSG. It
should be plain EBADMSG.

That also means the two existing negative assignments, -ENETUNREACH in the
tx retry-limit path and -ETIME in the simple-tx timeout path, already deliver
a wrapped value through TX_ABORT. Pre-existing and not this series; I mention
it only because it sits on the line I am touching, and I would rather not
copy the wrong convention into a new one.

Thanks,
Chaosheng