Re: [PATCH net-next 3/6] virtio_net: support device stats

[Simon Horman <horms@xxxxxxxxxx>]

On Fri, Dec 22, 2023 at 11:30:18AM +0800, Xuan Zhuo wrote:
> As the spec https://github.com/oasis-tcs/virtio-spec/commit/42f389989823039724f95bbbd243291ab0064f82
> 
> Virtio-net supports to get the stats from the device by ethtool -S <eth0>.
> 
> Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>

Hi Xuan Zhuo,

some minor feedback from my side relating to Sparse warnings.

...

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c

...

> +static int virtnet_get_hw_stats(struct virtnet_info *vi,
> +				struct virtnet_stats_ctx *ctx)
> +{
> +	struct virtio_net_ctrl_queue_stats *req;
> +	struct virtio_net_stats_reply_hdr *hdr;
> +	struct scatterlist sgs_in, sgs_out;
> +	u32 num_rx, num_tx, num_cq, offset;
> +	int qnum, i, j,  qid, res_size;
> +	struct virtnet_stats_map *m;
> +	void *reply, *p;
> +	u64 bitmap;
> +	int ok;
> +	u64 *v;

...

> +	for (p = reply; p - reply < res_size; p += virtio16_to_cpu(vi->vdev, hdr->size)) {

The type of the second parameter of _virtio16_to_cpu() is __virtio16.
But the type of the size field of virtio_net_stats_reply_hdr is __le16.
This does not seem correct.

> +		hdr = p;
> +
> +		qid = virtio16_to_cpu(vi->vdev, hdr->vq_index);

Similarly, the vq_index field of virtio_net_stats_reply_hdr is also __le16.

...

> +		for (i = 0; i < ARRAY_SIZE(virtio_net_stats_map); ++i) {
> +			m = &virtio_net_stats_map[i];
> +
> +			if (m->flag & bitmap)
> +				offset += m->num;
> +
> +			if (hdr->type != m->type)
> +				continue;
> +
> +			for (j = 0; j < m->num; ++j) {
> +				v = p + m->desc[j].offset;
> +				ctx->data[offset + j] = virtio64_to_cpu(vi->vdev, *v);

Here the type of the second parameter of virtio64_to_cpu() is __virtio64.
But the type of *v is u64.

> +			}
> +
> +			break;
> +		}
> +	}
> +
> +	kfree(reply);
> +	return 0;
> +}
> +

...

> @@ -3183,11 +3497,35 @@ static void virtnet_get_strings(struct net_device *dev, u32 stringset, u8 *data)
>  static int virtnet_get_sset_count(struct net_device *dev, int sset)
>  {
>  	struct virtnet_info *vi = netdev_priv(dev);
> +	struct virtnet_stats_ctx ctx = {0};
> +	u32 pair_count;
>  
>  	switch (sset) {
>  	case ETH_SS_STATS:
> -		return vi->curr_queue_pairs * (VIRTNET_RQ_STATS_LEN +
> -					       VIRTNET_SQ_STATS_LEN);
> +		if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_DEVICE_STATS) &&
> +		    !vi->device_stats_cap) {
> +			struct scatterlist sg;
> +
> +			sg_init_one(&sg, &vi->ctrl->stats_cap, sizeof(vi->ctrl->stats_cap));
> +
> +			if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_STATS,
> +						  VIRTIO_NET_CTRL_STATS_QUERY,
> +						  NULL, &sg)) {
> +				dev_warn(&dev->dev, "Fail to get stats capability\n");
> +			} else {
> +				__le64 v;
> +
> +				v = vi->ctrl->stats_cap.supported_stats_types[0];
> +				vi->device_stats_cap = virtio64_to_cpu(vi->vdev, v);

Similarly, the type of v is __le64.

> +			}
> +		}
> +
> +		virtnet_stats_ctx_init(vi, &ctx, NULL);
> +
> +		pair_count = VIRTNET_RQ_STATS_LEN + VIRTNET_SQ_STATS_LEN;
> +		pair_count += ctx.num_rx + ctx.num_tx;
> +
> +		return ctx.num_cq + vi->curr_queue_pairs * pair_count;
>  	default:
>  		return -EOPNOTSUPP;
>  	}

...