Received: by 2002:a05:6a10:206:0:0:0:0 with SMTP id 6csp725374pxj; Wed, 16 Jun 2021 12:06:00 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzEQAkV3CU20zQKLBha2hILJP2hv4WHpRkhpWPbbR1BH4eZcF+Qdd94XaN9HKnd4BMzLK0k X-Received: by 2002:a17:906:28d5:: with SMTP id p21mr1031083ejd.358.1623870360252; Wed, 16 Jun 2021 12:06:00 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1623870360; cv=none; d=google.com; s=arc-20160816; b=j0qC6qiRDFgK3p9bvMT/jFCT45IsAKF+05F0GhbAAnyhW+VexKch4n5bH2uVnrI6BJ ucVgZNOfFQScjkYRIgiolrq+KIEHtrYhc5PWvbgUOGJfQaMWl0YnLi1qXcjKuZYJV8OW c2nXizIKX4KymMfidgF1DVXkVn27I/oAufavZYLT6KRDndtNJpsH9E9nQsptxB/kxrtL WEgdGqThQ74/aEzbc2pXOZCNuIKizQf8e+ebmrl8e2SMKBP//97eSGthdhQBiiIz+WRS VWhVp/EWcIXKmmWCiKVq6p3lXcYjny9tw+wApw+fYg1WJkb1nb4IK+0y+LPe3+6vv2us tKdw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:subject:message-id:date:from:in-reply-to :references:mime-version; bh=Yak0Jw5qtLSF6XuLp21VlMysIAiLyA6w6CP8nitLvQk=; b=Zt6oJ6KvsgiKoLCN0/V4HLuLrXclubtzHvUv+RN52TyfE5mw+0ypNOZ00m/g1ZfmSJ 6UY3YG5fJvxlC5vfxPVt42tnY7YBhb68gDlBZ8BEzeQayM0uDTTwLVNLCq5HT6+sFRlA Po6k4p1eoLK5753NnZbZUacUMZZmJbUAntzGQCuJGNzEK7/mIlobipLkva68QCCtA1pd uDreE4yE3srqm3Xx9zfy7hcmeHIx7+j52106ajE/Do6ZgVuEP7e7yL2C347AKUbXXd0H zrlDTkk+UJT3Fak8j4uGBQetiQJht3ujt5xxsrZXZHcrwKKD7LXHZqOYEtrAwCUWg3SR 1FTw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id ce29si3004469ejc.92.2021.06.16.12.05.34; Wed, 16 Jun 2021 12:06:00 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233555AbhFPN4V (ORCPT + 99 others); Wed, 16 Jun 2021 09:56:21 -0400 Received: from mail-lj1-f175.google.com ([209.85.208.175]:42865 "EHLO mail-lj1-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233697AbhFPNzW (ORCPT ); Wed, 16 Jun 2021 09:55:22 -0400 Received: by mail-lj1-f175.google.com with SMTP id r16so3927987ljk.9; Wed, 16 Jun 2021 06:53:14 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Yak0Jw5qtLSF6XuLp21VlMysIAiLyA6w6CP8nitLvQk=; b=mEfyXChHnSHaCrGszybE5zi+Y5Q52iSpBk2fEZ+qYpEDlTE3eYnKoHB4hdc5ScNCm1 GyZl/a7GbkFK6zaAISQ0EOF+JcI9aj2N24fsVDDSGZ7A4/QKI2neA8IvlGdhXyLPeHLW 5U6c4CBETcf1zv16Yse7ZJuxvyJ3gYhHvUyy5oqCQ6uodcthYOKmiEgB1mgPdz4cNPj9 w439Q5blXZAhwG0R7xHA+cAGZ+YtsZA4cA2+gxN0KXs2CWi+4vlDt5pxyEJ5s0f0dbRI eq7LsmwrgdG7O0U+jVGmlYdEznTKvUFKgtZqrT4uILxDaNHtixOcmaZ92VxIYB8jTosy aEXw== X-Gm-Message-State: AOAM530Lkxbozny+eTZTw58Pjndsvpra/7V1OxBd3qWFMeMxgB98tCRd PPi6Ox/RRtIySu1/6L9A+lgz532hETy3baxFRfg= X-Received: by 2002:a2e:bf14:: with SMTP id c20mr4534138ljr.57.1623851594153; Wed, 16 Jun 2021 06:53:14 -0700 (PDT) MIME-Version: 1.0 References: <20210603151550.140727-1-mailhol.vincent@wanadoo.fr> <20210603151550.140727-3-mailhol.vincent@wanadoo.fr> <20210616094633.fwg6rsyxyvm2zc6d@pengutronix.de> In-Reply-To: <20210616094633.fwg6rsyxyvm2zc6d@pengutronix.de> From: Vincent MAILHOL Date: Wed, 16 Jun 2021 22:53:02 +0900 Message-ID: Subject: Re: [PATCH v2 2/2] can: netlink: add interface for CAN-FD Transmitter Delay Compensation (TDC) To: Marc Kleine-Budde Cc: linux-can , netdev , open list , Oliver Hartkopp Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed. 16 Jun 2021 at 18:46, Marc Kleine-Budde wrote: > On 04.06.2021 00:15:50, Vincent Mailhol wrote: > [...] > > > +static size_t can_tdc_get_size(const struct net_device *dev) > > +{ > > + struct can_priv *priv = netdev_priv(dev); > > + size_t size; > > + > > + if (!priv->tdc_const) > > + return 0; > > + > > + size = nla_total_size(0); /* nest IFLA_CAN_TDC */ > > + size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCV_MAX */ > > + size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCO_MAX */ > > + size += nla_total_size(sizeof(u32)); /* IFLA_CAN_TDCF_MAX */ > > + > > + if (priv->tdc.tdco) { > > Naively I'd say, iff the device has tdc_const give the user space the > tdc parameters, regardless if some value is 0 or not. > > What do you think? I thought about that. The first important remark is that if tdc.tdco is zero, then TDC is off (c.f. documentation of struct can_tdc::tdco). Let me illustrate my vision through examples. ** Case 1: link is not configured at all. ** Here, only the constant values are displayed. # ip --details link show can0 1: can0: mtu 16 qdisc noop state DOWN mode DEFAULT group default qlen 10 link/can promiscuity 0 minmtu 0 maxmtu 0 can state STOPPED (berr-counter tx 0 rx 0) restart-ms 0 ES582.1/ES584.1: tseg1 2..256 tseg2 2..128 sjw 1..128 brp 1..512 brp_inc 1 ES582.1/ES584.1: dtseg1 2..32 dtseg2 1..16 dsjw 1..8 dbrp 1..32 brp_inc 1 tdcv_max 0 tdco_max 127 tdcf_max 127 clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 ** Case 2: only the nominal bitrate is configured. ** The data bittiming variables (including TDC) are not shown. # ip --details link show can0 1: can0: mtu 16 qdisc noop state DOWN mode DEFAULT group default qlen 10 link/can promiscuity 0 minmtu 0 maxmtu 0 can state STOPPED (berr-counter tx 0 rx 0) restart-ms 0 bitrate 500000 sample-point 0.875 tq 12 prop-seg 69 phase-seg1 70phase-seg2 20 sjw 1 ES582.1/ES584.1: tseg1 2..256 tseg2 2..128 sjw 1..128 brp 1..512 brp_inc 1 ES582.1/ES584.1: dtseg1 2..32 dtseg2 1..16 dsjw 1..8 dbrp 1..32 brp_inc 1 tdcv_max 0 tdco_max 127 tdcf_max 127 clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 ** Case 3: both nominal and data bitrates are configured (but not TDC). ** Only the TDC variables are not shown. # ip --details link show can0 1: can0: mtu 72 qdisc noop state DOWN mode DEFAULT group default qlen 10 link/can promiscuity 0 minmtu 0 maxmtu 0 can state STOPPED (berr-counter tx 0 rx 0) restart-ms 0 bitrate 500000 sample-point 0.875 tq 12 prop-seg 69 phase-seg1 70phase-seg2 20 sjw 1 ES582.1/ES584.1: tseg1 2..256 tseg2 2..128 sjw 1..128 brp 1..512 brp_inc 1 dbitrate 2000000 dsample-point 0.750 dtq 12 dprop-seg 14 dphase-seg1 15 dphase-seg2 10 dsjw 1 ES582.1/ES584.1: dtseg1 2..32 dtseg2 1..16 dsjw 1..8 dbrp 1..32 brp_inc 1 tdcv_max 0 tdco_max 127 tdcf_max 127 clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 ** Case 4: nominal and data bitrates and TDC are configured. ** Everything is shown. # ip --details link show can0 1: can0: mtu 72 qdisc noop state DOWN mode DEFAULT group default qlen 10 link/can promiscuity 0 minmtu 0 maxmtu 0 can state STOPPED (berr-counter tx 0 rx 0) restart-ms 0 bitrate 1000000 sample-point 0.750 tq 12 prop-seg 29 phase-seg1 30phase-seg2 20 sjw 1 ES582.1/ES584.1: tseg1 2..256 tseg2 2..128 sjw 1..128 brp 1..512 brp_inc 1 dbitrate 5000000 dsample-point 0.750 dtq 12 dprop-seg 5 dphase-seg1 6 dphase-seg2 4 dsjw 1 tdcv 0 tdco 12 tdcf 0 ES582.1/ES584.1: dtseg1 2..32 dtseg2 1..16 dsjw 1..8 dbrp 1..32 brp_inc 1 tdcv_max 0 tdco_max 127 tdcf_max 127 clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 I think that we can agree on Cases 1, 2 (it would not make sense to display TDC without the data bittiming variables) and 4. The edge case is Case 3. It depends if we consider the TDC as a separate set or not. It is not silly to display the TDC whenever fd is on. I prefer to keep it the way I did it. But I would not object to changing this if you insist. That would mean: - if (priv->tdc.tdco) { + if (priv->data_bittiming.bitrate) { Finally, I have one side comment. It seems to me that you did not understand that the intent of | if (priv->tdc.tdco) was to actually check whether TDC was on or off. In other words, my code was unclear. I am now thinking to introduce an helper macro: static bool can_tdc_is_enabled(const struct can_priv *priv) |{ | return !!priv->tdc.tdco; |} The code would look more clear like that. - if (priv->tdc.tdco) { + if (can_tdc_is_enabled(priv) { Yours sincerely, Vincent