Re: [Intel-wired-lan] [PATCH net-next 4/5] ice: disable FW logging on driver unload

From: Tony Nguyen <anthony.l.nguyen@intel.com>
To: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>,
	<intel-wired-lan@lists.osuosl.org>
Subject: Re: [Intel-wired-lan] [PATCH net-next 4/5] ice: disable FW logging on driver unload
Date: Wed, 30 Nov 2022 16:40:02 -0800	[thread overview]
Message-ID: <857f7d38-edbe-0260-7fc3-925131beb5d7@intel.com> (raw)
In-Reply-To: <20221128214749.110-5-paul.m.stillwell.jr@intel.com>

On 11/28/2022 1:47 PM, Paul M Stillwell Jr wrote:
> The FW is running in it's own context irregardless of what the driver
> is doing. In this case, if the driver previously registered for FW
> log events and then the driver unloads without informing the FW to
> unregister for FW log events then the FW still has a timer running to
> output FW logs.
> 
> The next time the driver loads and tries to register for FW log events
> then the FW returns an error, but still enables the continued
> outputting of FW logs. This causes an IO error to devlink which isn't
> intuitive since the logs are still being output.
> 
> Fix this by disabling FW logging when the driver is being unloaded.
> 
> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_devlink.c | 32 +++++++++++++++-----
>   1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
> index ca67f2741f83..923556e6ae79 100644
> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
> @@ -378,6 +378,10 @@ static int ice_devlink_info_get(struct devlink *devlink,
>   enum ice_devlink_param_id {
>   	ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
>   	ICE_DEVLINK_PARAM_ID_TX_BALANCE,
> +	ICE_DEVLINK_PARAM_ID_FWLOG_SUPPORTED,
> +	ICE_DEVLINK_PARAM_ID_FWLOG_ENABLED,
> +	ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL,
> +	ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION,
>   };
>   
>   /**
> @@ -1484,14 +1488,6 @@ ice_devlink_enable_iw_validate(struct devlink *devlink, u32 id,
>   	return 0;
>   }
>   
> -enum ice_devlink_param_id {
> -	ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
> -	ICE_DEVLINK_PARAM_ID_FWLOG_SUPPORTED,
> -	ICE_DEVLINK_PARAM_ID_FWLOG_ENABLED,
> -	ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL,
> -	ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION,
> -};

Is there a reason you didn't add to the original enum in the previous 
patches and are it combining now?

> -
>   static int
>   ice_devlink_fwlog_supported_get(struct devlink *devlink, u32 id,
>   				struct devlink_param_gset_ctx *ctx)
> @@ -1743,6 +1739,26 @@ void ice_devlink_register(struct ice_pf *pf)
>    */
>   void ice_devlink_unregister(struct ice_pf *pf)
>   {
> +	struct ice_hw *hw = &pf->hw;
> +
> +	/* make sure FW logging is disabled to not put the FW in a weird state
> +	 * for the next driver load
> +	 */
> +	if (hw->fwlog_ena) {
> +		int status;
> +
> +		hw->fwlog_cfg.options &= ~ICE_FWLOG_OPTION_ARQ_ENA;
> +		status = ice_fwlog_set(hw, &hw->fwlog_cfg);
> +		if (status)
> +			dev_warn(ice_pf_to_dev(pf), "Unable to turn off FW logging, status: %d\n",
> +				 status);
> +
> +		status = ice_fwlog_unregister(hw);
> +		if (status)
> +			dev_warn(ice_pf_to_dev(pf), "Unable to unregister FW logging, status: %d\n",
> +				 status);
> +	}
> +
>   	devlink_unregister(priv_to_devlink(pf));
>   }
>   
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan