Re: [patch net-next 3/7] mlxfw: Propagate error messages through extack

[Jiri Pirko <jiri@resnulli.us>]

Thu, May 23, 2019 at 05:19:46PM CEST, dsahern@gmail.com wrote:
>On 5/23/19 3:45 AM, Jiri Pirko wrote:
>> @@ -57,11 +58,13 @@ static int mlxfw_fsm_state_wait(struct mlxfw_dev *mlxfw_dev, u32 fwhandle,
>>  	if (fsm_state_err != MLXFW_FSM_STATE_ERR_OK) {
>>  		pr_err("Firmware flash failed: %s\n",
>>  		       mlxfw_fsm_state_err_str[fsm_state_err]);
>> +		NL_SET_ERR_MSG_MOD(extack, "Firmware flash failed");
>>  		return -EINVAL;
>>  	}
>>  	if (curr_fsm_state != fsm_state) {
>>  		if (--times == 0) {
>>  			pr_err("Timeout reached on FSM state change");
>> +			NL_SET_ERR_MSG_MOD(extack, "Timeout reached on FSM state change");
>
>FSM? Is the meaning obvious to users?

It is specific to mlx drivers. But I think it is valuable to have
driver-specific terms in driver speficic extack messages.


>
>>  			return -ETIMEDOUT;
>>  		}
>>  		msleep(MLXFW_FSM_STATE_WAIT_CYCLE_MS);
>> @@ -76,7 +79,8 @@ static int mlxfw_fsm_state_wait(struct mlxfw_dev *mlxfw_dev, u32 fwhandle,
>>  
>>  static int mlxfw_flash_component(struct mlxfw_dev *mlxfw_dev,
>>  				 u32 fwhandle,
>> -				 struct mlxfw_mfa2_component *comp)
>> +				 struct mlxfw_mfa2_component *comp,
>> +				 struct netlink_ext_ack *extack)
>>  {
>>  	u16 comp_max_write_size;
>>  	u8 comp_align_bits;
>> @@ -96,6 +100,7 @@ static int mlxfw_flash_component(struct mlxfw_dev *mlxfw_dev,
>>  	if (comp->data_size > comp_max_size) {
>>  		pr_err("Component %d is of size %d which is bigger than limit %d\n",
>>  		       comp->index, comp->data_size, comp_max_size);
>> +		NL_SET_ERR_MSG_MOD(extack, "Component is which is bigger than limit");
>
>Need to drop 'is which'.

Will do.

>
>
>...
>
>> @@ -156,6 +163,7 @@ static int mlxfw_flash_components(struct mlxfw_dev *mlxfw_dev, u32 fwhandle,
>>  					      &component_count);
>>  	if (err) {
>>  		pr_err("Could not find device PSID in MFA2 file\n");
>> +		NL_SET_ERR_MSG_MOD(extack, "Could not find device PSID in MFA2 file");
>
>same here, is PSID understood by user?

PSID is actually exposed in "devlink dev info" for mlxsw.

>