lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <46f0b4c5-5245-4774-bc3c-a73c86df7599@intel.com>
Date: Thu, 6 Feb 2025 23:01:10 +0100
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Bart Van Assche <bvanassche@....org>
CC: Will Deacon <will@...nel.org>, Peter Zijlstra <peterz@...radead.org>,
	Christoph Hellwig <hch@....de>, Greg Kroah-Hartman
	<gregkh@...uxfoundation.org>, Marco Elver <elver@...gle.com>, "Nick
 Desaulniers" <ndesaulniers@...gle.com>, Nathan Chancellor
	<nathan@...nel.org>, Kees Cook <kees@...nel.org>, Jann Horn
	<jannh@...gle.com>, <linux-kernel@...r.kernel.org>, Tony Nguyen
	<anthony.l.nguyen@...el.com>
Subject: Re: [PATCH RFC 14/33] ice: Split ice_dcb_rebuild()

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@...el.com>
> Cc: Przemek Kitszel <przemyslaw.kitszel@...el.com>
> Signed-off-by: Bart Van Assche <bvanassche@....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);
>   }
>   


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ