On Wed, Sep 15, 2021 at 02:07:24AM +0300, Leon Romanovsky wrote:

 
+int rdma_counter_modify(struct ib_device *dev, u32 port, int index, bool enable)
+{
+	struct rdma_hw_stats *stats;
+	int ret;
+
+	if (!dev->ops.modify_hw_stat)
+		return -EOPNOTSUPP;
+
+	stats = ib_get_hw_stats_port(dev, port);
+	if (!stats)
+		return -EINVAL;
+
+	mutex_lock(&stats->lock);
+	ret = dev->ops.modify_hw_stat(dev, port, index, enable);
+	if (!ret)
+		enable ? clear_bit(index, stats->is_disabled) :
+			set_bit(index, stats->is_disabled);

This is not a kernel coding style write out the if, use success
oriented flow

Also, shouldn't this logic protect the driver from being called on
non-optional counters?

 	for (i = 0; i < data->stats->num_counters; i++) {
-		attr = &data->attrs[i];
+		if (data->stats->descs[i].flags & IB_STAT_FLAG_OPTIONAL)
+			continue;
+		attr = &data->attrs[pos];
 		sysfs_attr_init(&attr->attr.attr);
 		attr->attr.attr.name = data->stats->descs[i].name;
 		attr->attr.attr.mode = 0444;
 		attr->attr.show = hw_stat_device_show;
 		attr->show = show_hw_stats;
-		data->group.attrs[i] = &attr->attr.attr;
+		data->group.attrs[pos] = &attr->attr.attr;
+		pos++;
 	}

This isn't OK, the hw_stat_device_show() computes the stat index like
this:

	return stat_attr->show(ibdev, ibdev->hw_stats_data->stats,
			       stat_attr - ibdev->hw_stats_data->attrs, 0, buf);

Which assumes the stats are packed contiguously. This only works
because mlx5 is always putting the optional stats at the end.

 /**
  * struct rdma_stat_desc
  * @name - The name of the counter
- *
+ * @flags - Flags of the counter; For example, IB_STAT_FLAG_OPTIONAL
  */

The previous patch shouldn't have had the extra blank line then?

  

+int rdma_counter_modify(struct ib_device *dev, u32 port, int index,
+			bool is_add);

index should be unsigned int

The bool is called 'is_add' here but the implementation is 'enable' ?

Jason