netdev - Re: [PATCH v10 1/1] can: usb: etas_es58X: add support for ETAS ES58X CAN USB interfaces

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAMZ6RqKeGVsF+CcqoAWC7JXEo2oLTS5E5B3Jk4oeiF9XWEC3Sw@mail.gmail.com>
Date:   Fri, 15 Jan 2021 09:41:37 +0900
From:   Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To:     Oliver Hartkopp <socketcan@...tkopp.net>
Cc:     Marc Kleine-Budde <mkl@...gutronix.de>,
        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 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.

I think that 1/ is best.


Yours sincerely,
Vincent