Re: [Intel-wired-lan] [PATCH iwl-net v1] igc: Add lock to safeguard global Qbv variables

[Leon Romanovsky <leon@kernel.org>]

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