netdev - Re: ctucanfd: possible coding error in ctucan_set_secondary_sample

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251222182211.26893b94@pumpkin>
Date: Mon, 22 Dec 2025 18:22:11 +0000
From: David Laight <david.laight.linux@...il.com>
To: Marc Kleine-Budde <mkl@...gutronix.de>
Cc: Ondrej Ille <ondrej.ille@...il.com>, Andrea Daoud
 <andreadaoud6@...il.com>, Pavel Pisa <pisa@....felk.cvut.cz>,
 linux-can@...r.kernel.org, Wolfgang Grandegger <wg@...ndegger.com>, "David
 S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub
 Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Christophe
 JAILLET <christophe.jaillet@...adoo.fr>, netdev@...r.kernel.org
Subject: Re: ctucanfd: possible coding error in
 ctucan_set_secondary_sample_point causing SSP not enabled

On Mon, 22 Dec 2025 17:20:49 +0100
Marc Kleine-Budde <mkl@...gutronix.de> wrote:

> On 22.12.2025 16:51:07, Ondrej Ille wrote:
> > yes, your thinking is correct, there is a bug there.
> >
> > This was pointed to by another user right in the CTU CAN FD repository
> > where the Linux driver also lives:
> > https://github.com/Blebowski/CTU-CAN-FD/pull/2
> >
> > It is as you say, it should be:
> >
> > -- ssp_cfg |= FIELD_PREP(REG_TRV_DELAY_SSP_SRC, 0x1);
> > ++ ssp_cfg |= FIELD_PREP(REG_TRV_DELAY_SSP_SRC, 0x0);  
> 
> This statement has no effect, as 'ssp_cfg |= 0x0' is still 'ssp_cfg'.

The compiler will optimise it away - so it is the same as a comment.

> IMHO it's better to add a comment that says, why you don't set
> REG_TRV_DELAY_SSP_SRC. Another option is to add create a define that
> replaces 0x1 and 0x0 for REG_TRV_DELAY_SSP_SRC with a speaking name.

Looking at the header, the 'field' is two bits wide.
So what you really want the code to look like is:
	ssp_cfg |= REG_TRV_DELAY_SSP_SRC(n);
There is nothing to stop working - it just needs the right defines.
Sort of FIELD_PREP(GENMASK(25, 24), n) - but you can do a lot better than that.
The inverse is also possible:
	val = GET_VAL(REG_TRV_DELAY_SSP_SRC, reg_val);
#define GET_VAL(x, reg) ((reg & x(-1))/x(1))

    David

> 
> regards,
> Marc
>