linux-can.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Marc Kleine-Budde <mkl@pengutronix.de>
To: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
Cc: linux-can <linux-can@vger.kernel.org>,
	kernel@pengutronix.de,
	Arunachalam Santhanam <arunachalam.santhanam@in.bosch.com>
Subject: Re: [PATCH v13 01/11] can: etas_es58x: add core support for ETAS ES58X CAN USB interfaces
Date: Mon, 29 Mar 2021 11:18:42 +0200	[thread overview]
Message-ID: <20210329091842.xhssovbectrooj7f@pengutronix.de> (raw)
In-Reply-To: <CAMZ6RqL+n4tRy-B-W+fzW5B3QV6Bedrko57pU_0TE023Oxw_5w@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 7183 bytes --]

On 20.03.2021 19:33:02, Vincent MAILHOL wrote:
> > > +/**
> > > + * es58x_set_skb_timestamp() - Set the hardware timestamp of an skb.
> > > + * @netdev: CAN network device.
> > > + * @skb: socket buffer of a CAN message.
> > > + * @timestamp: Timestamp received from an ES58X device.
> > > + *
> > > + * Used for both received and loopback messages.
> > > + *
> > > + * Return: zero on success, -EFAULT if @skb is NULL.
> >
> > That's not correct.
> >
> > Please make sure to call it with skb != NULL and make it a void function.
> 
> Ack.
> All calls to that function already checked that the skb was not NULL.
> I will make the function void.
> 
> > Does it make sense for you to use of struct cyclecounter/struct timecounter.
> > Have a look at:
> >
> > https://lore.kernel.org/linux-can/20210304160328.2752293-6-mkl@pengutronix.de/
> >
> > As your device already provides a u64 timestamp you don't need the worker.
> 
> I saw your patch but because my device already uses a 64 bit
> counter, I did not see a benefit to do so.
> 
> Which problem would the struct cyclecounter/struct timecounter
> solve for my driver?

It probably doesn't matter.

> 
> > > + */
> > > +static int es58x_set_skb_timestamp(struct net_device *netdev,
> > > +                                struct sk_buff *skb, u64 timestamp)
> > > +{
> > > +     struct es58x_device *es58x_dev = es58x_priv(netdev)->es58x_dev;
> > > +     struct skb_shared_hwtstamps *hwts;
> > > +
> > > +     hwts = skb_hwtstamps(skb);
> > > +     /* Ignoring overflow (overflow on 64 bits timestamp with nano
> > > +      * second precision would occur after more than 500 years).
> > > +      */
> > > +     hwts->hwtstamp = ns_to_ktime(es58x_timestamp_to_ns(timestamp) +
> > > +                                  es58x_dev->realtime_diff_ns);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +/**
> > > + * es58x_rx_timestamp() - Handle a received timestamp.
> > > + * @es58x_dev: ES58X device.
> > > + * @timestamp: Timestamp received from a ES58X device.
> > > + *
> > > + * Calculate the difference between the ES58X device and the kernel
> > > + * internal clocks. This difference will be later used as an offset to
> > > + * convert the timestamps of RX and loopback messages to match the
> > > + * kernel system time (e.g. convert to UNIX time).
> >
> > I'm not sure if we want to have the timestamp based on the kernel system time.
> > The problem I see is that your clock is not synchronized to your system clock
> > and will probably drift, so you might accumulate an offset.
> >
> > When looking at candump output, a timestamp that more or less current time gives
> > a false sense of correctness, but if the timestamp is short after 01.0.1970 you
> > don't expect it to be correct.
> >
> > But this is a different problem.....
> 
> Here, we are discussing tastes and colors. I am perfectly aware
> that the two quartz used by the device and the kernel are not in
> sync and will drift away from each other. However, when looking
> at my traces, I like to have a hint of when an event occurs. In
> the worst case, the quartz might drift away up to four seconds a day,
> but the date, the hours and the minutes stay accurate.
> 
> So it is a discussion of making it convenient versus making it dummy proof.
> 
> If there is a strong consensus not to base the hardware timestamp
> on the kernel, then I will remove it. But if I have the choice, I
> prefer to keep it as it is.

I've changed the mcp251xfd timestamp to sync to the kernel time, too :)

[...]

> > > +/**
> > > + * es58x_rx_err_msg() - Handle a received CAN event or error message.
> > > + * @netdev: CAN network device.
> > > + * @error: Error code.
> > > + * @event: Event code.
> > > + * @timestamp: Timestamp received from a ES58X device.
> > > + *
> > > + * Handle the errors and events received by the ES58X device, create
> > > + * a CAN error skb and post it.
> > > + *
> > > + * In some rare cases the devices might get stucked alternating between
> > > + * CAN_STATE_ERROR_PASSIVE and CAN_STATE_ERROR_WARNING. To prevent
> > > + * this behavior, we force a bus off state if the device goes in
> > > + * CAN_STATE_ERROR_WARNING for ES58X_MAX_CONSECUTIVE_WARN consecutive
> > > + * times with no successful transmission or reception in between.
> > > + *
> > > + * Once the device is in bus off state, the only way to restart it is
> > > + * through the drivers/net/can/dev.c:can_restart() function. The
> > > + * device is technically capable to recover by itself under certain
> > > + * circumstances, however, allowing self recovery would create
> > > + * complex race conditions with drivers/net/can/dev.c:can_restart()
> > > + * and thus was not implemented. To activate automatic restart, please
> > > + * set the restart-ms parameter (e.g. ip link set can0 type can
> > > + * restart-ms 100).
> > > + *
> > > + * If the bus is really instable, this function would try to send a
> > > + * lot of log messages. Those are rate limited (i.e. you will see
> > > + * messages such as "net_ratelimit: XXX callbacks suppressed" in
> > > + * dmesg).
> > > + *
> > > + * Return: zero on success, errno when any error occurs.
> > > + */
> > > +int es58x_rx_err_msg(struct net_device *netdev, enum es58x_err error,
> > > +                  enum es58x_event event, u64 timestamp)
> > > +{
[...]
> > > +     switch (event) {
> > > +     case ES58X_EVENT_OK:    /* 0: No event */
> > > +             break;
> > > +
> > > +     case ES58X_EVENT_CRTL_ACTIVE:
> > > +             if (can->state == CAN_STATE_BUS_OFF) {
> > > +                     netdev_err(netdev,
> > > +                                "%s: state transition: BUS OFF -> ACTIVE\n",
> > > +                                __func__);
> >
> > Your device should not go autoamtically from bus off to active. Only when it's
> > restarted by the kernel (or the user).
> 
> Agree, this is why it is an error message.
> 
> CAN controllers are allowed to automatically recover under very
> specific conditions (c.f. ISO 11898-1, section 10.9.4 "Bus
> integration state": "Nodes that are in bus-off state shall
> re-enter the bus integration state immediately after detecting
> the idle condition if the bus-off recovery condition is not yet
> met").

On Linux the controllers should not recover automatically, but
controlled by the kernel as configured by restart-ms. So shut down the
controller in case of a bus off and wait for the kernel to recover via
priv->can.do_set_mode(CAN_MODE_START).

> Aside from the idle condition, I do not think that the nodes are
> allowed to self-recover.

ACK

> But because the can_restart() function does not use locks for the
> echo_skb[], I treat this scenario by printing an error message and
> ignoring it.

regards,
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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  reply	other threads:[~2021-03-29  9:19 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-19 12:41 [PATCH v13 00/11] Introducing ETAS ES58X CAN USB interfaces Marc Kleine-Budde
2021-03-19 12:41 ` [PATCH v13 01/11] can: etas_es58x: add core support for " Marc Kleine-Budde
2021-03-19 14:57   ` Marc Kleine-Budde
2021-03-20 10:33     ` Vincent MAILHOL
2021-03-29  9:18       ` Marc Kleine-Budde [this message]
2021-03-19 12:41 ` [PATCH v13 02/11] can: etas_es58x: make core driver compile without glue code drivers Marc Kleine-Budde
2021-03-19 12:41 ` [PATCH v13 03/11] can: etas_es58x: es58x_rx_err_msg() fix typo Marc Kleine-Budde
2021-03-19 12:41 ` [PATCH v13 04/11] can: etas_es58x: es58x_rx_cmd_ret_u32() fix kernel doc Marc Kleine-Budde
2021-03-19 12:41 ` [PATCH v13 05/11] can: etas_es58x: remove setting of dql.min_limit for now Marc Kleine-Budde
2021-03-19 13:39   ` Vincent MAILHOL
2021-03-19 14:06     ` Marc Kleine-Budde
2021-03-19 12:41 ` [PATCH v13 06/11] can: etas_es58x: add support for ETAS ES581.4 CAN USB interface Marc Kleine-Budde
2021-03-19 12:41 ` [PATCH v13 07/11] can: etas_es58x: es581_4: es581_4_sizeof_rx_tx_msg(): fix kernel doc Marc Kleine-Budde
2021-03-19 12:41 ` [PATCH v13 08/11] can: etas_es58x: es581_4: remove setting of dql.min_limit for now Marc Kleine-Budde
2021-03-19 12:41 ` [PATCH v13 09/11] can: etas_es58x: es58x_fd: add support for the ETAS ES58X_FD CAN USB interfaces Marc Kleine-Budde
2021-03-19 12:41 ` [PATCH v13 10/11] can: etas_es58x: es58x_fd: es58x_fd_sizeof_rx_tx_msg(): fix kernel doc Marc Kleine-Budde
2021-03-19 12:41 ` [PATCH v13 11/11] can: etas_es58x: es58x_fd: remove setting of dql.min_limit for now Marc Kleine-Budde
2021-03-19 13:32 ` [PATCH v13 00/11] Introducing ETAS ES58X CAN USB interfaces Vincent MAILHOL
2021-03-19 13:34   ` Marc Kleine-Budde

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210329091842.xhssovbectrooj7f@pengutronix.de \
    --to=mkl@pengutronix.de \
    --cc=arunachalam.santhanam@in.bosch.com \
    --cc=kernel@pengutronix.de \
    --cc=linux-can@vger.kernel.org \
    --cc=mailhol.vincent@wanadoo.fr \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).