Re: [RFC PATCH net-next 1/1] cxgb4: fix missing unlock on ETHOFLD desc collect fail path

[Rahul Lakkireddy]

On Thursday, September 09/22/22, 2022 at 02:32:36 -0300, Rafael Mendonca wrote:
> The label passed to the QDESC_GET for the ETHOFLD TXQ, RXQ, and FLQ, is the
> 'out' one, which skips the 'out_unlock' label, and thus doesn't unlock the
> 'uld_mutex' before returning. Additionally, since commit 5148e5950c67
> ("cxgb4: add EOTID tracking and software context dump"), the access to
> these ETHOFLD hardware queues should be protected by the 'mqprio_mutex'
> instead.
> 
> Fixes: 2d0cb84dd973 ("cxgb4: add ETHOFLD hardware queue support")
> Fixes: 5148e5950c67 ("cxgb4: add EOTID tracking and software context dump")
> Signed-off-by: Rafael Mendonca <rafaelmendsr@xxxxxxxxx>

Yes, the ETHOFLD queues do need to be accessed after unlocking the
uld_mutex and then taking the mqprio_mutex. Thanks for the fix!

Reviewed-by: Rahul Lakkireddy <rahul.lakkireddy@xxxxxxxxxxx>


> ---
>  .../net/ethernet/chelsio/cxgb4/cudbg_lib.c    | 28 +++++++++++++------
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cudbg_lib.c b/drivers/net/ethernet/chelsio/cxgb4/cudbg_lib.c
> index a7f291c89702..557c591a6ce3 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/cudbg_lib.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/cudbg_lib.c
> @@ -14,6 +14,7 @@
>  #include "cudbg_entity.h"
>  #include "cudbg_lib.h"
>  #include "cudbg_zlib.h"
> +#include "cxgb4_tc_mqprio.h"
>  
>  static const u32 t6_tp_pio_array[][IREG_NUM_ELEM] = {
>  	{0x7e40, 0x7e44, 0x020, 28}, /* t6_tp_pio_regs_20_to_3b */
> @@ -3458,7 +3459,7 @@ int cudbg_collect_qdesc(struct cudbg_init *pdbg_init,
>  			for (i = 0; i < utxq->ntxq; i++)
>  				QDESC_GET_TXQ(&utxq->uldtxq[i].q,
>  					      cudbg_uld_txq_to_qtype(j),
> -					      out_unlock);
> +					      out_unlock_uld);
>  		}
>  	}
>  
> @@ -3475,7 +3476,7 @@ int cudbg_collect_qdesc(struct cudbg_init *pdbg_init,
>  			for (i = 0; i < urxq->nrxq; i++)
>  				QDESC_GET_RXQ(&urxq->uldrxq[i].rspq,
>  					      cudbg_uld_rxq_to_qtype(j),
> -					      out_unlock);
> +					      out_unlock_uld);
>  		}
>  
>  		/* ULD FLQ */
> @@ -3487,7 +3488,7 @@ int cudbg_collect_qdesc(struct cudbg_init *pdbg_init,
>  			for (i = 0; i < urxq->nrxq; i++)
>  				QDESC_GET_FLQ(&urxq->uldrxq[i].fl,
>  					      cudbg_uld_flq_to_qtype(j),
> -					      out_unlock);
> +					      out_unlock_uld);
>  		}
>  
>  		/* ULD CIQ */
> @@ -3500,29 +3501,34 @@ int cudbg_collect_qdesc(struct cudbg_init *pdbg_init,
>  			for (i = 0; i < urxq->nciq; i++)
>  				QDESC_GET_RXQ(&urxq->uldrxq[base + i].rspq,
>  					      cudbg_uld_ciq_to_qtype(j),
> -					      out_unlock);
> +					      out_unlock_uld);
>  		}
>  	}
> +	mutex_unlock(&uld_mutex);
> +
> +	if (!padap->tc_mqprio)
> +		goto out;
>  
> +	mutex_lock(&padap->tc_mqprio->mqprio_mutex);
>  	/* ETHOFLD TXQ */
>  	if (s->eohw_txq)
>  		for (i = 0; i < s->eoqsets; i++)
>  			QDESC_GET_TXQ(&s->eohw_txq[i].q,
> -				      CUDBG_QTYPE_ETHOFLD_TXQ, out);
> +				      CUDBG_QTYPE_ETHOFLD_TXQ, out_unlock_mqprio);
>  
>  	/* ETHOFLD RXQ and FLQ */
>  	if (s->eohw_rxq) {
>  		for (i = 0; i < s->eoqsets; i++)
>  			QDESC_GET_RXQ(&s->eohw_rxq[i].rspq,
> -				      CUDBG_QTYPE_ETHOFLD_RXQ, out);
> +				      CUDBG_QTYPE_ETHOFLD_RXQ, out_unlock_mqprio);
>  
>  		for (i = 0; i < s->eoqsets; i++)
>  			QDESC_GET_FLQ(&s->eohw_rxq[i].fl,
> -				      CUDBG_QTYPE_ETHOFLD_FLQ, out);
> +				      CUDBG_QTYPE_ETHOFLD_FLQ, out_unlock_mqprio);
>  	}
>  
> -out_unlock:
> -	mutex_unlock(&uld_mutex);
> +out_unlock_mqprio:
> +	mutex_unlock(&padap->tc_mqprio->mqprio_mutex);
>  
>  out:
>  	qdesc_info->qdesc_entry_size = sizeof(*qdesc_entry);
> @@ -3559,6 +3565,10 @@ int cudbg_collect_qdesc(struct cudbg_init *pdbg_init,
>  #undef QDESC_GET
>  
>  	return rc;
> +
> +out_unlock_uld:
> +	mutex_unlock(&uld_mutex);
> +	goto out;
>  }
>  
>  int cudbg_collect_flash(struct cudbg_init *pdbg_init,
> -- 
> 2.34.1
>