Please make a separate patch to do any refactoring. Otherwise
nobody can evaluate your changes. Then any other changes on top of it.
> +static DEFINE_PER_CPU(atomic_t, tt_report_flag);
> +static DEFINE_PER_CPU(struct timer_list, tt_report_timer);
> +
> +static DEFINE_PER_CPU(unsigned long, last_time);
> +static DEFINE_PER_CPU(unsigned long, thermal_throttle_count);
> +static DEFINE_PER_CPU(unsigned long, last_count);
> +static DEFINE_PER_CPU(atomic_t, tt_enabled);
Do we really need all of this per CPU? I think global variables would
be just fine.
> +/* thermal_throttle_count_init -
> + * Initialize all the internal state variables. The inits to 0 are not
> + * that important since the BSS is always 0 in kernel.
> + *
> + * This function should be called ONLY if thermal throttle detection
> was + * enabled on this 'cpu'.
I don't think your comments are actually correct kerneldoc, so the scripts
parsing it would break.
> + */
> +void __cpuinit thermal_throttle_count_init(unsigned int cpu)
> +{
> + per_cpu(last_time, cpu) = INITIAL_JIFFIES;
> + per_cpu(thermal_throttle_count, cpu) = 0;
> + per_cpu(last_count, cpu) = 0;
Most of this can be just a normal initializer for the variable
> +}
> +
> +/* thermal_throttle_inc_count -
> + * This function is normally called by the thermal interrupt if
> + * the code determines that we just entered into a thermal event.
> + */
> +void thermal_throttle_inc_count(void)
> +{
> + __get_cpu_var(thermal_throttle_count)++;
> +}
Can you please just expand that in the caller. It looks like overabstraction.
> +
> + /* Prevent flooding the logs with thermal entries */
> + if (atomic_read(&__get_cpu_var(tt_report_flag)))
> + return 0;
Isn't the test the wrong way around?
I'm not sure what you need the timer for. Can't just just drive
that from the interrupt handlers and check time stamps to do
rate limiting? Then another timer wouldn't be needed.
> + /* get the interval */
> + intvl = (jiffies - __get_cpu_var(last_time)) / HZ;
Are you sure this handles jiffies wrapping correctly?
> +#ifdef CONFIG_HOTPLUG_CPU
> +static __cpuinit int thermal_throttle_remove_dev(struct sys_device *
> sys_dev) +{
> + sysfs_remove_group(&sys_dev->kobj, &thermal_throttle_attr_group);
> + return 0;
> +}
You don't remove the timer?
> +/* Get notified when a cpu comes on/off. Be hotplug friendly. */
And don't initialize it? Better get rid of it anyways.
> + /* connect to sysfs */
> + for_each_online_cpu(cpu) {
> + preempt_disable();
> + if (cpu_online(cpu)) {
If you put the preempt_disable around the loop you don't need a second
check and you even fix a race with new CPUs.
> +/* mce_log_thermal_throttle_event -
> + * Add an mce_log entry for the thermal throttle event 'evinfo'.
> 'status' + * should be whatever the user wants to put into the
> mce.status field, + * and historically has been the register value of
> the
> + * MSR_IA32_THERMAL_STATUS.
> + *
> + * This will typically be called from a thermal throttle interrupt
> + * if the event should be logged.
> + */
> +void mce_log_thermal_throttle_event(__u64 status,
> + struct therm_throt_event_info *evinfo)
> +{
Instead of having this evinfo structure you could just directly
fill in struct mces in the caller.
> + struct mce m;
> +
> + memset(&m, 0, sizeof(m));
> + m.cpu = evinfo->cpu;
> + m.bank = MCE_THERMAL_BANK;
So how should user space distingush that event from other thermal events?
> + m.status = status;
> + rdtscll(m.tsc);
> +
> + /* save the delta of the throttle count to let the consumer of this
> + * information know how many throttle instances have been seen.
> + *
> + * 'misc' is organized below as follows:
> + * bits 15:0 - Contains a capped value of thermal events for last
> + * interval.
> + * bits 31:16 - Time in seconds to qualify the interval
> + * (capped at 0xffff)
> + *
> + * We cap events count at 16 bits to leave room in 'misc' for future
> + * extensions to this report */
I trust you will be also submitting a mcelog patch to decode all this?