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

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

On 11/29/2022 4:05 AM, Wilczynski, Michal wrote:
> 
> 
> 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 :)
> 

Yeah, I looked at all the other devlink interfaces and couldn't find one 
that really worked. I considered adding a brand new interface, but 
didn't want to go that route unless we have to because I'm guessing that 
every device that can get FW logs has a slightly different way to do it.

If you have a suggestion then I'm all ears :).

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