lkml.org 
[lkml]   [2022]   [Nov]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [RFC PATCH 1/1] can: virtio: Initial virtio CAN driver.
From
Hello,

On 27.08.22 11:02, Oliver Hartkopp wrote:
>> +    if ((can_flags & ~CAN_KNOWN_FLAGS) != 0u) {
>
> For your entire patch:
>
> Please remove this pointless " != 0u)" stuff.
>
>     if (can_flags & ~CAN_KNOWN_FLAGS) {
>
> is just ok.
Too much MISRA habit in my head which has no place here. Did so.
>> +        if (can_id > CAN_EFF_MASK) {
>
> The MASK is not a number value.
>
> The check should be
>
> if (can_id & ~CAN_EFF_MASK) {
>
> or you simply mask the can_id value to be really sure without the
> netdev_warn() stuff.
CAN_EFF_MASK is now treated as bitmask. And decided to remove this
netdev_warn() stuff as proposed, masked the values. So I will miss
invalid combinations from the virtio CAN device (may also be buggy) I
might still be interested in but this excessive netdev_warn() usage was
in the end also for my taste too much.
>
> Are you sure that you could get undefined CAN ID values here?
>
>> +            stats->rx_dropped++;
>> +            netdev_warn(dev, "RX: CAN Ext Id 0x%08x too big\n",

The proposed virtio CAN specification says: "The type of a CAN message
identifier is determined by flags. The 3 most significant bits of can_id
do not bear the information about the type of the CAN message identifier
and are 0."

=> This trace could have been seen when a buggy device wasn't obeying
the 2nd sentence.

>> +        if (len != 0u) {
>> +            stats->rx_dropped++;
>> +            netdev_warn(dev, "RX: CAN Id 0x%08x: RTR with len != 0\n",
>> +                    can_id);
>
> This is not the right handling.
>
> Classical CAN frames with RTR bit set can have a DLC value from 0 .. F
> which is represented in
>
> can_frame.len (for values 0 .. 8)
> can_frame.len8_dlc (values 9 .. F; len must be 8)
>
> With the RTR bit set, the CAN controller does not send CAN data, but
> the DLC value is set from 0 .. F.

RTR frames! DLC values <> 0 but then no payload. This needed to be
reworked and fixed.

> When you silently sanitize the length value here, you should do the
> same with the can_id checks above and simply do a masking like
>
> can_id &= CAN_SFF_MASK or can_id &= CAN_EFF_MASK
>
Did so. Too much netdev_warn() in the code, really. Too much is too much.
>> +    (void)netif_receive_skb(skb);
>
> Why this "(void)" here and at other places in the patch? Please remove.
> Is there no error handling needed when netif_receive_skb() fails? Or
> ar least some statistics rollback?
Old MISRA habit to silence the warning when no error is expected to be
possible to occur. This has no place here and was replaced by some
better error handling evaluating the returned error not updating the
statistics as proposed. For the (void) below just omitted the (void), I
see no possible good error handling there and we're not going to run the
driver through the MISRA checker.
>> +
>> +putback:
>> +    /* Put processed RX buffer back into avail queue */
>> +    (void)virtio_can_add_inbuf(vq, can_rx, sizeof(struct
>> virtio_can_rx));
>> +
>> +    return 1; /* Queue was not emtpy so there may be more data */
>> +}
>
> Best regards,
> Oliver
>
Regards
Harald

(First answering all those review E-Mails, then will have to fight with
sending the changed code).

--
Dipl.-Ing. Harald Mommer
Senior Software Engineer

OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin

Phone: +49 (30) 60 98 540-0 <== Zentrale
Fax: +49 (30) 60 98 540-99
E-Mail: harald.mommer@opensynergy.com

www.opensynergy.com

Handelsregister: Amtsgericht Charlottenburg, HRB 108616B
Geschäftsführer/Managing Director: Regis Adjamah

\
 
 \ /
  Last update: 2022-11-03 14:04    [W:0.124 / U:0.672 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site