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

From: "Wilczynski, Michal" <michal.wilczynski@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: Tue, 29 Nov 2022 13:05:01 +0100	[thread overview]
Message-ID: <05ae38f4-2e9a-7d69-91b9-f983bfdbaaba@intel.com> (raw)
In-Reply-To: <20221128214749.110-5-paul.m.stillwell.jr@intel.com>

On 11/28/2022 10: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,

My understanding was the the community doesn't like private devlink params.
Was some investigation done whether the FW logging can be done using some
generic API ? I believe we had a discussion about this a long time ago. Maybe
I'm a bit out of a loop at this point :)

>  };
>  
>  /**
> @@ -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,
> -};
> -
>  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