| 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
| ||
|
Date: Mon, 22 Nov 2021 11:22:08 +0900
From: Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To: Oliver Hartkopp <socketcan@...tkopp.net>
Cc: Marc Kleine-Budde <mkl@...gutronix.de>, linux-can@...r.kernel.org,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
Jimmy Assarsson <extja@...ser.com>
Subject: Re: [PATCH] can: bittiming: replace CAN units with the SI metric
Le lun. 22 nov. 2021 à 03:27, Oliver Hartkopp <socketcan@...tkopp.net> a écrit :
>
>
>
> On 19.11.21 17:18, Vincent Mailhol wrote:
> > In [1], we introduced a set of units in linux/can/bittiming.h. Since
> > then, generic SI prefix were added to linux/units.h in [2]. Those new
> > prefix can perfectly replace the CAN specific units.
> >
> > This patch replaces all occurrences of the CAN units with their
> > corresponding prefix according to below table.
> >
> > CAN units SI metric prefix
> > -------------------------------
> > CAN_KBPS KILO
> > CAN_MBPS MEGA
> > CAM_MHZ MEGA
> >
> > The macro declarations are then removed from linux/can/bittiming.h
> >
> > [1] commit 1d7750760b70 ("can: bittiming: add CAN_KBPS, CAN_MBPS and
> > CAN_MHZ macros")
> >
> > [2] commit 26471d4a6cf8 ("units: Add SI metric prefix definitions")
> >
> > Suggested-by: Jimmy Assarsson <extja@...ser.com>
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@...adoo.fr>
> > ---
> > drivers/net/can/dev/bittiming.c | 5 +++--
> > drivers/net/can/usb/etas_es58x/es581_4.c | 5 +++--
> > drivers/net/can/usb/etas_es58x/es58x_fd.c | 5 +++--
> > include/linux/can/bittiming.h | 7 -------
> > 4 files changed, 9 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
> > index 0509625c3082..a5c9f973802a 100644
> > --- a/drivers/net/can/dev/bittiming.c
> > +++ b/drivers/net/can/dev/bittiming.c
> > @@ -4,6 +4,7 @@
> > * Copyright (C) 2008-2009 Wolfgang Grandegger <wg@...ndegger.com>
> > */
> >
> > +#include <linux/units.h>
> > #include <linux/can/dev.h>
> >
> > #ifdef CONFIG_CAN_CALC_BITTIMING
> > @@ -81,9 +82,9 @@ int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
> > if (bt->sample_point) {
> > sample_point_nominal = bt->sample_point;
> > } else {
> > - if (bt->bitrate > 800 * CAN_KBPS)
> > + if (bt->bitrate > 800 * KILO)
> > sample_point_nominal = 750;
> > - else if (bt->bitrate > 500 * CAN_KBPS)
> > + else if (bt->bitrate > 500 * KILO)
> > sample_point_nominal = 800;
> > else
> > sample_point_nominal = 875;
> > diff --git a/drivers/net/can/usb/etas_es58x/es581_4.c b/drivers/net/can/usb/etas_es58x/es581_4.c
> > index 14e360c9f2c9..ed340141c712 100644
> > --- a/drivers/net/can/usb/etas_es58x/es581_4.c
> > +++ b/drivers/net/can/usb/etas_es58x/es581_4.c
> > @@ -10,6 +10,7 @@
> > */
> >
> > #include <linux/kernel.h>
> > +#include <linux/units.h>
> > #include <asm/unaligned.h>
> >
> > #include "es58x_core.h"
> > @@ -469,8 +470,8 @@ const struct es58x_parameters es581_4_param = {
> > .bittiming_const = &es581_4_bittiming_const,
> > .data_bittiming_const = NULL,
> > .tdc_const = NULL,
> > - .bitrate_max = 1 * CAN_MBPS,
> > - .clock = {.freq = 50 * CAN_MHZ},
> > + .bitrate_max = 1 * MEGA,
> > + .clock = {.freq = 50 * MEGA},
>
> IMO we are losing information here.
>
> It feels you suggest to replace MHz with M.
When I introduced the CAN_{K,M}BPS and CAN_MHZ macros, my primary
intent was to avoid having to write more than five zeros in a
row (because the human brain is bad at counting those). And the
KILO/MEGA prefixes perfectly cover that intent.
You are correct to say that the information of the unit is
lost. But I assume this information to be implicit (frequencies
are in Hz, baudrate are in bits/second). So yes, I suggest
replacing MHz with M.
Do you really think that people will be confused by this change?
I am not strongly opposed to keeping it either (hey, I was the
one who introduced it in the first place). I just think that
using linux/units.h is sufficient.
> So where is the Hz information then?
It is in the comment of can_clock:freq :)
https://elixir.bootlin.com/linux/v5.15/source/include/uapi/linux/can/netlink.h#L63
> > .ctrlmode_supported = CAN_CTRLMODE_CC_LEN8_DLC,
> > .tx_start_of_frame = 0xAFAF,
> > .rx_start_of_frame = 0xFAFA,
> > diff --git a/drivers/net/can/usb/etas_es58x/es58x_fd.c b/drivers/net/can/usb/etas_es58x/es58x_fd.c
> > index 4f0cae29f4d8..aec299bed6dc 100644
> > --- a/drivers/net/can/usb/etas_es58x/es58x_fd.c
> > +++ b/drivers/net/can/usb/etas_es58x/es58x_fd.c
> > @@ -12,6 +12,7 @@
> > */
> >
> > #include <linux/kernel.h>
> > +#include <linux/units.h>
> > #include <asm/unaligned.h>
> >
> > #include "es58x_core.h"
> > @@ -522,8 +523,8 @@ const struct es58x_parameters es58x_fd_param = {
> > * Mbps work in an optimal environment but are not recommended
> > * for production environment.
> > */
> > - .bitrate_max = 8 * CAN_MBPS,
> > - .clock = {.freq = 80 * CAN_MHZ},
> > + .bitrate_max = 8 * MEGA,
> > + .clock = {.freq = 80 * MEGA},
> > .ctrlmode_supported = CAN_CTRLMODE_LOOPBACK | CAN_CTRLMODE_LISTENONLY |
> > CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_FD | CAN_CTRLMODE_FD_NON_ISO |
> > CAN_CTRLMODE_CC_LEN8_DLC | CAN_CTRLMODE_TDC_AUTO,
> > diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
> > index 20b50baf3a02..a81652d1c6f3 100644
> > --- a/include/linux/can/bittiming.h
> > +++ b/include/linux/can/bittiming.h
> > @@ -12,13 +12,6 @@
> > #define CAN_SYNC_SEG 1
> >
> >
> > -/* Kilobits and Megabits per second */
> > -#define CAN_KBPS 1000UL
> > -#define CAN_MBPS 1000000UL
> > -
> > -/* Megahertz */
> > -#define CAN_MHZ 1000000UL
>
> So what about
>
> #define CAN_KBPS KILO /* kilo bits per second */
> #define CAN_MBPS MEGA /* mega bits per second */
>
> #define CAN_MHZ MEGA /* mega hertz */
>
>
> ??
>
> Regards,
> Oliver
>
>
> > -
> > #define CAN_CTRLMODE_TDC_MASK \
> > (CAN_CTRLMODE_TDC_AUTO | CAN_CTRLMODE_TDC_MANUAL)
> >
> >
Powered by blists - more mailing lists