On Mon, 29 Jun 2020 21:00:57 +0000 Brady, Alan wrote:

quoted

quoted

+/* Helper macro to define an iecm_stat structure with proper size and type.
+ * Use this when defining constant statistics arrays. Note that
+@_type expects
+ * only a type name and is used multiple times.
+ */
+#define IECM_STAT(_type, _name, _stat) { \
+	.stat_string = _name, \
+	.sizeof_stat = sizeof_field(_type, _stat), \
+	.stat_offset = offsetof(_type, _stat) \ }
+
+/* Helper macro for defining some statistics related to queues */
+#define IECM_QUEUE_STAT(_name, _stat) \
+	IECM_STAT(struct iecm_queue, _name, _stat)
+
+/* Stats associated with a Tx queue */ static const struct iecm_stats
+iecm_gstrings_tx_queue_stats[] = {
+	IECM_QUEUE_STAT("%s-%u.packets", q_stats.tx.packets),
+	IECM_QUEUE_STAT("%s-%u.bytes", q_stats.tx.bytes), };
+
+/* Stats associated with an Rx queue */ static const struct
+iecm_stats iecm_gstrings_rx_queue_stats[] = {
+	IECM_QUEUE_STAT("%s-%u.packets", q_stats.rx.packets),
+	IECM_QUEUE_STAT("%s-%u.bytes", q_stats.rx.bytes),
+	IECM_QUEUE_STAT("%s-%u.generic_csum", q_stats.rx.generic_csum),
+	IECM_QUEUE_STAT("%s-%u.basic_csum", q_stats.rx.basic_csum),  

What's basic and generic? perhaps given them the Linux names?  

I believe these should be hw_csum for basic_csum and csum_valid for generic_csum, will fix.

I was thinking of just saying csum_complete and csum_unnecessary.

But generic_sum doesn't seem to be incremented in this patch, so hard
to tell what it is :S

quoted

quoted

+	q->itr.target_itr = coalesce_usecs;
+	if (use_adaptive_coalesce)
+		q->itr.target_itr |= IECM_ITR_DYNAMIC;
+	/* Update of static/dynamic ITR will be taken care when interrupt is
+	 * fired
+	 */
+	return 0;
+}
+
+/**
+ * iecm_set_q_coalesce - set ITR values for specific queue
+ * @vport: vport associated to the queue that need updating
+ * @ec: coalesce settings to program the device with
+ * @q_num: update ITR/INTRL (coalesce) settings for this queue
+number/index
+ * @is_rxq: is queue type Rx
+ *
+ * Return 0 on success, and negative on failure  */ static int
+iecm_set_q_coalesce(struct iecm_vport *vport, struct ethtool_coalesce *ec,
+		    int q_num, bool is_rxq)
+{
+	if (is_rxq) {
+		struct iecm_queue *rxq = iecm_find_rxq(vport, q_num);
+
+		if (rxq && __iecm_set_q_coalesce(ec, rxq))
+			return -EINVAL;
+	} else {
+		struct iecm_queue *txq = iecm_find_txq(vport, q_num);
+
+		if (txq && __iecm_set_q_coalesce(ec, txq))
+			return -EINVAL;
+	}  

What's the point? Callers always call this function with tx, then rx.
Just set both.  

As I understand it's possible to have a different number of TX and RX
queues.  Theoretically iecm_find_Xq will just return NULL if there's
no queue for some index so we could do both, but then we have to
figure which one is greater etc etc.  It seems less error prone and
clearer to me to just call it for the queues we need to.  We can make
this iecm_set_q_coalesce function a little less terse, perhaps that
is sufficient?

I don't feel strongly about this one, up to you.