login
Search
I have a Dell notebook with an PCIe ExpressCard slot. I also have a PCIe ExpressCard SATA controller (uses sata_sil24 driver). I would like to be able to hot plug/unplug the controller card at will. But alas, Linux doesn't cope with it *unless* I boot the kernel with the card initially inserted. 1. Booting Linux kernel (latest 2.6.23) without the card inserted means that the card will never be detected, regardless of how many times subsequently the card is inserted/removed/whatever. 2. Booting Linux kernel *with* the card inserted means that it is detected and used, and can be unplugged/replugged as I please, with intervening suspend/resume (RAM or disk) cycles not interfering. 3. Booting Linux kernel without the card inserted, and then doing a suspend-to-disk poweroff, inserting the card, and powering on again, the card's BIOS extension runs as normal. But on resume from the suspend-to-disk, the running kernel again never sees the card, even after removing/reinserting/whatever. 4. All of this leads me to believe that the kernel must be doing some kind of once-only scan of hardware at boot time, and never repeating it afterwards. Loading/unloading all of the PCI/PCIe hotplug stuff has no effect on this, so it must be broken elsewhere. 5. It is not likely to be a BIOS thing, because it still fails on power-on (with card inserted) after a suspend-to-disk, which appears to the BIOS exactly the same as any other power-on. 6. But it's probably a "kernel relies on BIOS data structure read at boot time" issue, based on the observations above. Suggestions? Is this a known defect? Maybe with a known fix lurking in a git tree somewhere? 00:00.0 Host bridge: Intel Corporation Mobile 945GM/PM/GMS/940GML and 945GT Express Memory Controller Hub (rev 03) 00:01.0 PCI bridge: Intel Corporation Mobile 945GM/PM/GMS/940GML and 945GT Express PCI Express Root Port (rev 03) 00:1b.0 Audio device: Intel Corporation 82801G (ICH7 Family) High Definition Audio Controller (rev ...
Actually, I must now take back some of that. Most of these tests were done a month or two ago. With 2.6.23.1 running, I just now redid all of the tests. Now it seems that pciehp fails to notice a newly inserted card only after a suspend/resume cycle with the slot empty. I can now get it to work again by just doing: 1. remove the card, so the slot is empty. 2. rmmod pciehp; modprobe pciehp 3. insert the card again -- it works! So we just need to fix an issue or two with suspend/resume (RAM) in pciehp. Cheers -
On Tue, 16 Oct 2007 11:21:36 -0400 Hi Mark, So, just to make sure I understand, your reproducer for the failing case is: 1. Boot laptop with no card. 2. Load pciehp 3. Suspend laptop (slot is still empty) 4. Resume laptop (slot is still empty) 5. insert card - card is not detected. Can you tell me which Dell laptop you have, and also send me the dmesg output of the failing case after loading pciehp with pciehp_debug=1. Thanks, Kristen -
Note that at this point I can insert/remove cards and they work fine. If I suspend with card inserted, then lspci still shows the card on resume Correct. Then rmmod pciehp; modprobe pciehp; and it works again. Another thing: if a card is already in the slot before pciehp is loaded (under any circumstances), then pciehp does *not* see the card until I unplug/replug it. I also checked my modprobe.d/ options, and I am using pciehp_force=1. I'm attaching a syslog capture (if you just want the kernel stuff, then just do: grep 'kernel|logger' syslog.txt Also attached is a full lspci -vv for this machine, which happens to be a Dell Inspiron 9400 with 2.1GHz Core2Duo and 2GB of RAM. Cheers
On Tue, 16 Oct 2007 14:39:33 -0400 OK - I suspected something like this. Most Dell computers don't support ExpressCard hotplug using Native PCIe -- in fact, I've not seen a single one, they explicitly disable it because they have not validated it or they have and something didn't work right. I'll take a look at what you've got, but be aware that you are forcing pciehp to load and operate on a system where they've certainly either not tested it, or tested it and something bad happened. -
Perhaps. But this one works perfectly, except for two driver bugs: 1. Driver does not notice already-inserted cards after modprobe. 2. Driver fails to function after suspend/resume until reloaded. Both of those are fixable in the kernel. Cheers -
Ahh.. point 2 in particular suffers from "suspend/resume" not implemented. Or rather, implemented as a pair of "do nothing" functions. Cheers -
This patch below seems to fix point 1 on my system,
causing pciehp to become aware of already-inserted cards on module load.
It's not perfect, but I believe it does show the kind of functionality
that's missing from the driver.
The resume() function will need something similar, to poll the slots
on resume and call pciehp_enable_slot() or pciehp_disable_slot()
as appropriate.
I suspect this is broken even on machines that do have ACPI BIOS
support.
Cheers
Not for kernel inclusion (yet), but..
Signed-off-by: Mark Lord <mlord@pobox.com>
--- old/drivers/pci/hotplug/pciehp_core.c 2007-10-12 12:43:44.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_core.c 2007-10-16 15:22:46.000000000 -0400
@@ -475,6 +475,9 @@
rc = t_slot->hpc_ops->power_off_slot(t_slot); /* Power off slot if not occupied*/
if (rc)
goto err_out_free_ctrl_slot;
+ } else {
+ extern int pciehp_enable_slot(struct slot *p_slot);
+ pciehp_enable_slot(t_slot);
}
return 0;
--- old/drivers/pci/hotplug/pciehp_ctrl.c 2007-10-12 12:43:44.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_ctrl.c 2007-10-16 15:22:44.000000000 -0400
@@ -37,7 +37,7 @@
#include "pciehp.h"
static void interrupt_event_handler(struct work_struct *work);
-static int pciehp_enable_slot(struct slot *p_slot);
+ int pciehp_enable_slot(struct slot *p_slot);
static int pciehp_disable_slot(struct slot *p_slot);
static int queue_interrupt_event(struct slot *p_slot, u32 event_type)
-
On Tue, 16 Oct 2007 15:31:29 -0400 No - it's not broken. Powering off the slot if it is not occupied is the right thing to do - the controller when it is working properly will detect the presence of a new adapter and interrupt. I'll try to duplicate your problem on a piece of hardware that has proper firmware support and validated hardware and then we'll go from there. We could very well have software problems, especially with ExpressCard since most pciehp use is for servers, but we should make sure we aren't writing workarounds for broken hardware first. -
On Tue, 16 Oct 2007 15:31:29 -0400 I tried to reproduce this on a Lenovo T61, which does have proper firmware support for _OSC, and also has been validated, and the driver which is in 2.6.23-git8 seems to work fine, even across suspend resume. I suspect that your system just doesn't support pcie hotplug properly. You might try getting a BIOS update from Dell. -
No, the hardware seems to work perfectly. We just have a software issue. We *know* it's only software because rmmod+modprobe fixes things, without any hardware intervention. I believe the code is leaning too heavily on the BIOS for stuff, and like lots of other parts of the kernel we'll need an alternate The machine already has the latest BIOS, thanks. Cheers -
On Tue, 16 Oct 2007 16:39:25 -0400 This reinitializes the controller, which is probably why things work the pitfall for forcing pciehp when the BIOS hasn't provided OSC is that you don't know for sure that you really have gained control of hot plug operation properly. You can obviously try it, using the provide forcing option as you have done, but the behavior is not predictable. -
The bigger concern is whether this likely to break things on systems
that *do* correctly implement ACPI support for PCIe hotplug?
- Ted
-
Dell laptops are known to have broken BIOS acpi code in regards to pci express hotplug issues. Seriously, we rely on acpi here for a lot of this, and unless it is properly set up, it will not work. And this is broken because Windows can't even handle this kind of thing, so it isn't tested :( So a short note to Dell might be helpful, although in the end, I think this looks like a kernel issue as you have proven with your patch. thanks, greg k-h -
Fix PCIe Hotplug so that it works with ExpressCard slots on Dell notebooks in conjunction with modparam of pciehp_force=1. The PCIe Hotplug driver has two shortcomings when used on Dell notebooks which lack ACPI BIOS support for PCIe hotplug: 1. The driver does not recognise cards that were inserted prior to the driver being modprobe'd. 2. The driver stops functioning after a suspend/resume (RAM) cycle, and needs to be rmmod'd and modprobe'd to get it working again. This patch addresses both issues. Issue 1 is fixed by modifying pciehp_probe() to either enable or disable the slot based on whether or not a card is detected at module init time. Issue 2 is fixed by implementing pciehp_resume() to do the same after having it first reinitialize the slot event registers. The code to reinitialize those registers has been broken away from pcie_init() so that it can be shared with pciehp_resume(). I'm not certain that locking contraints are correct in pciehp_resume(), so it would be quite useful for the PCIe hotplug folks to look this all over and provide suggestions/fixes as needed. There are also a few minor cosmetic changes to satisfy checkpatch.pl. pciehp.h | 3 pciehp_core.c | 31 ++++++-- pciehp_ctrl.c | 4 - pciehp_hpc.c | 207 +++++++++++++++++++++++++++++++++------------------------- 4 files changed, 144 insertions(+), 101 deletions(-) Signed-off-by: Mark Lord <mlord@pobox.com> --- --- old/drivers/pci/hotplug/pciehp.h 2007-10-12 12:43:44.000000000 -0400 +++ linux/drivers/pci/hotplug/pciehp.h 2007-10-16 17:53:48.000000000 -0400 @@ -161,6 +161,9 @@ extern int pciehp_unconfigure_device(struct slot *p_slot); extern void pciehp_queue_pushbutton_work(struct work_struct *work); int pcie_init(struct controller *ctrl, struct pcie_device *dev); +int pciehp_enable_slot(struct slot *p_slot); +int pciehp_disable_slot(struct slot *p_slot); +int pcie_init_enable_events(struct controller *ctrl, struct pcie_device *dev); static inline struct ...
On Tue, 16 Oct 2007 17:57:03 -0400 Please resubmit, breaking this patch into 3 separate patches 1 for your first issue you wish to address, 2 for the second, and 3 for cosmetic changes. Thanks, -
It'll have to be the other way around, then. The first patch will be one to fix the broken whitespace and lines > 80 characters that already exist in the code being touched. Otherwise *none* of the patches pass checkpatch.pl paranoia. Subsequent patches will then follow for the two issues. Cheers -
Wait, Dell explicitly says that pci hotplug of express cards is not supported and is broken on these laptops. This is because the version of Windows they support on these machines also does not support hotplugging these devices. The current code works just fine on hardware that actually supports this kind of functionality, as per the proper specs and requirements for this feature. So why try to go through these gyrations for hardware that is explicitly broken? thanks, greg k-h -
Because it is NOT broken. It works perfectly. There's just a couple of issues in the existing *working* Linux driver that need fixing, that's all. Cheers -
On Tue, 16 Oct 2007 15:03:54 -0700 Just to add to why sometimes vendors disable PCIe hotplug by not providing _OSC - sometimes there are problems with the hardware itself, and so due to schedule and other reasons, vendors will decide to just disable the support by not providing OSC, since really according to spec we shouldn't be running the PCIe hot plug driver if OSC doesn't exist or fails. This can be anything from "Well, we don't have time to test it" to actual hardware errata. At any rate, we don't know the reasons. I think when we force these things, you run the danger that things will break on you in some strange ways. Just out of curiosity, did you try using ACPI based hotplug for this laptop? Sometimes vendors will let you do that because then they can include hardware workarounds in the AML. -
Original single patch is now broken out into tiny pieces for easier review. Also, valuable feedback from Ted has been incorporated, to avoid any possible side effects on regular use of the PCIe hotplug stuff (when used without pciehp_force=1 mod parm). * * * Fix PCIe Hotplug so that it works with ExpressCard slots on Dell notebooks in conjunction with the modparam of pciehp_force=1. The PCIe Hotplug driver has two shortcomings when used on Dell notebooks which lack ACPI BIOS support for PCIe hotplug: 1. The driver does not recognise cards that were inserted prior to the driver being modprobe'd. 2. The driver stops functioning after a suspend/resume (RAM) cycle, and needs to be rmmod'd and modprobe'd to get it working again. This patch series addresses those issues, resulting in a completely functional PCIe Hotplug driver for Dell notebooks, and probably others as well, which may lack ACPI BIOS support for this. There are four patches in this series: 01_pciehp_cosmetic_fixes.patch -- cosmetic fixes, mostly to keep checkpatch.pl happy 02_pciehp_handle_preinserted_card.patch -- fixes problem number 1 (above). 03_pciehp_split_pcie_init.patch -- preparation for the resume patch. 04_pciehp_resume.patch -- fixes problem number 2 (above). diffstat summary for 01_pciehp_cosmetic_fixes.patch only: drivers/pci/hotplug/pciehp_core.c | 6 - drivers/pci/hotplug/pciehp_ctrl.c | 2 drivers/pci/hotplug/pciehp_hpc.c | 119 ++++++++++++++-------------- 3 files changed, 65 insertions(+), 62 deletions(-) diffstat summary for the other three patches combined: drivers/pci/hotplug/pciehp.h | 3 drivers/pci/hotplug/pciehp_core.c | 21 ++- drivers/pci/hotplug/pciehp_ctrl.c | 2 drivers/pci/hotplug/pciehp_hpc.c | 194 ++++++++++++++++------------ 4 files changed, 134 insertions(+), 86 deletions(-) Cheers -- Mark Lord <mlord@pobox.com> -
Whitespace and other cosmetic fixes so that checkpatch.pl
is happy with the remainder of patches in this series.
Signed-off-by: Mark Lord <mlord@pobox.com>
---
--- old/drivers/pci/hotplug/pciehp_core.c 2007-10-12 12:43:44.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_core.c 2007-10-16 21:14:03.000000000 -0400
@@ -470,9 +470,11 @@
t_slot = pciehp_find_slot(ctrl, ctrl->slot_device_offset);
- t_slot->hpc_ops->get_adapter_status(t_slot, &value); /* Check if slot is occupied */
+ /* Check if slot is occupied */
+ t_slot->hpc_ops->get_adapter_status(t_slot, &value);
if ((POWER_CTRL(ctrl->ctrlcap)) && !value) {
- rc = t_slot->hpc_ops->power_off_slot(t_slot); /* Power off slot if not occupied*/
+ /* Power off slot if not occupied*/
+ rc = t_slot->hpc_ops->power_off_slot(t_slot);
if (rc)
goto err_out_free_ctrl_slot;
}
--- old/drivers/pci/hotplug/pciehp_ctrl.c 2007-10-16 21:12:54.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_ctrl.c 2007-10-16 21:08:18.000000000 -0400
@@ -520,7 +520,7 @@
case INT_PRESENCE_OFF:
if (!HP_SUPR_RM(ctrl->ctrlcap))
break;
- dbg("Surprise Removal\n");
+ dbg("Surprise Event\n");
update_slot_info(p_slot);
handle_surprise_event(p_slot);
break;
--- old/drivers/pci/hotplug/pciehp_hpc.c 2007-10-16 21:12:54.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_hpc.c 2007-10-16 21:13:32.000000000 -0400
@@ -160,10 +160,10 @@
/* Link Width Encoding */
#define LNK_X1 0x01
#define LNK_X2 0x02
-#define LNK_X4 0x04
+#define LNK_X4 0x04
#define LNK_X8 0x08
#define LNK_X12 0x0C
-#define LNK_X16 0x10
+#define LNK_X16 0x10
#define LNK_X32 0x20
/*Field definitions of Link Status Register */
@@ -289,7 +289,7 @@
u16 slot_ctrl;
unsigned long flags;
- DBG_ENTER_ROUTINE
+ DBG_ENTER_ROUTINE
mutex_lock(&ctrl->ctrl_lock);
@@ -299,7 +299,7 @@
goto out;
}
- if ((slot_status & CMD_COMPLETED) == CMD_COMPLETED ) {
+ if ((slot_status & CMD_COMPLETED) == CMD_COMPLETED) {
...Fix pciehp_probe() to deal with pre-inserted ExpressCard cards,
but only when pciehp_force==1. Otherwise behaviour is unmodified.
Signed-off-by: Mark Lord <mlord@pobox.com>
---
--- old/drivers/pci/hotplug/pciehp_ctrl.c 2007-10-16 21:14:44.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_ctrl.c 2007-10-16 21:16:36.000000000 -0400
@@ -37,7 +37,6 @@
#include "pciehp.h"
static void interrupt_event_handler(struct work_struct *work);
-static int pciehp_enable_slot(struct slot *p_slot);
static int pciehp_disable_slot(struct slot *p_slot);
static int queue_interrupt_event(struct slot *p_slot, u32 event_type)
--- old/drivers/pci/hotplug/pciehp.h 2007-10-12 12:43:44.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp.h 2007-10-16 21:16:06.000000000 -0400
@@ -161,6 +161,7 @@
extern int pciehp_unconfigure_device(struct slot *p_slot);
extern void pciehp_queue_pushbutton_work(struct work_struct *work);
int pcie_init(struct controller *ctrl, struct pcie_device *dev);
+int pciehp_enable_slot(struct slot *p_slot);
static inline struct slot *pciehp_find_slot(struct controller *ctrl, u8 device)
{
--- old/drivers/pci/hotplug/pciehp_core.c 2007-10-16 21:14:44.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_core.c 2007-10-16 21:15:56.000000000 -0400
@@ -477,7 +477,8 @@
rc = t_slot->hpc_ops->power_off_slot(t_slot);
if (rc)
goto err_out_free_ctrl_slot;
- }
+ } else if (pciehp_force)
+ pciehp_enable_slot(t_slot);
return 0;
-
Split out the hotplug hardware initialization code from pcie_init()
into pcie_init_enable_events(), without changing any functionality.
Signed-off-by: Mark Lord <mlord@pobox.com>
---
--- old/drivers/pci/hotplug/pciehp_hpc.c 2007-10-16 21:14:44.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_hpc.c 2007-10-16 21:21:11.000000000 -0400
@@ -1163,101 +1163,23 @@
}
#endif
-int pcie_init(struct controller *ctrl, struct pcie_device *dev)
+int pcie_init_enable_events(struct controller *ctrl, struct pcie_device *dev)
{
int rc;
u16 temp_word;
- u16 cap_reg;
u16 intr_enable = 0;
u32 slot_cap;
- int cap_base;
- u16 slot_status, slot_ctrl;
+ u16 slot_status;
struct pci_dev *pdev;
DBG_ENTER_ROUTINE
pdev = dev->port;
- ctrl->pci_dev = pdev; /* save pci_dev in context */
-
- dbg("%s: hotplug controller vendor id 0x%x device id 0x%x\n",
- __FUNCTION__, pdev->vendor, pdev->device);
-
- if ((cap_base = pci_find_capability(pdev, PCI_CAP_ID_EXP)) == 0) {
- dbg("%s: Can't find PCI_CAP_ID_EXP (0x10)\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
-
- ctrl->cap_base = cap_base;
-
- dbg("%s: pcie_cap_base %x\n", __FUNCTION__, cap_base);
-
- rc = pciehp_readw(ctrl, CAPREG, &cap_reg);
- if (rc) {
- err("%s: Cannot read CAPREG register\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
- dbg("%s: CAPREG offset %x cap_reg %x\n",
- __FUNCTION__, ctrl->cap_base + CAPREG, cap_reg);
-
- if (((cap_reg & SLOT_IMPL) == 0)
- || (((cap_reg & DEV_PORT_TYPE) != 0x0040)
- && ((cap_reg & DEV_PORT_TYPE) != 0x0060))) {
- dbg("%s : This is not a root port"
- " or the port is not connected to a slot\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
-
rc = pciehp_readl(ctrl, SLOTCAP, &slot_cap);
if (rc) {
err("%s: Cannot read SLOTCAP register\n", __FUNCTION__);
goto abort_free_ctlr;
}
- dbg("%s: SLOTCAP offset %x slot_cap %x\n",
- __FUNCTION__, ctrl->cap_base + SLOTCAP, slot_cap);
-
- if (!(slot_cap & HP_CAP)) {
- dbg("%s : This slot is not ...Make use of the previously split out pcie_init_enable_events() function
to reinitialize the hotplug hardware on resume from suspend,
but only when pciehp_force==1. Otherwise behaviour is unmodified.
Signed-off-by: Mark Lord <mlord@pobox.com>
---
--- old/drivers/pci/hotplug/pciehp_ctrl.c 2007-10-16 21:17:27.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_ctrl.c 2007-10-16 21:28:36.000000000 -0400
@@ -37,7 +37,6 @@
#include "pciehp.h"
static void interrupt_event_handler(struct work_struct *work);
-static int pciehp_disable_slot(struct slot *p_slot);
static int queue_interrupt_event(struct slot *p_slot, u32 event_type)
{
--- old/drivers/pci/hotplug/pciehp.h 2007-10-16 21:17:27.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp.h 2007-10-16 21:28:42.000000000 -0400
@@ -162,6 +162,8 @@
extern void pciehp_queue_pushbutton_work(struct work_struct *work);
int pcie_init(struct controller *ctrl, struct pcie_device *dev);
int pciehp_enable_slot(struct slot *p_slot);
+int pciehp_disable_slot(struct slot *p_slot);
+int pcie_init_enable_events(struct controller *ctrl, struct pcie_device *dev);
static inline struct slot *pciehp_find_slot(struct controller *ctrl, u8 device)
{
--- old/drivers/pci/hotplug/pciehp_core.c 2007-10-16 21:17:27.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_core.c 2007-10-16 21:30:19.000000000 -0400
@@ -512,6 +512,24 @@
static int pciehp_resume (struct pcie_device *dev)
{
printk("%s ENTRY\n", __FUNCTION__);
+ if (pciehp_force) {
+ struct pci_dev *pdev = dev->port;
+ struct controller *ctrl = pci_get_drvdata(pdev);
+ struct slot *t_slot;
+ u8 status;
+
+ /* reinitialize the chipset's event detection logic */
+ pcie_init_enable_events(ctrl, dev);
+
+ t_slot = pciehp_find_slot(ctrl, ctrl->slot_device_offset);
+
+ /* Check if slot is occupied */
+ t_slot->hpc_ops->get_adapter_status(t_slot, &status);
+ if (status)
+ pciehp_enable_slot(t_slot);
+ else
+ pciehp_disable_slot(t_slot);
+ }
return 0;
}
...On Tue, 16 Oct 2007 21:55:30 -0400 OK - definitely in this case the right thing to do is not use this code unless you are forcing pciehp, thanks. I think I'd be careful when you rename this patch - non-ACPI ExpressCard slots is not what you want to say, as this fix is very specific for your machine. We have no idea if this will fix any other problems that occur when people force pciehp - and in many cases this either may not be needed, or may not be enough. I only care about the -
No, from reading through the driver it seems that any machine that lacks ACPI BIOS support for PCIe slots will suffer from the exact same problems. That's a large class of machines, not just my tiny little individual one here (of which *millions* have been manufactured and sold). But, whatever. -
I find the construct if () { ... } else ...; to be a bit jarring. How
about adding the extra braces?
--
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
-
On Tue, 16 Oct 2007 21:54:42 -0400 I think it would be ok to try allowing the slot to be enabled when not using pciehp_force mode. We can wrap it later if it proves to break Here it seems like what you want to do just go ahead and try to call pciehp_enable_slot always, but check the return value. If an adapter is not present, it will return -ENODEV, and then you can check to see if you have the ability to power off the slot, and try to power it off. -
Could you test that on ACPI-supported hardware and report back, please. ???? -
I'd argue these comments fall under "stating the bleedin' obvious", but
That doesn't seem like an obviously correct change to me. Can you
Normal style would be more like ...
if (((cap_reg & SLOT_IMPL) == 0) ||
(((cap_reg & DEV_PORT_TYPE) != 0x0040) &&
((cap_reg & DEV_PORT_TYPE) != 0x0060))) {
dbg("%s : This is not a root port or the port is not "
Why did you choose to break the format string?
info("HPC vendor_id %x device_id %x ss_vid %x ss_did %x\n",
pdev->vendor, pdev->device,
*boggle*
temp_word |= intr_enable;
--
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
-
Hey, they're original to the file. I'm just keeping checkpatch.pl happy here. Yeah. You clipped the top line: case INT_PRESENCE_ON: So that code can be run for both hot plug and hot remove operations. Dig out your text editor, and notice the excess whitespace at the end of the line, along with similar stuff on most other lines in this patch. This is all just stuff that checkpatch.pl complains about when the later patches are applied, so patch 01_... serves to reduce the noise level from checkpatch.pl for the *real* patches which follow. -ml -
I can see that they were there before. I just think the appropriate I know, but if you're going to remove whitespace from a line, you might as well change the line to look like idiomatic C rather than something an Ada programmer wrote. -- Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." -
Gag-me. That *is* bad. But the PCIe folks seem to be extra sensitive about the code, so I'm trying to change as little as possible here. That line was just the result of doing this in vim: :%s/[ ]*$//g I agree there's a lot of funny looking code in there, but that's not what this particular patch series is about. You could submit a follow-up patch to repair that stuff, if you like. Mmm.. I wonder how clever gcc is with a line like that.. ? ;) Cheers! -
On Tue, 16 Oct 2007 21:53:24 -0400 You just sent four patches all of which are identified as "Fix PCIe hotplug for non-ACPI ExpressCard slots (version 2)". Please do not do this. I went through the (tiresome) exercise of inventing unique names for them and then discovered that the first patch gets just a couple of rejects against the mainline tree.... Hunk #1 succeeded at 129 (offset -31 lines). Hunk #2 FAILED at 258. Hunk #3 succeeded at 262 (offset -37 lines). Hunk #4 FAILED at 295. Hunk #5 FAILED at 304. Hunk #6 FAILED at 313. Hunk #7 FAILED at 331. Hunk #8 FAILED at 363. Hunk #9 FAILED at 373. Hunk #10 FAILED at 391. Hunk #11 FAILED at 409. Hunk #12 FAILED at 417. Hunk #13 FAILED at 430. Hunk #14 FAILED at 440. Hunk #15 FAILED at 451. Hunk #16 FAILED at 459. Hunk #17 FAILED at 535. Hunk #18 FAILED at 546. Hunk #19 FAILED at 592. Hunk #20 FAILED at 612. Hunk #21 FAILED at 637. Hunk #22 FAILED at 694. Hunk #23 succeeded at 734 (offset -91 lines). Hunk #24 succeeded at 744 (offset -91 lines). Hunk #25 succeeded at 801 (offset -91 lines). Hunk #26 succeeded at 813 (offset -91 lines). Hunk #27 FAILED at 824. Hunk #28 FAILED at 843. Hunk #29 FAILED at 854. Hunk #30 FAILED at 894. Hunk #31 FAILED at 905. Hunk #32 FAILED at 924. Hunk #33 FAILED at 935. Hunk #34 FAILED at 975. Hunk #35 succeeded at 988 (offset -97 lines). Hunk #36 FAILED at 1066. Hunk #37 FAILED at 1078. Hunk #38 FAILED at 1102. Hunk #39 FAILED at 1145. Hunk #40 FAILED at 1171. Hunk #41 succeeded at 1235 (offset -96 lines). Hunk #42 FAILED at 1250. Hunk #43 succeeded at 1264 (offset -93 lines). 34 out of 43 hunks FAILED -- saving rejects to file drivers/pci/hotplug/pciehp_hpc.c.rej -
Eh? They were labelled as 1/4, 2/4, 3/4, and 4/4. But I'll make the rest of the subject line unique as well on resubmit, then. Thanks. So far, two postings, and zero comments from anyone on the actual code. Cheers -
On Wed, 17 Oct 2007 18:59:43 -0400 Please, review http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt - a lot of experiecne has gone into that... When preparing patches, always think "how will this appear to someone who is reading it in the git tree a year from now". Obviously stuff like "[patch 2/4]" won't be there, because it is meaningless once the patch hits the git tree. Also text such as "the previous patch" and "my patch from yesterday" and "John Doe's comments" and lots of other stuff which is appropriate to an email conversation is _not_ Thanks. That's much better than having me invent the subject, because when you choose the title, everyone agrees on what the patch is _called_ (as long as the title isn't so poorly chosen that I have to fix it). If the patch title is well-chosen then it becomes a nice google search key, so if someone wants to find out what we were thinking when we did a particular patch two years ago, they can just google for the title and go Actual code? That sounds hard ;) -
On Tue, 16 Oct 2007 14:39:33 -0400 One other thing to note here, the proper operation of hot plug dictates that if you boot your laptop with no adapter in the slot, you must first load the driver in order for the new adapter interrupt to be captured. So, if you insert a card without the driver loaded, it will not be enumerated by PCI until you remove it and the reinsert it. -
You mean that the existing code does not handle it properly. The hardware can see the card just fine, as reflected in the slot Bull puckey. If there's already a card in the slot when we turn on the power, the Linux kernel should find and use that card. This is flawed, and needs fixing. Cheers -
