Possible NULL sample_point in the new way of computing bit-timings

Possible NULL sample_point in the new way of computing bit-timings

[Stephane Grosjean]

Hi all,

With the new way of computing bit-timings introduced in linux-can-next, 
when CONFIG_CAN_CALC_BITTIMING is defined, I think there's a chance to 
set 0 in "bt->sample_point" of a can device. This is a different 
behavior than with the previous versions.

IMHO, there are cases where "can_update_sample_point()" might return 0 
("best_sample_point" is initialized to 0). There is no real damage in 
the main loop of "can_calc_bittiming()": its returned value is ignored 
(and only *sample_point_error matters). But it is not, outside this 
loop, when setting the (real) value of the sample_point. So I think that 
the real value of the sample_point must be computed using the final 
values of tseg and tseg2, something like:

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 7188137..de422b3 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -200,8 +200,8 @@ static int can_calc_bittiming(struct net_device 
*dev, struct can_bittiming *bt,
         }

         /* real sample point */
-       bt->sample_point = can_update_sample_point(btc, 
sample_point_nominal, best_tseg,
-                                         &tseg1, &tseg2, NULL);
+       bt->sample_point = 1000 * (best_tseg + CAN_CALC_SYNC_SEG - tseg2) /
+                                       (best_tseg + CAN_CALC_SYNC_SEG);

         v64 = (u64)best_brp * 1000 * 1000 * 1000;
         do_div(v64, priv->clock.freq);

The best would be to introduce a function that implements the formula...

Stéphane

--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt - HRB 9183 
Geschaeftsfuehrung: A.Gach, U.Wilhelm
--

Re: Possible NULL sample_point in the new way of computing bit-timings

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2125 bytes --]

On 08/09/2016 04:17 PM, Stephane Grosjean wrote:
> Hi all,
> 
> With the new way of computing bit-timings introduced in linux-can-next, 
> when CONFIG_CAN_CALC_BITTIMING is defined, I think there's a chance to 
> set 0 in "bt->sample_point" of a can device. This is a different 
> behavior than with the previous versions.
> 
> IMHO, there are cases where "can_update_sample_point()" might return 0 
> ("best_sample_point" is initialized to 0). There is no real damage in 
> the main loop of "can_calc_bittiming()": its returned value is ignored 
> (and only *sample_point_error matters). But it is not, outside this 
> loop, when setting the (real) value of the sample_point. So I think that 
> the real value of the sample_point must be computed using the final 
> values of tseg and tseg2, something like:
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 7188137..de422b3 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -200,8 +200,8 @@ static int can_calc_bittiming(struct net_device 
> *dev, struct can_bittiming *bt,
>          }
> 
>          /* real sample point */
> -       bt->sample_point = can_update_sample_point(btc, sample_point_nominal, best_tseg,
> -                                         &tseg1, &tseg2, NULL);
> +       bt->sample_point = 1000 * (best_tseg + CAN_CALC_SYNC_SEG - tseg2) /
> +                                       (best_tseg + CAN_CALC_SYNC_SEG);

You're using tseg2 as an input, where before it was used as an output
value. As the sample point has already been calculated in the loop, why
should it not be possible to calculate it again?

> 
>          v64 = (u64)best_brp * 1000 * 1000 * 1000;
>          do_div(v64, priv->clock.freq);
> 
> The best would be to introduce a function that implements the formula...

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]