Re: [Intel-wired-lan] [PATCH net-next v5 3/5] ice: add ability to query/set FW log level and resolution

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

On 1/12/2023 2:00 AM, Michal Swiatkowski wrote:
> On Wed, Jan 11, 2023 at 11:19:04AM -0800, Paul M Stillwell Jr wrote:
>> The E8xx has the ability to change the FW log level and
>> the granularity at which the logs get output. Enable
>> the ability to see what the current values are and to
>> change them.
>>
>> The following devlink commands are now supported:
>>
>> devlink dev param set <pci dev> name fwlog_enabled value <true/false> cmode runtime
>> devlink dev param set <pci dev> name fwlog_level value <0-4> cmode runtime
>> devlink dev param set <pci dev> name fwlog_resolution value <1-128> cmode runtime
>>
>> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>> ---
>>   .../net/ethernet/intel/ice/ice_adminq_cmd.h   |   2 +
>>   drivers/net/ethernet/intel/ice/ice_common.c   |   4 +-
>>   drivers/net/ethernet/intel/ice/ice_devlink.c  | 131 ++++++++-
>>   drivers/net/ethernet/intel/ice/ice_fwlog.c    | 278 +++++++++++++++++-
>>   drivers/net/ethernet/intel/ice/ice_fwlog.h    |   5 +
>>   drivers/net/ethernet/intel/ice/ice_type.h     |   1 +
>>   6 files changed, 416 insertions(+), 5 deletions(-)
>>   
> [...]
>> diff --git a/drivers/net/ethernet/intel/ice/ice_fwlog.c b/drivers/net/ethernet/intel/ice/ice_fwlog.c
>> index efea71b6c9f8..6973f01749f9 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_fwlog.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_fwlog.c
>> @@ -4,6 +4,165 @@
>>   #include "ice_common.h"
>>   #include "ice_fwlog.h"
>>   
>> +/**
>> + * cache_cfg - Cache FW logging config
>> + * @hw: pointer to the HW structure
>> + * @cfg: config to cache
>> + */
>> +static void cache_cfg(struct ice_hw *hw, struct ice_fwlog_cfg *cfg)
>> +{
>> +	hw->fwlog_cfg = *cfg;
>> +}
>> +
>> +/**
>> + * valid_module_entries - validate all the module entry IDs and log levels
>> + * @hw: pointer to the HW structure
>> + * @entries: entries to validate
>> + * @num_entries: number of entries to validate
>> + */
>> +static bool
>> +valid_module_entries(struct ice_hw *hw, struct ice_fwlog_module_entry *entries,
>> +		     u16 num_entries)
>> +{
>> +	int i;
>> +
>> +	if (!entries) {
>> +		ice_debug(hw, ICE_DBG_FW_LOG, "Null ice_fwlog_module_entry array\n");
>> +		return false;
>> +	}
>> +
>> +	if (!num_entries) {
>> +		ice_debug(hw, ICE_DBG_FW_LOG, "num_entries must be non-zero\n");
>> +		return false;
>> +	}
>> +
>> +	for (i = 0; i < num_entries; i++) {
>> +		struct ice_fwlog_module_entry *entry = &entries[i];
>> +
>> +		if (entry->module_id >= ICE_AQC_FW_LOG_ID_MAX) {
>> +			ice_debug(hw, ICE_DBG_FW_LOG, "Invalid module_id %u, max valid module_id is %u\n",
>> +				  entry->module_id, ICE_AQC_FW_LOG_ID_MAX - 1);
>> +			return false;
>> +		}
>> +
>> +		if (entry->log_level >= ICE_FWLOG_LEVEL_INVALID) {
>> +			ice_debug(hw, ICE_DBG_FW_LOG, "Invalid log_level %u, max valid log_level is %u\n",
>> +				  entry->log_level,
>> +				  ICE_AQC_FW_LOG_ID_MAX - 1);
>> +			return false;
>> +		}
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +/**
>> + * valid_cfg - validate entire configuration
>> + * @hw: pointer to the HW structure
>> + * @cfg: config to validate
>> + */
>> +static bool valid_cfg(struct ice_hw *hw, struct ice_fwlog_cfg *cfg)
>> +{
>> +	if (!cfg) {
>> +		ice_debug(hw, ICE_DBG_FW_LOG, "Null ice_fwlog_cfg\n");
>> +		return false;
>> +	}
>> +
>> +	if (cfg->log_resolution < ICE_AQC_FW_LOG_MIN_RESOLUTION ||
>> +	    cfg->log_resolution > ICE_AQC_FW_LOG_MAX_RESOLUTION) {
>> +		ice_debug(hw, ICE_DBG_FW_LOG, "Unsupported log_resolution %u, must be between %u and %u\n",
>> +			  cfg->log_resolution, ICE_AQC_FW_LOG_MIN_RESOLUTION,
>> +			  ICE_AQC_FW_LOG_MAX_RESOLUTION);
>> +		return false;
>> +	}
>> +
>> +	if (!valid_module_entries(hw, cfg->module_entries,
>> +				  ICE_AQC_FW_LOG_ID_MAX))
>> +		return false;
>> +
>> +	return true;
> return valid_module_entries();
> 

Done

>> +}
>> +
>> +/**
>> + * ice_fwlog_init - Initialize FW logging variables
>> + * @hw: pointer to the HW structure
>> + *
>> + * This function should be called on driver initialization during
>> + * ice_init_hw().
>> + */
>> +int ice_fwlog_init(struct ice_hw *hw)
>> +{
>> +	int status;
>> +
>> +	ice_fwlog_set_supported(hw);
>> +
>> +	if (ice_fwlog_supported(hw)) {
>> +		struct ice_fwlog_cfg *cfg;
>> +
>> +		cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
> I don't understand why hw->cfg can't be used here? It will simplify all
> this cache_cfg. Is there any reason to alloc config and than set hw->cfg
> to it in get flow?
> 
> When config parameters are being set from devlink context hw->cfg is
> used instead of new allocated config like here (and than casched).
> 

Good point, I removed the allocation of cfg here and removed the 
cache_cfg function.

>> +		if (!cfg)
>> +			return -ENOMEM;
>> +
>> +		/* read the current config from the FW and store it */
>> +		status = ice_fwlog_get(hw, cfg);
>> +		if (status)
>> +			return status;
> Shouldn't cfg be free here? If ice_fwlog_get fails cfg isn't casched.
> 

Not needed since I refactored the code to remove the allocation of cfg

>> +	}
>> +
>> +	return 0;
>> +}
>> +
> [...]
>>   
>> -- 
>> 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