linux-kernel - Re: [PATCH 07/21] can: netlink: remove comment in can

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b1bf6cc5-f972-4163-8619-e04b887e2d32@hartkopp.net>
Date: Thu, 4 Sep 2025 08:51:02 +0200
From: Oliver Hartkopp <socketcan@...tkopp.net>
To: Vincent Mailhol <mailhol@...nel.org>,
 Marc Kleine-Budde <mkl@...gutronix.de>
Cc: Stéphane Grosjean <stephane.grosjean@...-networks.com>,
 Robert Nawrath <mbro1689@...il.com>, Minh Le <minh.le.aj@...esas.com>,
 Duy Nguyen <duy.nguyen.rh@...esas.com>, linux-can@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 07/21] can: netlink: remove comment in can_validate()

Hi Vincent,

On 03.09.25 10:50, Vincent Mailhol wrote:
> The comment in can_validate() is just paraphrasing the code. When
> adding CAN XL, updating this comment would add some overhead work for
> no clear benefit.

I generally see that the code introduced by yourself has nearly no comments.

E.g. if you look at the [PATCH 12/21] can: netlink: add 
can_ctrlmode_changelink() the comments introduced by myself document the 
different steps as we had problems with the complexity there and it was 
hard to review either.

I would like to motivate you to generally add more comments.

When people (like me) look into that code that they haven't written 
themselves and there is not even a hint of "what's the idea of what we 
are doing here" then the code is hard to follow and to review.

We definitely don't need a full blown documentation on top of each 
function. But I like this comment you want to remove here and I would 
like to have more of it, so that people get an impression what they will 
see in the following code.

Best regards,
Oliver

> 
> Now that the function has been refactored and split into smaller
> pieces, let the code speak for itself. Remove the comment.
> 
> Signed-off-by: Vincent Mailhol <mailhol@...nel.org>
> ---
>   drivers/net/can/dev/netlink.c | 7 -------
>   1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> index f7b12057bc9c6c286aa0c4341d565a497254296d..6ea629331d20483c5e70567eb1be226a3b09882c 100644
> --- a/drivers/net/can/dev/netlink.c
> +++ b/drivers/net/can/dev/netlink.c
> @@ -143,13 +143,6 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
>   	u32 flags = 0;
>   	int err;
>   
> -	/* Make sure that valid CAN FD configurations always consist of
> -	 * - nominal/arbitration bittiming
> -	 * - data bittiming
> -	 * - control mode with CAN_CTRLMODE_FD set
> -	 * - TDC parameters are coherent (details below)
> -	 */
> -
>   	if (!data)
>   		return 0;
>   
>