netdev - Re: [patch net-next v2 1/3] dpll: extend uapi by lock status error attribute

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240201135311.GE530335@kernel.org>
Date: Thu, 1 Feb 2024 14:53:11 +0100
From: Simon Horman <horms@...nel.org>
To: Jiri Pirko <jiri@...nulli.us>
Cc: netdev@...r.kernel.org, kuba@...nel.org, pabeni@...hat.com,
	davem@...emloft.net, edumazet@...gle.com, vadim.fedorenko@...ux.dev,
	arkadiusz.kubalewski@...el.com, saeedm@...dia.com, leon@...nel.org,
	jesse.brandeburg@...el.com, anthony.l.nguyen@...el.com,
	rrameshbabu@...dia.com
Subject: Re: [patch net-next v2 1/3] dpll: extend uapi by lock status error
 attribute

On Tue, Jan 30, 2024 at 01:08:29PM +0100, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@...dia.com>
> 
> If the dpll devices goes to state "unlocked" or "holdover", it may be
> caused by an error. In that case, allow user to see what the error was.
> Introduce a new attribute and values it can carry.
> 
> Signed-off-by: Jiri Pirko <jiri@...dia.com>
> Acked-by: Vadim Fedorenko <vadim.fedorenko@...ux.dev>

The nit below notwithstanding, this looks good to me.

Reviewed-by: Simon Horman <horms@...nel.org>

...

> diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
> index b4e947f9bfbc..0c13d7f1a1bc 100644
> --- a/include/uapi/linux/dpll.h
> +++ b/include/uapi/linux/dpll.h
> @@ -50,6 +50,35 @@ enum dpll_lock_status {
>  	DPLL_LOCK_STATUS_MAX = (__DPLL_LOCK_STATUS_MAX - 1)
>  };
>  
> +/**
> + * enum dpll_lock_status_error - if previous status change was done due to a
> + *   failure, this provides information of dpll device lock status error. Valid
> + *   values for DPLL_A_LOCK_STATUS_ERROR attribute
> + * @DPLL_LOCK_STATUS_ERROR_NONE: dpll device lock status was changed without
> + *   any error
> + * @DPLL_LOCK_STATUS_ERROR_UNDEFINED: dpll device lock status was changed due
> + *   to undefined error. Driver fills this value up in case it is not able to
> + *   obtain suitable exact error type.
> + * @DPLL_LOCK_STATUS_ERROR_MEDIA_DOWN: dpll device lock status was changed
> + *   because of associated media got down. This may happen for example if dpll
> + *   device was previously locked on an input pin of type
> + *   PIN_TYPE_SYNCE_ETH_PORT.
> + * @DPLL_LOCK_STATUS_ERROR_FRACTIONAL_FREQUENCY_OFFSET_TOO_HIGH: the FFO
> + *   (Fractional Frequency Offset) between the RX and TX symbol rate on the
> + *   media got too high. This may happen for example if dpll device was
> + *   previously locked on an input pin of type PIN_TYPE_SYNCE_ETH_PORT.
> + */
> +enum dpll_lock_status_error {
> +	DPLL_LOCK_STATUS_ERROR_NONE = 1,
> +	DPLL_LOCK_STATUS_ERROR_UNDEFINED,
> +	DPLL_LOCK_STATUS_ERROR_MEDIA_DOWN,
> +	DPLL_LOCK_STATUS_ERROR_FRACTIONAL_FREQUENCY_OFFSET_TOO_HIGH,

nit: I'm all for descriptive names,
     but this one is rather long to say the least.

> +
> +	/* private: */
> +	__DPLL_LOCK_STATUS_ERROR_MAX,
> +	DPLL_LOCK_STATUS_ERROR_MAX = (__DPLL_LOCK_STATUS_ERROR_MAX - 1)
> +};
> +
>  #define DPLL_TEMP_DIVIDER	1000
>  
>  /**
> @@ -150,6 +179,7 @@ enum dpll_a {
>  	DPLL_A_LOCK_STATUS,
>  	DPLL_A_TEMP,
>  	DPLL_A_TYPE,
> +	DPLL_A_LOCK_STATUS_ERROR,
>  
>  	__DPLL_A_MAX,
>  	DPLL_A_MAX = (__DPLL_A_MAX - 1)
> -- 
> 2.43.0
> 
>