lkml.org 
[lkml]   [2010]   [May]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/2] x86 platform driver: intelligent power sharing driver
    On Mon, 10 May 2010 22:00:46 -0400
    Andrew Morton <akpm@linux-foundation.org> wrote:
    > > +#define thm_readb(off) readb(ips->regmap + (off))
    > > +#define thm_readw(off) readw(ips->regmap + (off))
    > > +#define thm_readl(off) readl(ips->regmap + (off))
    > > +#define thm_readq(off) readq(ips->regmap + (off))
    > > +
    > > +#define thm_writeb(off, val) writeb((val), ips->regmap + (off))
    > > +#define thm_writew(off, val) writew((val), ips->regmap + (off))
    > > +#define thm_writel(off, val) writel((val), ips->regmap + (off))
    >
    > ick.
    >
    > static inline unsigned short thm_readw(struct ips_driver *ips, unsigned offset)
    > {
    > readw(ips->regmap + offset);
    > }
    >
    > would be nicer.

    Yes, it would.

    > >
    > > ...
    > >
    > > +static void ips_enable_cpu_turbo(struct ips_driver *ips)
    > > +{
    > > + /* Already on, no need to mess with MSRs */
    > > + if (ips->__cpu_turbo_on)
    > > + return;
    > > +
    > > + on_each_cpu(do_enable_cpu_turbo, ips, 1);
    > > +
    > > + ips->__cpu_turbo_on = true;
    > > +}
    >
    > How does the code ensure that a hot-added CPU comes up in the correct
    > turbo state?

    It doesn't, I guess I'll have to add code to handle the hotplug case.

    > > +static bool cpu_exceeded(struct ips_driver *ips, int cpu)
    > > +{
    > > + unsigned long flags;
    > > + int avg;
    > > + bool ret = false;
    > > +
    > > + spin_lock_irqsave(&ips->turbo_status_lock, flags);
    > > + avg = cpu ? ips->ctv2_avg_temp : ips->ctv1_avg_temp;
    > > + if (avg > (ips->limits->core_temp_limit * 100))
    > > + ret = true;
    > > + if (ips->cpu_avg_power > ips->core_power_limit)
    > > + ret = true;
    > > + spin_unlock_irqrestore(&ips->turbo_status_lock, flags);
    > > +
    > > + if (ret)
    > > + printk(KERN_CRIT "CPU power or thermal limit exceeded\n");
    > > +
    > > + return ret;
    > > +}
    >
    > afacit these messages might come out at one-per-five-seconds max?
    >
    > I bet the driver blows up and someone's logs get spammed ;)

    Possibly. :) I added these at the request of Pavel; I could make them
    just print one time though...

    > > +sleep:
    > > + schedule_timeout_interruptible(msecs_to_jiffies(IPS_ADJUST_PERIOD));
    > > + } while(!kthread_should_stop());
    >
    > please run checkpatch.
    >
    > > + dev_dbg(&ips->dev->dev, "ips-adjust thread stopped\n");
    > > +
    > > + return 0;
    > > +}
    >
    > Did we really need a new kernel thread for this? Can't use
    > schedule_delayed_work() or something? Maybe slow-work, or one of the
    > other just-like-workqueues-only-different things we seem to keep
    > adding?

    Possibly, but I'm not sure if it would be a net code or complexity
    win... kthreads seem simple enough for this case, and since the driver
    only works on small CPU count machines, the extra threads don't seem to
    be a big deal.

    >
    > > +/*
    > > + * Helpers for reading out temp/power values and calculating their
    > > + * averages for the decision making and monitoring functions.
    > > + */
    > > +
    > > +static u16 calc_avg_temp(struct ips_driver *ips, u16 *array)
    > > +{
    > > + u64 total = 0;
    > > + int i;
    > > + u16 avg;
    > > +
    > > + for (i = 0; i < IPS_SAMPLE_COUNT; i++)
    > > + total += (u64)(array[i] * 100);
    >
    > Actually, that does work. Somehow the compiler will promote array[i]
    > to u64 _before_ doing the multiplication. I think. Still, it looks
    > like a deliberate attempt to trick the compiler into doing a
    > multiplicative overflow ;)
    >
    > > + avg = (u16)(total / (u64)IPS_SAMPLE_COUNT);
    >
    > Are you sure this won't emit a call to a non-existent 64-bit-divide
    > library function on i386?
    >
    > Did you mean for the driver to be available on 32-bit?

    Oh I forgot to include the 32 bit fixes here; I'll need to convert
    these to do_div64 I suppose.


    > > + last_msecs = jiffies_to_msecs(jiffies);
    > > + expire = jiffies + msecs_to_jiffies(IPS_SAMPLE_PERIOD);
    > > + mod_timer(&timer, expire);
    > > +
    > > + __set_current_state(TASK_UNINTERRUPTIBLE);
    >
    > This looks racy. Should set TASK_UNINTERRUPTIBLE _before_ arming the
    > timer.

    Ah ok, will fix.

    >
    > > + schedule();
    > > + __set_current_state(TASK_RUNNING);
    >
    > Unneeded - schedule() always returns in state TASK_RUNNING.

    Great.

    >
    > > + /* Calculate actual sample period for power averaging */
    > > + last_sample_period = jiffies_to_msecs(jiffies) - last_msecs;
    > > + if (!last_sample_period)
    > > + last_sample_period = 1;
    > > + } while(!kthread_should_stop());
    > > +
    > > + del_timer(&timer);
    >
    > Should be del_timer_sync(), I suspect.

    Wouldn't hurt at least.

    > > +static struct ips_mcp_limits *ips_detect_cpu(struct ips_driver *ips)
    > > +{
    > > + u64 turbo_power, misc_en;
    > > + struct ips_mcp_limits *limits = NULL;
    > > + u16 tdp;
    > > +
    > > + if (!(boot_cpu_data.x86 == 6 && boot_cpu_data.x86_model == 37)) {
    >
    > We don't have #defines for these things?

    I don't think so; they correspond to long marketing strings afaik.

    > > + dev_info(&ips->dev->dev, "Non-IPS CPU detected.\n");
    > > + goto out;
    > > + }
    > > +
    > > + rdmsrl(IA32_MISC_ENABLE, misc_en);
    > > + /*
    > > + * If the turbo enable bit isn't set, we shouldn't try to enable/disable
    > > + * turbo manually or we'll get an illegal MSR access, even though
    > > + * turbo will still be available.
    > > + */
    > > + if (!(misc_en & IA32_MISC_TURBO_EN))
    > > + ; /* add turbo MSR write allowed flag if necessary */
    > > +
    > > + if (strstr(boot_cpu_data.x86_model_id, "CPU M"))
    > > + limits = &ips_sv_limits;
    > > + else if (strstr(boot_cpu_data.x86_model_id, "CPU L"))
    > > + limits = &ips_lv_limits;
    > > + else if (strstr(boot_cpu_data.x86_model_id, "CPU U"))
    > > + limits = &ips_ulv_limits;
    > > + else
    > > + dev_info(&ips->dev->dev, "No CPUID match found.\n");
    > > +
    > > + rdmsrl(TURBO_POWER_CURRENT_LIMIT, turbo_power);
    > > + tdp = turbo_power & TURBO_TDP_MASK;
    > > +
    > > + /* Sanity check TDP against CPU */
    > > + if (limits->mcp_power_limit != (tdp / 8) * 1000) {
    > > + dev_warn(&ips->dev->dev, "Warning: CPU TDP doesn't match expected value (found %d, expected %d)\n",
    > > + tdp / 8, limits->mcp_power_limit / 1000);
    > > + }
    > > +
    > > +out:
    > > + return limits;
    > > +}
    > >
    > > ...
    > >
    > > +static struct pci_device_id ips_id_table[] = {
    >
    > DEFINE_PCI_DEVICE_TABLE()?

    I guess that would be a little cleaner.

    >
    > > + { PCI_DEVICE(PCI_VENDOR_ID_INTEL,
    > > + PCI_DEVICE_ID_INTEL_THERMAL_SENSOR), },
    > > + { 0, }
    > > +};
    > > +
    > > +MODULE_DEVICE_TABLE(pci, ips_id_table);
    > > +
    > > +static int ips_probe(struct pci_dev *dev, const struct pci_device_id *id)
    > > +{
    > > + u64 platform_info;
    > > + struct ips_driver *ips;
    > > + u32 hts;
    > > + int ret = 0;
    > > + u16 htshi, trc, trc_required_mask;
    > > + u8 tse;
    > > +
    > > + ips = kzalloc(sizeof(struct ips_driver), GFP_KERNEL);
    > > + if (!ips)
    > > + return -ENOMEM;
    > > +
    > > + pci_set_drvdata(dev, ips);
    > > + ips->dev = dev;
    > > +
    > > + ips->limits = ips_detect_cpu(ips);
    > > + if (!ips->limits) {
    > > + dev_info(&dev->dev, "IPS not supported on this CPU\n");
    > > + ret = -ENODEV;
    >
    > hpa sez ENXIO.

    Oh I've never used that value before, fine with me.

    >
    > > + goto error_free;
    > > + }
    > > +
    > > + spin_lock_init(&ips->turbo_status_lock);
    > > +
    > > + if (!pci_resource_start(dev, 0)) {
    > > + dev_err(&dev->dev, "TBAR not assigned, aborting\n");
    > > + ret = -ENODEV;
    >
    > ditto. And there are more.
    >
    > > + goto error_free;
    > > + }
    > > +
    > > + ret = pci_request_regions(dev, "ips thermal sensor");
    > > + if (ret) {
    > > + dev_err(&dev->dev, "thermal resource busy, aborting\n");
    > > + ret = -EBUSY;
    > > + goto error_free;
    > > + }
    >
    > There doesn't seem to be much point in assigning the
    > pci_request_regions() return value to `ret'. It could just do
    >
    > if (pci_request_regions(...)) {
    > ...
    > }
    >
    > or, better, propagate the pci_request_regions() return value.

    Yeah, I should remove the forced -EBUSY; though I think that's the only
    things pci_request_regions can return anyway...


    > > + if (ret) {
    > > + dev_err(&dev->dev, "request irq failed, aborting\n");
    > > + ret = -EBUSY;
    >
    > Again, don't trash callee's error code - propagate it.

    Yep.

    >
    > > + goto error_unmap;
    > > + }
    > > +
    > > + /* Enable aux, hot & critical interrupts */
    > > + thm_writeb(THM_TSPIEN, TSPIEN_AUX2_LOHI | TSPIEN_CRIT_LOHI |
    > > + TSPIEN_HOT_LOHI | TSPIEN_AUX_LOHI);
    > > + thm_writeb(THM_TEN, TEN_UPDATE_EN);
    > > +
    > > + /* Collect adjustment values */
    > > + ips->cta_val = thm_readw(THM_CTA);
    > > + ips->pta_val = thm_readw(THM_PTA);
    > > + ips->mgta_val = thm_readw(THM_MGTA);
    > > +
    > > + /* Save turbo limits & ratios */
    > > + rdmsrl(TURBO_POWER_CURRENT_LIMIT, ips->orig_turbo_limit);
    > > +
    > > + ips_enable_cpu_turbo(ips);
    > > + ips->cpu_turbo_enabled = true;
    > > +
    > > + /* Set up the work queue and monitor/adjust threads */
    > > + ips->monitor = kthread_run(ips_monitor, ips, "ips-monitor");
    > > + if (!ips->monitor) {
    > > + dev_err(&dev->dev,
    > > + "failed to create thermal monitor thread, aborting\n");
    > > + ret = -ENOMEM;
    > > + goto error_free_irq;
    > > + }
    > > +
    > > + ips->adjust = kthread_create(ips_adjust, ips, "ips-adjust");
    >
    > Nope, kthread_run() returns IS_ERR() on error.

    Oops, will fix.


    > > +#ifdef CONFIG_PM
    > > + .suspend = ips_suspend,
    > > + .resume = ips_resume,
    > > +#endif
    >
    > and nuke the ifdefs.

    Will do.

    >
    > > + .shutdown = ips_shutdown,
    > > +};
    > > +
    > >
    > > ...
    > >
    >
    >
    > I applied both patches, did `make allmodconfig' and tried to make
    > drivers/platform/x86/intel_ips.o:
    >
    > drivers/platform/x86/intel_ips.c: In function 'ips_get_i915_syms':
    > drivers/platform/x86/intel_ips.c:1361: error: 'i915_read_mch_val' undeclared (first use in this function)
    > drivers/platform/x86/intel_ips.c:1361: error: (Each undeclared identifier is reported only once
    > drivers/platform/x86/intel_ips.c:1361: error: for each function it appears in.)
    > drivers/platform/x86/intel_ips.c:1361: warning: type defaults to 'int' in declaration of 'type name'
    > drivers/platform/x86/intel_ips.c:1361: warning: cast from pointer to integer of different size
    > drivers/platform/x86/intel_ips.c:1361: warning: assignment makes pointer from integer without a cast
    > drivers/platform/x86/intel_ips.c:1364: error: 'i915_gpu_raise' undeclared (first use in this function)
    > drivers/platform/x86/intel_ips.c:1364: warning: type defaults to 'int' in declaration of 'type name'
    > drivers/platform/x86/intel_ips.c:1364: warning: cast from pointer to integer of different size
    > drivers/platform/x86/intel_ips.c:1364: warning: assignment makes pointer from integer without a cast
    > drivers/platform/x86/intel_ips.c:1367: error: 'i915_gpu_lower' undeclared (first use in this function)
    > drivers/platform/x86/intel_ips.c:1367: warning: type defaults to 'int' in declaration of 'type name'
    > drivers/platform/x86/intel_ips.c:1367: warning: cast from pointer to integer of different size
    > drivers/platform/x86/intel_ips.c:1367: warning: assignment makes pointer from integer without a cast
    > drivers/platform/x86/intel_ips.c:1370: error: 'i915_gpu_busy' undeclared (first use in this function)
    > drivers/platform/x86/intel_ips.c:1370: warning: type defaults to 'int' in declaration of 'type name'
    > drivers/platform/x86/intel_ips.c:1370: warning: cast from pointer to integer of different size
    > drivers/platform/x86/intel_ips.c:1370: warning: assignment makes pointer from integer without a cast
    > drivers/platform/x86/intel_ips.c:1373: error: 'i915_gpu_turbo_disable' undeclared (first use in this function)
    > drivers/platform/x86/intel_ips.c:1373: warning: type defaults to 'int' in declaration of 'type name'
    > drivers/platform/x86/intel_ips.c:1373: warning: cast from pointer to integer of different size
    > drivers/platform/x86/intel_ips.c:1373: warning: assignment makes pointer from integer without a cast
    >
    > Both x86_64 and i386 fail.

    Arg, forgot to include the i915_drm.h changes in the patch (they're
    sitting right here in my tree preventing compiler errors); will fix.

    --
    Jesse Barnes, Intel Open Source Technology Center


    \
     
     \ /
      Last update: 2010-05-12 10:15    [W:0.057 / U:211.784 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site