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

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

On 1/13/2023 4:24 PM, Jacob Keller wrote:
> 
> 
> On 1/13/2023 2:23 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.
>>
> 
> You could also catch this IO error and handle it if that is the only way
> to get IO error? 

I'm not sure if this is the only way to get this error or not. The error 
comes when we register for FW logging events. The documentation doesn't 
provide any info on what could cause an error or what errors we might 
see so I'm nervous on relying on this since other things may return the 
same error.

> Is there a way to check if firmware logging is already
> active after a driver reload? That might still be useful in the event
> that firmware logging gets enabled via some other version of the driver
> which doesn't disable it on unload...

Yes, but by the time we would check that (when we register for FW lgging 
events), we wouldn't know it without introducing some other flag for 
that purpose. I'm not sure it's worth adding a flag just for this...

> 
> Something like checking the adminq error and if its EIO then confirm
> that we are enabled and report success in that case.
> 
> Thanks,
> Jake
> 
>> 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 9e610827966f..f24693e42e35 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_main.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
>> @@ -4497,6 +4497,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);
>> +	}
>> +}
>> +
>>   /**
>>    * ice_cfg_netdev - Allocate, configure and register a netdev
>>    * @vsi: the VSI associated with the new netdev
>> @@ -5234,6 +5261,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);
> _______________________________________________
> 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