[PATCH iwl-next 0/3] ice: GNSS reading improvements

[PATCH iwl-next 0/3] ice: GNSS reading improvements

[Michal Schmidt]

This improves the reading of GNSS data. The main change is the lower
latency for received GNSS messages, which helps ts2phc do its job.

Michal Schmidt (3):
  ice: downgrade warning about gnss_insert_raw to debug level
  ice: lower the latency of GNSS reads
  ice: remove special delay after processing a GNSS read batch

 drivers/net/ethernet/intel/ice/ice_gnss.c | 34 ++++++++---------------
 drivers/net/ethernet/intel/ice/ice_gnss.h |  7 +++--
 2 files changed, 16 insertions(+), 25 deletions(-)

-- 
2.47.1

[PATCH iwl-next 1/3] ice: downgrade warning about gnss_insert_raw to debug level

[Michal Schmidt]

gnss_insert_raw() will reject the GNSS data the ice driver produces
whenever userspace has the gnss device open, but is not reading it fast
enough for whatever reason.

Do not spam kernel logs just because userspace misbehaves.

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

diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
index 66390eeb2343..9b1f970f4825 100644
--- a/drivers/net/ethernet/intel/ice/ice_gnss.c
+++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
@@ -146,9 +146,9 @@ static void ice_gnss_read(struct kthread_work *work)
 
 	count = gnss_insert_raw(pf->gnss_dev, buf, i);
 	if (count != i)
-		dev_warn(ice_pf_to_dev(pf),
-			 "gnss_insert_raw ret=%d size=%d\n",
-			 count, i);
+		dev_dbg(ice_pf_to_dev(pf),
+			"gnss_insert_raw ret=%d size=%d\n",
+			count, i);
 	delay = ICE_GNSS_TIMER_DELAY_TIME;
 free_buf:
 	free_page((unsigned long)buf);
-- 
2.47.1

[PATCH iwl-next 2/3] ice: lower the latency of GNSS reads

[Michal Schmidt]

The E810 is connected to the u-blox GNSS module over I2C. The ice driver
periodically (every ~20ms) sends AdminQ commands to poll the u-blox for
available data. Most of the time, there's no data. When the u-blox
finally responds that data is available, usually it's around 800 bytes.
It can be more or less, depending on how many NMEA messages were
configured using ubxtool. ice then proceeds to read all the data.
AdminQ and I2C are slow. The reading is performed in chunks of 15 bytes.
ice reads all of the data before passing it to the kernel GNSS subsystem
and onwards to userspace.

Improve the NMEA message receiving latency. Pass each 15-bytes chunk to
userspace as soon as it's received.

Tested-by: Miroslav Lichvar <mlichvar@redhat.com>
Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/intel/ice/ice_gnss.c | 29 +++++++----------------
 drivers/net/ethernet/intel/ice/ice_gnss.h |  6 ++++-
 2 files changed, 14 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
index 9b1f970f4825..7922311d2545 100644
--- a/drivers/net/ethernet/intel/ice/ice_gnss.c
+++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
@@ -88,10 +88,10 @@ static void ice_gnss_read(struct kthread_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;
+	char buf[ICE_MAX_I2C_DATA_SIZE];
 	struct ice_pf *pf;
 	struct ice_hw *hw;
 	__be16 data_len_b;
-	char *buf = NULL;
 	u8 i2c_params;
 	int err = 0;
 
@@ -121,16 +121,6 @@ static void ice_gnss_read(struct kthread_work *work)
 		goto requeue;
 
 	/* The u-blox has data_len bytes for us to read */
-
-	data_len = min_t(typeof(data_len), data_len, PAGE_SIZE);
-
-	buf = (char *)get_zeroed_page(GFP_KERNEL);
-	if (!buf) {
-		err = -ENOMEM;
-		goto requeue;
-	}
-
-	/* Read received data */
 	for (i = 0; i < data_len; i += bytes_read) {
 		unsigned int bytes_left = data_len - i;
 
@@ -139,19 +129,18 @@ static void ice_gnss_read(struct kthread_work *work)
 
 		err = ice_aq_read_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
 				      cpu_to_le16(ICE_GNSS_UBX_EMPTY_DATA),
-				      bytes_read, &buf[i], NULL);
+				      bytes_read, buf, NULL);
 		if (err)
-			goto free_buf;
+			goto requeue;
+
+		count = gnss_insert_raw(pf->gnss_dev, buf, bytes_read);
+		if (count != bytes_read)
+			dev_dbg(ice_pf_to_dev(pf),
+				"gnss_insert_raw ret=%d size=%d\n",
+				count, bytes_read);
 	}
 
-	count = gnss_insert_raw(pf->gnss_dev, buf, i);
-	if (count != i)
-		dev_dbg(ice_pf_to_dev(pf),
-			"gnss_insert_raw ret=%d size=%d\n",
-			count, i);
 	delay = ICE_GNSS_TIMER_DELAY_TIME;
-free_buf:
-	free_page((unsigned long)buf);
 requeue:
 	kthread_queue_delayed_work(gnss->kworker, &gnss->read_work, delay);
 	if (err)
diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.h b/drivers/net/ethernet/intel/ice/ice_gnss.h
index 15daf603ed7b..e0e939f1b102 100644
--- a/drivers/net/ethernet/intel/ice/ice_gnss.h
+++ b/drivers/net/ethernet/intel/ice/ice_gnss.h
@@ -8,7 +8,11 @@
 #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)
+/* ICE_MAX_I2C_DATA_SIZE is FIELD_MAX(ICE_AQC_I2C_DATA_SIZE_M).
+ * However, FIELD_MAX() does not evaluate to an integer constant expression,
+ * so it can't be used for the size of a non-VLA array.
+ */
+#define ICE_MAX_I2C_DATA_SIZE		15
 #define ICE_MAX_I2C_WRITE_BYTES		4
 
 /* u-blox ZED-F9T specific definitions */
-- 
2.47.1

[PATCH iwl-next 3/3] ice: remove special delay after processing a GNSS read batch

[Michal Schmidt]

I do not see a reason to have a special longer delay (100 ms) after
passing read GNSS data to userspace.

Just use the regular GNSS polling interval (20 ms).

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

diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
index 7922311d2545..83a2f0d4091e 100644
--- a/drivers/net/ethernet/intel/ice/ice_gnss.c
+++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
@@ -85,7 +85,6 @@ 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;
 	char buf[ICE_MAX_I2C_DATA_SIZE];
@@ -140,9 +139,9 @@ static void ice_gnss_read(struct kthread_work *work)
 				count, bytes_read);
 	}
 
-	delay = ICE_GNSS_TIMER_DELAY_TIME;
 requeue:
-	kthread_queue_delayed_work(gnss->kworker, &gnss->read_work, delay);
+	kthread_queue_delayed_work(gnss->kworker, &gnss->read_work,
+				   ICE_GNSS_POLL_DATA_DELAY_TIME);
 	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 e0e939f1b102..fa52727cd3d7 100644
--- a/drivers/net/ethernet/intel/ice/ice_gnss.h
+++ b/drivers/net/ethernet/intel/ice/ice_gnss.h
@@ -6,7 +6,6 @@
 
 #define ICE_E810T_GNSS_I2C_BUS		0x2
 #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
 /* ICE_MAX_I2C_DATA_SIZE is FIELD_MAX(ICE_AQC_I2C_DATA_SIZE_M).
  * However, FIELD_MAX() does not evaluate to an integer constant expression,
-- 
2.47.1

Re: [PATCH iwl-next 2/3] ice: lower the latency of GNSS reads

[Simon Horman]

On Thu, Dec 12, 2024 at 04:34:16PM +0100, Michal Schmidt wrote:
> The E810 is connected to the u-blox GNSS module over I2C. The ice driver
> periodically (every ~20ms) sends AdminQ commands to poll the u-blox for
> available data. Most of the time, there's no data. When the u-blox
> finally responds that data is available, usually it's around 800 bytes.
> It can be more or less, depending on how many NMEA messages were
> configured using ubxtool. ice then proceeds to read all the data.
> AdminQ and I2C are slow. The reading is performed in chunks of 15 bytes.
> ice reads all of the data before passing it to the kernel GNSS subsystem
> and onwards to userspace.
> 
> Improve the NMEA message receiving latency. Pass each 15-bytes chunk to
> userspace as soon as it's received.
> 
> Tested-by: Miroslav Lichvar <mlichvar@redhat.com>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH iwl-next 1/3] ice: downgrade warning about gnss_insert_raw to debug level

[Simon Horman]

On Thu, Dec 12, 2024 at 04:34:15PM +0100, Michal Schmidt wrote:
> gnss_insert_raw() will reject the GNSS data the ice driver produces
> whenever userspace has the gnss device open, but is not reading it fast
> enough for whatever reason.
> 
> Do not spam kernel logs just because userspace misbehaves.
> 
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH iwl-next 3/3] ice: remove special delay after processing a GNSS read batch

[Simon Horman]

On Thu, Dec 12, 2024 at 04:34:17PM +0100, Michal Schmidt wrote:
> I do not see a reason to have a special longer delay (100 ms) after
> passing read GNSS data to userspace.
> 
> Just use the regular GNSS polling interval (20 ms).
> 
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH iwl-next 2/3] ice: lower the latency of GNSS reads

[Przemek Kitszel]

On 12/12/24 16:34, Michal Schmidt wrote:
> The E810 is connected to the u-blox GNSS module over I2C. The ice driver
> periodically (every ~20ms) sends AdminQ commands to poll the u-blox for
> available data. Most of the time, there's no data. When the u-blox
> finally responds that data is available, usually it's around 800 bytes.
> It can be more or less, depending on how many NMEA messages were
> configured using ubxtool. ice then proceeds to read all the data.
> AdminQ and I2C are slow. The reading is performed in chunks of 15 bytes.
> ice reads all of the data before passing it to the kernel GNSS subsystem
> and onwards to userspace.
> 
> Improve the NMEA message receiving latency. Pass each 15-bytes chunk to
> userspace as soon as it's received.
> 

Thank you, overall it makes a good addition!
Please find some review feedback below.

> Tested-by: Miroslav Lichvar <mlichvar@redhat.com>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_gnss.c | 29 +++++++----------------
>   drivers/net/ethernet/intel/ice/ice_gnss.h |  6 ++++-
>   2 files changed, 14 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
> index 9b1f970f4825..7922311d2545 100644
> --- a/drivers/net/ethernet/intel/ice/ice_gnss.c
> +++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
> @@ -88,10 +88,10 @@ static void ice_gnss_read(struct kthread_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;
> +	char buf[ICE_MAX_I2C_DATA_SIZE];
>   	struct ice_pf *pf;
>   	struct ice_hw *hw;
>   	__be16 data_len_b;
> -	char *buf = NULL;
>   	u8 i2c_params;
>   	int err = 0;
>   
> @@ -121,16 +121,6 @@ static void ice_gnss_read(struct kthread_work *work)
>   		goto requeue;
>   
>   	/* The u-blox has data_len bytes for us to read */
> -
> -	data_len = min_t(typeof(data_len), data_len, PAGE_SIZE);

prior to your patch, the buffer is too small when there is more than
PAGE_SIZE bytes to read, that warrants sending it as -net
There is not that much code here, and with your description it is easy
to follow, and the change is really "atomic" (send out each time instead
of just once at the end), no refactors, so feels nice for -net IMO.

> -
> -	buf = (char *)get_zeroed_page(GFP_KERNEL);
> -	if (!buf) {
> -		err = -ENOMEM;
> -		goto requeue;
> -	}
> -
> -	/* Read received data */
>   	for (i = 0; i < data_len; i += bytes_read) {
>   		unsigned int bytes_left = data_len - i;
>   
> @@ -139,19 +129,18 @@ static void ice_gnss_read(struct kthread_work *work)
>   
>   		err = ice_aq_read_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
>   				      cpu_to_le16(ICE_GNSS_UBX_EMPTY_DATA),
> -				      bytes_read, &buf[i], NULL);
> +				      bytes_read, buf, NULL);
>   		if (err)
> -			goto free_buf;
> +			goto requeue;
> +
> +		count = gnss_insert_raw(pf->gnss_dev, buf, bytes_read);
> +		if (count != bytes_read)

Before there was nothing to do on this condition, but now it's in the
loop, so I would expect to either break or retry or otherwise recover
here. Just going with the next step of the loop when you have lost some
bytes feels wrong. Not sure how much about that case is theoretical,
perhaps API could be fixed instead.

> +			dev_dbg(ice_pf_to_dev(pf),

in case of v2, I would squash the first commit here, an "additional
paragraph" would be enough

> +				"gnss_insert_raw ret=%d size=%d\n",
> +				count, bytes_read);
>   	}
>   
> -	count = gnss_insert_raw(pf->gnss_dev, buf, i);
> -	if (count != i)
> -		dev_dbg(ice_pf_to_dev(pf),
> -			"gnss_insert_raw ret=%d size=%d\n",
> -			count, i);
>   	delay = ICE_GNSS_TIMER_DELAY_TIME;
> -free_buf:
> -	free_page((unsigned long)buf);
>   requeue:
>   	kthread_queue_delayed_work(gnss->kworker, &gnss->read_work, delay);
>   	if (err)
> diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.h b/drivers/net/ethernet/intel/ice/ice_gnss.h
> index 15daf603ed7b..e0e939f1b102 100644
> --- a/drivers/net/ethernet/intel/ice/ice_gnss.h
> +++ b/drivers/net/ethernet/intel/ice/ice_gnss.h
> @@ -8,7 +8,11 @@
>   #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)
> +/* ICE_MAX_I2C_DATA_SIZE is FIELD_MAX(ICE_AQC_I2C_DATA_SIZE_M).
> + * However, FIELD_MAX() does not evaluate to an integer constant expression,
> + * so it can't be used for the size of a non-VLA array.
> + */
> +#define ICE_MAX_I2C_DATA_SIZE		15

static_assert() is better than doc to say that two values are the same

>   #define ICE_MAX_I2C_WRITE_BYTES		4
>   
>   /* u-blox ZED-F9T specific definitions */

Re: [PATCH iwl-next 2/3] ice: lower the latency of GNSS reads

[Michal Schmidt]

On Mon, Dec 16, 2024 at 6:39 AM Przemek Kitszel
<przemyslaw.kitszel@intel.com> wrote:
> On 12/12/24 16:34, Michal Schmidt wrote:
> > The E810 is connected to the u-blox GNSS module over I2C. The ice driver
> > periodically (every ~20ms) sends AdminQ commands to poll the u-blox for
> > available data. Most of the time, there's no data. When the u-blox
> > finally responds that data is available, usually it's around 800 bytes.
> > It can be more or less, depending on how many NMEA messages were
> > configured using ubxtool. ice then proceeds to read all the data.
> > AdminQ and I2C are slow. The reading is performed in chunks of 15 bytes.
> > ice reads all of the data before passing it to the kernel GNSS subsystem
> > and onwards to userspace.
> >
> > Improve the NMEA message receiving latency. Pass each 15-bytes chunk to
> > userspace as soon as it's received.
> >
>
> Thank you, overall it makes a good addition!
> Please find some review feedback below.
>
> > Tested-by: Miroslav Lichvar <mlichvar@redhat.com>
> > Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> > ---
> >   drivers/net/ethernet/intel/ice/ice_gnss.c | 29 +++++++----------------
> >   drivers/net/ethernet/intel/ice/ice_gnss.h |  6 ++++-
> >   2 files changed, 14 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
> > index 9b1f970f4825..7922311d2545 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_gnss.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
> > @@ -88,10 +88,10 @@ static void ice_gnss_read(struct kthread_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;
> > +     char buf[ICE_MAX_I2C_DATA_SIZE];
> >       struct ice_pf *pf;
> >       struct ice_hw *hw;
> >       __be16 data_len_b;
> > -     char *buf = NULL;
> >       u8 i2c_params;
> >       int err = 0;
> >
> > @@ -121,16 +121,6 @@ static void ice_gnss_read(struct kthread_work *work)
> >               goto requeue;
> >
> >       /* The u-blox has data_len bytes for us to read */
> > -
> > -     data_len = min_t(typeof(data_len), data_len, PAGE_SIZE);
>
> prior to your patch, the buffer is too small when there is more than
> PAGE_SIZE bytes to read, that warrants sending it as -net
> There is not that much code here, and with your description it is easy
> to follow, and the change is really "atomic" (send out each time instead
> of just once at the end), no refactors, so feels nice for -net IMO.

OK, the next version will target -net.

> > -
> > -     buf = (char *)get_zeroed_page(GFP_KERNEL);
> > -     if (!buf) {
> > -             err = -ENOMEM;
> > -             goto requeue;
> > -     }
> > -
> > -     /* Read received data */
> >       for (i = 0; i < data_len; i += bytes_read) {
> >               unsigned int bytes_left = data_len - i;
> >
> > @@ -139,19 +129,18 @@ static void ice_gnss_read(struct kthread_work *work)
> >
> >               err = ice_aq_read_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
> >                                     cpu_to_le16(ICE_GNSS_UBX_EMPTY_DATA),
> > -                                   bytes_read, &buf[i], NULL);
> > +                                   bytes_read, buf, NULL);
> >               if (err)
> > -                     goto free_buf;
> > +                     goto requeue;
> > +
> > +             count = gnss_insert_raw(pf->gnss_dev, buf, bytes_read);
> > +             if (count != bytes_read)
>
> Before there was nothing to do on this condition, but now it's in the
> loop, so I would expect to either break or retry or otherwise recover
> here. Just going with the next step of the loop when you have lost some
> bytes feels wrong.

Userspace should handle corrupt NMEA (or UBX) messages anyway. And in
the driver we don't interpret the protocol, so we don't know where the
next valid message starts. I don't see what else we can do.

> Not sure how much about that case is theoretical,
> perhaps API could be fixed instead.

It might be a good idea to change the gnss subsystem API to allow
overwriting old buffered data, rather than reject new data.
[+CC:Johan].

> > +                     dev_dbg(ice_pf_to_dev(pf),
>
> in case of v2, I would squash the first commit here, an "additional
> paragraph" would be enough

OK, I will squash the two patches.

> > +                             "gnss_insert_raw ret=%d size=%d\n",
> > +                             count, bytes_read);
> >       }
> >
> > -     count = gnss_insert_raw(pf->gnss_dev, buf, i);
> > -     if (count != i)
> > -             dev_dbg(ice_pf_to_dev(pf),
> > -                     "gnss_insert_raw ret=%d size=%d\n",
> > -                     count, i);
> >       delay = ICE_GNSS_TIMER_DELAY_TIME;
> > -free_buf:
> > -     free_page((unsigned long)buf);
> >   requeue:
> >       kthread_queue_delayed_work(gnss->kworker, &gnss->read_work, delay);
> >       if (err)
> > diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.h b/drivers/net/ethernet/intel/ice/ice_gnss.h
> > index 15daf603ed7b..e0e939f1b102 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_gnss.h
> > +++ b/drivers/net/ethernet/intel/ice/ice_gnss.h
> > @@ -8,7 +8,11 @@
> >   #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)
> > +/* ICE_MAX_I2C_DATA_SIZE is FIELD_MAX(ICE_AQC_I2C_DATA_SIZE_M).
> > + * However, FIELD_MAX() does not evaluate to an integer constant expression,
> > + * so it can't be used for the size of a non-VLA array.
> > + */
> > +#define ICE_MAX_I2C_DATA_SIZE                15
>
> static_assert() is better than doc to say that two values are the same

Unfortunately, you can't use FIELD_MAX() in a static_assert(), for the
same reason you can't use it for sizing an array. You'll get either
"error: expression in static assertion is not constant", or "error:
braced-group within expression allowed only inside a function",
depending on where you put the static_assert().

I tried improving this some time ago:
https://lore.kernel.org/lkml/20240515172732.288391-1-mschmidt@redhat.com/T/#u
... but it required some more work.

Michal

> >   #define ICE_MAX_I2C_WRITE_BYTES             4
> >
> >   /* u-blox ZED-F9T specific definitions */
>

Re: [PATCH iwl-next 1/3] ice: downgrade warning about gnss_insert_raw to debug level

[Kolacinski, Karol]

On 12.12.2024 16:34 CET Michal Schmidt wrote:
> gnss_insert_raw() will reject the GNSS data the ice driver produces
> whenever userspace has the gnss device open, but is not reading it fast
> enough for whatever reason.
>
> Do not spam kernel logs just because userspace misbehaves.
>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_gnss.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Karol Kolacinski <karol.kolacinski@intel.com>

Re: [PATCH iwl-next 2/3] ice: lower the latency of GNSS reads

[Kolacinski, Karol]

On 12.12.2024 16:34 CET, Michal Schmidt <mschmidt@redhat.com> wrote:
> The E810 is connected to the u-blox GNSS module over I2C. The ice driver
> periodically (every ~20ms) sends AdminQ commands to poll the u-blox for
> available data. Most of the time, there's no data. When the u-blox
> finally responds that data is available, usually it's around 800 bytes.
> It can be more or less, depending on how many NMEA messages were
> configured using ubxtool. ice then proceeds to read all the data.
> AdminQ and I2C are slow. The reading is performed in chunks of 15 bytes.
> ice reads all of the data before passing it to the kernel GNSS subsystem
> and onwards to userspace.
>
> Improve the NMEA message receiving latency. Pass each 15-bytes chunk to
> userspace as soon as it's received.
>
> Tested-by: Miroslav Lichvar <mlichvar@redhat.com>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_gnss.c | 29 +++++++----------------
>  drivers/net/ethernet/intel/ice/ice_gnss.h |  6 ++++-
>  2 files changed, 14 insertions(+), 21 deletions(-)

Reviewed-by: Karol Kolacinski <karol.kolacinski@intel.com>