| 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
| ||
|
Message-ID: <3daf39cb-2835-379d-86df-91b17282594a@pengutronix.de>
Date: Fri, 15 Jan 2021 08:26:01 +0100
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Vincent MAILHOL <mailhol.vincent@...adoo.fr>,
Oliver Hartkopp <socketcan@...tkopp.net>
Cc: linux-can <linux-can@...r.kernel.org>,
Arunachalam Santhanam <arunachalam.santhanam@...bosch.com>,
Wolfgang Grandegger <wg@...ndegger.com>,
"David S . Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Jimmy Assarsson <extja@...ser.com>,
Masahiro Yamada <masahiroy@...nel.org>,
"open list : NETWORKING DRIVERS" <netdev@...r.kernel.org>
Subject: Re: [PATCH v10 1/1] can: usb: etas_es58X: add support for ETAS ES58X
CAN USB interfaces
On 1/15/21 1:41 AM, Vincent MAILHOL wrote:
> On Fri. 15 Jan 2021 at 02:23, Oliver Hartkopp <socketcan@...tkopp.net> wrote:
>>
>> Hi Vincent,
>>
>> On 12.01.21 14:05, Vincent Mailhol wrote:
>>> This driver supports the ES581.4, ES582.1 and ES584.1 interfaces from
>>> ETAS GmbH (https://www.etas.com/en/products/es58x.php).
>>
>> (..)
>>
>>> diff --git a/drivers/net/can/usb/etas_es58x/es58x_fd.c b/drivers/net/can/usb/etas_es58x/es58x_fd.c
>>> new file mode 100644
>>> index 000000000000..6b9534f23c96
>>> --- /dev/null
>>> +++ b/drivers/net/can/usb/etas_es58x/es58x_fd.c
>>
>> (..)
>>
>>> +static void es58x_fd_print_bittiming(struct net_device *netdev,
>>> + struct es58x_fd_bittiming
>>> + *es58x_fd_bittiming, char *type)
>>> +{
>>> + netdev_vdbg(netdev, "bitrate %s = %d\n", type,
>>> + le32_to_cpu(es58x_fd_bittiming->bitrate));
>>> + netdev_vdbg(netdev, "tseg1 %s = %d\n", type,
>>> + le16_to_cpu(es58x_fd_bittiming->tseg1));
>>> + netdev_vdbg(netdev, "tseg2 %s = %d\n", type,
>>> + le16_to_cpu(es58x_fd_bittiming->tseg2));
>>> + netdev_vdbg(netdev, "brp %s = %d\n", type,
>>> + le16_to_cpu(es58x_fd_bittiming->brp));
>>> + netdev_vdbg(netdev, "sjw %s = %d\n", type,
>>> + le16_to_cpu(es58x_fd_bittiming->sjw));
>>> +}
>>
>> What is the reason for this code?
>>
>> These values can be retrieved with the 'ip' tool and are probably
>> interesting for development - but not in the final code.
>
> First thing, I used netdev_vdbg() (verbose debug). That macro
> will only produce code if VERBOSE_DEBUG is defined. Normal users
> will not see those. So yes, this is mostly for development.
>
> Also, just realised that netdev_vdbg() is barely used
> anywhere (only three files use it:
> https://elixir.bootlin.com/linux/v5.11-rc3/C/ident/netdev_vdbg).
>
> I guess that I will remove it :)
>
>>> +
>>> +static void es58x_fd_print_conf(struct net_device *netdev,
>>> + struct es58x_fd_tx_conf_msg *tx_conf_msg)
>>> +{
>>> + es58x_fd_print_bittiming(netdev, &tx_conf_msg->nominal_bittiming,
>>> + "nominal");
>>> + netdev_vdbg(netdev, "samples_per_bit = %d\n",
>>> + tx_conf_msg->samples_per_bit);
>>> + netdev_vdbg(netdev, "sync_edge = %d\n",
>>> + tx_conf_msg->sync_edge);
>>> + netdev_vdbg(netdev, "physical_layer = %d\n",
>>> + tx_conf_msg->physical_layer);
>>> + netdev_vdbg(netdev, "self_reception = %d\n",
>>> + tx_conf_msg->self_reception_mode);
>>> + netdev_vdbg(netdev, "ctrlmode = %d\n", tx_conf_msg->ctrlmode);
>>> + netdev_vdbg(netdev, "canfd_enabled = %d\n",
>>> + tx_conf_msg->canfd_enabled);
>>> + if (tx_conf_msg->canfd_enabled) {
>>> + es58x_fd_print_bittiming(netdev,
>>> + &tx_conf_msg->data_bittiming, "data");
>>> + netdev_vdbg(netdev,
>>> + "Transmitter Delay Compensation = %d\n",
>>> + tx_conf_msg->tdc);
>>> + netdev_vdbg(netdev,
>>> + "Transmitter Delay Compensation Offset = %d\n",
>>> + le16_to_cpu(tx_conf_msg->tdco));
>>> + netdev_vdbg(netdev,
>>> + "Transmitter Delay Compensation Filter = %d\n",
>>> + le16_to_cpu(tx_conf_msg->tdcf));
>>> + }
>>> +}
>>
>> Same here.
>>
>> Either the information can be retrieved with the 'ip' tool OR the are
>> not necessary as set to some reasonable default anyway
>
> Ack, will remove.
>
>> OR we should
>> implement the functionality in the general CAN driver infrastructure.
>
> Would make sense to me to add the tdco (Transmitter Delay
> Compensation Offset). Ref: ISO 11898-1 section
> 11.3.3 "Transmitter delay compensation"
>
> I would just like your opinion on one topic: the tdco is specific
> to CAN FD. If we add it, we have two choices:
> 1. put it in struct can_bittiming: that will mean that we will
> have an unused field for classical CAN (field bittiming of
> struct can_priv).
> 2. put it in struct can_priv (but outside of struct
> can_bittiming): no unused field but less pretty.
3. Deprecate struct can_bittiming as the user space interface
and transfer each member individually via netlink. Extend
the kernel-only can_bittiming by the tdc related
parameters, and add these to the new netlink interface.
I prefer this, as I want to extend the bittiming_const in this way, too. There
are CAN controllers, where the bit timing calculation:
> bt->prop_seg = tseg1 / 2;
> bt->phase_seg1 = tseg1 - bt->prop_seg;
doesn't work anymore, as they have asymmetric prog_seg and phase_seg1, so that
splitting tseg1 in half doesn't work anymore.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists