Too much MISRA habit in my head which has no place here. Did so.+ 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.
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.+ 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.
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",
+ 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.
When you silently sanitize the length value here, you should do the same with the can_id checks above and simply do a masking likeDid so. Too much netdev_warn() in the code, really. Too much is too much.
can_id &= CAN_SFF_MASK or can_id &= CAN_EFF_MASK
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.+ (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?
Regards+
+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