Re: [RFC PATCH 1/1] can: virtio: Initial virtio CAN driver.

[Harald Mommer <hmo@opensynergy.com>]

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