lkml.org 
[lkml]   [2022]   [Aug]   [27]   [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
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

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