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

[Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>]

On 1/12/2023 2:49 AM, Michal Swiatkowski wrote:
> On Wed, Jan 11, 2023 at 11:19:05AM -0800, 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_main.c | 29 +++++++++++++++++++++++
>>   1 file changed, 29 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
>> index 1b5debc3109d..593efc064f5b 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_main.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
>> @@ -4500,6 +4500,33 @@ static void ice_unregister_netdev(struct ice_vsi *vsi)
>>   	clear_bit(ICE_VSI_NETDEV_REGISTERED, vsi->state);
>>   }
>>   
>> +/**
>> + * ice_pf_fwlog_deinit - clear FW logging metadata on device exit
>> + * @pf: pointer to the PF struct
>> + */
>> +static void ice_pf_fwlog_deinit(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);
> Shouldn't hw->fwlog_cfg be free on deinit? Or if not here, where does
> this happen?
> 

No need to free hw->fwlog_cfg since it's not a pointer with memory 
allocated, it's a member of ice_hw. See ice_type.h, 863

>> +	}
>> +}
>> +
>>   /**
>>    * ice_cfg_netdev - Allocate, configure and register a netdev
>>    * @vsi: the VSI associated with the new netdev
>> @@ -5237,6 +5264,8 @@ static void ice_remove(struct pci_dev *pdev)
>>   		msleep(100);
>>   	}
>>   
>> +	ice_pf_fwlog_deinit(pf);
>> +
>>   	if (test_bit(ICE_FLAG_SRIOV_ENA, pf->flags)) {
>>   		set_bit(ICE_VF_RESETS_DISABLED, pf->state);
>>   		ice_free_vfs(pf);
>> -- 
>> 2.35.1
>>
>> _______________________________________________
>> Intel-wired-lan mailing list
>> Intel-wired-lan@osuosl.org
>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan