| Message ID | 20230711040820.16235-1-muhammad.husaini.zulkifli@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [iwl-net,v1] igc: Add lock to safeguard global Qbv variables | expand |
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 >
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 > >
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; }
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(+)