1. Patchew
  2. linux
  3. [PATCH 00/21] can: netlink: preparation before introduction of CAN XL step 2/2
  4. View patch

[PATCH 07/21] can: netlink: remove comment in can_validate()

Vincent Mailhol posted 21 patches 4 weeks, 1 day ago
There is a newer version of this series
Re: [PATCH 07/21] can: netlink: remove comment in can_validate()
Posted by Oliver Hartkopp 4 weeks, 1 day ago 
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@kernel.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;
>   
>
Re: [PATCH 07/21] can: netlink: remove comment in can_validate()
Posted by Vincent Mailhol 4 weeks ago 
On 04/09/2025 at 15:51, Oliver Hartkopp wrote:
> 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.

I tend to disagree. While it is true that I added no C-style comment blocks, I
added a ton of error messages which I would argue are documentation.

For example, this code:

	/* If one of the CAN_CTRLMODE_TDC_* flag is set then TDC
	 * must be set and vice-versa
	 */
	if ((tdc_auto || tdc_manual) != !!data_tdc)
		return -EOPNOTSUPP;

was transformed into:

	/* If one of the CAN_CTRLMODE_TDC_* flag is set then TDC
	 * must be set and vice-versa
	 */
	if ((tdc_auto || tdc_manual) && !data_tdc) {
		NL_SET_ERR_MSG(extack, "TDC parameters are missing");
		return -EOPNOTSUPP;
	}
	if (!(tdc_auto || tdc_manual) && data_tdc) {
		NL_SET_ERR_MSG(extack, "TDC mode (auto or manual) is missing");
		return -EOPNOTSUPP;
	}

Which I think is a huge improvement on the documentation. And this has real
value because the user do not have to look at the source code anymore to
understand why an

  ip link set can ...

returned an error.

> 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.

Those comments are still here.

> I would like to motivate you to generally add more comments.
This is a refactoring series. I kept all existing comments except of one and
then added a more comments in the form of error message. I am not adding code,
just moving it around, so I do not really get why I should be adding even more
comments to the existing code.

> 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.

Is this an issue in my code refactoring or an issue with the existing code?

> 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.


Yours sincerely,
Vincent Mailhol
Re: [PATCH 07/21] can: netlink: remove comment in can_validate()
Posted by Oliver Hartkopp 3 weeks, 6 days ago 

On 04.09.25 11:48, Vincent Mailhol wrote:
> On 04/09/2025 at 15:51, Oliver Hartkopp wrote:
>> 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.
> 
> I tend to disagree. While it is true that I added no C-style comment blocks, I
> added a ton of error messages which I would argue are documentation.
> 
> For example, this code:
> 
> 	/* If one of the CAN_CTRLMODE_TDC_* flag is set then TDC
> 	 * must be set and vice-versa
> 	 */
> 	if ((tdc_auto || tdc_manual) != !!data_tdc)
> 		return -EOPNOTSUPP;
> 
> was transformed into:
> 
> 	/* If one of the CAN_CTRLMODE_TDC_* flag is set then TDC
> 	 * must be set and vice-versa
> 	 */
> 	if ((tdc_auto || tdc_manual) && !data_tdc) {
> 		NL_SET_ERR_MSG(extack, "TDC parameters are missing");
> 		return -EOPNOTSUPP;
> 	}
> 	if (!(tdc_auto || tdc_manual) && data_tdc) {
> 		NL_SET_ERR_MSG(extack, "TDC mode (auto or manual) is missing");
> 		return -EOPNOTSUPP;
> 	}
> 
> Which I think is a huge improvement on the documentation. And this has real
> value because the user do not have to look at the source code anymore to
> understand why an
> 
>    ip link set can ...
> 
> returned an error.
> 
>> 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.
> 
> Those comments are still here.
> 
>> I would like to motivate you to generally add more comments.
> This is a refactoring series. I kept all existing comments except of one and
> then added a more comments in the form of error message. I am not adding code,
> just moving it around, so I do not really get why I should be adding even more
> comments to the existing code.
> 
>> 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.
> 
> Is this an issue in my code refactoring or an issue with the existing code?
> 
>> 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.
> 

No need to defend yourself with specific references or even feel 
personally attacked.

My overall feeling is that you spend an excellent effort in commit 
messages but this information is then omitted in code comments.

As I've already written "I would like to motivate you to generally add 
more comments.". And this can also happen when refactoring things where 
new functions are created which reduces the context to the original code 
section.

Best regards,
Oliver
Re: [PATCH 07/21] can: netlink: remove comment in can_validate()
Posted by Vincent Mailhol 3 weeks, 6 days ago 
On 05/09/2025 at 19:55, Oliver Hartkopp wrote:

(..)

> No need to defend yourself with specific references or even feel personally
> attacked.

Thanks. I was not sure how to read your previous message.

> My overall feeling is that you spend an excellent effort in commit messages but
> this information is then omitted in code comments.
> 
> As I've already written "I would like to motivate you to generally add more
> comments.". And this can also happen when refactoring things where new functions
> are created which reduces the context to the original code section.

My current mind set is that I want to do more ironing on the upcoming XL
patches. Because I do the documentation last once everything is working well,
this is still on my TODO list.

And when this is done, there is also

  Documentation/networking/can.rst

which need an update. At the moment, I am rather happy by just keeping the
existing documentation in this refactor series and want to put the extra effort
on the new stuff. Thinking of the upcoming work and of my current bandwidth, I
am really not in the mood into injecting more time in the refactor.

That said, on a second thought, I finally decided to keep the comment which I
previously wanted to remove. I will just move it from can_validate() to
can_validate_databittiming() in patch 06/21 "can: netlink: add
can_validate_databittiming()".


Yours sincerely,
Vincent Mailhol

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.