Re: [PATCH RFC can-next 3/3] can: add can diag interface

[Filippo Storniolo <fstornio@redhat.com>]

On Wed, Apr 08, 2026 at 06:54:31PM +0200, Oliver Hartkopp wrote:

Hi Oliver,

>>>>+/* Response */
>>>>+struct can_diag_msg {
>>>>+     __u8    cdiag_family;   /* AF_CAN */
>>>>+     __u8    cdiag_state;
>>>>+     __u16   cdiag_protocol;
>>>>+     __u16   cdiag_type;
>>>>+     __u16   pad16;
>>>>+     __u32   cdiag_ino;
>>>>+     canid_t cdiag_tx_id; /* meaningful only for ISO-TP */
>>>>+     canid_t cdiag_rx_id; /* meaningful only for ISO-TP */
>>>
>>>What about the J1939 addressing here which is part of the struct
>>>sockaddr_can too?
>>>
>>
>>Right, I missed this during implementation. I guess we can either
>>extend the structure so that it contains also the addressing
>>information of J1939 or add a new attribyte in the netlink message,
>>similar to what it has been done for the uid.
>>
>>IMHO, I think the second option is better, especially when
>>we dump information that are valid only for specific protocols.
>>Otherwise, we would have a bigger netlink message for every socket,
>>even if they do not need some fields.
>>
>>However, we can evaluate pros and cons and refine it.
>
>In sockaddr_can the ISO-TP and J1939 address information is placed in 
>an union as they can not be valid at the same time and the 
>cdiag_protocol also provides the information what kind of content we 
>would have read, right?
>

Yes, you are right. To be more specific, my idea is to have the
can_diag_msg struct like the following:

/* Response */
struct can_diag_msg {
	__u8	cdiag_family;	/* AF_CAN */
	__u8	cdiag_state;
	__u16	cdiag_protocol;
	__u16	cdiag_type;
	__u16	pad16;
	__u32	cdiag_ino;
	__s32	cdiag_ifindex;
	__u32	cdiag_cookie[2];
};

and then add the following structures in the uapi:

struct can_diag_isotp_id {
	canid_t tx_id;
	canid_t rx_id;
};

struct can_diag_j1939_id {
	__u64	name;
	__u32	pgn;
	__u8	addr;
};

By defining two new attributes for j1939 and isotp,
we can fill the netlink response like this:

struct can_diag_isotp_id isotp_id; 
isotp_id.tx_id = can_addr.can_addr.tp.tx_id;
isotp_id.rx_id = can_addr.can_addr.tp.rx_id;

rep->cdiag_rx_id = can_addr.can_addr.tp.rx_id;
if (sk->sk_protocol == CAN_ISOTP) {
	nla_put(skb, CAN_DIAG_ISOTP_ID, sizeof(can_diag_isotp_id), &isotp_id);
}

and same for j1939.

This is useful because we don't have any un-needed fields
in the can_diag_msg structure, but we can still provide to the
userspace information relevant only for specific protocols.

Cheers,
Filippo