[PATCH net 0/3][pull request] Intel Wired LAN Driver Updates 2022-08-29 (ice)

[PATCH net 0/3][pull request] Intel Wired LAN Driver Updates 2022-08-29 (ice)

[Tony Nguyen]

This series contains updates to ice driver only.

Przemyslaw fixes memory leak of DMA memory due to incorrect freeing of
rx_buf.

Michal S corrects incorrect call to free memory.

Michal M adds mock implementation for set_termios to allow operation on
tools that require it.

The following are changes since commit cb10b0f91c5f76de981ef927e7dadec60c5a5d96:
  Merge branch 'u64_stats-fixups'
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 100GbE

Michal Michalik (1):
  ice: Add set_termios tty operations handle to GNSS

Michal Swiatkowski (1):
  ice: use bitmap_free instead of devm_kfree

Przemyslaw Patynowski (1):
  ice: Fix DMA mappings leak

 drivers/net/ethernet/intel/ice/ice_base.c | 17 ------
 drivers/net/ethernet/intel/ice/ice_gnss.c | 15 ++++++
 drivers/net/ethernet/intel/ice/ice_main.c | 10 +++-
 drivers/net/ethernet/intel/ice/ice_xsk.c  | 63 +++++++++++++++++++++++
 drivers/net/ethernet/l/ice/ice_xsk.h  |  8 +++
 5 files changed, 95 insertions(+), 18 deletions(-)

-- 
2.35.1

[PATCH net 1/3] ice: Fix DMA mappings leak

[Tony Nguyen]

From: Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>

Fix leak, when user changes ring parameters.
During reallocation of RX buffers, new DMA mappings are created for
those buffers. New buffers with different RX ring count should
substitute older ones, but those buffers were freed in ice_vsi_cfg_rxq
and reallocated again with ice_alloc_rx_buf. kfree on rx_buf caused
leak of already mapped DMA.
Reallocate ZC with xdp_buf struct, when BPF program loads. Reallocate
back to rx_buf, when BPF program unloads.
If BPF program is loaded/unloaded and XSK pools are created, reallocate
RX queues accordingly in XDP_SETUP_XSK_POOL handler.

Steps for reproduction:
while :
do
	for ((i=0; i<=8160; i=i+32))
	do
		ethtool -G enp130s0f0 rx $i tx $i
		sleep 0.5
		ethtool -g enp130s0f0
	done
done

Fixes: 617f3e1b588c ("ice: xsk: allocate separate memory for XDP SW ring")
Signed-off-by: Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>
Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
Tested-by: Chandan <chandanx.rout@intel.com> (A Contingent Worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_base.c | 17 ------
 drivers/net/ethernet/intel/ice/ice_main.c |  8 +++
 drivers/net/ethernet/intel/ice/ice_xsk.c  | 63 +++++++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_xsk.h  |  8 +++
 4 files changed, 79 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_base.c b/drivers/net/ethernet/intel/ice/ice_base.c
index 136d7911adb4..1e3243808178 100644
--- a/drivers/net/ethernet/intel/ice/ice_base.c
+++ b/drivers/net/ethernet/intel/ice/ice_base.c
@@ -7,18 +7,6 @@
 #include "ice_dcb_lib.h"
 #include "ice_sriov.h"
 
-static bool ice_alloc_rx_buf_zc(struct ice_rx_ring *rx_ring)
-{
-	rx_ring->xdp_buf = kcalloc(rx_ring->count, sizeof(*rx_ring->xdp_buf), GFP_KERNEL);
-	return !!rx_ring->xdp_buf;
-}
-
-static bool ice_alloc_rx_buf(struct ice_rx_ring *rx_ring)
-{
-	rx_ring->rx_buf = kcalloc(rx_ring->count, sizeof(*rx_ring->rx_buf), GFP_KERNEL);
-	return !!rx_ring->rx_buf;
-}
-
 /**
  * __ice_vsi_get_qs_contig - Assign a contiguous chunk of queues to VSI
  * @qs_cfg: gathered variables needed for PF->VSI queues assignment
@@ -519,11 +507,8 @@ int ice_vsi_cfg_rxq(struct ice_rx_ring *ring)
 			xdp_rxq_info_reg(&ring->xdp_rxq, ring->netdev,
 					 ring->q_index, ring->q_vector->napi.napi_id);
 
-		kfree(ring->rx_buf);
 		ring->xsk_pool = ice_xsk_pool(ring);
 		if (ring->xsk_pool) {
-			if (!ice_alloc_rx_buf_zc(ring))
-				return -ENOMEM;
 			xdp_rxq_info_unreg_mem_model(&ring->xdp_rxq);
 
 			ring->rx_buf_len =
@@ -538,8 +523,6 @@ int ice_vsi_cfg_rxq(struct ice_rx_ring *ring)
 			dev_info(dev, "Registered XDP mem model MEM_TYPE_XSK_BUFF_POOL on Rx ring %d\n",
 				 ring->q_index);
 		} else {
-			if (!ice_alloc_rx_buf(ring))
-				return -ENOMEM;
 			if (!xdp_rxq_info_is_reg(&ring->xdp_rxq))
 				/* coverity[check_return] */
 				xdp_rxq_info_reg(&ring->xdp_rxq,
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 173fe6c31341..e5bc69a9a37c 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -2898,10 +2898,18 @@ ice_xdp_setup_prog(struct ice_vsi *vsi, struct bpf_prog *prog,
 			if (xdp_ring_err)
 				NL_SET_ERR_MSG_MOD(extack, "Setting up XDP Tx resources failed");
 		}
+		/* reallocate Rx queues that are used for zero-copy */
+		xdp_ring_err = ice_realloc_zc_buf(vsi, true);
+		if (xdp_ring_err)
+			NL_SET_ERR_MSG_MOD(extack, "Setting up XDP Rx resources failed");
 	} else if (ice_is_xdp_ena_vsi(vsi) && !prog) {
 		xdp_ring_err = ice_destroy_xdp_rings(vsi);
 		if (xdp_ring_err)
 			NL_SET_ERR_MSG_MOD(extack, "Freeing XDP Tx resources failed");
+		/* reallocate Rx queues that were used for zero-copy */
+		xdp_ring_err = ice_realloc_zc_buf(vsi, false);
+		if (xdp_ring_err)
+			NL_SET_ERR_MSG_MOD(extack, "Freeing XDP Rx resources failed");
 	} else {
 		/* safe to call even when prog == vsi->xdp_prog as
 		 * dev_xdp_install in net/core/dev.c incremented prog's
diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
index e48e29258450..03ce85f6e6df 100644
--- a/drivers/net/ethernet/intel/ice/ice_xsk.c
+++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
@@ -192,6 +192,7 @@ static int ice_qp_dis(struct ice_vsi *vsi, u16 q_idx)
 	err = ice_vsi_ctrl_one_rx_ring(vsi, false, q_idx, true);
 	if (err)
 		return err;
+	ice_clean_rx_ring(rx_ring);
 
 	ice_qvec_toggle_napi(vsi, q_vector, false);
 	ice_qp_clean_rings(vsi, q_idx);
@@ -316,6 +317,62 @@ ice_xsk_pool_enable(struct ice_vsi *vsi, struct xsk_buff_pool *pool, u16 qid)
 	return 0;
 }
 
+/**
+ * ice_realloc_rx_xdp_bufs - reallocate for either XSK or normal buffer
+ * @rx_ring: Rx ring
+ * @pool_present: is pool for XSK present
+ *
+ * Try allocating memory and return ENOMEM, if failed to allocate.
+ * If allocation was successful, substitute buffer with allocated one.
+ * Returns 0 on success, negative on failure
+ */
+static int
+ice_realloc_rx_xdp_bufs(struct ice_rx_ring *rx_ring, bool pool_present)
+{
+	size_t elem_size = pool_present ? sizeof(*rx_ring->xdp_buf) :
+					  sizeof(*rx_ring->rx_buf);
+	void *sw_ring = kcalloc(rx_ring->count, elem_size, GFP_KERNEL);
+
+	if (!sw_ring)
+		return -ENOMEM;
+
+	if (pool_present) {
+		kfree(rx_ring->rx_buf);
+		rx_ring->rx_buf = NULL;
+		rx_ring->xdp_buf = sw_ring;
+	} else {
+		kfree(rx_ring->xdp_buf);
+		rx_ring->xdp_buf = NULL;
+		rx_ring->rx_buf = sw_ring;
+	}
+
+	return 0;
+}
+
+/**
+ * ice_realloc_zc_buf - reallocate XDP ZC queue pairs
+ * @vsi: Current VSI
+ * @zc: is zero copy set
+ *
+ * Reallocate buffer for rx_rings that might be used by XSK.
+ * XDP requires more memory, than rx_buf provides.
+ * Returns 0 on success, negative on failure
+ */
+int ice_realloc_zc_buf(struct ice_vsi *vsi, bool zc)
+{
+	struct ice_rx_ring *rx_ring;
+	unsigned long q;
+
+	for_each_set_bit(q, vsi->af_xdp_zc_qps,
+			 max_t(int, vsi->alloc_txq, vsi->alloc_rxq)) {
+		rx_ring = vsi->rx_rings[q];
+		if (ice_realloc_rx_xdp_bufs(rx_ring, zc))
+			return -ENOMEM;
+	}
+
+	return 0;
+}
+
 /**
  * ice_xsk_pool_setup - enable/disable a buffer pool region depending on its state
  * @vsi: Current VSI
@@ -345,11 +402,17 @@ int ice_xsk_pool_setup(struct ice_vsi *vsi, struct xsk_buff_pool *pool, u16 qid)
 	if_running = netif_running(vsi->netdev) && ice_is_xdp_ena_vsi(vsi);
 
 	if (if_running) {
+		struct ice_rx_ring *rx_ring = vsi->rx_rings[qid];
+
 		ret = ice_qp_dis(vsi, qid);
 		if (ret) {
 			netdev_err(vsi->netdev, "ice_qp_dis error = %d\n", ret);
 			goto xsk_pool_if_up;
 		}
+
+		ret = ice_realloc_rx_xdp_bufs(rx_ring, pool_present);
+		if (ret)
+			goto xsk_pool_if_up;
 	}
 
 	pool_failure = pool_present ? ice_xsk_pool_enable(vsi, pool, qid) :
diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.h b/drivers/net/ethernet/intel/ice/ice_xsk.h
index 21faec8e97db..4edbe81eb646 100644
--- a/drivers/net/ethernet/intel/ice/ice_xsk.h
+++ b/drivers/net/ethernet/intel/ice/ice_xsk.h
@@ -27,6 +27,7 @@ bool ice_xsk_any_rx_ring_ena(struct ice_vsi *vsi);
 void ice_xsk_clean_rx_ring(struct ice_rx_ring *rx_ring);
 void ice_xsk_clean_xdp_ring(struct ice_tx_ring *xdp_ring);
 bool ice_xmit_zc(struct ice_tx_ring *xdp_ring, u32 budget, int napi_budget);
+int ice_realloc_zc_buf(struct ice_vsi *vsi, bool zc);
 #else
 static inline bool
 ice_xmit_zc(struct ice_tx_ring __always_unused *xdp_ring,
@@ -72,5 +73,12 @@ ice_xsk_wakeup(struct net_device __always_unused *netdev,
 
 static inline void ice_xsk_clean_rx_ring(struct ice_rx_ring *rx_ring) { }
 static inline void ice_xsk_clean_xdp_ring(struct ice_tx_ring *xdp_ring) { }
+
+static inline int
+ice_realloc_zc_buf(struct ice_vsi __always_unused *vsi,
+		   bool __always_unused zc)
+{
+	return 0;
+}
 #endif /* CONFIG_XDP_SOCKETS */
 #endif /* !_ICE_XSK_H_ */
-- 
2.35.1

[PATCH net 2/3] ice: use bitmap_free instead of devm_kfree

[Tony Nguyen]

From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

pf->avail_txqs was allocated using bitmap_zalloc, bitmap_free should be
used to free this memory.

Fixes: 78b5713ac1241 ("ice: Alloc queue management bitmaps and arrays dynamically")
Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index e5bc69a9a37c..8c30eea61b6d 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -3913,7 +3913,7 @@ static int ice_init_pf(struct ice_pf *pf)
 
 	pf->avail_rxqs = bitmap_zalloc(pf->max_pf_rxqs, GFP_KERNEL);
 	if (!pf->avail_rxqs) {
-		devm_kfree(ice_pf_to_dev(pf), pf->avail_txqs);
+		bitmap_free(pf->avail_txqs);
 		pf->avail_txqs = NULL;
 		return -ENOMEM;
 	}
-- 
2.35.1

[PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS

[Tony Nguyen]

From: Michal Michalik <michal.michalik@intel.com>

Some third party tools (ex. ubxtool) try to change GNSS TTY parameters
(ex. speed). While being optional implementation, without set_termios
handle this operation fails and prevents those third party tools from
working. TTY interface in ice driver is virtual and doesn't need any change
on set_termios, so is left empty. Add this mock to support all Linux TTY
APIs.

Fixes: 43113ff73453 ("ice: add TTY for GNSS module for E810T device")
Signed-off-by: Michal Michalik <michal.michalik@intel.com>
Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_gnss.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
index b5a7f246d230..c2dc5e5c8fa4 100644
--- a/drivers/net/ethernet/intel/ice/ice_gnss.c
+++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
@@ -404,11 +404,26 @@ static unsigned int ice_gnss_tty_write_room(struct tty_struct *tty)
 	return ICE_GNSS_TTY_WRITE_BUF;
 }
 
+/**
+ * ice_gnss_tty_set_termios - mock for set_termios tty operations
+ * @tty: pointer to the tty_struct
+ * @new_termios: pointer to the new termios parameters
+ */
+static void
+ice_gnss_tty_set_termios(struct tty_struct *tty, struct ktermios *new_termios)
+{
+	/* Some 3rd party tools (ex. ubxtool) want to change the TTY parameters.
+	 * In our virtual interface (I2C communication over FW AQ) we don't have
+	 * to change anything, but we need to implement it to unblock tools.
+	 */
+}
+
 static const struct tty_operations tty_gps_ops = {
 	.open =		ice_gnss_tty_open,
 	.close =	ice_gnss_tty_close,
 	.write =	ice_gnss_tty_write,
 	.write_room =	ice_gnss_tty_write_room,
+	.set_termios =  ice_gnss_tty_set_termios,
 };
 
 /**
-- 
2.35.1

Re: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS

[Jakub Kicinski]

On Mon, 29 Aug 2022 15:00:49 -0700 Tony Nguyen wrote:
> From: Michal Michalik <michal.michalik@intel.com>
> 
> Some third party tools (ex. ubxtool) try to change GNSS TTY parameters
> (ex. speed). While being optional implementation, without set_termios
> handle this operation fails and prevents those third party tools from
> working. TTY interface in ice driver is virtual and doesn't need any change
> on set_termios, so is left empty. Add this mock to support all Linux TTY
> APIs.
> 
> Fixes: 43113ff73453 ("ice: add TTY for GNSS module for E810T device")
> Signed-off-by: Michal Michalik <michal.michalik@intel.com>
> Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>

Please CC GNSS and TTY maintainers on the patches relating to 
the TTY/GNSS channel going forward.

CC: Greg, Jiri, Johan

We'll pull in a day or two if there are no objections.

> diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
> index b5a7f246d230..c2dc5e5c8fa4 100644
> --- a/drivers/net/ethernet/intel/ice/ice_gnss.c
> +++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
> @@ -404,11 +404,26 @@ static unsigned int ice_gnss_tty_write_room(struct tty_struct *tty)
>  	return ICE_GNSS_TTY_WRITE_BUF;
>  }
>  
> +/**
> + * ice_gnss_tty_set_termios - mock for set_termios tty operations
> + * @tty: pointer to the tty_struct
> + * @new_termios: pointer to the new termios parameters
> + */
> +static void
> +ice_gnss_tty_set_termios(struct tty_struct *tty, struct ktermios *new_termios)
> +{
> +	/* Some 3rd party tools (ex. ubxtool) want to change the TTY parameters.
> +	 * In our virtual interface (I2C communication over FW AQ) we don't have
> +	 * to change anything, but we need to implement it to unblock tools.
> +	 */
> +}
> +
>  static const struct tty_operations tty_gps_ops = {
>  	.open =		ice_gnss_tty_open,
>  	.close =	ice_gnss_tty_close,
>  	.write =	ice_gnss_tty_write,
>  	.write_room =	ice_gnss_tty_write_room,
> +	.set_termios =  ice_gnss_tty_set_termios,
>  };
>  
>  /**

Re: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS

[Tony Nguyen]

On 8/31/2022 2:54 PM, Jakub Kicinski wrote:
> Please CC GNSS and TTY maintainers on the patches relating to
> the TTY/GNSS channel going forward.

I'll be sure to include them on future patches relating to this.

Thanks,
Tony

Re: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS

[Greg Kroah-Hartman]

On Wed, Aug 31, 2022 at 02:54:39PM -0700, Jakub Kicinski wrote:
> On Mon, 29 Aug 2022 15:00:49 -0700 Tony Nguyen wrote:
> > From: Michal Michalik <michal.michalik@intel.com>
> > 
> > Some third party tools (ex. ubxtool) try to change GNSS TTY parameters
> > (ex. speed). While being optional implementation, without set_termios
> > handle this operation fails and prevents those third party tools from
> > working.

What tools are "blocked" by this?  And what is the problem they have
with just the default happening here?  You are now doing nothing, while
if you do not have the callback, at least a basic "yes, we accepted
these values" happens which was intended for userspace to not know that
there was a problem here.

> TTY interface in ice driver is virtual and doesn't need any change
> > on set_termios, so is left empty. Add this mock to support all Linux TTY
> > APIs.

"mock"?



> > 
> > Fixes: 43113ff73453 ("ice: add TTY for GNSS module for E810T device")
> > Signed-off-by: Michal Michalik <michal.michalik@intel.com>
> > Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> 
> Please CC GNSS and TTY maintainers on the patches relating to 
> the TTY/GNSS channel going forward.
> 
> CC: Greg, Jiri, Johan
> 
> We'll pull in a day or two if there are no objections.

Please see above, I'd like to know what is really failing here and why
as forcing drivers to have "empty" functions like this is not good and
never the goal.

thanks,

greg k-h

Re: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS

[Jakub Kicinski]

On Thu, 1 Sep 2022 07:46:23 +0200 Greg Kroah-Hartman wrote:
> > Please CC GNSS and TTY maintainers on the patches relating to 
> > the TTY/GNSS channel going forward.
> > 
> > CC: Greg, Jiri, Johan
> > 
> > We'll pull in a day or two if there are no objections.  
> 
> Please see above, I'd like to know what is really failing here and why
> as forcing drivers to have "empty" functions like this is not good and
> never the goal.

Thanks for a prompt look!

Tony, I presume you may want to sidetrack this patch for now and ship
the rest so lemme toss this version of the series.

Re: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS

[Tony Nguyen]

On 9/1/2022 1:18 PM, Jakub Kicinski wrote:
> On Thu, 1 Sep 2022 07:46:23 +0200 Greg Kroah-Hartman wrote:
>>> Please CC GNSS and TTY maintainers on the patches relating to
>>> the TTY/GNSS channel going forward.
>>>
>>> CC: Greg, Jiri, Johan
>>>
>>> We'll pull in a day or two if there are no objections.
>>
>> Please see above, I'd like to know what is really failing here and why
>> as forcing drivers to have "empty" functions like this is not good and
>> never the goal.
> 
> Thanks for a prompt look!
> 
> Tony, I presume you may want to sidetrack this patch for now and ship
> the rest so lemme toss this version of the series.

Hi Jakub,

Yes, I'll drop this one and send the others.

I'm working internally to get responses to the other questions/comments 
for this as well.

Thanks,
Tony

RE: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS

[Michalik, Michal]

Hello Greg,

Much thanks for a feedback. Please excuse me for delayed answer, we tried to collect all
the required information before returning to you - but we are still working on it.

Best regards,
M^2

> 
> -----Original Message-----
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> 
> Sent: Thursday, September 1, 2022 7:46 AM
> To: Jakub Kicinski <kuba@kernel.org>
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; pabeni@redhat.com; edumazet@google.com; Michalik, Michal <michal.michalik@intel.com>; netdev@vger.kernel.org; richardcochran@gmail.com; G, GurucharanX <gurucharanx.g@intel.com>; Jiri Slaby <jirislaby@kernel.org>; Johan Hovold <johan@kernel.org>
> Subject: Re: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS
> 
> On Wed, Aug 31, 2022 at 02:54:39PM -0700, Jakub Kicinski wrote:
> > On Mon, 29 Aug 2022 15:00:49 -0700 Tony Nguyen wrote:
> > > From: Michal Michalik <michal.michalik@intel.com>
> > > 
> > > Some third party tools (ex. ubxtool) try to change GNSS TTY parameters
> > > (ex. speed). While being optional implementation, without set_termios
> > > handle this operation fails and prevents those third party tools from
> > > working.
> 
> What tools are "blocked" by this?  And what is the problem they have
> with just the default happening here?  You are now doing nothing, while
> if you do not have the callback, at least a basic "yes, we accepted
> these values" happens which was intended for userspace to not know that
> there was a problem here.
> 

As I stated in the commit message, the example tool is ubxtool - while trying to
connect to the GPS module the error appreared:
Traceback (most recent call last):

	  File "/usr/local/bin/ubxtool", line 378, in <module>
		io_handle = gps.gps_io(
	  File "/usr/local/lib/python3.9/site-packages/gps/gps.py", line 309, in __init__
		self.ser = Serial.Serial(
	  File "/usr/local/lib/python3.9/site-packages/serial/serialutil.py", line 244, in __init__
		self.open()
	  File "/usr/local/lib/python3.9/site-packages/serial/serialposix.py", line 332, in open
		self._reconfigure_port(force_update=True)
	  File "/usr/local/lib/python3.9/site-packages/serial/serialposix.py", line 517, in _reconfigure_port
		termios.tcsetattr(
	termios.error: (22, 'Invalid argument')
	
Adding this empty function solved the problem.

> > TTY interface in ice driver is virtual and doesn't need any change
> > > on set_termios, so is left empty. Add this mock to support all Linux TTY
> > > APIs.
> 
> "mock"?

Please excuse me if I used the wrong terminology. What I meant by "mock" was
this empty function, which did nothing but satisfied API requirements.

> 
> > > 
> > > Fixes: 43113ff73453 ("ice: add TTY for GNSS module for E810T device")
> > > Signed-off-by: Michal Michalik <michal.michalik@intel.com>
> > > Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
> > > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > 
> > Please CC GNSS and TTY maintainers on the patches relating to 
> > the TTY/GNSS channel going forward.
> > 
> > CC: Greg, Jiri, Johan
> > 
> > We'll pull in a day or two if there are no objections.
> 
> Please see above, I'd like to know what is really failing here and why
> as forcing drivers to have "empty" functions like this is not good and
> never the goal.

If I should elaborate more on the reproduction, please leave me a note.

> 
> thanks,
> 
> greg k-h
>

Re: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS

[Greg Kroah-Hartman]

On Mon, Sep 05, 2022 at 07:32:44PM +0000, Michalik, Michal wrote:
> Hello Greg,
> 
> Much thanks for a feedback. Please excuse me for delayed answer, we tried to collect all
> the required information before returning to you - but we are still working on it.
> 
> Best regards,
> M^2
> 
> > 
> > -----Original Message-----
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> 
> > Sent: Thursday, September 1, 2022 7:46 AM
> > To: Jakub Kicinski <kuba@kernel.org>
> > Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; pabeni@redhat.com; edumazet@google.com; Michalik, Michal <michal.michalik@intel.com>; netdev@vger.kernel.org; richardcochran@gmail.com; G, GurucharanX <gurucharanx.g@intel.com>; Jiri Slaby <jirislaby@kernel.org>; Johan Hovold <johan@kernel.org>
> > Subject: Re: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS
> > 
> > On Wed, Aug 31, 2022 at 02:54:39PM -0700, Jakub Kicinski wrote:
> > > On Mon, 29 Aug 2022 15:00:49 -0700 Tony Nguyen wrote:
> > > > From: Michal Michalik <michal.michalik@intel.com>
> > > > 
> > > > Some third party tools (ex. ubxtool) try to change GNSS TTY parameters
> > > > (ex. speed). While being optional implementation, without set_termios
> > > > handle this operation fails and prevents those third party tools from
> > > > working.
> > 
> > What tools are "blocked" by this?  And what is the problem they have
> > with just the default happening here?  You are now doing nothing, while
> > if you do not have the callback, at least a basic "yes, we accepted
> > these values" happens which was intended for userspace to not know that
> > there was a problem here.
> > 
> 
> As I stated in the commit message, the example tool is ubxtool - while trying to
> connect to the GPS module the error appreared:
> Traceback (most recent call last):
> 
> 	  File "/usr/local/bin/ubxtool", line 378, in <module>
> 		io_handle = gps.gps_io(
> 	  File "/usr/local/lib/python3.9/site-packages/gps/gps.py", line 309, in __init__
> 		self.ser = Serial.Serial(
> 	  File "/usr/local/lib/python3.9/site-packages/serial/serialutil.py", line 244, in __init__
> 		self.open()
> 	  File "/usr/local/lib/python3.9/site-packages/serial/serialposix.py", line 332, in open
> 		self._reconfigure_port(force_update=True)
> 	  File "/usr/local/lib/python3.9/site-packages/serial/serialposix.py", line 517, in _reconfigure_port
> 		termios.tcsetattr(
> 	termios.error: (22, 'Invalid argument')
> 	
> Adding this empty function solved the problem.

That seems very wrong, please work to fix this by NOT having an empty
function like this as it should not be required.

thanks,

greg k-h

RE: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS

[Michalik, Michal]

Greg,

Thanks - answer inline.

BR,
M^2

> 
> -----Original Message-----
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> 
> Sent: Tuesday, September 6, 2022 8:16 AM
> To: Michalik, Michal <michal.michalik@intel.com>
> Cc: Jakub Kicinski <kuba@kernel.org>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; pabeni@redhat.com; edumazet@google.com; netdev@vger.kernel.org; richardcochran@gmail.com; G, GurucharanX <gurucharanx.g@intel.com>; Jiri Slaby <jirislaby@kernel.org>; Johan Hovold <johan@kernel.org>
> Subject: Re: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS
> 
> On Mon, Sep 05, 2022 at 07:32:44PM +0000, Michalik, Michal wrote:
> > Hello Greg,
> > 
> > Much thanks for a feedback. Please excuse me for delayed answer, we tried to collect all
> > the required information before returning to you - but we are still working on it.
> > 
> > Best regards,
> > M^2
> > 
> > > 
> > > -----Original Message-----
> > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> 
> > > Sent: Thursday, September 1, 2022 7:46 AM
> > > To: Jakub Kicinski <kuba@kernel.org>
> > > Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; pabeni@redhat.com; edumazet@google.com; Michalik, Michal <michal.michalik@intel.com>; netdev@vger.kernel.org; richardcochran@gmail.com; G, GurucharanX <gurucharanx.g@intel.com>; Jiri Slaby <jirislaby@kernel.org>; Johan Hovold <johan@kernel.org>
> > > Subject: Re: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS
> > > 
> > > On Wed, Aug 31, 2022 at 02:54:39PM -0700, Jakub Kicinski wrote:
> > > > On Mon, 29 Aug 2022 15:00:49 -0700 Tony Nguyen wrote:
> > > > > From: Michal Michalik <michal.michalik@intel.com>
> > > > > 
> > > > > Some third party tools (ex. ubxtool) try to change GNSS TTY parameters
> > > > > (ex. speed). While being optional implementation, without set_termios
> > > > > handle this operation fails and prevents those third party tools from
> > > > > working.
> > > 
> > > What tools are "blocked" by this?  And what is the problem they have
> > > with just the default happening here?  You are now doing nothing, while
> > > if you do not have the callback, at least a basic "yes, we accepted
> > > these values" happens which was intended for userspace to not know that
> > > there was a problem here.
> > > 
> > 
> > As I stated in the commit message, the example tool is ubxtool - while trying to
> > connect to the GPS module the error appreared:
> > Traceback (most recent call last):
> > 
> > 	  File "/usr/local/bin/ubxtool", line 378, in <module>
> > 		io_handle = gps.gps_io(
> > 	  File "/usr/local/lib/python3.9/site-packages/gps/gps.py", line 309, in __init__
> > 		self.ser = Serial.Serial(
> > 	  File "/usr/local/lib/python3.9/site-packages/serial/serialutil.py", line 244, in __init__
> > 		self.open()
> > 	  File "/usr/local/lib/python3.9/site-packages/serial/serialposix.py", line 332, in open
> > 		self._reconfigure_port(force_update=True)
> > 	  File "/usr/local/lib/python3.9/site-packages/serial/serialposix.py", line 517, in _reconfigure_port
> > 		termios.tcsetattr(
> > 	termios.error: (22, 'Invalid argument')
> > 	
> > Adding this empty function solved the problem.
> 
> That seems very wrong, please work to fix this by NOT having an empty
> function like this as it should not be required.
> 

Thanks for sharing the feedback - I love the possibility to learn from you and
other community members, because I'm really new in here.

I don't get one thing, though. You are saying, it "seem" wrong and that
"should not" be required but I observe different behavior. I have prepared
a very simple code to reproduce the issue:
	#include <termios.h>
	#include <unistd.h>
	#include <stdio.h>
	#include <fcntl.h>
	#include <errno.h>

	int main()
	{
		struct termios tty;
		int fd;
		
		fd = open("/dev/ttyGNSS_0300", O_RDWR | O_NOCTTY | O_SYNC);

		if (fd < 0) {
				printf("Error - TTY not open.\n");
				return -1;
		}
				
		if (tcgetattr (fd, &tty) != 0) {
			printf("Error on get - errno=%i\n", errno);
			return -1;
		}
		tty.c_cflag |= CS8; // try to set 8 data bits 
		if (tcsetattr(fd, TCSANOW, &tty) != 0) {
			printf("Error on set - errno=%i\n", errno);
			return -1;
		}

		close(fd);
		printf("Done.\n");
	}

In this case, when I don't satisfy this API, I get an errno 22. If add this
empty function and therefore implement the full API it works as expected (no
error). In our case no action is needed, therefore we have an empty function.
At the moment, I'm not sure how I should fix it other way - since no action
on HW is neccessary.

Of course in the meantime we are working on investigating if we can easily
align to existing GNSS interface accroding to community suggestions. Still,
we believe that this fix is solving the problem at the moment. 

> thanks,
> 
> greg k-h
>

Re: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS

[Greg Kroah-Hartman]

On Tue, Sep 06, 2022 at 08:55:59PM +0000, Michalik, Michal wrote:
> Greg,
> 
> Thanks - answer inline.

As is required, no need to put this on your emails, as top-posting is
not allowed :)

> > -----Original Message-----
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> 
> > Sent: Tuesday, September 6, 2022 8:16 AM
> > To: Michalik, Michal <michal.michalik@intel.com>
> > Cc: Jakub Kicinski <kuba@kernel.org>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; pabeni@redhat.com; edumazet@google.com; netdev@vger.kernel.org; richardcochran@gmail.com; G, GurucharanX <gurucharanx.g@intel.com>; Jiri Slaby <jirislaby@kernel.org>; Johan Hovold <johan@kernel.org>
> > Subject: Re: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS

Please fix your email client to not do this.

> > > Adding this empty function solved the problem.
> > 
> > That seems very wrong, please work to fix this by NOT having an empty
> > function like this as it should not be required.
> > 
> 
> I don't get one thing, though. You are saying, it "seem" wrong and that
> "should not" be required but I observe different behavior. I have prepared
> a very simple code to reproduce the issue:
> 	#include <termios.h>
> 	#include <unistd.h>
> 	#include <stdio.h>
> 	#include <fcntl.h>
> 	#include <errno.h>
> 
> 	int main()
> 	{
> 		struct termios tty;
> 		int fd;
> 		
> 		fd = open("/dev/ttyGNSS_0300", O_RDWR | O_NOCTTY | O_SYNC);
> 
> 		if (fd < 0) {
> 				printf("Error - TTY not open.\n");
> 				return -1;
> 		}
> 				
> 		if (tcgetattr (fd, &tty) != 0) {
> 			printf("Error on get - errno=%i\n", errno);
> 			return -1;
> 		}
> 		tty.c_cflag |= CS8; // try to set 8 data bits 
> 		if (tcsetattr(fd, TCSANOW, &tty) != 0) {
> 			printf("Error on set - errno=%i\n", errno);
> 			return -1;
> 		}
> 
> 		close(fd);
> 		printf("Done.\n");
> 	}
> 
> In this case, when I don't satisfy this API, I get an errno 22.

You get the error on the first get or the set?

> If add this
> empty function and therefore implement the full API it works as expected (no
> error). In our case no action is needed, therefore we have an empty function.
> At the moment, I'm not sure how I should fix it other way - since no action
> on HW is neccessary.

This should not be needed as I thought the default would be "just ignore
this", but maybe not.  Can you look into this problem please and figure
out why this is required and fix that up?

> Of course in the meantime we are working on investigating if we can easily
> align to existing GNSS interface accroding to community suggestions. Still,
> we believe that this fix is solving the problem at the moment. 

Let's fix the root problem here, not paper over it.

thanks,

greg k-h

RE: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS

[Michalik, Michal]

> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> 
> Sent: Wednesday, September 7, 2022 7:46 AM
> 
> On Tue, Sep 06, 2022 at 08:55:59PM +0000, Michalik, Michal wrote:
> > Greg,
> > 
> > Thanks - answer inline.
> 
> As is required, no need to put this on your emails, as top-posting is
> not allowed :)

Thanks - lesson learned.

> 
> > > -----Original Message-----
> > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> 
> > > Sent: Tuesday, September 6, 2022 8:16 AM
> > > To: Michalik, Michal <michal.michalik@intel.com>
> > > Cc: Jakub Kicinski <kuba@kernel.org>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; pabeni@redhat.com; edumazet@google.com; netdev@vger.kernel.org; richardcochran@gmail.com; G, GurucharanX <gurucharanx.g@intel.com>; Jiri Slaby <jirislaby@kernel.org>; Johan Hovold <johan@kernel.org>
> > > Subject: Re: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS
> 
> Please fix your email client to not do this.
> 

Thanks, I will no longer send it. 

> > > > Adding this empty function solved the problem.
> > > 
> > > That seems very wrong, please work to fix this by NOT having an empty
> > > function like this as it should not be required.
> > > 
> > 
> > I don't get one thing, though. You are saying, it "seem" wrong and that
> > "should not" be required but I observe different behavior. I have prepared
> > a very simple code to reproduce the issue:
> > 	#include <termios.h>
> > 	#include <unistd.h>
> > 	#include <stdio.h>
> > 	#include <fcntl.h>
> > 	#include <errno.h>
> > 
> > 	int main()
> > 	{
> > 		struct termios tty;
> > 		int fd;
> > 		
> > 		fd = open("/dev/ttyGNSS_0300", O_RDWR | O_NOCTTY | O_SYNC);
> > 
> > 		if (fd < 0) {
> > 				printf("Error - TTY not open.\n");
> > 				return -1;
> > 		}
> > 				
> > 		if (tcgetattr (fd, &tty) != 0) {
> > 			printf("Error on get - errno=%i\n", errno);
> > 			return -1;
> > 		}
> > 		tty.c_cflag |= CS8; // try to set 8 data bits 
> > 		if (tcsetattr(fd, TCSANOW, &tty) != 0) {
> > 			printf("Error on set - errno=%i\n", errno);
> > 			return -1;
> > 		}
> > 
> > 		close(fd);
> > 		printf("Done.\n");
> > 	}
> > 
> > In this case, when I don't satisfy this API, I get an errno 22.
> 
> You get the error on the first get or the set?
> 

On "set" - I'm doing get only to prove interface is working and read current
attributes to change only one of them.

> > If add this
> > empty function and therefore implement the full API it works as expected (no
> > error). In our case no action is needed, therefore we have an empty function.
> > At the moment, I'm not sure how I should fix it other way - since no action
> > on HW is neccessary.
> 
> This should not be needed as I thought the default would be "just ignore
> this", but maybe not.  Can you look into this problem please and figure
> out why this is required and fix that up?
> 
> > Of course in the meantime we are working on investigating if we can easily
> > align to existing GNSS interface accroding to community suggestions. Still,
> > we believe that this fix is solving the problem at the moment. 
> 
> Let's fix the root problem here, not paper over it.
> 

Ok - now I understand what you mean. If I get it correctly in your opinion the
kernel should not return error 22 if this handler is not set. I thought it is
an expected behavior, but I will take a look if that might be a bug.

Thanks,
M^2

> thanks,
> 
> greg k-h
>

Re: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS

[Johan Hovold]

On Wed, Aug 31, 2022 at 02:54:39PM -0700, Jakub Kicinski wrote:
> On Mon, 29 Aug 2022 15:00:49 -0700 Tony Nguyen wrote:
> > From: Michal Michalik <michal.michalik@intel.com>
> > 
> > Some third party tools (ex. ubxtool) try to change GNSS TTY parameters
> > (ex. speed). While being optional implementation, without set_termios
> > handle this operation fails and prevents those third party tools from
> > working. TTY interface in ice driver is virtual and doesn't need any change
> > on set_termios, so is left empty. Add this mock to support all Linux TTY
> > APIs.
> > 
> > Fixes: 43113ff73453 ("ice: add TTY for GNSS module for E810T device")
> > Signed-off-by: Michal Michalik <michal.michalik@intel.com>
> > Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> 
> Please CC GNSS and TTY maintainers on the patches relating to 
> the TTY/GNSS channel going forward.
> 
> CC: Greg, Jiri, Johan
> 
> We'll pull in a day or two if there are no objections.

Hmm. Why was this implemented as a roll-your-own tty driver instead of
using the GNSS subsystem, which also would have allowed for a smaller
(and likely less buggy) implementation?

Looks like this was merged in 5.18 with 43113ff73453 ("ice: add TTY for
GNSS module for E810T device") without any input from people familiar
with tty either.

Johan

Re: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS

[Jiri Slaby]

On 01. 09. 22, 8:44, Johan Hovold wrote:
> Looks like this was merged in 5.18 with 43113ff73453 ("ice: add TTY for
> GNSS module for E810T device") without any input from people familiar
> with tty either.

FWIW doesn't it crash in ice_gnss_tty_write() on parallel tty opens due to:
          tty->driver_data = NULL;
in ice_gnss_tty_open()?

There are many "interesting" constructs in the driver...

thanks,
-- 
js
suse labs

Re: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS

[Jiri Slaby]

On 02. 09. 22, 9:42, Jiri Slaby wrote:
> On 01. 09. 22, 8:44, Johan Hovold wrote:
>> Looks like this was merged in 5.18 with 43113ff73453 ("ice: add TTY for
>> GNSS module for E810T device") without any input from people familiar
>> with tty either.
> 
> FWIW doesn't it crash in ice_gnss_tty_write() on parallel tty opens due to:
>           tty->driver_data = NULL;
> in ice_gnss_tty_open()?

Oh, the driver checks for tty->driver_data in every operation (gee). So 
at least that crash is mitigated. The userspace will "only" receive 
EFAULT from time to time.

> There are many "interesting" constructs in the driver...

The checks belong among this "interesting" constructs too.

> thanks,-- 
js
suse labs

RE: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS

[Michalik, Michal]

Hi Jiri,

Agree with you, we will work on aligning to the GNSS subsystem if that would be
possible in our implementation according to Johan suggestion.

We will also make sure to make our code less "interesting", when comes to constructs.

Thanks a lot!

BR,
M^2

> -----Original Message-----
> From: Jiri Slaby <jirislaby@kernel.org> 
> Sent: Friday, September 2, 2022 12:47 PM
> To: Johan Hovold <johan@kernel.org>; Jakub Kicinski <kuba@kernel.org>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Cc: davem@davemloft.net; pabeni@redhat.com; edumazet@google.com; Michalik, Michal <michal.michalik@intel.com>; netdev@vger.kernel.org; richardcochran@gmail.com; G, GurucharanX <gurucharanx.g@intel.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Subject: Re: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS
> 
> On 02. 09. 22, 9:42, Jiri Slaby wrote:
> > On 01. 09. 22, 8:44, Johan Hovold wrote:
> >> Looks like this was merged in 5.18 with 43113ff73453 ("ice: add TTY for
> >> GNSS module for E810T device") without any input from people familiar
> >> with tty either.
> > 
> > FWIW doesn't it crash in ice_gnss_tty_write() on parallel tty opens due to:
> >           tty->driver_data = NULL;
> > in ice_gnss_tty_open()?
> 
> Oh, the driver checks for tty->driver_data in every operation (gee). So 
> at least that crash is mitigated. The userspace will "only" receive 
> EFAULT from time to time.
> 
> > There are many "interesting" constructs in the driver...
> 
> The checks belong among this "interesting" constructs too.
> 
> > thanks,-- 
> js
> suse labs
>

RE: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS

[Michalik, Michal]

Hello Johan,

Much thanks for valuable feedback. We really appreciate that you have pointed that
problem so we can fix it as soon as possible. 

BR,
M^2

> -----Original Message-----
> From: Johan Hovold <johan@kernel.org> 
> Sent: Thursday, September 1, 2022 8:45 AM
> To: Jakub Kicinski <kuba@kernel.org>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Cc: davem@davemloft.net; pabeni@redhat.com; edumazet@google.com; Michalik, Michal <michal.michalik@intel.com>; netdev@vger.kernel.org; richardcochran@gmail.com; G, GurucharanX <gurucharanx.g@intel.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby <jirislaby@kernel.org>
> Subject: Re: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS
> 
> On Wed, Aug 31, 2022 at 02:54:39PM -0700, Jakub Kicinski wrote:
> > On Mon, 29 Aug 2022 15:00:49 -0700 Tony Nguyen wrote:
> > > From: Michal Michalik <michal.michalik@intel.com>
> > > 
> > > Some third party tools (ex. ubxtool) try to change GNSS TTY parameters
> > > (ex. speed). While being optional implementation, without set_termios
> > > handle this operation fails and prevents those third party tools from
> > > working. TTY interface in ice driver is virtual and doesn't need any change
> > > on set_termios, so is left empty. Add this mock to support all Linux TTY
> > > APIs.
> > > 
> > > Fixes: 43113ff73453 ("ice: add TTY for GNSS module for E810T device")
> > > Signed-off-by: Michal Michalik <michal.michalik@intel.com>
> > > Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
> > > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > 
> > Please CC GNSS and TTY maintainers on the patches relating to 
> > the TTY/GNSS channel going forward.
> > 
> > CC: Greg, Jiri, Johan
> > 
> > We'll pull in a day or two if there are no objections.
> 
> Hmm. Why was this implemented as a roll-your-own tty driver instead of
> using the GNSS subsystem, which also would have allowed for a smaller
> (and likely less buggy) implementation?
> 
> Looks like this was merged in 5.18 with 43113ff73453 ("ice: add TTY for
> GNSS module for E810T device") without any input from people familiar
> with tty either.
> 

Original author is off, but to be completely honest with you - we most likely
did not noticed the possiblity to align to GNSS subsystem. That is a mistake -
we are working on understanding if we can easily fix that. We will back you you
as soon as came up with the solution.

> Johan
>