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:   Sat, 2 Dec 2017 10:04:58 -0800
From:   Richard Cochran <richardcochran@...il.com>
To:     Sagar Arun Kamble <sagar.a.kamble@...el.com>
Cc:     linux-kernel@...r.kernel.org,
        Chris Wilson <chris@...is-wilson.co.uk>,
        John Stultz <john.stultz@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Stephen Boyd <sboyd@...eaurora.org>,
        linux-arm-kernel@...ts.infradead.org, netdev@...r.kernel.org,
        intel-wired-lan@...ts.osuosl.org, linux-rdma@...r.kernel.org,
        alsa-devel@...a-project.org, kvmarm@...ts.cs.columbia.edu
Subject: Re: [PATCH 1/1] timecounter: Make cyclecounter struct part of
 timecounter struct

On Sat, Dec 02, 2017 at 10:01:35AM +0530, Sagar Arun Kamble wrote:
> There is no real need for the users of timecounters to define cyclecounter
> and timecounter variables separately. Since timecounter will always be
> based on cyclecounter, have cyclecounter struct as member of timecounter
> struct.

Overall, this is a welcome change.  However, it doesn't go far enough,
IMHO, and I'll explain that more below.

> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_clock.c b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> index 0247885..35987b5 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_clock.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_clock.c
...

As it stands now, timecounter_init() is used for two totally different
reasons.  Some callers only want to set the time, ...

> @@ -207,7 +207,7 @@ static int mlx4_en_phc_settime(struct ptp_clock_info *ptp,
>  
>  	/* reset the timecounter */
>  	write_seqlock_irqsave(&mdev->clock_lock, flags);
> -	timecounter_init(&mdev->clock, &mdev->cycles, ns);
> +	timecounter_init(&mdev->clock, ns);
>  	write_sequnlock_irqrestore(&mdev->clock_lock, flags);
>  
>  	return 0;

... while others initialize the data structure the first time:

> @@ -274,17 +274,17 @@ void mlx4_en_init_timestamp(struct mlx4_en_dev *mdev)
>  
>  	seqlock_init(&mdev->clock_lock);
>  
> -	memset(&mdev->cycles, 0, sizeof(mdev->cycles));
> -	mdev->cycles.read = mlx4_en_read_clock;
> -	mdev->cycles.mask = CLOCKSOURCE_MASK(48);
> -	mdev->cycles.shift = freq_to_shift(dev->caps.hca_core_clock);
> -	mdev->cycles.mult =
> -		clocksource_khz2mult(1000 * dev->caps.hca_core_clock, mdev->cycles.shift);
> -	mdev->nominal_c_mult = mdev->cycles.mult;
> +	memset(&mdev->clock.cc, 0, sizeof(mdev->clock.cc));
> +	mdev->clock.cc.read = mlx4_en_read_clock;
> +	mdev->clock.cc.mask = CLOCKSOURCE_MASK(48);
> +	mdev->clock.cc.shift = freq_to_shift(dev->caps.hca_core_clock);
> +	mdev->clock.cc.mult =
> +		clocksource_khz2mult(1000 * dev->caps.hca_core_clock,
> +				     mdev->clock.cc.shift);
> +	mdev->nominal_c_mult = mdev->clock.cc.mult;
>  
>  	write_seqlock_irqsave(&mdev->clock_lock, flags);
> -	timecounter_init(&mdev->clock, &mdev->cycles,
> -			 ktime_to_ns(ktime_get_real()));
> +	timecounter_init(&mdev->clock, ktime_to_ns(ktime_get_real()));

I'd like to see two followup patches to this one:

1. Convert timecounter_init() callers to a new timecounter_reset()
   function where the intent is to reset the time.

2. Change timecounter_init() to take the cyclecounter fields as
   arguments.

	void timecounter_init(struct timecounter *tc,
			      u64 (*read)(const struct cyclecounter *cc),
			      u64 mask,
			      u32 mult,
			      u32 shift,
			      u64 start_tstamp);

Then we can clean up all this stuff:

	mdev->clock.cc.read = mlx4_en_read_clock;
	mdev->clock.cc.mask = CLOCKSOURCE_MASK(48);
	mdev->clock.cc.shift = freq_to_shift(dev->caps.hca_core_clock);
	mdev->clock.cc.mult = clocksource_khz2mult(...);

This second step can be phased in by calling the new function
timecounter_initialize() and converting the drivers one by one.

> diff --git a/include/linux/timecounter.h b/include/linux/timecounter.h
> index 2496ad4..6daca06 100644
> --- a/include/linux/timecounter.h
> +++ b/include/linux/timecounter.h
...
> @@ -98,7 +98,6 @@ static inline void timecounter_adjtime(struct timecounter *tc, s64 delta)
>  /**
>   * timecounter_init - initialize a time counter
>   * @tc:			Pointer to time counter which is to be initialized/reset
> - * @cc:			A cycle counter, ready to be used.

This "ready to used" requirement should go.  The init() function
should make the instance ready to be used all at once.

Thanks,
Richard

Powered by blists - more mailing lists