[PATCH net 0/4][pull request] Intel Wired LAN Driver Updates 2022-12-07 (ice)

[PATCH net 0/4][pull request] Intel Wired LAN Driver Updates 2022-12-07 (ice)

[Tony Nguyen]

This series contains updates to ice driver only.

Anatolii adds an additional kthread worker for extended timestamp work as
AdminQ calls are disrupting timestamp work.

Dave adds replugging of aux device when channels are reconfigured so
updated resources can be redistributed.

Mateusz replaces unregister_netdev() call with call to clear rings as
there can be a deadlock with the former call.

Michal fixes a broken URL.

The following are changes since commit 87a39882b5ab3127700ac4b9277608075f98eda2:
  net: dsa: mv88e6xxx: accept phy-mode = "internal" for internal PHY ports
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 100GbE

Anatolii Gerasymenko (1):
  ice: Create a separate kthread to handle ptp extts work

Dave Ertman (1):
  ice: Correctly handle aux device when num channels change

Mateusz Palczewski (1):
  ice: Fix deadlock on the rtnl_mutex

Michal Wilczynski (1):
  ice: Fix broken link in ice NAPI doc

 .../device_drivers/ethernet/intel/ice.rst         |  2 +-
 drivers/net/ethernet/intel/ice/ice.h              |  2 ++
 drivers/net/ethernet/intel/ice/ice_ethtool.c      |  6 ++++++
 drivers/net/ethernet/intel/ice/ice_idc.c          |  3 +++
 drivers/net/ethernet/intel/ice/ice_lib.c          | 10 ++++------
 drivers/net/ethernet/intel/ice/ice_main.c         |  8 +++++++-
 drivers/net/ethernet/intel/ice/ice_ptp.c          | 15 ++++++++++++++-
 drivers/net/ethernet/intel/ice/ice_ptp.h          |  2 ++
 8 files changed, 39 insertions(+), 9 deletions(-)

-- 
2.35.1

[PATCH net 1/4] ice: Create a separate kthread to handle ptp extts work

[Tony Nguyen]

From: Anatolii Gerasymenko <anatolii.gerasymenko@intel.com>

ice_ptp_extts_work() and ice_ptp_periodic_work() are both scheduled on
the same kthread_worker pf.ptp.kworker. But, ice_ptp_periodic_work()
sends messages to AQ and waits for responses. This causes
ice_ptp_extts_work() to be blocked while waiting to be scheduled. This
causes problems with the reading of the incoming signal timestamps,
which disrupts a 100 Hz signal.

Create an additional kthread_worker pf.ptp.kworker_extts to service only
ice_ptp_extts_work() as soon as possible.

Fixes: 77a781155a65 ("ice: enable receive hardware timestamping")
Signed-off-by: Anatolii Gerasymenko <anatolii.gerasymenko@intel.com>
Tested-by: Gurucharan G <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 |  5 ++++-
 drivers/net/ethernet/intel/ice/ice_ptp.c  | 15 ++++++++++++++-
 drivers/net/ethernet/intel/ice/ice_ptp.h  |  2 ++
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index ca2898467dcb..d0f14e73e8da 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -3106,7 +3106,10 @@ static irqreturn_t ice_misc_intr(int __always_unused irq, void *data)
 						     GLTSYN_STAT_EVENT1_M |
 						     GLTSYN_STAT_EVENT2_M);
 		ena_mask &= ~PFINT_OICR_TSYN_EVNT_M;
-		kthread_queue_work(pf->ptp.kworker, &pf->ptp.extts_work);
+
+		if (pf->ptp.kworker_extts)
+			kthread_queue_work(pf->ptp.kworker_extts,
+					   &pf->ptp.extts_work);
 	}
 
 #define ICE_AUX_CRIT_ERR (PFINT_OICR_PE_CRITERR_M | PFINT_OICR_HMC_ERR_M | PFINT_OICR_PE_PUSH_M)
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 0f668468d141..f9e20622ad9f 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -2604,7 +2604,7 @@ static int ice_ptp_init_owner(struct ice_pf *pf)
  */
 static int ice_ptp_init_work(struct ice_pf *pf, struct ice_ptp *ptp)
 {
-	struct kthread_worker *kworker;
+	struct kthread_worker *kworker, *kworker_extts;
 
 	/* Initialize work functions */
 	kthread_init_delayed_work(&ptp->work, ice_ptp_periodic_work);
@@ -2620,6 +2620,13 @@ static int ice_ptp_init_work(struct ice_pf *pf, struct ice_ptp *ptp)
 
 	ptp->kworker = kworker;
 
+	kworker_extts = kthread_create_worker(0, "ice-ptp-extts-%s",
+					      dev_name(ice_pf_to_dev(pf)));
+	if (IS_ERR(kworker_extts))
+		return PTR_ERR(kworker_extts);
+
+	ptp->kworker_extts = kworker_extts;
+
 	/* Start periodic work going */
 	kthread_queue_delayed_work(ptp->kworker, &ptp->work, 0);
 
@@ -2719,11 +2726,17 @@ void ice_ptp_release(struct ice_pf *pf)
 
 	ice_ptp_port_phy_stop(&pf->ptp.port);
 	mutex_destroy(&pf->ptp.port.ps_lock);
+
 	if (pf->ptp.kworker) {
 		kthread_destroy_worker(pf->ptp.kworker);
 		pf->ptp.kworker = NULL;
 	}
 
+	if (pf->ptp.kworker_extts) {
+		kthread_destroy_worker(pf->ptp.kworker_extts);
+		pf->ptp.kworker_extts = NULL;
+	}
+
 	if (!pf->ptp.clock)
 		return;
 
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
index 028349295b71..c63ad2c9af4c 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
@@ -165,6 +165,7 @@ struct ice_ptp_port {
  * @ext_ts_chan: the external timestamp channel in use
  * @ext_ts_irq: the external timestamp IRQ in use
  * @kworker: kwork thread for handling periodic work
+ * @kworker_extts: kworker thread for handling extts work
  * @perout_channels: periodic output data
  * @info: structure defining PTP hardware capabilities
  * @clock: pointer to registered PTP clock device
@@ -186,6 +187,7 @@ struct ice_ptp {
 	u8 ext_ts_chan;
 	u8 ext_ts_irq;
 	struct kthread_worker *kworker;
+	struct kthread_worker *kworker_extts;
 	struct ice_perout_channel perout_channels[GLTSYN_TGT_H_IDX_MAX];
 	struct ptp_clock_info info;
 	struct ptp_clock *clock;
-- 
2.35.1

[PATCH net 2/4] ice: Correctly handle aux device when num channels change

[Tony Nguyen]

From: Dave Ertman <david.m.ertman@intel.com>

When the number of channels/queues changes on an interface, it is necessary
to change how those resources are distributed to the auxiliary device for
maintaining RDMA functionality.  To do this, the best way is to unplug, and
then re-plug the auxiliary device.  This will cause all current resource
allocation to be released, and then re-requested under the new state.

Since the set_channel command from ethtool comes in while holding the RTNL
lock, it is necessary to offset the plugging and unplugging of auxiliary
device to another context.  For this purpose, set the flags for UNPLUG and
PLUG in the PF state, then respond to them in the service task.

Also, since the auxiliary device will be unplugged/plugged at the end of
the flow, it is better to not send the event for TCs changing in the
middle of the flow.  This will prevent a timing issue between the events
and the probe/release calls conflicting.

Fixes: 348048e724a0 ("ice: Implement iidc operations")
Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
Tested-by: Gurucharan G <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.h         | 2 ++
 drivers/net/ethernet/intel/ice/ice_ethtool.c | 6 ++++++
 drivers/net/ethernet/intel/ice/ice_idc.c     | 3 +++
 drivers/net/ethernet/intel/ice/ice_main.c    | 3 +++
 4 files changed, 14 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index 001500afc4a6..092e572768fe 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -281,6 +281,7 @@ enum ice_pf_state {
 	ICE_FLTR_OVERFLOW_PROMISC,
 	ICE_VF_DIS,
 	ICE_CFG_BUSY,
+	ICE_SET_CHANNELS,
 	ICE_SERVICE_SCHED,
 	ICE_SERVICE_DIS,
 	ICE_FD_FLUSH_REQ,
@@ -485,6 +486,7 @@ enum ice_pf_flags {
 	ICE_FLAG_VF_VLAN_PRUNING,
 	ICE_FLAG_LINK_LENIENT_MODE_ENA,
 	ICE_FLAG_PLUG_AUX_DEV,
+	ICE_FLAG_UNPLUG_AUX_DEV,
 	ICE_FLAG_MTU_CHANGED,
 	ICE_FLAG_GNSS,			/* GNSS successfully initialized */
 	ICE_PF_FLAGS_NBITS		/* must be last */
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index b7be84bbe72d..37e174a19860 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -3536,6 +3536,8 @@ static int ice_set_channels(struct net_device *dev, struct ethtool_channels *ch)
 		return -EINVAL;
 	}
 
+	set_bit(ICE_SET_CHANNELS, pf->state);
+
 	ice_vsi_recfg_qs(vsi, new_rx, new_tx);
 
 	if (!netif_is_rxfh_configured(dev))
@@ -3543,6 +3545,10 @@ static int ice_set_channels(struct net_device *dev, struct ethtool_channels *ch)
 
 	/* Update rss_size due to change in Rx queues */
 	vsi->rss_size = ice_get_valid_rss_size(&pf->hw, new_rx);
+	clear_bit(ICE_SET_CHANNELS, pf->state);
+
+	set_bit(ICE_FLAG_UNPLUG_AUX_DEV, pf->flags);
+	set_bit(ICE_FLAG_PLUG_AUX_DEV, pf->flags);
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/intel/ice/ice_idc.c b/drivers/net/ethernet/intel/ice/ice_idc.c
index 895c32bcc8b5..9bf6fa5ed4c8 100644
--- a/drivers/net/ethernet/intel/ice/ice_idc.c
+++ b/drivers/net/ethernet/intel/ice/ice_idc.c
@@ -37,6 +37,9 @@ void ice_send_event_to_aux(struct ice_pf *pf, struct iidc_event *event)
 	if (WARN_ON_ONCE(!in_task()))
 		return;
 
+	if (test_bit(ICE_SET_CHANNELS, pf->state))
+		return;
+
 	mutex_lock(&pf->adev_mutex);
 	if (!pf->adev)
 		goto finish;
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index d0f14e73e8da..d58f55a72ab3 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -2300,6 +2300,9 @@ static void ice_service_task(struct work_struct *work)
 		}
 	}
 
+	if (test_and_clear_bit(ICE_FLAG_UNPLUG_AUX_DEV, pf->flags))
+		ice_unplug_aux_dev(pf);
+
 	if (test_bit(ICE_FLAG_PLUG_AUX_DEV, pf->flags)) {
 		/* Plug aux device per request */
 		ice_plug_aux_dev(pf);
-- 
2.35.1

[PATCH net 3/4] ice: Fix deadlock on the rtnl_mutex

[Tony Nguyen]

From: Mateusz Palczewski <mateusz.palczewski@intel.com>

There is a deadlock on rtnl_mutex when attempting to take the lock
in unregister_netdev() after it has already been taken by
ethnl_set_channels(). This happened when unregister_netdev() was
called inside of ice_vsi_rebuild().
Fix that by removing the unregister_netdev() usage and replace it with
ice_vsi_clear_rings() that deallocates the tx and rx rings for the VSI.

Fixes: df0f847915b4 ("ice: Move common functions out of ice_main.c part 6/7")
Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
Tested-by: Gurucharan G <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_lib.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index 7276badfa19e..96098c8b5daf 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -3395,12 +3395,10 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, bool init_vsi)
 err_vectors:
 	ice_vsi_free_q_vectors(vsi);
 err_rings:
-	if (vsi->netdev) {
-		vsi->current_netdev_flags = 0;
-		unregister_netdev(vsi->netdev);
-		free_netdev(vsi->netdev);
-		vsi->netdev = NULL;
-	}
+	ice_vsi_clear_rings(vsi);
+	set_bit(ICE_RESET_FAILED, pf->state);
+	kfree(coalesce);
+	return ret;
 err_vsi:
 	ice_vsi_clear(vsi);
 	set_bit(ICE_RESET_FAILED, pf->state);
-- 
2.35.1

[PATCH net 4/4] ice: Fix broken link in ice NAPI doc

[Tony Nguyen]

From: Michal Wilczynski <michal.wilczynski@intel.com>

Current link for NAPI documentation in ice driver doesn't work - it
returns 404. Update the link to the working one.

Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
Acked-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 Documentation/networking/device_drivers/ethernet/intel/ice.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/networking/device_drivers/ethernet/intel/ice.rst b/Documentation/networking/device_drivers/ethernet/intel/ice.rst
index dc2e60ced927..b481b81f3be5 100644
--- a/Documentation/networking/device_drivers/ethernet/intel/ice.rst
+++ b/Documentation/networking/device_drivers/ethernet/intel/ice.rst
@@ -819,7 +819,7 @@ NAPI
 ----
 This driver supports NAPI (Rx polling mode).
 For more information on NAPI, see
-https://www.linuxfoundation.org/collaborate/workgroups/networking/napi
+https://wiki.linuxfoundation.org/networking/napi
 
 
 MACVLAN
-- 
2.35.1

Re: [PATCH net 1/4] ice: Create a separate kthread to handle ptp extts work

[Saeed Mahameed]

On 07 Dec 13:10, Tony Nguyen wrote:
>From: Anatolii Gerasymenko <anatolii.gerasymenko@intel.com>
>
>ice_ptp_extts_work() and ice_ptp_periodic_work() are both scheduled on
>the same kthread_worker pf.ptp.kworker. But, ice_ptp_periodic_work()
>sends messages to AQ and waits for responses. This causes
>ice_ptp_extts_work() to be blocked while waiting to be scheduled. This
>causes problems with the reading of the incoming signal timestamps,
>which disrupts a 100 Hz signal.
>

Sounds like an optimization rather than a bug fix, unless you explain what
the symptoms are and how critical this patch is.

code LGTM, although i find it wasteful to create a kthread per device event
type, but i can't think of a better way.

>Create an additional kthread_worker pf.ptp.kworker_extts to service only
>ice_ptp_extts_work() as soon as possible.
>
>Fixes: 77a781155a65 ("ice: enable receive hardware timestamping")
>Signed-off-by: Anatolii Gerasymenko <anatolii.gerasymenko@intel.com>
>Tested-by: Gurucharan G <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 |  5 ++++-
> drivers/net/ethernet/intel/ice/ice_ptp.c  | 15 ++++++++++++++-
> drivers/net/ethernet/intel/ice/ice_ptp.h  |  2 ++
> 3 files changed, 20 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
>index ca2898467dcb..d0f14e73e8da 100644
>--- a/drivers/net/ethernet/intel/ice/ice_main.c
>+++ b/drivers/net/ethernet/intel/ice/ice_main.c
>@@ -3106,7 +3106,10 @@ static irqreturn_t ice_misc_intr(int __always_unused irq, void *data)
> 						     GLTSYN_STAT_EVENT1_M |
> 						     GLTSYN_STAT_EVENT2_M);
> 		ena_mask &= ~PFINT_OICR_TSYN_EVNT_M;
>-		kthread_queue_work(pf->ptp.kworker, &pf->ptp.extts_work);
>+
>+		if (pf->ptp.kworker_extts)
>+			kthread_queue_work(pf->ptp.kworker_extts,
>+					   &pf->ptp.extts_work);
> 	}
>
> #define ICE_AUX_CRIT_ERR (PFINT_OICR_PE_CRITERR_M | PFINT_OICR_HMC_ERR_M | PFINT_OICR_PE_PUSH_M)
>diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>index 0f668468d141..f9e20622ad9f 100644
>--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
>@@ -2604,7 +2604,7 @@ static int ice_ptp_init_owner(struct ice_pf *pf)
>  */
> static int ice_ptp_init_work(struct ice_pf *pf, struct ice_ptp *ptp)
> {
>-	struct kthread_worker *kworker;
>+	struct kthread_worker *kworker, *kworker_extts;
>
> 	/* Initialize work functions */
> 	kthread_init_delayed_work(&ptp->work, ice_ptp_periodic_work);
>@@ -2620,6 +2620,13 @@ static int ice_ptp_init_work(struct ice_pf *pf, struct ice_ptp *ptp)
>
> 	ptp->kworker = kworker;
>
>+	kworker_extts = kthread_create_worker(0, "ice-ptp-extts-%s",
>+					      dev_name(ice_pf_to_dev(pf)));
>+	if (IS_ERR(kworker_extts))
>+		return PTR_ERR(kworker_extts);
>+
>+	ptp->kworker_extts = kworker_extts;
>+
> 	/* Start periodic work going */
> 	kthread_queue_delayed_work(ptp->kworker, &ptp->work, 0);
>
>@@ -2719,11 +2726,17 @@ void ice_ptp_release(struct ice_pf *pf)
>
> 	ice_ptp_port_phy_stop(&pf->ptp.port);
> 	mutex_destroy(&pf->ptp.port.ps_lock);
>+
> 	if (pf->ptp.kworker) {
> 		kthread_destroy_worker(pf->ptp.kworker);
> 		pf->ptp.kworker = NULL;
> 	}
>
>+	if (pf->ptp.kworker_extts) {
>+		kthread_destroy_worker(pf->ptp.kworker_extts);
>+		pf->ptp.kworker_extts = NULL;
>+	}
>+
> 	if (!pf->ptp.clock)
> 		return;
>
>diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
>index 028349295b71..c63ad2c9af4c 100644
>--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
>+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
>@@ -165,6 +165,7 @@ struct ice_ptp_port {
>  * @ext_ts_chan: the external timestamp channel in use
>  * @ext_ts_irq: the external timestamp IRQ in use
>  * @kworker: kwork thread for handling periodic work
>+ * @kworker_extts: kworker thread for handling extts work
>  * @perout_channels: periodic output data
>  * @info: structure defining PTP hardware capabilities
>  * @clock: pointer to registered PTP clock device
>@@ -186,6 +187,7 @@ struct ice_ptp {
> 	u8 ext_ts_chan;
> 	u8 ext_ts_irq;
> 	struct kthread_worker *kworker;
>+	struct kthread_worker *kworker_extts;
> 	struct ice_perout_channel perout_channels[GLTSYN_TGT_H_IDX_MAX];
> 	struct ptp_clock_info info;
> 	struct ptp_clock *clock;
>-- 
>2.35.1
>

Re: [PATCH net 2/4] ice: Correctly handle aux device when num channels change

[Saeed Mahameed]

On 07 Dec 13:10, Tony Nguyen wrote:
>From: Dave Ertman <david.m.ertman@intel.com>
>
>When the number of channels/queues changes on an interface, it is necessary
>to change how those resources are distributed to the auxiliary device for
>maintaining RDMA functionality.  To do this, the best way is to unplug, and

Can you please explain how an ethtool can affect RDMA functionality ?
don't you have full bifurcation between the two eth and rdma interfaces .. 

>then re-plug the auxiliary device.  This will cause all current resource
>allocation to be released, and then re-requested under the new state.
>

I find this really disruptive, changing number of netdev queues to cause
full aux devs restart !

>Since the set_channel command from ethtool comes in while holding the RTNL
>lock, it is necessary to offset the plugging and unplugging of auxiliary
>device to another context.  For this purpose, set the flags for UNPLUG and
>PLUG in the PF state, then respond to them in the service task.
>
>Also, since the auxiliary device will be unplugged/plugged at the end of
>the flow, it is better to not send the event for TCs changing in the
>middle of the flow.  This will prevent a timing issue between the events
>and the probe/release calls conflicting.
>
>Fixes: 348048e724a0 ("ice: Implement iidc operations")
>Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
>Tested-by: Gurucharan G <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.h         | 2 ++
> drivers/net/ethernet/intel/ice/ice_ethtool.c | 6 ++++++
> drivers/net/ethernet/intel/ice/ice_idc.c     | 3 +++
> drivers/net/ethernet/intel/ice/ice_main.c    | 3 +++
> 4 files changed, 14 insertions(+)
>
>diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
>index 001500afc4a6..092e572768fe 100644
>--- a/drivers/net/ethernet/intel/ice/ice.h
>+++ b/drivers/net/ethernet/intel/ice/ice.h
>@@ -281,6 +281,7 @@ enum ice_pf_state {
> 	ICE_FLTR_OVERFLOW_PROMISC,
> 	ICE_VF_DIS,
> 	ICE_CFG_BUSY,
>+	ICE_SET_CHANNELS,
> 	ICE_SERVICE_SCHED,
> 	ICE_SERVICE_DIS,
> 	ICE_FD_FLUSH_REQ,
>@@ -485,6 +486,7 @@ enum ice_pf_flags {
> 	ICE_FLAG_VF_VLAN_PRUNING,
> 	ICE_FLAG_LINK_LENIENT_MODE_ENA,
> 	ICE_FLAG_PLUG_AUX_DEV,
>+	ICE_FLAG_UNPLUG_AUX_DEV,
> 	ICE_FLAG_MTU_CHANGED,
> 	ICE_FLAG_GNSS,			/* GNSS successfully initialized */
> 	ICE_PF_FLAGS_NBITS		/* must be last */
>diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
>index b7be84bbe72d..37e174a19860 100644
>--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
>+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
>@@ -3536,6 +3536,8 @@ static int ice_set_channels(struct net_device *dev, struct ethtool_channels *ch)
> 		return -EINVAL;
> 	}
>
>+	set_bit(ICE_SET_CHANNELS, pf->state);
>+
> 	ice_vsi_recfg_qs(vsi, new_rx, new_tx);
>
> 	if (!netif_is_rxfh_configured(dev))
>@@ -3543,6 +3545,10 @@ static int ice_set_channels(struct net_device *dev, struct ethtool_channels *ch)
>
> 	/* Update rss_size due to change in Rx queues */
> 	vsi->rss_size = ice_get_valid_rss_size(&pf->hw, new_rx);
>+	clear_bit(ICE_SET_CHANNELS, pf->state);
>+

you just set this new state a few lines ago, clearing the bit in the same
function few lines later seems to be an abuse of the pf state machine, 
couldn't you just pass a parameter to the functions which needed this
information ? 

>+	set_bit(ICE_FLAG_UNPLUG_AUX_DEV, pf->flags);
>+	set_bit(ICE_FLAG_PLUG_AUX_DEV, pf->flags);
>
> 	return 0;
> }
>diff --git a/drivers/net/ethernet/intel/ice/ice_idc.c b/drivers/net/ethernet/intel/ice/ice_idc.c
>index 895c32bcc8b5..9bf6fa5ed4c8 100644
>--- a/drivers/net/ethernet/intel/ice/ice_idc.c
>+++ b/drivers/net/ethernet/intel/ice/ice_idc.c
>@@ -37,6 +37,9 @@ void ice_send_event_to_aux(struct ice_pf *pf, struct iidc_event *event)
> 	if (WARN_ON_ONCE(!in_task()))
> 		return;
>
>+	if (test_bit(ICE_SET_CHANNELS, pf->state))
>+		return;
>+
> 	mutex_lock(&pf->adev_mutex);
> 	if (!pf->adev)
> 		goto finish;
>diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
>index d0f14e73e8da..d58f55a72ab3 100644
>--- a/drivers/net/ethernet/intel/ice/ice_main.c
>+++ b/drivers/net/ethernet/intel/ice/ice_main.c
>@@ -2300,6 +2300,9 @@ static void ice_service_task(struct work_struct *work)
> 		}
> 	}
>
>+	if (test_and_clear_bit(ICE_FLAG_UNPLUG_AUX_DEV, pf->flags))
>+		ice_unplug_aux_dev(pf);
>+
> 	if (test_bit(ICE_FLAG_PLUG_AUX_DEV, pf->flags)) {
> 		/* Plug aux device per request */
> 		ice_plug_aux_dev(pf);
>-- 
>2.35.1
>

Re: [PATCH net 1/4] ice: Create a separate kthread to handle ptp extts work

[Jacob Keller]

On 12/7/2022 2:19 PM, Saeed Mahameed wrote:
> On 07 Dec 13:10, Tony Nguyen wrote:
>> From: Anatolii Gerasymenko <anatolii.gerasymenko@intel.com>
>>
>> ice_ptp_extts_work() and ice_ptp_periodic_work() are both scheduled on
>> the same kthread_worker pf.ptp.kworker. But, ice_ptp_periodic_work()
>> sends messages to AQ and waits for responses. This causes
>> ice_ptp_extts_work() to be blocked while waiting to be scheduled. This
>> causes problems with the reading of the incoming signal timestamps,
>> which disrupts a 100 Hz signal.
>>
> 
> Sounds like an optimization rather than a bug fix, unless you explain what
> the symptoms are and how critical this patch is.

I'm not the original author, but I think Anatolii is out right now. I'll 
try to explain for him.

The problem is that extts must execute to read the timestamp value from 
the incoming signal. It has to complete before the next level change. 
Otherwise I believe if we don't read it in time the next level change 
will be ignored.

For a 100Hz signal, this means it has to process within 10 milliseconds. 
Kworkers can only execute one task at a time. If the periodic work is 
already blocking and currently processing an AdminQ message it might go 
to sleep for a while until it can process the message. Even if the task 
goes to sleep, the other kthread item cannot execute on the same 
kworker. This can potentially result in a delay long enough to prevent 
the external timestamp work function from reading the external timestamp 
within the time limit. I believe this results in missed external 
timestamp events. I am not certain if this loss is permanent or only 
transient.

I would consider that a bug, because it results in loss of 
functionality, not just something like lower CPU usage.

> 
> code LGTM, although i find it wasteful to create a kthread per device event
> type, but i can't think of a better way.
> 

Right. I can't think of another good way either. I think we can't 
process this directly in the interrupt which is why we had processed it 
in a kthread item to begin with.

Thanks,
Jake

Re: [PATCH net 1/4] ice: Create a separate kthread to handle ptp extts work

[Richard Cochran]

On Wed, Dec 07, 2022 at 01:10:37PM -0800, Tony Nguyen wrote:
> From: Anatolii Gerasymenko <anatolii.gerasymenko@intel.com>
> 
> ice_ptp_extts_work() and ice_ptp_periodic_work() are both scheduled on
> the same kthread_worker pf.ptp.kworker. But, ice_ptp_periodic_work()
> sends messages to AQ and waits for responses. This causes
> ice_ptp_extts_work() to be blocked while waiting to be scheduled. This
> causes problems with the reading of the incoming signal timestamps,
> which disrupts a 100 Hz signal.
> 
> Create an additional kthread_worker pf.ptp.kworker_extts to service only
> ice_ptp_extts_work() as soon as possible.

Looks like this driver isn't using the do_aux_work callback.  That
would provide a kthread worker for free.  Why not use that?

Thanks,
Richard

Re: [PATCH net 1/4] ice: Create a separate kthread to handle ptp extts work

[Tony Nguyen]

On 12/7/2022 4:27 PM, Richard Cochran wrote:
> On Wed, Dec 07, 2022 at 01:10:37PM -0800, Tony Nguyen wrote:
>> From: Anatolii Gerasymenko <anatolii.gerasymenko@intel.com>
>>
>> ice_ptp_extts_work() and ice_ptp_periodic_work() are both scheduled on
>> the same kthread_worker pf.ptp.kworker. But, ice_ptp_periodic_work()
>> sends messages to AQ and waits for responses. This causes
>> ice_ptp_extts_work() to be blocked while waiting to be scheduled. This
>> causes problems with the reading of the incoming signal timestamps,
>> which disrupts a 100 Hz signal.
>>
>> Create an additional kthread_worker pf.ptp.kworker_extts to service only
>> ice_ptp_extts_work() as soon as possible.
> 
> Looks like this driver isn't using the do_aux_work callback.  That
> would provide a kthread worker for free.  Why not use that?

The author is no longer with Intel anymore; we have another developer 
who is looking into this. Will make the change or get back to you.

Thanks,
Tony

RE: [PATCH net 2/4] ice: Correctly handle aux device when num channels change

[Ertman, David M]

> -----Original Message-----
> From: Saeed Mahameed <saeed@kernel.org>
> Sent: Wednesday, December 7, 2022 2:26 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> edumazet@google.com; Ertman, David M <david.m.ertman@intel.com>;
> netdev@vger.kernel.org; Saleem, Shiraz <shiraz.saleem@intel.com>; Ismail,
> Mustafa <mustafa.ismail@intel.com>; jgg@nvidia.com; leonro@nvidia.com;
> linux-rdma@vger.kernel.org; G, GurucharanX <gurucharanx.g@intel.com>
> Subject: Re: [PATCH net 2/4] ice: Correctly handle aux device when num
> channels change
> 
> On 07 Dec 13:10, Tony Nguyen wrote:
> >From: Dave Ertman <david.m.ertman@intel.com>
> >
> >When the number of channels/queues changes on an interface, it is
> necessary
> >to change how those resources are distributed to the auxiliary device for
> >maintaining RDMA functionality.  To do this, the best way is to unplug, and
> 
> Can you please explain how an ethtool can affect RDMA functionality ?
> don't you have full bifurcation between the two eth and rdma interfaces ..
> 
This patch is to address a bug where the number of queues for the interface was
changed and the RDMA lost functionality due to queues being re-assigned.

The PF is managing and setting aside resources for the RDMA aux dev. Then the 
RDMA aux driver will request resources from the PF driver.  Changes in
the total number of resources make it so that resources previously
allocated to RDMA aux driver may not be available any more.  A re-allocation
is necessary to ensure that RDMA has all of the queues that it thinks it does.

> >then re-plug the auxiliary device.  This will cause all current resource
> >allocation to be released, and then re-requested under the new state.
> >
> 
> I find this really disruptive, changing number of netdev queues to cause
> full aux devs restart !
> 

Changing the number of queues available to the interface *is* a disruptive action.
The netdev  and VSI have to be re-configured for queues per TC and the RDMA aux
driver has to re-allocate qsets to attach queue-pairs to.

> >Since the set_channel command from ethtool comes in while holding the
> RTNL
> >lock, it is necessary to offset the plugging and unplugging of auxiliary
> >device to another context.  For this purpose, set the flags for UNPLUG and
> >PLUG in the PF state, then respond to them in the service task.
> >
> >Also, since the auxiliary device will be unplugged/plugged at the end of
> >the flow, it is better to not send the event for TCs changing in the
> >middle of the flow.  This will prevent a timing issue between the events
> >and the probe/release calls conflicting.
> >
> >Fixes: 348048e724a0 ("ice: Implement iidc operations")
> >Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
> >Tested-by: Gurucharan G <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.h         | 2 ++
> > drivers/net/ethernet/intel/ice/ice_ethtool.c | 6 ++++++
> > drivers/net/ethernet/intel/ice/ice_idc.c     | 3 +++
> > drivers/net/ethernet/intel/ice/ice_main.c    | 3 +++
> > 4 files changed, 14 insertions(+)
> >
> >diff --git a/drivers/net/ethernet/intel/ice/ice.h
> b/drivers/net/ethernet/intel/ice/ice.h
> >index 001500afc4a6..092e572768fe 100644
> >--- a/drivers/net/ethernet/intel/ice/ice.h
> >+++ b/drivers/net/ethernet/intel/ice/ice.h
> >@@ -281,6 +281,7 @@ enum ice_pf_state {
> > 	ICE_FLTR_OVERFLOW_PROMISC,
> > 	ICE_VF_DIS,
> > 	ICE_CFG_BUSY,
> >+	ICE_SET_CHANNELS,
> > 	ICE_SERVICE_SCHED,
> > 	ICE_SERVICE_DIS,
> > 	ICE_FD_FLUSH_REQ,
> >@@ -485,6 +486,7 @@ enum ice_pf_flags {
> > 	ICE_FLAG_VF_VLAN_PRUNING,
> > 	ICE_FLAG_LINK_LENIENT_MODE_ENA,
> > 	ICE_FLAG_PLUG_AUX_DEV,
> >+	ICE_FLAG_UNPLUG_AUX_DEV,
> > 	ICE_FLAG_MTU_CHANGED,
> > 	ICE_FLAG_GNSS,			/* GNSS successfully
> initialized */
> > 	ICE_PF_FLAGS_NBITS		/* must be last */
> >diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> >index b7be84bbe72d..37e174a19860 100644
> >--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> >+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> >@@ -3536,6 +3536,8 @@ static int ice_set_channels(struct net_device
> *dev, struct ethtool_channels *ch)
> > 		return -EINVAL;
> > 	}
> >
> >+	set_bit(ICE_SET_CHANNELS, pf->state);
> >+
> > 	ice_vsi_recfg_qs(vsi, new_rx, new_tx);
> >
> > 	if (!netif_is_rxfh_configured(dev))
> >@@ -3543,6 +3545,10 @@ static int ice_set_channels(struct net_device
> *dev, struct ethtool_channels *ch)
> >
> > 	/* Update rss_size due to change in Rx queues */
> > 	vsi->rss_size = ice_get_valid_rss_size(&pf->hw, new_rx);
> >+	clear_bit(ICE_SET_CHANNELS, pf->state);
> >+
> 
> you just set this new state a few lines ago, clearing the bit in the same
> function few lines later seems to be an abuse of the pf state machine,
> couldn't you just pass a parameter to the functions which needed this
> information ?
> 

How is this abusing the PF state machine?  There is a 3 deep function call that needs
the information that this is a set_channel context, and each of those functions is called
from several locations - how is changing all of those functions to include a parameter
(that will be false for all of them but this instance) be less abusive than setting and
clearing a bit?

> >+	set_bit(ICE_FLAG_UNPLUG_AUX_DEV, pf->flags);
> >+	set_bit(ICE_FLAG_PLUG_AUX_DEV, pf->flags);
> >
> > 	return 0;
> > }

Re: [PATCH net 2/4] ice: Correctly handle aux device when num channels change

[Saeed Mahameed]

On 09 Dec 17:21, Ertman, David M wrote:
>> -----Original Message-----
>> From: Saeed Mahameed <saeed@kernel.org>
>> Sent: Wednesday, December 7, 2022 2:26 PM
>> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
>> Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
>> edumazet@google.com; Ertman, David M <david.m.ertman@intel.com>;
>> netdev@vger.kernel.org; Saleem, Shiraz <shiraz.saleem@intel.com>; Ismail,
>> Mustafa <mustafa.ismail@intel.com>; jgg@nvidia.com; leonro@nvidia.com;
>> linux-rdma@vger.kernel.org; G, GurucharanX <gurucharanx.g@intel.com>
>> Subject: Re: [PATCH net 2/4] ice: Correctly handle aux device when num
>> channels change
>>
>> On 07 Dec 13:10, Tony Nguyen wrote:
>> >From: Dave Ertman <david.m.ertman@intel.com>
>> >
>> >When the number of channels/queues changes on an interface, it is
>> necessary
>> >to change how those resources are distributed to the auxiliary device for
>> >maintaining RDMA functionality.  To do this, the best way is to unplug, and
>>
>> Can you please explain how an ethtool can affect RDMA functionality ?
>> don't you have full bifurcation between the two eth and rdma interfaces ..
>>
>This patch is to address a bug where the number of queues for the interface was
>changed and the RDMA lost functionality due to queues being re-assigned.
>

sounds like an rdma or PF bug.

>The PF is managing and setting aside resources for the RDMA aux dev. Then the
>RDMA aux driver will request resources from the PF driver.  Changes in
>the total number of resources make it so that resources previously
>allocated to RDMA aux driver may not be available any more.  A re-allocation
>is necessary to ensure that RDMA has all of the queues that it thinks it does.
>

IMO it's wrong to re-initialize a parallel subsystems due to an ethtool, 
ethtool is meant to control the netdev interface, not rdma. Either
statically allocate these resources on boot or just store them in a free 
pool in PF until next time rdma reloads by an rdma user command outside of
netdev.

>> >then re-plug the auxiliary device.  This will cause all current resource
>> >allocation to be released, and then re-requested under the new state.
>> >
>>
>> I find this really disruptive, changing number of netdev queues to cause
>> full aux devs restart !
>>
>
>Changing the number of queues available to the interface *is* a disruptive action.

yes to the netdev, not to rdma or usb, or the pci bus, you get my point.

>The netdev  and VSI have to be re-configured for queues per TC and the RDMA aux
>driver has to re-allocate qsets to attach queue-pairs to.
>

sure for netdev, but it doesn't make sense to reload rdma, if rdma was
using X queues, it should continue using X queues.. if you can't support
dynamic netdev changes without disturbing rdma, then block ethtool until
user unloads rdma. 

>> >Since the set_channel command from ethtool comes in while holding the
>> RTNL
>> >lock, it is necessary to offset the plugging and unplugging of auxiliary
>> >device to another context.  For this purpose, set the flags for UNPLUG and
>> >PLUG in the PF state, then respond to them in the service task.
>> >
>> >Also, since the auxiliary device will be unplugged/plugged at the end of
>> >the flow, it is better to not send the event for TCs changing in the
>> >middle of the flow.  This will prevent a timing issue between the events
>> >and the probe/release calls conflicting.
>> >
>> >Fixes: 348048e724a0 ("ice: Implement iidc operations")
>> >Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
>> >Tested-by: Gurucharan G <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.h         | 2 ++
>> > drivers/net/ethernet/intel/ice/ice_ethtool.c | 6 ++++++
>> > drivers/net/ethernet/intel/ice/ice_idc.c     | 3 +++
>> > drivers/net/ethernet/intel/ice/ice_main.c    | 3 +++
>> > 4 files changed, 14 insertions(+)
>> >
>> >diff --git a/drivers/net/ethernet/intel/ice/ice.h
>> b/drivers/net/ethernet/intel/ice/ice.h
>> >index 001500afc4a6..092e572768fe 100644
>> >--- a/drivers/net/ethernet/intel/ice/ice.h
>> >+++ b/drivers/net/ethernet/intel/ice/ice.h
>> >@@ -281,6 +281,7 @@ enum ice_pf_state {
>> > 	ICE_FLTR_OVERFLOW_PROMISC,
>> > 	ICE_VF_DIS,
>> > 	ICE_CFG_BUSY,
>> >+	ICE_SET_CHANNELS,
>> > 	ICE_SERVICE_SCHED,
>> > 	ICE_SERVICE_DIS,
>> > 	ICE_FD_FLUSH_REQ,
>> >@@ -485,6 +486,7 @@ enum ice_pf_flags {
>> > 	ICE_FLAG_VF_VLAN_PRUNING,
>> > 	ICE_FLAG_LINK_LENIENT_MODE_ENA,
>> > 	ICE_FLAG_PLUG_AUX_DEV,
>> >+	ICE_FLAG_UNPLUG_AUX_DEV,
>> > 	ICE_FLAG_MTU_CHANGED,
>> > 	ICE_FLAG_GNSS,			/* GNSS successfully
>> initialized */
>> > 	ICE_PF_FLAGS_NBITS		/* must be last */
>> >diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c
>> b/drivers/net/ethernet/intel/ice/ice_ethtool.c
>> >index b7be84bbe72d..37e174a19860 100644
>> >--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
>> >+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
>> >@@ -3536,6 +3536,8 @@ static int ice_set_channels(struct net_device
>> *dev, struct ethtool_channels *ch)
>> > 		return -EINVAL;
>> > 	}
>> >
>> >+	set_bit(ICE_SET_CHANNELS, pf->state);
>> >+
>> > 	ice_vsi_recfg_qs(vsi, new_rx, new_tx);
>> >
>> > 	if (!netif_is_rxfh_configured(dev))
>> >@@ -3543,6 +3545,10 @@ static int ice_set_channels(struct net_device
>> *dev, struct ethtool_channels *ch)
>> >
>> > 	/* Update rss_size due to change in Rx queues */
>> > 	vsi->rss_size = ice_get_valid_rss_size(&pf->hw, new_rx);
>> >+	clear_bit(ICE_SET_CHANNELS, pf->state);
>> >+
>>
>> you just set this new state a few lines ago, clearing the bit in the same
>> function few lines later seems to be an abuse of the pf state machine,
>> couldn't you just pass a parameter to the functions which needed this
>> information ?
>>
>
>How is this abusing the PF state machine?  There is a 3 deep function call that needs
>the information that this is a set_channel context, and each of those functions is called
>from several locations - how is changing all of those functions to include a parameter

this is exactly the abuse i was talking about, setting a flag on a global
state field because the function call is too deep.

>(that will be false for all of them but this instance) be less abusive than setting and
>clearing a bit?

this is not future sustainable and not reviewer friendly, it's a local
parameter and shouldn't be a global flag.

Anyway this is your driver, i am not going to force you to do something you
don't like.

but for the reloading of rdma due to an ethtool is a no for me..
let's find a common place for all vendors to express such limitations,
e.g. devlink .. 

>
>> >+	set_bit(ICE_FLAG_UNPLUG_AUX_DEV, pf->flags);
>> >+	set_bit(ICE_FLAG_PLUG_AUX_DEV, pf->flags);
>> >
>> > 	return 0;
>> > }
>

Re: [PATCH net 2/4] ice: Correctly handle aux device when num channels change

[Jason Gunthorpe]

On Fri, Dec 09, 2022 at 11:28:28AM -0800, Saeed Mahameed wrote:

> IMO it's wrong to re-initialize a parallel subsystems due to an ethtool,
> ethtool is meant to control the netdev interface, not rdma.

We've gotten into locking trouble doing stuff like this before.

If you are holding any locks do not try to unplug/plug an aux device.

Jason

RE: [PATCH net 2/4] ice: Correctly handle aux device when num channels change

[Ertman, David M]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Friday, December 9, 2022 11:33 AM
> To: Saeed Mahameed <saeed@kernel.org>
> Cc: Ertman, David M <david.m.ertman@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; davem@davemloft.net; kuba@kernel.org;
> pabeni@redhat.com; edumazet@google.com; netdev@vger.kernel.org;
> Saleem, Shiraz <shiraz.saleem@intel.com>; Ismail, Mustafa
> <mustafa.ismail@intel.com>; leonro@nvidia.com; linux-
> rdma@vger.kernel.org; G, GurucharanX <gurucharanx.g@intel.com>
> Subject: Re: [PATCH net 2/4] ice: Correctly handle aux device when num
> channels change
> 
> On Fri, Dec 09, 2022 at 11:28:28AM -0800, Saeed Mahameed wrote:
> 
> > IMO it's wrong to re-initialize a parallel subsystems due to an ethtool,
> > ethtool is meant to control the netdev interface, not rdma.
> 
> We've gotten into locking trouble doing stuff like this before.
> 
> If you are holding any locks do not try to unplug/plug an aux device.
> 
> Jason

The unplug/plug is done outside the ethtool context.  No locks are being held.

DaveE

Re: [PATCH net 2/4] ice: Correctly handle aux device when num channels change

[Jason Gunthorpe]

On Mon, Dec 12, 2022 at 05:03:39PM +0000, Ertman, David M wrote:
> > On Fri, Dec 09, 2022 at 11:28:28AM -0800, Saeed Mahameed wrote:
> > 
> > > IMO it's wrong to re-initialize a parallel subsystems due to an ethtool,
> > > ethtool is meant to control the netdev interface, not rdma.
> > 
> > We've gotten into locking trouble doing stuff like this before.
> > 
> > If you are holding any locks do not try to unplug/plug an aux device.
> > 
> > Jason
> 
> The unplug/plug is done outside the ethtool context.  No locks are being held.

That's a good, trick, so I'm skeptical *no* locks are held.

Jason

RE: [PATCH net 2/4] ice: git send-email --suppress-cc=all --to e1000-patches@eclists.intel.com

[Ertman, David M]

> -----Original Message-----
> From: Saeed Mahameed <saeed@kernel.org>
> Sent: Friday, December 9, 2022 11:28 AM
> To: Ertman, David M <david.m.ertman@intel.com>
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>;
> davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> edumazet@google.com; netdev@vger.kernel.org; Saleem, Shiraz
> <shiraz.saleem@intel.com>; Ismail, Mustafa <mustafa.ismail@intel.com>;
> jgg@nvidia.com; leonro@nvidia.com; linux-rdma@vger.kernel.org; G,
> GurucharanX <gurucharanx.g@intel.com>
> Subject: Re: [PATCH net 2/4] ice: Correctly handle aux device when num
> channels change
> 
> On 09 Dec 17:21, Ertman, David M wrote:
> >> -----Original Message-----
> >> From: Saeed Mahameed <saeed@kernel.org>
> >> Sent: Wednesday, December 7, 2022 2:26 PM
> >> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> >> Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> >> edumazet@google.com; Ertman, David M <david.m.ertman@intel.com>;
> >> netdev@vger.kernel.org; Saleem, Shiraz <shiraz.saleem@intel.com>;
> Ismail,
> >> Mustafa <mustafa.ismail@intel.com>; jgg@nvidia.com;
> leonro@nvidia.com;
> >> linux-rdma@vger.kernel.org; G, GurucharanX
> <gurucharanx.g@intel.com>
> >> Subject: Re: [PATCH net 2/4] ice: Correctly handle aux device when num
> >> channels change
> >>
> >> On 07 Dec 13:10, Tony Nguyen wrote:
> >> >From: Dave Ertman <david.m.ertman@intel.com>
> >> >
> >> >When the number of channels/queues changes on an interface, it is
> >> necessary
> >> >to change how those resources are distributed to the auxiliary device for
> >> >maintaining RDMA functionality.  To do this, the best way is to unplug,
> and
> >>
> >> Can you please explain how an ethtool can affect RDMA functionality ?
> >> don't you have full bifurcation between the two eth and rdma interfaces
> ..
> >>
> >This patch is to address a bug where the number of queues for the
> interface was
> >changed and the RDMA lost functionality due to queues being re-assigned.
> >
> 
> sounds like an rdma or PF bug.
> 
> >The PF is managing and setting aside resources for the RDMA aux dev. Then
> the
> >RDMA aux driver will request resources from the PF driver.  Changes in
> >the total number of resources make it so that resources previously
> >allocated to RDMA aux driver may not be available any more.  A re-
> allocation
> >is necessary to ensure that RDMA has all of the queues that it thinks it does.
> >
> 
> IMO it's wrong to re-initialize a parallel subsystems due to an ethtool,
> ethtool is meant to control the netdev interface, not rdma. Either
> statically allocate these resources on boot or just store them in a free
> pool in PF until next time rdma reloads by an rdma user command outside of
> netdev.
> 
> >> >then re-plug the auxiliary device.  This will cause all current resource
> >> >allocation to be released, and then re-requested under the new state.
> >> >
> >>
> >> I find this really disruptive, changing number of netdev queues to cause
> >> full aux devs restart !
> >>
> >
> >Changing the number of queues available to the interface *is* a disruptive
> action.
> 
> yes to the netdev, not to rdma or usb, or the pci bus, you get my point.
> 
> >The netdev  and VSI have to be re-configured for queues per TC and the
> RDMA aux
> >driver has to re-allocate qsets to attach queue-pairs to.
> >
> 
> sure for netdev, but it doesn't make sense to reload rdma, if rdma was
> using X queues, it should continue using X queues.. if you can't support
> dynamic netdev changes without disturbing rdma, then block ethtool until
> user unloads rdma.
> 
> >> >Since the set_channel command from ethtool comes in while holding
> the
> >> RTNL
> >> >lock, it is necessary to offset the plugging and unplugging of auxiliary
> >> >device to another context.  For this purpose, set the flags for UNPLUG
> and
> >> >PLUG in the PF state, then respond to them in the service task.
> >> >
> >> >Also, since the auxiliary device will be unplugged/plugged at the end of
> >> >the flow, it is better to not send the event for TCs changing in the
> >> >middle of the flow.  This will prevent a timing issue between the events
> >> >and the probe/release calls conflicting.
> >> >
> >> >Fixes: 348048e724a0 ("ice: Implement iidc operations")
> >> >Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
> >> >Tested-by: Gurucharan G <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.h         | 2 ++
> >> > drivers/net/ethernet/intel/ice/ice_ethtool.c | 6 ++++++
> >> > drivers/net/ethernet/intel/ice/ice_idc.c     | 3 +++
> >> > drivers/net/ethernet/intel/ice/ice_main.c    | 3 +++
> >> > 4 files changed, 14 insertions(+)
> >> >
> >> >diff --git a/drivers/net/ethernet/intel/ice/ice.h
> >> b/drivers/net/ethernet/intel/ice/ice.h
> >> >index 001500afc4a6..092e572768fe 100644
> >> >--- a/drivers/net/ethernet/intel/ice/ice.h
> >> >+++ b/drivers/net/ethernet/intel/ice/ice.h
> >> >@@ -281,6 +281,7 @@ enum ice_pf_state {
> >> > 	ICE_FLTR_OVERFLOW_PROMISC,
> >> > 	ICE_VF_DIS,
> >> > 	ICE_CFG_BUSY,
> >> >+	ICE_SET_CHANNELS,
> >> > 	ICE_SERVICE_SCHED,
> >> > 	ICE_SERVICE_DIS,
> >> > 	ICE_FD_FLUSH_REQ,
> >> >@@ -485,6 +486,7 @@ enum ice_pf_flags {
> >> > 	ICE_FLAG_VF_VLAN_PRUNING,
> >> > 	ICE_FLAG_LINK_LENIENT_MODE_ENA,
> >> > 	ICE_FLAG_PLUG_AUX_DEV,
> >> >+	ICE_FLAG_UNPLUG_AUX_DEV,
> >> > 	ICE_FLAG_MTU_CHANGED,
> >> > 	ICE_FLAG_GNSS,			/* GNSS successfully
> >> initialized */
> >> > 	ICE_PF_FLAGS_NBITS		/* must be last */
> >> >diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> >> b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> >> >index b7be84bbe72d..37e174a19860 100644
> >> >--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> >> >+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> >> >@@ -3536,6 +3536,8 @@ static int ice_set_channels(struct net_device
> >> *dev, struct ethtool_channels *ch)
> >> > 		return -EINVAL;
> >> > 	}
> >> >
> >> >+	set_bit(ICE_SET_CHANNELS, pf->state);
> >> >+
> >> > 	ice_vsi_recfg_qs(vsi, new_rx, new_tx);
> >> >
> >> > 	if (!netif_is_rxfh_configured(dev))
> >> >@@ -3543,6 +3545,10 @@ static int ice_set_channels(struct net_device
> >> *dev, struct ethtool_channels *ch)
> >> >
> >> > 	/* Update rss_size due to change in Rx queues */
> >> > 	vsi->rss_size = ice_get_valid_rss_size(&pf->hw, new_rx);
> >> >+	clear_bit(ICE_SET_CHANNELS, pf->state);
> >> >+
> >>
> >> you just set this new state a few lines ago, clearing the bit in the same
> >> function few lines later seems to be an abuse of the pf state machine,
> >> couldn't you just pass a parameter to the functions which needed this
> >> information ?
> >>
> >
> >How is this abusing the PF state machine?  There is a 3 deep function call
> that needs
> >the information that this is a set_channel context, and each of those
> functions is called
> >from several locations - how is changing all of those functions to include a
> parameter
> 
> this is exactly the abuse i was talking about, setting a flag on a global
> state field because the function call is too deep.
> 
> >(that will be false for all of them but this instance) be less abusive than
> setting and
> >clearing a bit?
> 
> this is not future sustainable and not reviewer friendly, it's a local
> parameter and shouldn't be a global flag.
> 
> Anyway this is your driver, i am not going to force you to do something you
> don't like.
> 
> but for the reloading of rdma due to an ethtool is a no for me..
> let's find a common place for all vendors to express such limitations,
> e.g. devlink ..

The set_channels function will cause the VSI controlling the pool of resources that
RDMA uses to be re-allocated ad reconfigured.  So, based on this, I will submit a new patch
to prevent set_channels when RDMA is probed as you suggested.

Please abandon this patch, new one will be submitted.

DaveE



> 
> >
> >> >+	set_bit(ICE_FLAG_UNPLUG_AUX_DEV, pf->flags);
> >> >+	set_bit(ICE_FLAG_PLUG_AUX_DEV, pf->flags);
> >> >
> >> > 	return 0;
> >> > }
> >

Re: [PATCH net 1/4] ice: Create a separate kthread to handle ptp extts work

[Richard Cochran]

On Mon, Dec 12, 2022 at 11:24:22PM +0000, Kolacinski, Karol wrote:

> According to do_aux_work description, it is used to do 'auxiliary (periodic)
> operations'.
> ice_ptp_extts_work is not exactly periodic as the work is queued only when the
> interrupt comes.

You can use it in any way that you like, periodic or not.  The return
value from the driver callback determines if and when the next
callback occurs.

Thanks,
Richard

Re: [PATCH net 1/4] ice: Create a separate kthread to handle ptp extts work

[Kolacinski, Karol]

>On 12/7/2022 4:27 PM, Richard Cochran wrote:
>> On Wed, Dec 07, 2022 at 01:10:37PM -0800, Tony Nguyen wrote:
>>> From: Anatolii Gerasymenko anatolii.gerasymenko@intel.com
>>>
>>> ice_ptp_extts_work() and ice_ptp_periodic_work() are both scheduled on
>>> the same kthread_worker pf.ptp.kworker. But, ice_ptp_periodic_work()
>>> sends messages to AQ and waits for responses. This causes
>>> ice_ptp_extts_work() to be blocked while waiting to be scheduled. This
>>> causes problems with the reading of the incoming signal timestamps,
>>> which disrupts a 100 Hz signal.
>>>
>>> Create an additional kthread_worker pf.ptp.kworker_extts to service only
>>> ice_ptp_extts_work() as soon as possible.
>> 
>> Looks like this driver isn't using the do_aux_work callback.  That
>> would provide a kthread worker for free.  Why not use that?

According to do_aux_work description, it is used to do 'auxiliary (periodic)
operations'.
ice_ptp_extts_work is not exactly periodic as the work is queued only when the
interrupt comes.

Thanks,
Karol