[PATCH net-next v2 0/6] ice: lower CPU usage with GNSS

[PATCH net-next v2 0/6] ice: lower CPU usage with GNSS

[Michal Schmidt]

This series lowers the CPU usage of the ice driver when using its
provided /dev/gnss*.

v2:
 - Changed subject of patch 1. Requested by Andrew Lunn.
 - Added patch 2 to change the polling interval as recommended by Intel.
 - Added patch 3 to remove sq_cmd_timeout as suggested by Simon Horman.

Michal Schmidt (6):
  ice: do not busy-wait to read GNSS data
  ice: increase the GNSS data polling interval to 20 ms
  ice: remove ice_ctl_q_info::sq_cmd_timeout
  ice: sleep, don't busy-wait, for ICE_CTL_Q_SQ_CMD_TIMEOUT
  ice: remove unused buffer copy code in ice_sq_send_cmd_retry()
  ice: sleep, don't busy-wait, in the SQ send retry loop

 drivers/net/ethernet/intel/ice/ice_common.c   | 29 +++++--------
 drivers/net/ethernet/intel/ice/ice_controlq.c | 12 +++---
 drivers/net/ethernet/intel/ice/ice_controlq.h |  3 +-
 drivers/net/ethernet/intel/ice/ice_gnss.c     | 42 +++++++++----------
 drivers/net/ethernet/intel/ice/ice_gnss.h     |  3 +-
 5 files changed, 36 insertions(+), 53 deletions(-)

-- 
2.39.2

[PATCH net-next v2 1/6] ice: do not busy-wait to read GNSS data

[Michal Schmidt]

The ice-gnss-<dev_name> kernel thread, which reads data from the u-blox
GNSS module, keep a CPU core almost 100% busy. The main reason is that
it busy-waits for data to become available.

A simple improvement would be to replace the "mdelay(10);" in
ice_gnss_read() with sleeping. A better fix is to not do any waiting
directly in the function and just requeue this delayed work as needed.
The advantage is that canceling the work from ice_gnss_exit() becomes
immediate, rather than taking up to ~2.5 seconds (ICE_MAX_UBX_READ_TRIES
* 10 ms).

This lowers the CPU usage of the ice-gnss-<dev_name> thread on my system
from ~90 % to ~8 %.

I am not sure if the larger 0.1 s pause after inserting data into the
gnss subsystem is really necessary, but I'm keeping that as it was.

Of course, ideally the driver would not have to poll at all, but I don't
know if the E810 can watch for GNSS data availability over the i2c bus
by itself and notify the driver.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/intel/ice/ice_gnss.c | 42 ++++++++++-------------
 drivers/net/ethernet/intel/ice/ice_gnss.h |  3 +-
 2 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
index 8dec748bb53a..2ea8a2b11bcd 100644
--- a/drivers/net/ethernet/intel/ice/ice_gnss.c
+++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
@@ -117,6 +117,7 @@ static void ice_gnss_read(struct kthread_work *work)
 {
 	struct gnss_serial *gnss = container_of(work, struct gnss_serial,
 						read_work.work);
+	unsigned long delay = ICE_GNSS_POLL_DATA_DELAY_TIME;
 	unsigned int i, bytes_read, data_len, count;
 	struct ice_aqc_link_topo_addr link_topo;
 	struct ice_pf *pf;
@@ -136,11 +137,6 @@ static void ice_gnss_read(struct kthread_work *work)
 		return;
 
 	hw = &pf->hw;
-	buf = (char *)get_zeroed_page(GFP_KERNEL);
-	if (!buf) {
-		err = -ENOMEM;
-		goto exit;
-	}
 
 	memset(&link_topo, 0, sizeof(struct ice_aqc_link_topo_addr));
 	link_topo.topo_params.index = ICE_E810T_GNSS_I2C_BUS;
@@ -151,25 +147,24 @@ static void ice_gnss_read(struct kthread_work *work)
 	i2c_params = ICE_GNSS_UBX_DATA_LEN_WIDTH |
 		     ICE_AQC_I2C_USE_REPEATED_START;
 
-	/* Read data length in a loop, when it's not 0 the data is ready */
-	for (i = 0; i < ICE_MAX_UBX_READ_TRIES; i++) {
-		err = ice_aq_read_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
-				      cpu_to_le16(ICE_GNSS_UBX_DATA_LEN_H),
-				      i2c_params, (u8 *)&data_len_b, NULL);
-		if (err)
-			goto exit_buf;
+	err = ice_aq_read_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
+			      cpu_to_le16(ICE_GNSS_UBX_DATA_LEN_H),
+			      i2c_params, (u8 *)&data_len_b, NULL);
+	if (err)
+		goto requeue;
 
-		data_len = be16_to_cpu(data_len_b);
-		if (data_len != 0 && data_len != U16_MAX)
-			break;
+	data_len = be16_to_cpu(data_len_b);
+	if (data_len == 0 || data_len == U16_MAX)
+		goto requeue;
 
-		mdelay(10);
-	}
+	/* The u-blox has data_len bytes for us to read */
 
 	data_len = min_t(typeof(data_len), data_len, PAGE_SIZE);
-	if (!data_len) {
+
+	buf = (char *)get_zeroed_page(GFP_KERNEL);
+	if (!buf) {
 		err = -ENOMEM;
-		goto exit_buf;
+		goto requeue;
 	}
 
 	/* Read received data */
@@ -183,7 +178,7 @@ static void ice_gnss_read(struct kthread_work *work)
 				      cpu_to_le16(ICE_GNSS_UBX_EMPTY_DATA),
 				      bytes_read, &buf[i], NULL);
 		if (err)
-			goto exit_buf;
+			goto free_buf;
 	}
 
 	count = gnss_insert_raw(pf->gnss_dev, buf, i);
@@ -191,10 +186,11 @@ static void ice_gnss_read(struct kthread_work *work)
 		dev_warn(ice_pf_to_dev(pf),
 			 "gnss_insert_raw ret=%d size=%d\n",
 			 count, i);
-exit_buf:
+	delay = ICE_GNSS_TIMER_DELAY_TIME;
+free_buf:
 	free_page((unsigned long)buf);
-	kthread_queue_delayed_work(gnss->kworker, &gnss->read_work,
-				   ICE_GNSS_TIMER_DELAY_TIME);
+requeue:
+	kthread_queue_delayed_work(gnss->kworker, &gnss->read_work, delay);
 exit:
 	if (err)
 		dev_dbg(ice_pf_to_dev(pf), "GNSS failed to read err=%d\n", err);
diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.h b/drivers/net/ethernet/intel/ice/ice_gnss.h
index 4d49e5b0b4b8..640df7411373 100644
--- a/drivers/net/ethernet/intel/ice/ice_gnss.h
+++ b/drivers/net/ethernet/intel/ice/ice_gnss.h
@@ -5,6 +5,7 @@
 #define _ICE_GNSS_H_
 
 #define ICE_E810T_GNSS_I2C_BUS		0x2
+#define ICE_GNSS_POLL_DATA_DELAY_TIME	(HZ / 100) /* poll every 10 ms */
 #define ICE_GNSS_TIMER_DELAY_TIME	(HZ / 10) /* 0.1 second per message */
 #define ICE_GNSS_TTY_WRITE_BUF		250
 #define ICE_MAX_I2C_DATA_SIZE		FIELD_MAX(ICE_AQC_I2C_DATA_SIZE_M)
@@ -20,8 +21,6 @@
  * passed as I2C addr parameter.
  */
 #define ICE_GNSS_UBX_WRITE_BYTES	(ICE_MAX_I2C_WRITE_BYTES + 1)
-#define ICE_MAX_UBX_READ_TRIES		255
-#define ICE_MAX_UBX_ACK_READ_TRIES	4095
 
 struct gnss_write_buf {
 	struct list_head queue;
-- 
2.39.2

[PATCH net-next v2 2/6] ice: increase the GNSS data polling interval to 20 ms

[Michal Schmidt]

Double the GNSS data polling interval from 10 ms to 20 ms.
According to Karol Kolacinski from the Intel team, they have been
planning to make this change.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/intel/ice/ice_gnss.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.h b/drivers/net/ethernet/intel/ice/ice_gnss.h
index 640df7411373..b8bb8b63d081 100644
--- a/drivers/net/ethernet/intel/ice/ice_gnss.h
+++ b/drivers/net/ethernet/intel/ice/ice_gnss.h
@@ -5,7 +5,7 @@
 #define _ICE_GNSS_H_
 
 #define ICE_E810T_GNSS_I2C_BUS		0x2
-#define ICE_GNSS_POLL_DATA_DELAY_TIME	(HZ / 100) /* poll every 10 ms */
+#define ICE_GNSS_POLL_DATA_DELAY_TIME	(HZ / 50) /* poll every 20 ms */
 #define ICE_GNSS_TIMER_DELAY_TIME	(HZ / 10) /* 0.1 second per message */
 #define ICE_GNSS_TTY_WRITE_BUF		250
 #define ICE_MAX_I2C_DATA_SIZE		FIELD_MAX(ICE_AQC_I2C_DATA_SIZE_M)
-- 
2.39.2

[PATCH net-next v2 3/6] ice: remove ice_ctl_q_info::sq_cmd_timeout

[Michal Schmidt]

sq_cmd_timeout is initialized to ICE_CTL_Q_SQ_CMD_TIMEOUT and never
changed, so just use the constant directly.

Suggested-by: Simon Horman <simon.horman@corigine.com>
Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/intel/ice/ice_common.c   | 2 +-
 drivers/net/ethernet/intel/ice/ice_controlq.c | 5 +----
 drivers/net/ethernet/intel/ice/ice_controlq.h | 1 -
 3 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index c2fda4fa4188..f4c256563248 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -2000,7 +2000,7 @@ void ice_release_res(struct ice_hw *hw, enum ice_aq_res_ids res)
 	/* there are some rare cases when trying to release the resource
 	 * results in an admin queue timeout, so handle them correctly
 	 */
-	while ((status == -EIO) && (total_delay < hw->adminq.sq_cmd_timeout)) {
+	while ((status == -EIO) && (total_delay < ICE_CTL_Q_SQ_CMD_TIMEOUT)) {
 		mdelay(1);
 		status = ice_aq_release_res(hw, res, 0, NULL);
 		total_delay++;
diff --git a/drivers/net/ethernet/intel/ice/ice_controlq.c b/drivers/net/ethernet/intel/ice/ice_controlq.c
index 6bcfee295991..c8fb10106ec3 100644
--- a/drivers/net/ethernet/intel/ice/ice_controlq.c
+++ b/drivers/net/ethernet/intel/ice/ice_controlq.c
@@ -637,9 +637,6 @@ static int ice_init_ctrlq(struct ice_hw *hw, enum ice_ctl_q q_type)
 		return -EIO;
 	}
 
-	/* setup SQ command write back timeout */
-	cq->sq_cmd_timeout = ICE_CTL_Q_SQ_CMD_TIMEOUT;
-
 	/* allocate the ATQ */
 	ret_code = ice_init_sq(hw, cq);
 	if (ret_code)
@@ -1066,7 +1063,7 @@ ice_sq_send_cmd(struct ice_hw *hw, struct ice_ctl_q_info *cq,
 
 		udelay(ICE_CTL_Q_SQ_CMD_USEC);
 		total_delay++;
-	} while (total_delay < cq->sq_cmd_timeout);
+	} while (total_delay < ICE_CTL_Q_SQ_CMD_TIMEOUT);
 
 	/* if ready, copy the desc back to temp */
 	if (ice_sq_done(hw, cq)) {
diff --git a/drivers/net/ethernet/intel/ice/ice_controlq.h b/drivers/net/ethernet/intel/ice/ice_controlq.h
index c07e9cc9fc6e..e790b2f4e437 100644
--- a/drivers/net/ethernet/intel/ice/ice_controlq.h
+++ b/drivers/net/ethernet/intel/ice/ice_controlq.h
@@ -87,7 +87,6 @@ struct ice_ctl_q_info {
 	enum ice_ctl_q qtype;
 	struct ice_ctl_q_ring rq;	/* receive queue */
 	struct ice_ctl_q_ring sq;	/* send queue */
-	u32 sq_cmd_timeout;		/* send queue cmd write back timeout */
 	u16 num_rq_entries;		/* receive queue depth */
 	u16 num_sq_entries;		/* send queue depth */
 	u16 rq_buf_size;		/* receive queue buffer size */
-- 
2.39.2

[PATCH net-next v2 4/6] ice: sleep, don't busy-wait, for ICE_CTL_Q_SQ_CMD_TIMEOUT

[Michal Schmidt]

The driver polls for ice_sq_done() with a 100 µs period for up to 1 s
and it uses udelay to do that.

Let's use usleep_range instead. We know sleeping is allowed here,
because we're holding a mutex (cq->sq_lock). To preserve the total
max waiting time, measure the timeout in jiffies.

ICE_CTL_Q_SQ_CMD_TIMEOUT is used also in ice_release_res(), but there
the polling period is 1 ms (i.e. 10 times longer). Since the timeout was
expressed in terms of the number of loops, the total timeout in this
function is 10 s. I do not know if this is intentional. This patch keeps
it.

The patch lowers the CPU usage of the ice-gnss-<dev_name> kernel thread
on my system from ~8 % to less than 1 %.

I received a report of high CPU usage with ptp4l where the busy-waiting
in ice_sq_send_cmd dominated the profile. This patch has been tested in
that usecase too and it made a huge improvement there.

Tested-by: Brent Rowsell <browsell@redhat.com>
Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/intel/ice/ice_common.c   | 14 +++++++-------
 drivers/net/ethernet/intel/ice/ice_controlq.c |  9 +++++----
 drivers/net/ethernet/intel/ice/ice_controlq.h |  2 +-
 3 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index f4c256563248..3638598d732b 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -1992,19 +1992,19 @@ ice_acquire_res(struct ice_hw *hw, enum ice_aq_res_ids res,
  */
 void ice_release_res(struct ice_hw *hw, enum ice_aq_res_ids res)
 {
-	u32 total_delay = 0;
+	unsigned long timeout;
 	int status;
 
-	status = ice_aq_release_res(hw, res, 0, NULL);
-
 	/* there are some rare cases when trying to release the resource
 	 * results in an admin queue timeout, so handle them correctly
 	 */
-	while ((status == -EIO) && (total_delay < ICE_CTL_Q_SQ_CMD_TIMEOUT)) {
-		mdelay(1);
+	timeout = jiffies + 10 * ICE_CTL_Q_SQ_CMD_TIMEOUT;
+	do {
 		status = ice_aq_release_res(hw, res, 0, NULL);
-		total_delay++;
-	}
+		if (status != -EIO)
+			break;
+		usleep_range(1000, 2000);
+	} while (time_before(jiffies, timeout));
 }
 
 /**
diff --git a/drivers/net/ethernet/intel/ice/ice_controlq.c b/drivers/net/ethernet/intel/ice/ice_controlq.c
index c8fb10106ec3..d2faf1baad2f 100644
--- a/drivers/net/ethernet/intel/ice/ice_controlq.c
+++ b/drivers/net/ethernet/intel/ice/ice_controlq.c
@@ -964,7 +964,7 @@ ice_sq_send_cmd(struct ice_hw *hw, struct ice_ctl_q_info *cq,
 	struct ice_aq_desc *desc_on_ring;
 	bool cmd_completed = false;
 	struct ice_sq_cd *details;
-	u32 total_delay = 0;
+	unsigned long timeout;
 	int status = 0;
 	u16 retval = 0;
 	u32 val = 0;
@@ -1057,13 +1057,14 @@ ice_sq_send_cmd(struct ice_hw *hw, struct ice_ctl_q_info *cq,
 		cq->sq.next_to_use = 0;
 	wr32(hw, cq->sq.tail, cq->sq.next_to_use);
 
+	timeout = jiffies + ICE_CTL_Q_SQ_CMD_TIMEOUT;
 	do {
 		if (ice_sq_done(hw, cq))
 			break;
 
-		udelay(ICE_CTL_Q_SQ_CMD_USEC);
-		total_delay++;
-	} while (total_delay < ICE_CTL_Q_SQ_CMD_TIMEOUT);
+		usleep_range(ICE_CTL_Q_SQ_CMD_USEC,
+			     ICE_CTL_Q_SQ_CMD_USEC * 3 / 2);
+	} while (time_before(jiffies, timeout));
 
 	/* if ready, copy the desc back to temp */
 	if (ice_sq_done(hw, cq)) {
diff --git a/drivers/net/ethernet/intel/ice/ice_controlq.h b/drivers/net/ethernet/intel/ice/ice_controlq.h
index e790b2f4e437..950b7f4a7a05 100644
--- a/drivers/net/ethernet/intel/ice/ice_controlq.h
+++ b/drivers/net/ethernet/intel/ice/ice_controlq.h
@@ -34,7 +34,7 @@ enum ice_ctl_q {
 };
 
 /* Control Queue timeout settings - max delay 1s */
-#define ICE_CTL_Q_SQ_CMD_TIMEOUT	10000 /* Count 10000 times */
+#define ICE_CTL_Q_SQ_CMD_TIMEOUT	HZ    /* Wait max 1s */
 #define ICE_CTL_Q_SQ_CMD_USEC		100   /* Check every 100usec */
 #define ICE_CTL_Q_ADMIN_INIT_TIMEOUT	10    /* Count 10 times */
 #define ICE_CTL_Q_ADMIN_INIT_MSEC	100   /* Check every 100msec */
-- 
2.39.2

[PATCH net-next v2 5/6] ice: remove unused buffer copy code in ice_sq_send_cmd_retry()

[Michal Schmidt]

The 'buf_cpy'-related code in ice_sq_send_cmd_retry() looks broken.
'buf' is nowhere copied into 'buf_cpy'.

The reason this does not cause problems is that all commands for which
'is_cmd_for_retry' is true go with a NULL buf.

Let's remove 'buf_cpy'. Add a WARN_ON in case the assumption no longer
holds in the future.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/intel/ice/ice_common.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 3638598d732b..c6200564304e 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -1619,7 +1619,6 @@ ice_sq_send_cmd_retry(struct ice_hw *hw, struct ice_ctl_q_info *cq,
 {
 	struct ice_aq_desc desc_cpy;
 	bool is_cmd_for_retry;
-	u8 *buf_cpy = NULL;
 	u8 idx = 0;
 	u16 opcode;
 	int status;
@@ -1629,11 +1628,8 @@ ice_sq_send_cmd_retry(struct ice_hw *hw, struct ice_ctl_q_info *cq,
 	memset(&desc_cpy, 0, sizeof(desc_cpy));
 
 	if (is_cmd_for_retry) {
-		if (buf) {
-			buf_cpy = kzalloc(buf_size, GFP_KERNEL);
-			if (!buf_cpy)
-				return -ENOMEM;
-		}
+		/* All retryable cmds are direct, without buf. */
+		WARN_ON(buf);
 
 		memcpy(&desc_cpy, desc, sizeof(desc_cpy));
 	}
@@ -1645,17 +1641,12 @@ ice_sq_send_cmd_retry(struct ice_hw *hw, struct ice_ctl_q_info *cq,
 		    hw->adminq.sq_last_status != ICE_AQ_RC_EBUSY)
 			break;
 
-		if (buf_cpy)
-			memcpy(buf, buf_cpy, buf_size);
-
 		memcpy(desc, &desc_cpy, sizeof(desc_cpy));
 
 		mdelay(ICE_SQ_SEND_DELAY_TIME_MS);
 
 	} while (++idx < ICE_SQ_SEND_MAX_EXECUTE);
 
-	kfree(buf_cpy);
-
 	return status;
 }
 
-- 
2.39.2

[PATCH net-next v2 6/6] ice: sleep, don't busy-wait, in the SQ send retry loop

[Michal Schmidt]

10 ms is a lot of time to spend busy-waiting. Sleeping is clearly
allowed here, because we have just returned from ice_sq_send_cmd(),
which takes a mutex.

On kernels with HZ=100, this msleep may be twice as long, but I don't
think it matters.
I did not actually observe any retries happening here.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/intel/ice/ice_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index c6200564304e..0157f6e98d3e 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -1643,7 +1643,7 @@ ice_sq_send_cmd_retry(struct ice_hw *hw, struct ice_ctl_q_info *cq,
 
 		memcpy(desc, &desc_cpy, sizeof(desc_cpy));
 
-		mdelay(ICE_SQ_SEND_DELAY_TIME_MS);
+		msleep(ICE_SQ_SEND_DELAY_TIME_MS);
 
 	} while (++idx < ICE_SQ_SEND_MAX_EXECUTE);
 
-- 
2.39.2

RE: [PATCH net-next v2 2/6] ice: increase the GNSS data polling interval to 20 ms

[Kubalewski, Arkadiusz]

>From: Michal Schmidt <mschmidt@redhat.com>
>Sent: Wednesday, April 12, 2023 10:19 AM
>
>Double the GNSS data polling interval from 10 ms to 20 ms.
>According to Karol Kolacinski from the Intel team, they have been
>planning to make this change.
>
>Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
>---
> drivers/net/ethernet/intel/ice/ice_gnss.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.h
>b/drivers/net/ethernet/intel/ice/ice_gnss.h
>index 640df7411373..b8bb8b63d081 100644
>--- a/drivers/net/ethernet/intel/ice/ice_gnss.h
>+++ b/drivers/net/ethernet/intel/ice/ice_gnss.h
>@@ -5,7 +5,7 @@
> #define _ICE_GNSS_H_
>
> #define ICE_E810T_GNSS_I2C_BUS		0x2
>-#define ICE_GNSS_POLL_DATA_DELAY_TIME	(HZ / 100) /* poll every 10 ms */
>+#define ICE_GNSS_POLL_DATA_DELAY_TIME	(HZ / 50) /* poll every 20 ms */
> #define ICE_GNSS_TIMER_DELAY_TIME	(HZ / 10) /* 0.1 second per message */
> #define ICE_GNSS_TTY_WRITE_BUF		250
> #define ICE_MAX_I2C_DATA_SIZE
>	FIELD_MAX(ICE_AQC_I2C_DATA_SIZE_M)
>--
>2.39.2

Looks good, thank you Michal!

Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>

RE: [PATCH net-next v2 5/6] ice: remove unused buffer copy code in ice_sq_send_cmd_retry()

[Kubalewski, Arkadiusz]

>From: Michal Schmidt <mschmidt@redhat.com>
>Sent: Wednesday, April 12, 2023 10:19 AM
>
>The 'buf_cpy'-related code in ice_sq_send_cmd_retry() looks broken.
>'buf' is nowhere copied into 'buf_cpy'.
>
>The reason this does not cause problems is that all commands for which
>'is_cmd_for_retry' is true go with a NULL buf.
>
>Let's remove 'buf_cpy'. Add a WARN_ON in case the assumption no longer
>holds in the future.
>
>Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
>---
> drivers/net/ethernet/intel/ice/ice_common.c | 13 ++-----------
> 1 file changed, 2 insertions(+), 11 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/ice/ice_common.c
>b/drivers/net/ethernet/intel/ice/ice_common.c
>index 3638598d732b..c6200564304e 100644
>--- a/drivers/net/ethernet/intel/ice/ice_common.c
>+++ b/drivers/net/ethernet/intel/ice/ice_common.c
>@@ -1619,7 +1619,6 @@ ice_sq_send_cmd_retry(struct ice_hw *hw, struct
>ice_ctl_q_info *cq,
> {
> 	struct ice_aq_desc desc_cpy;
> 	bool is_cmd_for_retry;
>-	u8 *buf_cpy = NULL;
> 	u8 idx = 0;
> 	u16 opcode;
> 	int status;
>@@ -1629,11 +1628,8 @@ ice_sq_send_cmd_retry(struct ice_hw *hw, struct
>ice_ctl_q_info *cq,
> 	memset(&desc_cpy, 0, sizeof(desc_cpy));
>
> 	if (is_cmd_for_retry) {
>-		if (buf) {
>-			buf_cpy = kzalloc(buf_size, GFP_KERNEL);
>-			if (!buf_cpy)
>-				return -ENOMEM;
>-		}
>+		/* All retryable cmds are direct, without buf. */
>+		WARN_ON(buf);
>
> 		memcpy(&desc_cpy, desc, sizeof(desc_cpy));
> 	}
>@@ -1645,17 +1641,12 @@ ice_sq_send_cmd_retry(struct ice_hw *hw, struct
>ice_ctl_q_info *cq,
> 		    hw->adminq.sq_last_status != ICE_AQ_RC_EBUSY)
> 			break;
>
>-		if (buf_cpy)
>-			memcpy(buf, buf_cpy, buf_size);
>-
> 		memcpy(desc, &desc_cpy, sizeof(desc_cpy));
>
> 		mdelay(ICE_SQ_SEND_DELAY_TIME_MS);
>
> 	} while (++idx < ICE_SQ_SEND_MAX_EXECUTE);
>
>-	kfree(buf_cpy);
>-
> 	return status;
> }
>
>--
>2.39.2

Looks good, thank you Michal!

Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>

RE: [PATCH net-next v2 6/6] ice: sleep, don't busy-wait, in the SQ send retry loop

[Kubalewski, Arkadiusz]

>From: Michal Schmidt <mschmidt@redhat.com>
>Sent: Wednesday, April 12, 2023 10:19 AM
>
>10 ms is a lot of time to spend busy-waiting. Sleeping is clearly
>allowed here, because we have just returned from ice_sq_send_cmd(),
>which takes a mutex.
>
>On kernels with HZ=100, this msleep may be twice as long, but I don't
>think it matters.
>I did not actually observe any retries happening here.
>
>Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
>---
> drivers/net/ethernet/intel/ice/ice_common.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/net/ethernet/intel/ice/ice_common.c
>b/drivers/net/ethernet/intel/ice/ice_common.c
>index c6200564304e..0157f6e98d3e 100644
>--- a/drivers/net/ethernet/intel/ice/ice_common.c
>+++ b/drivers/net/ethernet/intel/ice/ice_common.c
>@@ -1643,7 +1643,7 @@ ice_sq_send_cmd_retry(struct ice_hw *hw, struct
>ice_ctl_q_info *cq,
>
> 		memcpy(desc, &desc_cpy, sizeof(desc_cpy));
>
>-		mdelay(ICE_SQ_SEND_DELAY_TIME_MS);
>+		msleep(ICE_SQ_SEND_DELAY_TIME_MS);
>
> 	} while (++idx < ICE_SQ_SEND_MAX_EXECUTE);
>
>--
>2.39.2

Looks good, thank you Michal!

Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>

RE: [PATCH net-next v2 3/6] ice: remove ice_ctl_q_info::sq_cmd_timeout

[Kubalewski, Arkadiusz]

>From: Michal Schmidt <mschmidt@redhat.com>
>Sent: Wednesday, April 12, 2023 10:19 AM
>
>sq_cmd_timeout is initialized to ICE_CTL_Q_SQ_CMD_TIMEOUT and never
>changed, so just use the constant directly.
>
>Suggested-by: Simon Horman <simon.horman@corigine.com>
>Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
>---
> drivers/net/ethernet/intel/ice/ice_common.c   | 2 +-
> drivers/net/ethernet/intel/ice/ice_controlq.c | 5 +----
> drivers/net/ethernet/intel/ice/ice_controlq.h | 1 -
> 3 files changed, 2 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/ice/ice_common.c
>b/drivers/net/ethernet/intel/ice/ice_common.c
>index c2fda4fa4188..f4c256563248 100644
>--- a/drivers/net/ethernet/intel/ice/ice_common.c
>+++ b/drivers/net/ethernet/intel/ice/ice_common.c
>@@ -2000,7 +2000,7 @@ void ice_release_res(struct ice_hw *hw, enum
>ice_aq_res_ids res)
> 	/* there are some rare cases when trying to release the resource
> 	 * results in an admin queue timeout, so handle them correctly
> 	 */
>-	while ((status == -EIO) && (total_delay < hw->adminq.sq_cmd_timeout))
>{
>+	while ((status == -EIO) && (total_delay < ICE_CTL_Q_SQ_CMD_TIMEOUT))
>{
> 		mdelay(1);
> 		status = ice_aq_release_res(hw, res, 0, NULL);
> 		total_delay++;
>diff --git a/drivers/net/ethernet/intel/ice/ice_controlq.c
>b/drivers/net/ethernet/intel/ice/ice_controlq.c
>index 6bcfee295991..c8fb10106ec3 100644
>--- a/drivers/net/ethernet/intel/ice/ice_controlq.c
>+++ b/drivers/net/ethernet/intel/ice/ice_controlq.c
>@@ -637,9 +637,6 @@ static int ice_init_ctrlq(struct ice_hw *hw, enum
>ice_ctl_q q_type)
> 		return -EIO;
> 	}
>
>-	/* setup SQ command write back timeout */
>-	cq->sq_cmd_timeout = ICE_CTL_Q_SQ_CMD_TIMEOUT;
>-
> 	/* allocate the ATQ */
> 	ret_code = ice_init_sq(hw, cq);
> 	if (ret_code)
>@@ -1066,7 +1063,7 @@ ice_sq_send_cmd(struct ice_hw *hw, struct
>ice_ctl_q_info *cq,
>
> 		udelay(ICE_CTL_Q_SQ_CMD_USEC);
> 		total_delay++;
>-	} while (total_delay < cq->sq_cmd_timeout);
>+	} while (total_delay < ICE_CTL_Q_SQ_CMD_TIMEOUT);
>
> 	/* if ready, copy the desc back to temp */
> 	if (ice_sq_done(hw, cq)) {
>diff --git a/drivers/net/ethernet/intel/ice/ice_controlq.h
>b/drivers/net/ethernet/intel/ice/ice_controlq.h
>index c07e9cc9fc6e..e790b2f4e437 100644
>--- a/drivers/net/ethernet/intel/ice/ice_controlq.h
>+++ b/drivers/net/ethernet/intel/ice/ice_controlq.h
>@@ -87,7 +87,6 @@ struct ice_ctl_q_info {
> 	enum ice_ctl_q qtype;
> 	struct ice_ctl_q_ring rq;	/* receive queue */
> 	struct ice_ctl_q_ring sq;	/* send queue */
>-	u32 sq_cmd_timeout;		/* send queue cmd write back timeout */
> 	u16 num_rq_entries;		/* receive queue depth */
> 	u16 num_sq_entries;		/* send queue depth */
> 	u16 rq_buf_size;		/* receive queue buffer size */
>--
>2.39.2

Looks good, thank you Michal!

Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>

RE: [PATCH net-next v2 1/6] ice: do not busy-wait to read GNSS data

[Kubalewski, Arkadiusz]

>From: Michal Schmidt <mschmidt@redhat.com>
>Sent: Wednesday, April 12, 2023 10:19 AM
>
>The ice-gnss-<dev_name> kernel thread, which reads data from the u-blox
>GNSS module, keep a CPU core almost 100% busy. The main reason is that
>it busy-waits for data to become available.
>
>A simple improvement would be to replace the "mdelay(10);" in
>ice_gnss_read() with sleeping. A better fix is to not do any waiting
>directly in the function and just requeue this delayed work as needed.
>The advantage is that canceling the work from ice_gnss_exit() becomes
>immediate, rather than taking up to ~2.5 seconds (ICE_MAX_UBX_READ_TRIES
>* 10 ms).
>
>This lowers the CPU usage of the ice-gnss-<dev_name> thread on my system
>from ~90 % to ~8 %.
>
>I am not sure if the larger 0.1 s pause after inserting data into the
>gnss subsystem is really necessary, but I'm keeping that as it was.
>
>Of course, ideally the driver would not have to poll at all, but I don't
>know if the E810 can watch for GNSS data availability over the i2c bus
>by itself and notify the driver.
>
>Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
>---
> drivers/net/ethernet/intel/ice/ice_gnss.c | 42 ++++++++++-------------
> drivers/net/ethernet/intel/ice/ice_gnss.h |  3 +-
> 2 files changed, 20 insertions(+), 25 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c
>b/drivers/net/ethernet/intel/ice/ice_gnss.c
>index 8dec748bb53a..2ea8a2b11bcd 100644
>--- a/drivers/net/ethernet/intel/ice/ice_gnss.c
>+++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
>@@ -117,6 +117,7 @@ static void ice_gnss_read(struct kthread_work *work)
> {
> 	struct gnss_serial *gnss = container_of(work, struct gnss_serial,
> 						read_work.work);
>+	unsigned long delay = ICE_GNSS_POLL_DATA_DELAY_TIME;
> 	unsigned int i, bytes_read, data_len, count;
> 	struct ice_aqc_link_topo_addr link_topo;
> 	struct ice_pf *pf;
>@@ -136,11 +137,6 @@ static void ice_gnss_read(struct kthread_work *work)
> 		return;
>
> 	hw = &pf->hw;
>-	buf = (char *)get_zeroed_page(GFP_KERNEL);
>-	if (!buf) {
>-		err = -ENOMEM;
>-		goto exit;
>-	}
>
> 	memset(&link_topo, 0, sizeof(struct ice_aqc_link_topo_addr));
> 	link_topo.topo_params.index = ICE_E810T_GNSS_I2C_BUS;
>@@ -151,25 +147,24 @@ static void ice_gnss_read(struct kthread_work *work)
> 	i2c_params = ICE_GNSS_UBX_DATA_LEN_WIDTH |
> 		     ICE_AQC_I2C_USE_REPEATED_START;
>
>-	/* Read data length in a loop, when it's not 0 the data is ready */
>-	for (i = 0; i < ICE_MAX_UBX_READ_TRIES; i++) {
>-		err = ice_aq_read_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
>-				      cpu_to_le16(ICE_GNSS_UBX_DATA_LEN_H),
>-				      i2c_params, (u8 *)&data_len_b, NULL);
>-		if (err)
>-			goto exit_buf;
>+	err = ice_aq_read_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
>+			      cpu_to_le16(ICE_GNSS_UBX_DATA_LEN_H),
>+			      i2c_params, (u8 *)&data_len_b, NULL);
>+	if (err)
>+		goto requeue;
>
>-		data_len = be16_to_cpu(data_len_b);
>-		if (data_len != 0 && data_len != U16_MAX)
>-			break;
>+	data_len = be16_to_cpu(data_len_b);
>+	if (data_len == 0 || data_len == U16_MAX)
>+		goto requeue;
>
>-		mdelay(10);
>-	}
>+	/* The u-blox has data_len bytes for us to read */
>
> 	data_len = min_t(typeof(data_len), data_len, PAGE_SIZE);
>-	if (!data_len) {
>+
>+	buf = (char *)get_zeroed_page(GFP_KERNEL);
>+	if (!buf) {
> 		err = -ENOMEM;
>-		goto exit_buf;
>+		goto requeue;
> 	}
>
> 	/* Read received data */
>@@ -183,7 +178,7 @@ static void ice_gnss_read(struct kthread_work *work)
> 				      cpu_to_le16(ICE_GNSS_UBX_EMPTY_DATA),
> 				      bytes_read, &buf[i], NULL);
> 		if (err)
>-			goto exit_buf;
>+			goto free_buf;
> 	}
>
> 	count = gnss_insert_raw(pf->gnss_dev, buf, i);
>@@ -191,10 +186,11 @@ static void ice_gnss_read(struct kthread_work *work)
> 		dev_warn(ice_pf_to_dev(pf),
> 			 "gnss_insert_raw ret=%d size=%d\n",
> 			 count, i);
>-exit_buf:
>+	delay = ICE_GNSS_TIMER_DELAY_TIME;
>+free_buf:
> 	free_page((unsigned long)buf);
>-	kthread_queue_delayed_work(gnss->kworker, &gnss->read_work,
>-				   ICE_GNSS_TIMER_DELAY_TIME);
>+requeue:
>+	kthread_queue_delayed_work(gnss->kworker, &gnss->read_work, delay);
> exit:
> 	if (err)
> 		dev_dbg(ice_pf_to_dev(pf), "GNSS failed to read err=%d\n",
>err);
>diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.h
>b/drivers/net/ethernet/intel/ice/ice_gnss.h
>index 4d49e5b0b4b8..640df7411373 100644
>--- a/drivers/net/ethernet/intel/ice/ice_gnss.h
>+++ b/drivers/net/ethernet/intel/ice/ice_gnss.h
>@@ -5,6 +5,7 @@
> #define _ICE_GNSS_H_
>
> #define ICE_E810T_GNSS_I2C_BUS		0x2
>+#define ICE_GNSS_POLL_DATA_DELAY_TIME	(HZ / 100) /* poll every 10 ms */
> #define ICE_GNSS_TIMER_DELAY_TIME	(HZ / 10) /* 0.1 second per message */
> #define ICE_GNSS_TTY_WRITE_BUF		250
> #define ICE_MAX_I2C_DATA_SIZE
>	FIELD_MAX(ICE_AQC_I2C_DATA_SIZE_M)
>@@ -20,8 +21,6 @@
>  * passed as I2C addr parameter.
>  */
> #define ICE_GNSS_UBX_WRITE_BYTES	(ICE_MAX_I2C_WRITE_BYTES + 1)
>-#define ICE_MAX_UBX_READ_TRIES		255
>-#define ICE_MAX_UBX_ACK_READ_TRIES	4095
>
> struct gnss_write_buf {
> 	struct list_head queue;
>--
>2.39.2

Looks good, thank you Michal!

Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>

RE: [PATCH net-next v2 4/6] ice: sleep, don't busy-wait, for ICE_CTL_Q_SQ_CMD_TIMEOUT

[Kubalewski, Arkadiusz]

>From: Michal Schmidt <mschmidt@redhat.com>
>Sent: Wednesday, April 12, 2023 10:19 AM
>
>The driver polls for ice_sq_done() with a 100 µs period for up to 1 s
>and it uses udelay to do that.
>
>Let's use usleep_range instead. We know sleeping is allowed here,
>because we're holding a mutex (cq->sq_lock). To preserve the total
>max waiting time, measure the timeout in jiffies.
>
>ICE_CTL_Q_SQ_CMD_TIMEOUT is used also in ice_release_res(), but there
>the polling period is 1 ms (i.e. 10 times longer). Since the timeout was
>expressed in terms of the number of loops, the total timeout in this
>function is 10 s. I do not know if this is intentional. This patch keeps
>it.
>
>The patch lowers the CPU usage of the ice-gnss-<dev_name> kernel thread
>on my system from ~8 % to less than 1 %.
>
>I received a report of high CPU usage with ptp4l where the busy-waiting
>in ice_sq_send_cmd dominated the profile. This patch has been tested in
>that usecase too and it made a huge improvement there.
>
>Tested-by: Brent Rowsell <browsell@redhat.com>
>Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
>---
> drivers/net/ethernet/intel/ice/ice_common.c   | 14 +++++++-------
> drivers/net/ethernet/intel/ice/ice_controlq.c |  9 +++++----
> drivers/net/ethernet/intel/ice/ice_controlq.h |  2 +-
> 3 files changed, 13 insertions(+), 12 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/ice/ice_common.c
>b/drivers/net/ethernet/intel/ice/ice_common.c
>index f4c256563248..3638598d732b 100644
>--- a/drivers/net/ethernet/intel/ice/ice_common.c
>+++ b/drivers/net/ethernet/intel/ice/ice_common.c
>@@ -1992,19 +1992,19 @@ ice_acquire_res(struct ice_hw *hw, enum
>ice_aq_res_ids res,
>  */
> void ice_release_res(struct ice_hw *hw, enum ice_aq_res_ids res)
> {
>-	u32 total_delay = 0;
>+	unsigned long timeout;
> 	int status;
>
>-	status = ice_aq_release_res(hw, res, 0, NULL);
>-
> 	/* there are some rare cases when trying to release the resource
> 	 * results in an admin queue timeout, so handle them correctly
> 	 */
>-	while ((status == -EIO) && (total_delay < ICE_CTL_Q_SQ_CMD_TIMEOUT))
>{
>-		mdelay(1);
>+	timeout = jiffies + 10 * ICE_CTL_Q_SQ_CMD_TIMEOUT;
>+	do {
> 		status = ice_aq_release_res(hw, res, 0, NULL);
>-		total_delay++;
>-	}
>+		if (status != -EIO)
>+			break;
>+		usleep_range(1000, 2000);
>+	} while (time_before(jiffies, timeout));
> }
>
> /**
>diff --git a/drivers/net/ethernet/intel/ice/ice_controlq.c
>b/drivers/net/ethernet/intel/ice/ice_controlq.c
>index c8fb10106ec3..d2faf1baad2f 100644
>--- a/drivers/net/ethernet/intel/ice/ice_controlq.c
>+++ b/drivers/net/ethernet/intel/ice/ice_controlq.c
>@@ -964,7 +964,7 @@ ice_sq_send_cmd(struct ice_hw *hw, struct
>ice_ctl_q_info *cq,
> 	struct ice_aq_desc *desc_on_ring;
> 	bool cmd_completed = false;
> 	struct ice_sq_cd *details;
>-	u32 total_delay = 0;
>+	unsigned long timeout;
> 	int status = 0;
> 	u16 retval = 0;
> 	u32 val = 0;
>@@ -1057,13 +1057,14 @@ ice_sq_send_cmd(struct ice_hw *hw, struct
>ice_ctl_q_info *cq,
> 		cq->sq.next_to_use = 0;
> 	wr32(hw, cq->sq.tail, cq->sq.next_to_use);
>
>+	timeout = jiffies + ICE_CTL_Q_SQ_CMD_TIMEOUT;
> 	do {
> 		if (ice_sq_done(hw, cq))
> 			break;
>
>-		udelay(ICE_CTL_Q_SQ_CMD_USEC);
>-		total_delay++;
>-	} while (total_delay < ICE_CTL_Q_SQ_CMD_TIMEOUT);
>+		usleep_range(ICE_CTL_Q_SQ_CMD_USEC,
>+			     ICE_CTL_Q_SQ_CMD_USEC * 3 / 2);
>+	} while (time_before(jiffies, timeout));
>
> 	/* if ready, copy the desc back to temp */
> 	if (ice_sq_done(hw, cq)) {
>diff --git a/drivers/net/ethernet/intel/ice/ice_controlq.h
>b/drivers/net/ethernet/intel/ice/ice_controlq.h
>index e790b2f4e437..950b7f4a7a05 100644
>--- a/drivers/net/ethernet/intel/ice/ice_controlq.h
>+++ b/drivers/net/ethernet/intel/ice/ice_controlq.h
>@@ -34,7 +34,7 @@ enum ice_ctl_q {
> };
>
> /* Control Queue timeout settings - max delay 1s */
>-#define ICE_CTL_Q_SQ_CMD_TIMEOUT	10000 /* Count 10000 times */
>+#define ICE_CTL_Q_SQ_CMD_TIMEOUT	HZ    /* Wait max 1s */
> #define ICE_CTL_Q_SQ_CMD_USEC		100   /* Check every 100usec */
> #define ICE_CTL_Q_ADMIN_INIT_TIMEOUT	10    /* Count 10 times */
> #define ICE_CTL_Q_ADMIN_INIT_MSEC	100   /* Check every 100msec */
>--
>2.39.2

Looks good, thank you Michal!

Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>

RE: [PATCH net-next v2 0/6] ice: lower CPU usage with GNSS

[Kubalewski, Arkadiusz]

>From: Michal Schmidt <mschmidt@redhat.com>
>Sent: Wednesday, April 12, 2023 10:19 AM
>
>This series lowers the CPU usage of the ice driver when using its
>provided /dev/gnss*.
>
>v2:
> - Changed subject of patch 1. Requested by Andrew Lunn.
> - Added patch 2 to change the polling interval as recommended by Intel.
> - Added patch 3 to remove sq_cmd_timeout as suggested by Simon Horman.
>
>Michal Schmidt (6):
>  ice: do not busy-wait to read GNSS data
>  ice: increase the GNSS data polling interval to 20 ms
>  ice: remove ice_ctl_q_info::sq_cmd_timeout
>  ice: sleep, don't busy-wait, for ICE_CTL_Q_SQ_CMD_TIMEOUT
>  ice: remove unused buffer copy code in ice_sq_send_cmd_retry()
>  ice: sleep, don't busy-wait, in the SQ send retry loop
>
> drivers/net/ethernet/intel/ice/ice_common.c   | 29 +++++--------
> drivers/net/ethernet/intel/ice/ice_controlq.c | 12 +++---
> drivers/net/ethernet/intel/ice/ice_controlq.h |  3 +-
> drivers/net/ethernet/intel/ice/ice_gnss.c     | 42 +++++++++----------
> drivers/net/ethernet/intel/ice/ice_gnss.h     |  3 +-
> 5 files changed, 36 insertions(+), 53 deletions(-)
>
>--
>2.39.2

In general I couldn't find any issues with the series.
Thank you all for suggestions and your work on this.

Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>

Re: [PATCH net-next v2 3/6] ice: remove ice_ctl_q_info::sq_cmd_timeout

[Simon Horman]

On Wed, Apr 12, 2023 at 10:19:26AM +0200, Michal Schmidt wrote:
> sq_cmd_timeout is initialized to ICE_CTL_Q_SQ_CMD_TIMEOUT and never
> changed, so just use the constant directly.
> 
> Suggested-by: Simon Horman <simon.horman@corigine.com>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>

LGTM, thanks.

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next v2 1/6] ice: do not busy-wait to read GNSS data

[Simon Horman]

On Wed, Apr 12, 2023 at 10:19:24AM +0200, Michal Schmidt wrote:
> The ice-gnss-<dev_name> kernel thread, which reads data from the u-blox
> GNSS module, keep a CPU core almost 100% busy. The main reason is that
> it busy-waits for data to become available.
> 
> A simple improvement would be to replace the "mdelay(10);" in
> ice_gnss_read() with sleeping. A better fix is to not do any waiting
> directly in the function and just requeue this delayed work as needed.
> The advantage is that canceling the work from ice_gnss_exit() becomes
> immediate, rather than taking up to ~2.5 seconds (ICE_MAX_UBX_READ_TRIES
> * 10 ms).
> 
> This lowers the CPU usage of the ice-gnss-<dev_name> thread on my system
> from ~90 % to ~8 %.
> 
> I am not sure if the larger 0.1 s pause after inserting data into the
> gnss subsystem is really necessary, but I'm keeping that as it was.
> 
> Of course, ideally the driver would not have to poll at all, but I don't
> know if the E810 can watch for GNSS data availability over the i2c bus
> by itself and notify the driver.
> 
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next v2 2/6] ice: increase the GNSS data polling interval to 20 ms

[Simon Horman]

On Wed, Apr 12, 2023 at 10:19:25AM +0200, Michal Schmidt wrote:
> Double the GNSS data polling interval from 10 ms to 20 ms.
> According to Karol Kolacinski from the Intel team, they have been
> planning to make this change.
> 
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next v2 4/6] ice: sleep, don't busy-wait, for ICE_CTL_Q_SQ_CMD_TIMEOUT

[Simon Horman]

On Wed, Apr 12, 2023 at 10:19:27AM +0200, Michal Schmidt wrote:
> The driver polls for ice_sq_done() with a 100 µs period for up to 1 s
> and it uses udelay to do that.
> 
> Let's use usleep_range instead. We know sleeping is allowed here,
> because we're holding a mutex (cq->sq_lock). To preserve the total
> max waiting time, measure the timeout in jiffies.
> 
> ICE_CTL_Q_SQ_CMD_TIMEOUT is used also in ice_release_res(), but there
> the polling period is 1 ms (i.e. 10 times longer). Since the timeout was
> expressed in terms of the number of loops, the total timeout in this
> function is 10 s. I do not know if this is intentional. This patch keeps
> it.
> 
> The patch lowers the CPU usage of the ice-gnss-<dev_name> kernel thread
> on my system from ~8 % to less than 1 %.
> 
> I received a report of high CPU usage with ptp4l where the busy-waiting
> in ice_sq_send_cmd dominated the profile. This patch has been tested in
> that usecase too and it made a huge improvement there.
> 
> Tested-by: Brent Rowsell <browsell@redhat.com>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next v2 5/6] ice: remove unused buffer copy code in ice_sq_send_cmd_retry()

[Simon Horman]

On Wed, Apr 12, 2023 at 10:19:28AM +0200, Michal Schmidt wrote:
> The 'buf_cpy'-related code in ice_sq_send_cmd_retry() looks broken.
> 'buf' is nowhere copied into 'buf_cpy'.
> 
> The reason this does not cause problems is that all commands for which
> 'is_cmd_for_retry' is true go with a NULL buf.
> 
> Let's remove 'buf_cpy'. Add a WARN_ON in case the assumption no longer
> holds in the future.
> 
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next v2 6/6] ice: sleep, don't busy-wait, in the SQ send retry loop

[Simon Horman]

On Wed, Apr 12, 2023 at 10:19:29AM +0200, Michal Schmidt wrote:
> 10 ms is a lot of time to spend busy-waiting. Sleeping is clearly
> allowed here, because we have just returned from ice_sq_send_cmd(),
> which takes a mutex.
> 
> On kernels with HZ=100, this msleep may be twice as long, but I don't
> think it matters.
> I did not actually observe any retries happening here.
> 
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

RE: [Intel-wired-lan] [PATCH net-next v2 1/6] ice: do not busy-wait to read GNSS data

[Mekala, SunithaX D]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Michal Schmidt
> Sent: Wednesday, April 12, 2023 1:19 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Andrew Lunn <andrew@lunn.ch>; netdev@vger.kernel.org; Brandeburg, Jesse <jesse.brandeburg@intel.com>; Kolacinski, Karol <karol.kolacinski@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Simon Horman <simon.horman@corigine.com>
> Subject: [Intel-wired-lan] [PATCH net-next v2 1/6] ice: do not busy-wait to read GNSS data
>
> The ice-gnss-<dev_name> kernel thread, which reads data from the u-blox GNSS module, keep a CPU core almost 100% busy. The main reason is that it busy-waits for data to become available.
>
> A simple improvement would be to replace the "mdelay(10);" in
ice_gnss_read() with sleeping. A better fix is to not do any waiting directly in the function and just requeue this delayed work as needed.
The advantage is that canceling the work from ice_gnss_exit() becomes immediate, rather than taking up to ~2.5 seconds (ICE_MAX_UBX_READ_TRIES
* 10 ms).
>
> This lowers the CPU usage of the ice-gnss-<dev_name> thread on my system from ~90 % to ~8 %.
>
> I am not sure if the larger 0.1 s pause after inserting data into the gnss subsystem is really necessary, but I'm keeping that as it was.
> 
> Of course, ideally the driver would not have to poll at all, but I don't know if the E810 can watch for GNSS data availability over the i2c bus by itself and notify the driver.
>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
> drivers/net/ethernet/intel/ice/ice_gnss.c | 42 ++++++++++-------------  drivers/net/ethernet/intel/ice/ice_gnss.h |  3 +-
> 2 files changed, 20 insertions(+), 25 deletions(-)
>
Tested-by: Sunitha Mekala <sunithax.d.mekala@intel.com> (A Contingent worker at Intel)

RE: [Intel-wired-lan] [PATCH net-next v2 2/6] ice: increase the GNSS data polling interval to 20 ms

[Mekala, SunithaX D]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Michal Schmidt
> Sent: Wednesday, April 12, 2023 1:19 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Andrew Lunn <andrew@lunn.ch>; netdev@vger.kernel.org; Brandeburg, Jesse <jesse.brandeburg@intel.com>; Kolacinski, Karol <karol.kolacinski@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Simon Horman <simon.horman@corigine.com>
> Subject: [Intel-wired-lan] [PATCH net-next v2 2/6] ice: increase the GNSS data polling interval to 20 ms
>
> Double the GNSS data polling interval from 10 ms to 20 ms.
> According to Karol Kolacinski from the Intel team, they have been planning to make this change.
>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_gnss.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
Tested-by: Sunitha Mekala <sunithax.d.mekala@intel.com> (A Contingent worker at Intel) 

RE: [Intel-wired-lan] [PATCH net-next v2 3/6] ice: remove ice_ctl_q_info::sq_cmd_timeout

[Mekala, SunithaX D]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Michal Schmidt
> Sent: Wednesday, April 12, 2023 1:19 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Andrew Lunn <andrew@lunn.ch>; netdev@vger.kernel.org; Brandeburg, Jesse <jesse.brandeburg@intel.com>; Kolacinski, Karol <karol.kolacinski@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Simon Horman <simon.horman@corigine.com>
> Subject: [Intel-wired-lan] [PATCH net-next v2 3/6] ice: remove ice_ctl_q_info::sq_cmd_timeout
>
> sq_cmd_timeout is initialized to ICE_CTL_Q_SQ_CMD_TIMEOUT and never changed, so just use the constant directly.
>
> Suggested-by: Simon Horman <simon.horman@corigine.com>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_common.c   | 2 +-
>  drivers/net/ethernet/intel/ice/ice_controlq.c | 5 +----  drivers/net/ethernet/intel/ice/ice_controlq.h | 1 -
>  3 files changed, 2 insertions(+), 6 deletions(-)
>
Tested-by: Sunitha Mekala <sunithax.d.mekala@intel.com> (A Contingent worker at Intel)

RE: [Intel-wired-lan] [PATCH net-next v2 0/6] ice: lower CPU usage with GNSS

[Mekala, SunithaX D]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Michal Schmidt
> Sent: Wednesday, April 12, 2023 1:19 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Andrew Lunn <andrew@lunn.ch>; netdev@vger.kernel.org; Brandeburg, Jesse <jesse.brandeburg@intel.com>; Kolacinski, Karol <karol.kolacinski@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Simon Horman <simon.horman@corigine.com>
> Subject: [Intel-wired-lan] [PATCH net-next v2 0/6] ice: lower CPU usage with GNSS
>
> This series lowers the CPU usage of the ice driver when using its provided /dev/gnss*.
>
> v2:
>  - Changed subject of patch 1. Requested by Andrew Lunn.
>  - Added patch 2 to change the polling interval as recommended by Intel.
>  - Added patch 3 to remove sq_cmd_timeout as suggested by Simon Horman.
>
> Michal Schmidt (6):
>  ice: do not busy-wait to read GNSS data
>   ice: increase the GNSS data polling interval to 20 ms
>   ice: remove ice_ctl_q_info::sq_cmd_timeout
>   ice: sleep, don't busy-wait, for ICE_CTL_Q_SQ_CMD_TIMEOUT
>   ice: remove unused buffer copy code in ice_sq_send_cmd_retry()
>   ice: sleep, don't busy-wait, in the SQ send retry loop
>
>  drivers/net/ethernet/intel/ice/ice_common.c   | 29 +++++--------
>  drivers/net/ethernet/intel/ice/ice_controlq.c | 12 +++---  drivers/net/ethernet/intel/ice/ice_controlq.h |  3 +-
>  drivers/net/ethernet/intel/ice/ice_gnss.c     | 42 +++++++++----------
>  drivers/net/ethernet/intel/ice/ice_gnss.h     |  3 +-
>  5 files changed, 36 insertions(+), 53 deletions(-)
>
Tested-by: Sunitha Mekala <sunithax.d.mekala@intel.com> (A Contingent worker at Intel) 

RE: [Intel-wired-lan] [PATCH net-next v2 5/6] ice: remove unused buffer copy code in ice_sq_send_cmd_retry()

[Mekala, SunithaX D]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Michal Schmidt
> Sent: Wednesday, April 12, 2023 1:19 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Andrew Lunn <andrew@lunn.ch>; netdev@vger.kernel.org; Brandeburg, Jesse <jesse.brandeburg@intel.com>; Kolacinski, Karol <karol.kolacinski@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Simon Horman <simon.horman@corigine.com>
> Subject: [Intel-wired-lan] [PATCH net-next v2 5/6] ice: remove unused buffer copy code in ice_sq_send_cmd_retry()
>
> The 'buf_cpy'-related code in ice_sq_send_cmd_retry() looks broken.
> 'buf' is nowhere copied into 'buf_cpy'.
>
> The reason this does not cause problems is that all commands for which 'is_cmd_for_retry' is true go with a NULL buf.
>
> Let's remove 'buf_cpy'. Add a WARN_ON in case the assumption no longer holds in the future.
>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_common.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
>
Tested-by: Sunitha Mekala <sunithax.d.mekala@intel.com> (A Contingent worker at Intel)

RE: [Intel-wired-lan] [PATCH net-next v2 6/6] ice: sleep, don't busy-wait, in the SQ send retry loop

[Mekala, SunithaX D]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Michal Schmidt
> Sent: Wednesday, April 12, 2023 1:19 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Andrew Lunn <andrew@lunn.ch>; netdev@vger.kernel.org; Brandeburg, Jesse <jesse.brandeburg@intel.com>; Kolacinski, Karol <karol.kolacinski@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Simon Horman <simon.horman@corigine.com>
> Subject: [Intel-wired-lan] [PATCH net-next v2 6/6] ice: sleep, don't busy-wait, in the SQ send retry loop
>
> 10 ms is a lot of time to spend busy-waiting. Sleeping is clearly allowed here, because we have just returned from ice_sq_send_cmd(), which takes a mutex.
>
> On kernels with HZ=100, this msleep may be twice as long, but I don't think it matters.
> I did not actually observe any retries happening here.
>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
Tested-by: Sunitha Mekala <sunithax.d.mekala@intel.com> (A Contingent worker at Intel)

RE: [Intel-wired-lan] [PATCH net-next v2 4/6] ice: sleep, don't busy-wait, for ICE_CTL_Q_SQ_CMD_TIMEOUT

[Mekala, SunithaX D]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Michal Schmidt
> Sent: Wednesday, April 12, 2023 1:19 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Brent Rowsell <browsell@redhat.com>; Andrew Lunn <andrew@lunn.ch>; netdev@vger.kernel.org; Brandeburg, Jesse <jesse.brandeburg@intel.com>; Kolacinski, Karol <karol.kolacinski@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Simon Horman <simon.horman@corigine.com>
> Subject: [Intel-wired-lan] [PATCH net-next v2 4/6] ice: sleep, don't busy-wait, for ICE_CTL_Q_SQ_CMD_TIMEOUT
>
> The driver polls for ice_sq_done() with a 100 µs period for up to 1 s and it uses udelay to do that.
>
> Let's use usleep_range instead. We know sleeping is allowed here, because we're holding a mutex (cq->sq_lock). To preserve the total max waiting time, measure the timeout in jiffies.
>
> ICE_CTL_Q_SQ_CMD_TIMEOUT is used also in ice_release_res(), but there the polling period is 1 ms (i.e. 10 times longer). Since the timeout was expressed in terms of the number of loops, the total timeout in this function is 10 s. I do not know if this is intentional. This patch keeps it.
>
> The patch lowers the CPU usage of the ice-gnss-<dev_name> kernel thread on my system from ~8 % to less than 1 %.
>
> I received a report of high CPU usage with ptp4l where the busy-waiting in ice_sq_send_cmd dominated the profile. This patch has been tested in that usecase too and it made a huge improvement there.
>
> Tested-by: Brent Rowsell <browsell@redhat.com>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
 > drivers/net/ethernet/intel/ice/ice_common.c   | 14 +++++++-------
 > drivers/net/ethernet/intel/ice/ice_controlq.c |  9 +++++----  drivers/net/ethernet/intel/ice/ice_controlq.h |  2 +-
 > 3 files changed, 13 insertions(+), 12 deletions(-)
> 
Tested-by: Sunitha Mekala <sunithax.d.mekala@intel.com> (A Contingent worker at Intel)