Do not unlock pf->tc_mutex if it has not been locked. Jumping to
'dcb_error' causes pf->tc_mutex to be unlocked. This bug has been detected
by the Clang thread-safety analyzer.

Cc: Tony Nguyen <anthony.l.nguyen@intel.com>
Cc: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Fixes: 242b5e068b25 ("ice: Fix DCB rebuild after reset")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/net/ethernet/intel/ice/ice_dcb_lib.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
index 69a4b84f935f..65545613e1e8 100644
--- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
@@ -563,7 +563,8 @@ void ice_dcb_rebuild(struct ice_pf *pf)
 	ret = ice_query_port_ets(pf->hw.port_info, &buf, sizeof(buf), NULL);
 	if (ret) {
 		dev_err(dev, "Query Port ETS failed\n");
-		goto dcb_error;
+		disable_dcb(pf);
+		return;
 	}
 
 	mutex_lock(&pf->tc_mutex);

On 2/6/2025 9:50 AM, Bart Van Assche wrote:
> Do not unlock pf->tc_mutex if it has not been locked. Jumping to
> 'dcb_error' causes pf->tc_mutex to be unlocked. This bug has been detected
> by the Clang thread-safety analyzer.

Thanks for catching this Bart. I think it would be better to move the 
tc_mutex up to cover this call. Though unlikely, the DCB settings could 
change after this call so it would be better to protect this under the 
mutex. Also, as the error path is changing configuration, that should be 
done under this mutex as well.

Thanks,
Tony

> Cc: Tony Nguyen <anthony.l.nguyen@intel.com>
> Cc: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Fixes: 242b5e068b25 ("ice: Fix DCB rebuild after reset")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/net/ethernet/intel/ice/ice_dcb_lib.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> index 69a4b84f935f..65545613e1e8 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> @@ -563,7 +563,8 @@ void ice_dcb_rebuild(struct ice_pf *pf)
>   	ret = ice_query_port_ets(pf->hw.port_info, &buf, sizeof(buf), NULL);
>   	if (ret) {
>   		dev_err(dev, "Query Port ETS failed\n");
> -		goto dcb_error;
> +		disable_dcb(pf);
> +		return;
>   	}
>   
>   	mutex_lock(&pf->tc_mutex);

On 2/6/25 1:35 PM, Tony Nguyen wrote:
> On 2/6/2025 9:50 AM, Bart Van Assche wrote:
>> Do not unlock pf->tc_mutex if it has not been locked. Jumping to
>> 'dcb_error' causes pf->tc_mutex to be unlocked. This bug has been 
>> detected
>> by the Clang thread-safety analyzer.
> 
> Thanks for catching this Bart. I think it would be better to move the 
> tc_mutex up to cover this call. Though unlikely, the DCB settings could 
> change after this call so it would be better to protect this under the 
> mutex. Also, as the error path is changing configuration, that should be 
> done under this mutex as well.

Thanks Tony for having taken a look. Is this perhaps the change that you
want me to make?

Thanks,

Bart.

diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c 
b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
index a7c510832824..d185b1aba7a4 100644
--- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
@@ -537,14 +537,14 @@ void ice_dcb_rebuild(struct ice_pf *pf)
  	struct ice_dcbx_cfg *err_cfg;
  	int ret;

+	mutex_lock(&pf->tc_mutex);
+
  	ret = ice_query_port_ets(pf->hw.port_info, &buf, sizeof(buf), NULL);
  	if (ret) {
  		dev_err(dev, "Query Port ETS failed\n");
  		goto dcb_error;
  	}

-	mutex_lock(&pf->tc_mutex);
-
  	if (!pf->hw.port_info->qos_cfg.is_sw_lldp)
  		ice_cfg_etsrec_defaults(pf->hw.port_info);

On 2/6/2025 1:44 PM, Bart Van Assche wrote:
> On 2/6/25 1:35 PM, Tony Nguyen wrote:
>> On 2/6/2025 9:50 AM, Bart Van Assche wrote:
>>> Do not unlock pf->tc_mutex if it has not been locked. Jumping to
>>> 'dcb_error' causes pf->tc_mutex to be unlocked. This bug has been 
>>> detected
>>> by the Clang thread-safety analyzer.
>>
>> Thanks for catching this Bart. I think it would be better to move the 
>> tc_mutex up to cover this call. Though unlikely, the DCB settings 
>> could change after this call so it would be better to protect this 
>> under the mutex. Also, as the error path is changing configuration, 
>> that should be done under this mutex as well.
> 
> Thanks Tony for having taken a look. Is this perhaps the change that you
> want me to make?

Yes, looks good to me.

Thanks,
Tony

> Thanks,
> 
> Bart.
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c b/drivers/net/ 
> ethernet/intel/ice/ice_dcb_lib.c
> index a7c510832824..d185b1aba7a4 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> @@ -537,14 +537,14 @@ void ice_dcb_rebuild(struct ice_pf *pf)
>       struct ice_dcbx_cfg *err_cfg;
>       int ret;
> 
> +    mutex_lock(&pf->tc_mutex);
> +
>       ret = ice_query_port_ets(pf->hw.port_info, &buf, sizeof(buf), NULL);
>       if (ret) {
>           dev_err(dev, "Query Port ETS failed\n");
>           goto dcb_error;
>       }
> 
> -    mutex_lock(&pf->tc_mutex);
> -
>       if (!pf->hw.port_info->qos_cfg.is_sw_lldp)
>           ice_cfg_etsrec_defaults(pf->hw.port_info);
> 
>