All of lore.kernel.org
 help / color / mirror / Atom feed
* [Intel-wired-lan] [PATCH iwl-net v1] igc: Add lock to safeguard global Qbv variables
@ 2023-07-11  4:08 Muhammad Husaini Zulkifli
  2023-07-11  7:45 ` Leon Romanovsky
  0 siblings, 1 reply; 3+ messages in thread
From: Muhammad Husaini Zulkifli @ 2023-07-11  4:08 UTC (permalink / raw)
  To: intel-wired-lan; +Cc: leon, anthony.l.nguyen

Access to shared variables through hrtimer requires locking in order
to protect the variables because actions to write into these variables
(oper_gate_closed, admin_gate_closed, and qbv_transition) might potentially
occur simultaneously. This patch provides a locking mechanisms to avoid
such scenarios.

Fixes: 175c241288c0 ("igc: Fix TX Hang issue when QBV Gate is closed")
Suggested-by: Leon Romanovsky <leon@kernel.org>
Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
---
 drivers/net/ethernet/intel/igc/igc.h      | 4 ++++
 drivers/net/ethernet/intel/igc/igc_main.c | 7 +++++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 9db384f66a8e..38901d2a4680 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -195,6 +195,10 @@ struct igc_adapter {
 	u32 qbv_config_change_errors;
 	bool qbv_transition;
 	unsigned int qbv_count;
+	/* Access to oper_gate_closed, admin_gate_closed and qbv_transition
+	 * are protected by the qbv_tx_lock.
+	 */
+	spinlock_t qbv_tx_lock;
 
 	/* OS defined structs */
 	struct pci_dev *pdev;
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index bdeb36790d77..cae717cb506e 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -4801,6 +4801,7 @@ static int igc_sw_init(struct igc_adapter *adapter)
 	adapter->nfc_rule_count = 0;
 
 	spin_lock_init(&adapter->stats64_lock);
+	spin_lock_init(&adapter->qbv_tx_lock);
 	/* Assume MSI-X interrupts, will be checked during IRQ allocation */
 	adapter->flags |= IGC_FLAG_HAS_MSIX;
 
@@ -6619,8 +6620,11 @@ static enum hrtimer_restart igc_qbv_scheduling_timer(struct hrtimer *timer)
 {
 	struct igc_adapter *adapter = container_of(timer, struct igc_adapter,
 						   hrtimer);
+	unsigned long flags;
 	unsigned int i;
 
+	spin_lock_irqsave(&adapter->qbv_tx_lock, flags);
+
 	adapter->qbv_transition = true;
 	for (i = 0; i < adapter->num_tx_queues; i++) {
 		struct igc_ring *tx_ring = adapter->tx_ring[i];
@@ -6633,6 +6637,9 @@ static enum hrtimer_restart igc_qbv_scheduling_timer(struct hrtimer *timer)
 		}
 	}
 	adapter->qbv_transition = false;
+
+	spin_unlock_irqrestore(&adapter->qbv_tx_lock, flags);
+
 	return HRTIMER_NORESTART;
 }
 
-- 
2.17.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net v1] igc: Add lock to safeguard global Qbv variables
  2023-07-11  4:08 [Intel-wired-lan] [PATCH iwl-net v1] igc: Add lock to safeguard global Qbv variables Muhammad Husaini Zulkifli
@ 2023-07-11  7:45 ` Leon Romanovsky
  2023-07-16  5:55   ` Zulkifli, Muhammad Husaini
  0 siblings, 1 reply; 3+ messages in thread
From: Leon Romanovsky @ 2023-07-11  7:45 UTC (permalink / raw)
  To: Muhammad Husaini Zulkifli; +Cc: intel-wired-lan, anthony.l.nguyen

On Tue, Jul 11, 2023 at 12:08:20PM +0800, Muhammad Husaini Zulkifli wrote:
> Access to shared variables through hrtimer requires locking in order
> to protect the variables because actions to write into these variables
> (oper_gate_closed, admin_gate_closed, and qbv_transition) might potentially
> occur simultaneously. This patch provides a locking mechanisms to avoid
> such scenarios.

I have a general comment as this patch is targeted to iwl-next and not to net-next,
the locking should protect access to shared variables and it means that
lock should be used in all places where these variables are used and not
only in one function.

Thanks

> 
> Fixes: 175c241288c0 ("igc: Fix TX Hang issue when QBV Gate is closed")
> Suggested-by: Leon Romanovsky <leon@kernel.org>
> Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc.h      | 4 ++++
>  drivers/net/ethernet/intel/igc/igc_main.c | 7 +++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index 9db384f66a8e..38901d2a4680 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -195,6 +195,10 @@ struct igc_adapter {
>  	u32 qbv_config_change_errors;
>  	bool qbv_transition;
>  	unsigned int qbv_count;
> +	/* Access to oper_gate_closed, admin_gate_closed and qbv_transition
> +	 * are protected by the qbv_tx_lock.
> +	 */
> +	spinlock_t qbv_tx_lock;
>  
>  	/* OS defined structs */
>  	struct pci_dev *pdev;
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index bdeb36790d77..cae717cb506e 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -4801,6 +4801,7 @@ static int igc_sw_init(struct igc_adapter *adapter)
>  	adapter->nfc_rule_count = 0;
>  
>  	spin_lock_init(&adapter->stats64_lock);
> +	spin_lock_init(&adapter->qbv_tx_lock);
>  	/* Assume MSI-X interrupts, will be checked during IRQ allocation */
>  	adapter->flags |= IGC_FLAG_HAS_MSIX;
>  
> @@ -6619,8 +6620,11 @@ static enum hrtimer_restart igc_qbv_scheduling_timer(struct hrtimer *timer)
>  {
>  	struct igc_adapter *adapter = container_of(timer, struct igc_adapter,
>  						   hrtimer);
> +	unsigned long flags;
>  	unsigned int i;
>  
> +	spin_lock_irqsave(&adapter->qbv_tx_lock, flags);
> +
>  	adapter->qbv_transition = true;
>  	for (i = 0; i < adapter->num_tx_queues; i++) {
>  		struct igc_ring *tx_ring = adapter->tx_ring[i];
> @@ -6633,6 +6637,9 @@ static enum hrtimer_restart igc_qbv_scheduling_timer(struct hrtimer *timer)
>  		}
>  	}
>  	adapter->qbv_transition = false;
> +
> +	spin_unlock_irqrestore(&adapter->qbv_tx_lock, flags);
> +
>  	return HRTIMER_NORESTART;
>  }
>  
> -- 
> 2.17.1
> 
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net v1] igc: Add lock to safeguard global Qbv variables
  2023-07-11  7:45 ` Leon Romanovsky
@ 2023-07-16  5:55   ` Zulkifli, Muhammad Husaini
  0 siblings, 0 replies; 3+ messages in thread
From: Zulkifli, Muhammad Husaini @ 2023-07-16  5:55 UTC (permalink / raw)
  To: Leon Romanovsky; +Cc: intel-wired-lan, Nguyen, Anthony L

Dear Leon,

Sorry for the late response. Replied inline.

> -----Original Message-----
> From: Leon Romanovsky <leon@kernel.org>
> Sent: Tuesday, 11 July, 2023 3:45 PM
> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
> Cc: intel-wired-lan@osuosl.org; Neftin, Sasha <sasha.neftin@intel.com>;
> Gomes, Vinicius <vinicius.gomes@intel.com>; naamax.meir@linux.intel.com;
> Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Subject: Re: [PATCH iwl-net v1] igc: Add lock to safeguard global Qbv
> variables
> 
> On Tue, Jul 11, 2023 at 12:08:20PM +0800, Muhammad Husaini Zulkifli wrote:
> > Access to shared variables through hrtimer requires locking in order
> > to protect the variables because actions to write into these variables
> > (oper_gate_closed, admin_gate_closed, and qbv_transition) might
> > potentially occur simultaneously. This patch provides a locking
> > mechanisms to avoid such scenarios.
> 
> I have a general comment as this patch is targeted to iwl-next and not to net-
> next, the locking should protect access to shared variables and it means that
> lock should be used in all places where these variables are used and not only
> in one function.

Previous patch of "igc: Fix TX Hang issue when QBV Gate is closed" was submitted to
Iwl-net. This patch were introduced as a fix to that patch. Should we submit to iwl-net 
instead of Iwl-net-next?

You're correct. To prohibit unauthorized access to these variables in several locations, 
let me send the v2. These variables could be modified with different contexts.
Looks like, we might need to modify code in igc_tsn_clear_schedule() and 
igc_save_qbv_schedule() as well. I was thinking about separating the qbv portion
into a diff function to improve the readability of the code in igc_tsn_clear_schedule(). 
Let me send version 2 and see if that is OK.

Thanks,
Husaini

> 
> Thanks
> 
> >
> > Fixes: 175c241288c0 ("igc: Fix TX Hang issue when QBV Gate is closed")
> > Suggested-by: Leon Romanovsky <leon@kernel.org>
> > Signed-off-by: Muhammad Husaini Zulkifli
> > <muhammad.husaini.zulkifli@intel.com>
> > ---
> >  drivers/net/ethernet/intel/igc/igc.h      | 4 ++++
> >  drivers/net/ethernet/intel/igc/igc_main.c | 7 +++++++
> >  2 files changed, 11 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/intel/igc/igc.h
> > b/drivers/net/ethernet/intel/igc/igc.h
> > index 9db384f66a8e..38901d2a4680 100644
> > --- a/drivers/net/ethernet/intel/igc/igc.h
> > +++ b/drivers/net/ethernet/intel/igc/igc.h
> > @@ -195,6 +195,10 @@ struct igc_adapter {
> >  	u32 qbv_config_change_errors;
> >  	bool qbv_transition;
> >  	unsigned int qbv_count;
> > +	/* Access to oper_gate_closed, admin_gate_closed and
> qbv_transition
> > +	 * are protected by the qbv_tx_lock.
> > +	 */
> > +	spinlock_t qbv_tx_lock;
> >
> >  	/* OS defined structs */
> >  	struct pci_dev *pdev;
> > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c
> > b/drivers/net/ethernet/intel/igc/igc_main.c
> > index bdeb36790d77..cae717cb506e 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_main.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> > @@ -4801,6 +4801,7 @@ static int igc_sw_init(struct igc_adapter *adapter)
> >  	adapter->nfc_rule_count = 0;
> >
> >  	spin_lock_init(&adapter->stats64_lock);
> > +	spin_lock_init(&adapter->qbv_tx_lock);
> >  	/* Assume MSI-X interrupts, will be checked during IRQ allocation */
> >  	adapter->flags |= IGC_FLAG_HAS_MSIX;
> >
> > @@ -6619,8 +6620,11 @@ static enum hrtimer_restart
> > igc_qbv_scheduling_timer(struct hrtimer *timer)  {
> >  	struct igc_adapter *adapter = container_of(timer, struct igc_adapter,
> >  						   hrtimer);
> > +	unsigned long flags;
> >  	unsigned int i;
> >
> > +	spin_lock_irqsave(&adapter->qbv_tx_lock, flags);
> > +
> >  	adapter->qbv_transition = true;
> >  	for (i = 0; i < adapter->num_tx_queues; i++) {
> >  		struct igc_ring *tx_ring = adapter->tx_ring[i]; @@ -6633,6
> +6637,9
> > @@ static enum hrtimer_restart igc_qbv_scheduling_timer(struct hrtimer
> *timer)
> >  		}
> >  	}
> >  	adapter->qbv_transition = false;
> > +
> > +	spin_unlock_irqrestore(&adapter->qbv_tx_lock, flags);
> > +
> >  	return HRTIMER_NORESTART;
> >  }
> >
> > --
> > 2.17.1
> >
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-07-16  5:55 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-11  4:08 [Intel-wired-lan] [PATCH iwl-net v1] igc: Add lock to safeguard global Qbv variables Muhammad Husaini Zulkifli
2023-07-11  7:45 ` Leon Romanovsky
2023-07-16  5:55   ` Zulkifli, Muhammad Husaini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.