Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1336623

RE: [patch 04/11] x86/perf/intel_uncore: Cleanup hardware on exit

Show all headers | View raw

On Wed, 17 Feb 2016, Liang, Kan wrote:
> > When tearing down the boxes nothing undoes the hardware state which
> > was setup by box->init_box(). Add a box->exit_box() callback and
> > implement it for the uncores which have an init_box() callback.
> 
> I don't think we need exit_box.
> Because in disable_box we already freezes the box.

init_box() != enable_box() 
exit_box() != disable_box()

And we certainly want to clear stuff which enables things at a different
msr/config word location than what you do with the enable/disable callbacks.

I'm not a fan of leaving hardware in some random state.

> Also, writing 0 cannot clear hardware state. It will unfreeze the box.
> The counter will start to count.

Nonsense.
 
> > @@ -201,6 +201,11 @@ static void nhmex_uncore_msr_init_box(st
> >  	wrmsrl(NHMEX_U_MSR_PMON_GLOBAL_CTL,
> > NHMEX_U_PMON_GLOBAL_EN_ALL);  }

> > +static void nhmex_uncore_msr_exit_box(struct intel_uncore_box *box)
> > {
> > +	wrmsrl(NHMEX_U_MSR_PMON_GLOBAL_CTL, 0); }

The reset value for this register is 0. So how is that wrong?

> > +static void snb_uncore_msr_exit_box(struct intel_uncore_box *box) {
> > +	if (box->pmu->pmu_idx == 0)
> > +		wrmsrl(SNB_UNC_PERF_GLOBAL_CTL, 0);
> > +}

Ditto.

> > +static void snb_uncore_imc_exit_box(struct intel_uncore_box *box) {
> > +	iounmap(box->io_addr);

That's definitely required, because it would leak a mapping.

I know Intel folks do not care about error handling and a few reference leaks,
but I care very much.

If there is a single instance of exit_box() in that patch which flips the
wrong bits, then please point it out with the proper reference in the manual
and not with such half baken statements as above.

Thanks,

	tglx

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[patch 00/11] x86/perf/intel_uncore: Cleanup and enhancements Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 14:50 +0100
  [patch 05/11] x86/perf/intel_uncore: Make code readable Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 15:00 +0100
  [patch 01/11] x86/perf/intel_uncore: Remove pointless mask check Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 15:00 +0100
  [patch 02/11] x86/perf/intel_uncore: Simplify error rollback Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 15:00 +0100
  [patch 09/11] x86/perf/intel_uncore: Make PCI and MSR uncore  independent Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 15:00 +0100
  [patch 06/11] x86/topology: Provide helper to retrieve number of cpu  packages Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 15:00 +0100
  [patch 03/11] x86/perf/intel_uncore: Fix error handling Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 15:00 +0100
  [patch 11/11] x86/perf/intel_uncore: Make it modular Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 15:00 +0100
  [patch 10/11] cpumask: Export cpumask_any_but Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 15:00 +0100
  [patch 04/11] x86/perf/intel_uncore: Cleanup hardware on exit Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 15:00 +0100
    RE: [patch 04/11] x86/perf/intel_uncore: Cleanup hardware on exit "Liang, Kan" <kan.liang@intel.com> - 2016-02-17 16:50 +0100
      RE: [patch 04/11] x86/perf/intel_uncore: Cleanup hardware on exit Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 19:20 +0100
        RE: [patch 04/11] x86/perf/intel_uncore: Cleanup hardware on exit "Liang, Kan" <kan.liang@intel.com> - 2016-02-17 23:00 +0100
          RE: [patch 04/11] x86/perf/intel_uncore: Cleanup hardware on exit Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 23:10 +0100

csiph-web