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

[Saeed Mahameed <saeed@kernel.org>]

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
>