netdev - Re: [PATCH iwl-net 1/3] ice: Fix improper extts handling

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]

Date: Wed, 19 Jun 2024 17:42:47 +0100
From: Simon Horman <horms@...nel.org>
To: Karol Kolacinski <karol.kolacinski@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
	anthony.l.nguyen@...el.com, przemyslaw.kitszel@...el.com,
	Milena Olech <milena.olech@...el.com>,
	Jacob Keller <jacob.e.keller@...el.com>
Subject: Re: [PATCH iwl-net 1/3] ice: Fix improper extts handling

On Tue, Jun 18, 2024 at 12:41:36PM +0200, Karol Kolacinski wrote:
> From: Milena Olech <milena.olech@...el.com>
> 
> Extts events are disabled and enabled by the application ts2phc.
> However, in case where the driver is removed when the application is
> running, channel remains enabled. As a result, in the next run of the
> app, two channels are enabled and the information "extts on unexpected
> channel" is printed to the user.
> 
> To avoid that, extts events shall be disabled when PTP is released.
> 
> Fixes: 172db5f91d5f ("ice: add support for auxiliary input/output pins")
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
> Co-developed-by: Jacob Keller <jacob.e.keller@...el.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
> Signed-off-by: Milena Olech <milena.olech@...el.com>
> Signed-off-by: Karol Kolacinski <karol.kolacinski@...el.com>

Hi Milena and Karol,

Some feedback from my side.

...

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 0f17fc1181d2..30f1f910e6d9 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -1584,27 +1584,24 @@ void ice_ptp_extts_event(struct ice_pf *pf)
>  /**
>   * ice_ptp_cfg_extts - Configure EXTTS pin and channel
>   * @pf: Board private structure
> - * @ena: true to enable; false to disable
>   * @chan: GPIO channel (0-3)
> - * @gpio_pin: GPIO pin
> - * @extts_flags: request flags from the ptp_extts_request.flags
> - */
> -static int
> -ice_ptp_cfg_extts(struct ice_pf *pf, bool ena, unsigned int chan, u32 gpio_pin,
> -		  unsigned int extts_flags)
> + * @config: desired EXTTS configuration.
> + * @store: If set to true, the values will be stored
> + *
> + * Configure an external timestamp event on the requested channel.
> +  */

nit: There is an extra leading space on the line above.

     Also, although not strictly related to this change,
     please consider adding a Returns: section to this kernel doc.

> +static void ice_ptp_cfg_extts(struct ice_pf *pf, unsigned int chan,
> +			      struct ice_extts_channel *config, bool store)

...

> @@ -1869,21 +1915,31 @@ static int ice_ptp_gpio_enable_e823(struct ptp_clock_info *info,
>  				    struct ptp_clock_request *rq, int on)
>  {
>  	struct ice_pf *pf = ptp_info_to_pf(info);
> -	struct ice_perout_channel clk_cfg = {0};
>  	int err;
>  
>  	switch (rq->type) {
>  	case PTP_CLK_REQ_PPS:
> +	{
> +		struct ice_perout_channel clk_cfg = {};
> +
>  		clk_cfg.gpio_pin = PPS_PIN_INDEX;
>  		clk_cfg.period = NSEC_PER_SEC;
>  		clk_cfg.ena = !!on;
>  
>  		err = ice_ptp_cfg_clkout(pf, PPS_CLK_GEN_CHAN, &clk_cfg, true);
>  		break;
> +	}
>  	case PTP_CLK_REQ_EXTTS:
> -		err = ice_ptp_cfg_extts(pf, !!on, rq->extts.index,
> -					TIME_SYNC_PIN_INDEX, rq->extts.flags);
> +	{
> +		struct ice_extts_channel extts_cfg = {};
> +
> +		extts_cfg.flags = rq->extts.flags;
> +		extts_cfg.gpio_pin = TIME_SYNC_PIN_INDEX;
> +		extts_cfg.ena = !!on;
> +
> +		ice_ptp_cfg_extts(pf, rq->extts.index, &extts_cfg, true);
>  		break;

This function returns err.
But with this patch err is uninitialised here.

Perhaps err be set to the return value of ice_ptp_cfg_extts()
as it was before this patch?

Flagged by allmodconfig W=1 builds with gcc-13 and clang-18, and Smatch.

> +	}
>  	default:
>  		return -EOPNOTSUPP;
>  	}

...