Re: [PATCH RFC] can fd: Add separate bittiming infrastructure

[Stephane Grosjean <s.grosjean@peak-system.com>]

Le 18/01/2014 18:34, Oliver Hartkopp a écrit :
> Hi Stephane,
>
> On 17.01.2014 08:44, Stephane Grosjean wrote:
>
>>> +#define CAN_CTRLMODE_FD            0x20    /* CAN FD mode */
>> - What is exactly the goal of this new CAN_CTRLMODE_FD please? Did you define
>> it to allow user to enable the CANFD function into the hardware, for example?
>> If yes, isn't it redundant with setting the MTU to 72 bytes?
>>
>> - Moreover, how a CANFD -able driver has to handle a CANFD frame read from the
>> CANFD controller, when its network device MTU *ISNOT* == 72 ??? Should it
>> discard the CANFD frame?
> The stuff in linux/net/can/ takes care that only applications that know (and
> enable) CAN FD get CAN frames with a length > 8 byte.
>
> E.g. today a CAN FD frame with only 8 bytes (aka CANFD8) can be passed to
> applications that are not CAN FD capable. In this case the size (MTU)
> is reduced from CANFD_MTU to CAN_MTU, see:
>
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/net/can/raw.c?id=e2d265d3b587f5f6f8febc0222aace93302ff0be
>
> and
>
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/net/can/raw.c#n753
>
> This leads IMO to two questions:
>
> 1. Is it a good idea to provide CANFD8 frames to legacy applications?

Well, at first, I didn't very agree with this idea ... But I'm not able 
to find an example against, so ...
My first option was to define a new socket option, or using the 
"dev->mtu" field value:

If the application sets this option to "on" (or the MTU to 72) so it 
tells the CAN core that it is able to receive ANY CANFD frames.
If the application doesn't set one or the other option, as all existing 
"legacy" CAN applications does not, well, the CAN core won't give it ANY 
incoming CANFD frame, that is, any CAN frame with "skb->len != CAN_MTU" 
if "dev->mtu != 72". Something like

  static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
                     struct packet_type *pt, struct net_device *orig_dev)
  {
          struct canfd_frame *cfd = (struct canfd_frame *)skb->data;

          if (unlikely(!net_eq(dev_net(dev), &init_net)))
                  goto drop;

+        if (dev->mtu != CANFD_MTU)
+               goto drop;
+
          if (WARN_ONCE(dev->type != ARPHRD_CAN ||
                        skb->len != CANFD_MTU ||
                        cfd->len > CANFD_MAX_DLEN,
                        "PF_CAN: dropped non conform CAN FD skbuf: "
                        "dev type %d, len %d, datalen %d\n",
                        dev->type, skb->len, cfd->len))
                  goto drop;

          can_receive(skb, dev);
          return NET_RX_SUCCESS;

  drop:
          kfree_skb(skb);
          return NET_RX_DROP;
  }

Why trying to be backward compatible at any price?

> 2. If yes, should the CAN FD controller driver be able to make legacy frames
>     CANDF8 frames to support the higher data bitrate for legacy applications?

Same as above, I have no real arguments against this.

> Maybe even the first question has to be denied (and the adaption in can/raw.c
> should be removed then) ?!?
> My idea behind CAN_CTRLMODE_FD is to enable/disable the CAN FD mode in the
> CAN FD capable controller chip.
>
> Depending on this CAN_CTRLMODE_FD the CAN FD functionality is enabled when
> this feature is supported(!) by the driver, see priv->ctrlmode_supported:

Ok I agree with the usage of this new flag in "ctrmode_supported".

> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/can/dev.c#n669
>
> Depending on CAN_CTRLMODE_FD the MTU is then set to CAN_MTU or CANFD_MTU

So? Do you mean that the *CAN core* will set the MTU to CAN_MTU or 
CANFD_MTU according to "priv->ctrlmode_supported & CAN_CTRLMODE_FD" ?
(Atm, the MTU is changed from user space only).

> (note that the interface has to be 'down' when configuring this feature -
> that's just the same as with configuring the bitrate).
>
> When CAN_CTRLMODE_FD was set but the second bitrate was *not* set before the
> interface is requested to be set to 'up' this leads to the same error as if
> the (single and only) bitrate is missing today.

So, this means that the arbitration bitrate could not be the default 
data bitrate, doesn't it?

> I hope this would be a reasonable solution and I was able to describe it well.
>
> Comming back to question 2 from above, we would then need an additional
> controlmode:
>
> #define CAN_CTRLMODE_TX_FD8 0x40    /* send legacy CAN frames in FD mode */

...
> But I'm not really sure if these two 'compatibility' tweaks for legacy
> applications to use CAN FD8 is a wanted/needed feature.
>
> Any thoughts?
>
>>>      /*
>>>     * CAN device statistics
>>> @@ -122,6 +123,9 @@ enum {
>>>        IFLA_CAN_RESTART_MS,
>>>        IFLA_CAN_RESTART,
>>>        IFLA_CAN_BERR_COUNTER,
>>> +    IFLA_CAN_DATA_BITTIMING,
>>> +    IFLA_CAN_DATA_BITTIMING_CONST,
>>> +    IFLA_CAN_DATA_CLOCK,
>>>        __IFLA_CAN_MAX
>>>    };
>> - by defining another clock for the data bitrate, do you suppose that some
>> hardwares could use different clocks for both arbitration and data bitrates?
> Yes. But this is probably not necessary. I just looked into the M_CAN
> specification. They have *one* CAN clock and two different baud rate
> prescalers. So I would tend to remove IFLA_CAN_DATA_CLOCK in the next RFC
> until a real requirement emerges from some real hardware.

Ack

> Best regards,
> Oliver
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
PEAK-System Technik GmbH, Otto-Roehm-Strasse 69, D-64293 Darmstadt 
Geschaeftsleitung: A.Gach/U.Wilhelm,St.Nr.:007/241/13586 FA Darmstadt 
HRB-9183 Darmstadt, Ust.IdNr.:DE 202220078, WEE-Reg.-Nr.: DE39305391 
Tel.+49 (0)6151-817320 / Fax:+49 (0)6151-817329, info@peak-system.com