Messages in this thread | | | Date | Sat, 27 Aug 2022 11:02:28 +0200 | Subject | Re: [RFC PATCH 1/1] can: virtio: Initial virtio CAN driver. | From | Oliver Hartkopp <> |
| |
Hi Harald,
On 8/25/22 15:44, Harald Mommer wrote:
> +/* > + * This function is the NAPI RX poll function and NAPI guarantees that this > + * function is not invoked simulataneously on multiply processors. > + * Read a RX message from the used queue and sends it to the upper layer. > + * (See also m_can.c / m_can_read_fifo()). > + */ > +static int virtio_can_read_rx_queue(struct virtqueue *vq) > +{ > + struct virtio_can_priv *priv = vq->vdev->priv; > + struct net_device *dev = priv->dev; > + struct net_device_stats *stats = &dev->stats; > + struct virtio_can_rx *can_rx; > + struct canfd_frame *cf; > + struct sk_buff *skb; > + unsigned int len; > + const unsigned int header_size = offsetof(struct virtio_can_rx, sdu); > + u16 msg_type; > + u32 can_flags; > + u32 can_id; > + > + can_rx = virtqueue_get_buf(vq, &len); > + if (!can_rx) > + return 0; /* No more data */ > + > + BUG_ON(len < header_size); > + > + /* virtio_can_hexdump(can_rx, len, 0u); */ > + > + if (priv->can.state >= CAN_STATE_ERROR_PASSIVE) { > + netdev_dbg(dev, "%s(): Controller not active\n", __func__); > + goto putback; > + } > + > + msg_type = le16_to_cpu(can_rx->msg_type); > + if (msg_type != VIRTIO_CAN_RX) { > + netdev_warn(dev, "RX: Got unknown msg_type %04x\n", msg_type); > + goto putback; > + } > + > + len -= header_size; /* Payload only now */ > + can_flags = le32_to_cpu(can_rx->flags); > + can_id = le32_to_cpu(can_rx->can_id); > + > + 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.
> + stats->rx_dropped++; > + netdev_warn(dev, "RX: CAN Id 0x%08x: Invalid flags 0x%x\n", > + can_id, can_flags); > + goto putback; > + } > + > + if ((can_flags & VIRTIO_CAN_FLAGS_EXTENDED) != 0u) {
e.g. here too ...
> + 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",
As it is no value 'too big' is not the right comment here.
> + can_id); > + goto putback; > + } > + can_id |= CAN_EFF_FLAG; > + } else { > + if (can_id > CAN_SFF_MASK) {
same here
> + stats->rx_dropped++; > + netdev_warn(dev, "RX: CAN Std Id 0x%08x too big\n",
and here
> + can_id); > + goto putback; > + } > + } > + > + if ((can_flags & VIRTIO_CAN_FLAGS_RTR) != 0u) { > + if (!virtio_has_feature(vq->vdev, VIRTIO_CAN_F_RTR_FRAMES)) { > + stats->rx_dropped++; > + netdev_warn(dev, "RX: CAN Id 0x%08x: RTR not negotiated\n", > + can_id); > + goto putback; > + } > + if ((can_flags & VIRTIO_CAN_FLAGS_FD) != 0u) { > + stats->rx_dropped++; > + netdev_warn(dev, "RX: CAN Id 0x%08x: RTR with FD not possible\n", > + can_id); > + goto putback; > + } > + 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.
> + goto putback; > + } > + can_id |= CAN_RTR_FLAG; > + } > + > + if ((can_flags & VIRTIO_CAN_FLAGS_FD) != 0u) { > + if (!virtio_has_feature(vq->vdev, VIRTIO_CAN_F_CAN_FD)) { > + stats->rx_dropped++; > + netdev_warn(dev, "RX: CAN Id 0x%08x: FD not negotiated\n", > + can_id); > + goto putback; > + } > + > + skb = alloc_canfd_skb(priv->dev, &cf); > + if (len > CANFD_MAX_DLEN) > + len = CANFD_MAX_DLEN;
No netdev_warn() here? ;-)
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
> + } else { > + if (!virtio_has_feature(vq->vdev, VIRTIO_CAN_F_CAN_CLASSIC)) { > + stats->rx_dropped++; > + netdev_warn(dev, "RX: CAN Id 0x%08x: classic not negotiated\n", > + can_id); > + goto putback; > + } > + > + skb = alloc_can_skb(priv->dev, (struct can_frame **)&cf); > + if (len > CAN_MAX_DLEN) > + len = CAN_MAX_DLEN; > + } > + if (!skb) { > + stats->rx_dropped++; > + netdev_warn(dev, "RX: No skb available\n"); > + goto putback; > + } > + > + cf->can_id = can_id; > + cf->len = len; > + if ((can_flags & VIRTIO_CAN_FLAGS_RTR) == 0u) { > + /* Copy if no RTR frame. RTR frames have a DLC but no payload */ > + memcpy(cf->data, can_rx->sdu, len);
This would need some rework together with the RTR handling too.
> + } > + > + stats->rx_packets++; > + stats->rx_bytes += cf->len; > + > + /* Use netif_rx() for interrupt context */
?? What is this comment about?
> + (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?
> + > +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
| |