Messages in this thread | | | Date | Thu, 3 Nov 2022 14:02:25 +0100 | Subject | Re: [RFC PATCH 1/1] can: virtio: Initial virtio CAN driver. | From | Harald Mommer <> |
| |
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
| |