linux-kernel - Re: [PATCH v2] can: m_can: use us_to_ktime() in m_can_set

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 for Android: free password hash cracker in your pocket

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

Message-ID: <6b6ba240-e13a-4e99-ae76-500a7b530238@kernel.org>
Date: Tue, 26 Aug 2025 17:38:17 +0900
From: Vincent Mailhol <mailhol@...nel.org>
To: Markus Elfring <Markus.Elfring@....de>, Xichao Zhao
 <zhao.xichao@...o.com>, linux-can@...r.kernel.org,
 Marc Kleine-Budde <mkl@...gutronix.de>
Cc: LKML <linux-kernel@...r.kernel.org>,
 Chandrasekar Ramakrishnan <rcsekar@...sung.com>
Subject: Re: [PATCH v2] can: m_can: use us_to_ktime() in m_can_set_coalesce()

On 26/08/2025 at 14:43, Markus Elfring wrote:
>> Replace the if-else statement with a ternary operator to
>> set cdev->irq_timer_wait. Use us_to_ktime() instead of
>> ns_to_ktime() with NSEC_PER_USEC multiplication. Simplify
> …
> 
> You should occasionally use more than 57 characters in text lines
> of such a change description.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.17-rc3#n638
> 
> 
> Will an enumeration become helpful here?
> 
> 
> …> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -2211,13 +2211,9 @@ static int m_can_set_coalesce(struct net_device *dev,
> …> +	cdev->irq_timer_wait = us_to_ktime(cdev->rx_coalesce_usecs_irq ?
>> +					   cdev->rx_coalesce_usecs_irq :
>> +					   cdev->tx_coalesce_usecs_irq);
> …
> 
> I am curious how coding style preferences will evolve further also for
> the usage of the conditional operator at such a place.

The preferred style in the kernel is actually to *not* use the conditional
operator in such case and to use an explicit if/else. I appreciate that the
conditional operator is more succinct, but squeezing the code is not a goal. The
priority is readability, and the if/else does a better job at this.

And I this is not my personnal opinion. For example, see this message from Greg:

  https://lore.kernel.org/all/20250311150130.7a875e63@bahia/

TLDR; the v1 was better than the v2. Speaking of the format, the only nitpick I
might have is that after your change, the code fits in one line without
exceeding the 80th column:

	if (cdev->rx_coalesce_usecs_irq)
		cdev->irq_timer_wait = us_to_ktime(cdev->rx_coalesce_usecs_irq);
 	else
		cdev->irq_timer_wait = us_to_ktime(cdev->tx_coalesce_usecs_irq);


Yours sincerely,
Vincent Mailhol