From: Andi Kleen <ak@linux.intel.com>
To: Len Brown <lenb@kernel.org>
Cc: Ingo Molnar <mingo@elte.hu>,
linux-acpi@vger.kernel.org,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Venki Pallipadi <venkatesh.pallipadi@intel.com>,
Andy Henroid <andrew.d.henroid@intel.com>,
Linus Torvalds <torvalds@linux-foundation.org>,
Thomas Gleixner <tglx@linutronix.de>,
"H. Peter Anvin" <hpa@zytor.com>
Subject: Re: [PATCH 2/2] i7300_idle driver v1.55
Date: Tue, 21 Oct 2008 18:47:42 +0200 [thread overview]
Message-ID: <48FE07AE.4010203@linux.intel.com> (raw)
In-Reply-To: <alpine.LFD.1.10.0810161805360.3026@localhost.localdomain>
Len Brown wrote:
> diff --git a/drivers/dma/ioat_dma.c b/drivers/dma/ioat_dma.c
> index bc8c6e3..f8396ca 100644
> --- a/drivers/dma/ioat_dma.c
> +++ b/drivers/dma/ioat_dma.c
> @@ -171,6 +171,9 @@ static int ioat_dma_enumerate_channels(struct ioatdma_device *device)
> xfercap_scale = readb(device->reg_base + IOAT_XFERCAP_OFFSET);
> xfercap = (xfercap_scale == 0 ? -1 : (1UL << xfercap_scale));
>
> +#if CONFIG_I7300_IDLE_IOAT_CHANNEL
> + device->common.chancnt--;
> +#endif
I still think this lone decrement looks fishy. Can there please be some
explanation how it exactly relates to the i7300 idle driver, where the
matching increment is, etc.?
> +config I7300_IDLE
> + tristate "Intel chipset idle power saving driver"
It would be probably good to mention the word memory here.
> + select I7300_IDLE_IOAT_CHANNEL
> + depends on X86_64
> + help
> + Enable idle power savings with certain Intel server chipsets.
And here too.
> + The chipset must have I/O AT support, such as the Intel 7300.
> + The power savings depends on the type and quantity of DRAM devices.
> +static int debug;
> +module_param_named(debug, debug, uint, 0644);
> +MODULE_PARM_DESC(debug, "Enable debug printks in this driver");
> +#define dprintk(fmt, arg...) \
> + do { if (debug) printk(KERN_INFO I7300_PRINT fmt, ##arg); } while (0)
2.6.28 just got a new dynamic printk facility, which could be used.
> +
> +/*
> + * Value to set THRTLOW to when initiating throttling
> + * 0 = No throttling
> + * 1 = Throttle when > 4 activations per eval window (Maximum throttling)
> + * 2 = Throttle when > 8 activations
> + * 168 = Throttle when > 168 activations (Minimum throttling)
> + */
> +#define MAX_THRTLWLIMIT 168
> +static uint i7300_idle_thrtlowlm = 1;
> +module_param_named(thrtlwlimit, i7300_idle_thrtlowlm, uint, 0644);
Just imagining how someone would pronounce this parameter @) Will they
get damages when their tongue ends up in a knot?
> +static cpumask_t idle_cpumask;
Would it make sense to cache align this field? I could imagine
it false sharing with some frequently written variable could be quite
bad.
> + writeb(IOAT_CHANCMD_RESET, ioat_chanbase + IOAT1_CHANCMD_OFFSET);
> + writeb(IOAT_CHANCMD_START, ioat_chanbase + IOAT1_CHANCMD_OFFSET);
> +
> + udelay(1000);
> +
> + chan_sts = readq(ioat_chanbase + IOAT1_CHANSTS_OFFSET) &
> + IOAT_CHANSTS_DMA_TRANSFER_STATUS;
Wouldn't it be better to poll here instead of udelay?
> + /* Wait for a while for the channel to halt before releasing */
> + for (i = 0; i < 10; i++) {
> + writeb(IOAT_CHANCMD_RESET,
> + ioat_chanbase + IOAT1_CHANCMD_OFFSET);
> +
> + chan_sts = readq(ioat_chanbase + IOAT1_CHANSTS_OFFSET) &
> + IOAT_CHANSTS_DMA_TRANSFER_STATUS;
> +
> + if (chan_sts != IOAT_CHANSTS_DMA_TRANSFER_STATUS_ACTIVE) {
> + writew(0, ioat_chanbase + IOAT_CHANCTRL_OFFSET);
> + break;
> + }
> + udelay(1000);
Same here.
> + spin_lock_irqsave(&i7300_idle_lock, flags);
This lock still scares me. In the worst case with very frequent
idle frequencies this could bounce around a lot.
It would be better to only invoke it less frequently,
i.e. when the driver determines there is a long idle
time coming up.
> +/* Check for known platforms with I/O-AT */
> +static int __init i7300_idle_platform_probe(void)
> +{
> + int i;
> +
> + fbd_dev = pci_get_bus_and_slot(MEMCTL_BUS, MEMCTL_DEVFN)
Is there a specific reason you cannot match this by pci vendor/devid
like all standard drivers do?
If there is a good reason add a comment.
;
> +static void __exit i7300_idle_exit(void)
> +{
> + idle_notifier_unregister(&i7300_idle_nb);
I still think this needs some kind of idle synchronization.
-Andi
next prev parent reply other threads:[~2008-10-21 16:48 UTC|newest]
Thread overview: 111+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-10-11 6:35 ACPI patch queue for 2.6.28 Len Brown
2008-10-11 6:35 ` [PATCH 01/85] ACPI: Add missing acpi.debug_layer Documentation hunk from Thomas Renninger Len Brown
2008-10-11 6:35 ` [PATCH 02/85] acpi: replace AE_BAD_ADDRESS exception code with AE_ERROR Len Brown
2008-10-11 6:35 ` [PATCH 03/85] ACPI: Get the device power state in the course of scanning device Len Brown
2008-10-11 6:35 ` [PATCH 04/85] ACPI: Attach the ACPI device to the ACPI handle as early as possible Len Brown
2008-10-11 6:35 ` [PATCH 05/85] ACPI: Add "acpi.power_nocheck=1" to disable power state check in power transition Len Brown
2008-10-11 6:35 ` [PATCH 06/85] ACPI: Add DMI check " Len Brown
2008-10-11 6:35 ` [PATCH 07/85] ACPI : Load device driver according to the status of acpi device Len Brown
2008-10-11 6:35 ` [PATCH 08/85] ACPICA: Copy dynamically loaded tables to local buffer Len Brown
2008-10-11 6:35 ` [PATCH 09/85] ACPICA: Add check for invalid handle in acpi_get_object_info Len Brown
2008-10-11 6:35 ` [PATCH 10/85] ACPICA: Allow same ACPI table to be loaded/unloaded more than once Len Brown
2008-10-11 6:35 ` [PATCH 11/85] ACPICA: Fix wrong resource descriptor length for 64-bit build Len Brown
2008-10-11 6:35 ` [PATCH 12/85] ACPICA: Fix table compare code, length then data Len Brown
2008-10-11 6:35 ` [PATCH 13/85] ACPICA: Return status from global init function Len Brown
2008-10-11 6:35 ` [PATCH 14/85] ACPICA: Add function to dereference returned reference objects Len Brown
2008-10-11 6:35 ` [PATCH 15/85] ACPICA: Fix warning for 64-bit build Len Brown
2008-10-11 6:35 ` [PATCH 16/85] ACPICA: Cleanup macro definition file Len Brown
2008-10-11 6:35 ` [PATCH 17/85] ACPICA: Return method arg count from acpi_get_object_info Len Brown
2008-10-11 6:35 ` [PATCH 18/85] ACPICA: Update version to 20080701 Len Brown
2008-10-11 6:35 ` [PATCH 19/85] ACPICA: Add function to decode reference obj types to strings Len Brown
2008-10-11 6:35 ` [PATCH 20/85] ACPICA: Improve object conversion error messages Len Brown
2008-10-11 6:35 ` [PATCH 21/85] ACPICA: x2APIC support: changes for MADT and SRAT ACPI tables Len Brown
2008-10-11 6:35 ` [PATCH 22/85] ACPICA: Update version to 20080729 Len Brown
2008-10-11 6:35 ` [PATCH 23/85] ACPI: Clear WAK_STS on resume Len Brown
2008-10-13 9:01 ` Christian Borntraeger
2008-10-11 6:35 ` [PATCH 24/85] ACPI : Add the support for _TTS object Len Brown
2008-10-11 6:35 ` [PATCH 25/85] Introduce FW_BUG, FW_WARN and FW_INFO to consistenly tell users about BIOS bugs Len Brown
2008-10-11 6:35 ` [PATCH 26/85] ACPI: cpufreq, processor: Detect old BIOS, not supporting CPU freq on a recent CPU Len Brown
2008-10-11 6:35 ` [PATCH 27/85] CPUFREQ: powernow-k8: Try to detect old BIOS, not supporting CPU freq on a recent AMD CPUs Len Brown
2008-10-11 6:35 ` [PATCH 28/85] ACPI: dock: avoid check _STA method Len Brown
2008-10-11 6:35 ` [PATCH 29/85] dock: fix eject request process (2.6.27-rc1 regression) Len Brown
2008-10-11 6:35 ` [PATCH 30/85] dock: add _LCK support Len Brown
2008-10-11 6:35 ` [PATCH 31/85] dock: add bay and battery hotplug support Len Brown
2008-10-11 6:35 ` [PATCH 32/85] ACPI: introduce notifier change to avoid duplicates Len Brown
2008-10-11 6:35 ` [PATCH 33/85] ACPI: fix hotplug race Len Brown
2008-10-11 6:35 ` [PATCH 34/85] libata: remove functions now handed by ACPI dock driver Len Brown
2008-10-11 6:35 ` [PATCH 35/85] dock: introduce .uevent for devices in dock, eg libata Len Brown
2008-10-11 6:35 ` [PATCH 36/85] bay: remove driver, all functions now handled by dock driver Len Brown
2008-10-11 6:35 ` [PATCH 37/85] dock: fix for ATA bay in a dock station Len Brown
2008-10-11 6:35 ` [PATCH 38/85] dock: add 'type' sysfs file Len Brown
2008-10-11 6:35 ` [PATCH 39/85] dock: Shaohua Li is new maintainer Len Brown
2008-10-11 6:36 ` [PATCH 40/85] ACPI suspend: Always use the 32-bit waking vector Len Brown
2008-10-11 6:36 ` [PATCH 41/85] panasonic-laptop: add Panasonic Let's Note laptop extras driver v0.94 Len Brown
2008-10-11 6:36 ` [PATCH 42/85] ACPI: EC: do transaction from interrupt context Len Brown
2008-10-11 6:36 ` [PATCH 43/85] ACPI: EC: Rename some variables Len Brown
2008-10-11 6:36 ` [PATCH 44/85] ACPICA: add preemption point after each opcode parse Len Brown
2008-10-11 6:36 ` [PATCH 45/85] ACPI Suspend: Enable ACPI during resume if SCI_EN is not set Len Brown
2008-10-11 6:36 ` [PATCH 46/85] ACPI: don't enable control method power button as wakeup device when Fixed Power button is used Len Brown
2008-10-11 6:36 ` [PATCH 47/85] acer-wmi: Add rfkill support for wireless and bluetooth Len Brown
2008-10-11 6:36 ` [PATCH 48/85] acer-wmi: Remove wireless and bluetooth sysfs entries Len Brown
2008-10-11 6:36 ` [PATCH 49/85] ACPI: WMI: Enable event methods when registering notifiers Len Brown
2008-10-11 6:36 ` [PATCH 50/85] ACPI: Change acpi_evaluate_integer to support 64-bit on 32-bit kernels Len Brown
2008-10-11 6:36 ` [PATCH 51/85] ACPI: fix FADT parsing Len Brown
2008-10-11 6:36 ` [PATCH 52/85] fujitsu-laptop: better handling of P8010 hotkey Len Brown
2008-10-11 6:36 ` [PATCH 53/85] i7300_idle driver v1.55 Len Brown
2008-10-11 8:33 ` Ingo Molnar
2008-10-15 22:32 ` Len Brown
2008-10-16 22:05 ` [PATCH 1/2] x86: allow module driver to register idle notifier Len Brown
2008-10-21 1:57 ` Len Brown
2008-10-21 11:49 ` Ingo Molnar
2008-10-16 22:08 ` [PATCH 2/2] i7300_idle driver v1.55 Len Brown
2008-10-20 17:32 ` Randy Dunlap
2008-10-21 22:09 ` Pallipadi, Venkatesh
2008-10-21 22:15 ` Randy Dunlap
2008-10-21 23:02 ` Pallipadi, Venkatesh
2008-10-21 16:47 ` Andi Kleen [this message]
2008-10-21 22:34 ` Pallipadi, Venkatesh
2008-10-22 7:19 ` Andi Kleen
2008-10-22 23:34 ` [PATCH] Disable ioat channel only on platforms where ile driver can load Venki Pallipadi
2008-10-22 23:51 ` [PATCH] Cleanup of i7300_idle based on review comments Venki Pallipadi
2008-10-24 17:03 ` Len Brown
2008-10-24 17:02 ` [PATCH] Disable ioat channel only on platforms where ile driver can load Len Brown
[not found] ` <f12847240810270941v68a1cf7aqfa7db14fceb61fe5@mail.gmail.com>
2008-10-28 16:30 ` Sosnowski, Maciej
2008-10-22 23:40 ` [PATCH 2/2] i7300_idle driver v1.55 Venki Pallipadi
2008-10-23 1:11 ` Andi Kleen
2008-10-23 4:59 ` Pallipadi, Venkatesh
2008-10-23 12:26 ` Andi Kleen
2008-10-23 15:00 ` Paul E. McKenney
2008-10-23 16:26 ` Andi Kleen
2008-10-11 6:36 ` [PATCH 54/85] eeepc-laptop: Use standard interfaces Len Brown
2008-10-11 6:36 ` [PATCH 55/85] x86: remove magic number from ACPI sleep stack buffer Len Brown
2008-10-11 6:36 ` [PATCH 56/85] x86: trim " Len Brown
2008-10-11 6:36 ` [PATCH 57/85] ACPI: acpi_driver_data could only be applied to acpi_device Len Brown
2008-10-11 6:36 ` [PATCH 58/85] ACPI: toshiba_acpi.c fix sparse signedness mismatch warnings Len Brown
2008-10-11 6:36 ` [PATCH 59/85] ACPI: catch calls of acpi_driver_data on pointer of wrong type Len Brown
2008-10-11 6:36 ` [PATCH 60/85] toshiba_acpi: depends on INPUT Len Brown
2008-10-11 6:36 ` [PATCH 61/85] PNP: fix debug formatting (cosmetic) Len Brown
2008-10-11 6:36 ` [PATCH 62/85] PNPACPI: use dev_printk when possible Len Brown
2008-10-11 6:36 ` [PATCH 63/85] PNP: convert the last few pnp_info() uses to printk() Len Brown
2008-10-11 6:36 ` [PATCH 64/85] PNP: use new vsprintf symbolic function pointer format Len Brown
2008-10-11 6:36 ` [PATCH 65/85] PNP: remove some uses of DEBUG ifdef Len Brown
2008-10-11 6:36 ` [PATCH 66/85] PNP: add CONFIG_PNP_DEBUG_MESSAGES and pnp_dbg() Len Brown
2008-10-11 6:36 ` [PATCH 67/85] PNP: convert to using pnp_dbg() Len Brown
2008-10-11 6:36 ` [PATCH 68/85] PNP: remove old CONFIG_PNP_DEBUG option Len Brown
2008-10-11 6:36 ` [PATCH 69/85] ACPI: don't load acpi_cpufreq if acpi=off Len Brown
2008-10-11 6:36 ` [PATCH 70/85] ACPI: remove unused have_arch_parse_srat Len Brown
2008-10-11 6:36 ` [PATCH 71/85] acer-wmi: Remove private workqueue Len Brown
2008-10-11 6:36 ` [PATCH 72/85] Subject: ACPI dock: Use ACPI_EXCEPTION instead of printk(KERN_ERR Len Brown
2008-10-11 6:36 ` [PATCH 73/85] ACPI cm_sbs: Debug interface used for error message cleanup Len Brown
2008-10-11 6:36 ` [PATCH 74/85] ACPI memhotplug: " Len Brown
2008-10-11 6:36 ` [PATCH 75/85] ACPI processor: " Len Brown
2008-10-11 6:36 ` [PATCH 76/85] ACPI video: " Len Brown
2008-10-11 6:36 ` [PATCH 77/85] ACPI scan: " Len Brown
2008-10-11 6:36 ` [PATCH 78/85] ACPI fan: " Len Brown
2008-10-11 6:36 ` [PATCH 79/85] ACPI osl: " Len Brown
2008-10-11 6:36 ` [PATCH 80/85] ACPI thermal: " Len Brown
2008-10-11 6:36 ` [PATCH 81/85] ACPI system: " Len Brown
2008-10-11 6:36 ` [PATCH 82/85] ACPI: remove unused acpi_is_child_device() Len Brown
2008-10-11 6:36 ` [PATCH 83/85] ACPI: Enable EC device immediately after ACPI full initialization Len Brown
2008-10-11 6:36 ` [PATCH 84/85] delete drivers/acpi/bay.c Len Brown
2008-10-11 6:36 ` [PATCH 85/85] panasonic-laptop: fix build Len Brown
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=48FE07AE.4010203@linux.intel.com \
--to=ak@linux.intel.com \
--cc=andrew.d.henroid@intel.com \
--cc=hpa@zytor.com \
--cc=lenb@kernel.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@elte.hu \
--cc=tglx@linutronix.de \
--cc=torvalds@linux-foundation.org \
--cc=venkatesh.pallipadi@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).