Re: [PATCH RFC 14/33] ice: Split ice_dcb_rebuild()

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 2/6/25 18:50, Bart Van Assche wrote:
> Prepare for adding a second caller. No functionality has been changed.
> 
> Cc: Tony Nguyen <anthony.l.nguyen@intel.com>
> Cc: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Thank you for fixes for our driver!
Just minor issues from me below.

> ---
>   drivers/net/ethernet/intel/ice/ice_dcb_lib.c | 46 +++++++++++---------
>   1 file changed, 25 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> index a7c510832824..69a4b84f935f 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> @@ -526,6 +526,30 @@ ice_dcb_need_recfg(struct ice_pf *pf, struct ice_dcbx_cfg *old_cfg,
>   	return need_reconfig;
>   }
>   
> +static void disable_dcb(struct ice_pf *pf)

you have to prefix it with ice_

> +{
> +	struct ice_dcbx_cfg *err_cfg;
> +	struct device *dev = ice_pf_to_dev(pf);

Networking code mandates to sort declaration lines by their length,
longest to shortest. We call that "reverse xmass tree rule".

> +
> +	dev_err(dev, "Disabling DCB until new settings occur\n");

even for copy-pasted code, it hurts to have this as error here

> +	err_cfg = kzalloc(sizeof(*err_cfg), GFP_KERNEL);
> +	if (!err_cfg)
> +		return;
> +
> +	err_cfg->etscfg.willing = true;
> +	err_cfg->etscfg.tcbwtable[0] = ICE_TC_MAX_BW;
> +	err_cfg->etscfg.tsatable[0] = ICE_IEEE_TSA_ETS;
> +	memcpy(&err_cfg->etsrec, &err_cfg->etscfg, sizeof(err_cfg->etsrec));
> +	/* Coverity warns the return code of ice_pf_dcb_cfg() is not checked
> +	 * here as is done for other calls to that function. That check is
> +	 * not necessary since this is in this function's error cleanup path.
> +	 * Suppress the Coverity warning with the following comment...
> +	 */
> +	/* coverity[check_return] */

the comment was silly in the first place, but makes even less sense when
extracted to helper function

> +	ice_pf_dcb_cfg(pf, err_cfg, false);
> +	kfree(err_cfg);
> +}
> +
>   /**
>    * ice_dcb_rebuild - rebuild DCB post reset
>    * @pf: physical function instance
> @@ -534,7 +558,6 @@ void ice_dcb_rebuild(struct ice_pf *pf)
>   {
>   	struct ice_aqc_port_ets_elem buf = { 0 };
>   	struct device *dev = ice_pf_to_dev(pf);
> -	struct ice_dcbx_cfg *err_cfg;
>   	int ret;
>   
>   	ret = ice_query_port_ets(pf->hw.port_info, &buf, sizeof(buf), NULL);
> @@ -574,26 +597,7 @@ void ice_dcb_rebuild(struct ice_pf *pf)
>   	return;
>   
>   dcb_error:
> -	dev_err(dev, "Disabling DCB until new settings occur\n");
> -	err_cfg = kzalloc(sizeof(*err_cfg), GFP_KERNEL);
> -	if (!err_cfg) {
> -		mutex_unlock(&pf->tc_mutex);
> -		return;
> -	}
> -
> -	err_cfg->etscfg.willing = true;
> -	err_cfg->etscfg.tcbwtable[0] = ICE_TC_MAX_BW;
> -	err_cfg->etscfg.tsatable[0] = ICE_IEEE_TSA_ETS;
> -	memcpy(&err_cfg->etsrec, &err_cfg->etscfg, sizeof(err_cfg->etsrec));
> -	/* Coverity warns the return code of ice_pf_dcb_cfg() is not checked
> -	 * here as is done for other calls to that function. That check is
> -	 * not necessary since this is in this function's error cleanup path.
> -	 * Suppress the Coverity warning with the following comment...
> -	 */
> -	/* coverity[check_return] */
> -	ice_pf_dcb_cfg(pf, err_cfg, false);
> -	kfree(err_cfg);
> -
> +	disable_dcb(pf);
>   	mutex_unlock(&pf->tc_mutex);
>   }
>