| Message ID | 20121213200857.GA24939@otc-wbsnb-06 (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Thu, Dec 13, 2012 at 12:08 PM, Kirill A. Shutemov <kirill.shutemov@linux.intel.com> wrote: > On Thu, Dec 13, 2012 at 10:48:20AM -0800, Greg KH wrote: >> On Thu, Dec 13, 2012 at 05:31:48PM +0200, Kirill A. Shutemov wrote: >> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> >> > >> > BIOS on Intel DZ77RE-75K motherboard notifies OS about Thunderbolt >> > hotplug before devices behind Thunderbolt are ready to be enumerated. >> > >> > Let's delay enumeration by 2 seconds. >> > >> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> >> > --- >> > drivers/pci/hotplug/acpiphp_glue.c | 16 +++++++++++++++- >> > 1 file changed, 15 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c >> > index 1a2b3ca..165987a 100644 >> > --- a/drivers/pci/hotplug/acpiphp_glue.c >> > +++ b/drivers/pci/hotplug/acpiphp_glue.c >> > @@ -49,6 +49,7 @@ >> > #include <linux/mutex.h> >> > #include <linux/slab.h> >> > #include <linux/acpi.h> >> > +#include <linux/dmi.h> >> > >> > #include "../pci.h" >> > #include "acpiphp.h" >> > @@ -1327,6 +1328,19 @@ out: >> > static void handle_hotplug_event_bridge(acpi_handle handle, u32 type, >> > void *context) >> > { >> > + unsigned long delay = 0; >> > + const char *board_name; >> > + >> > + board_name = dmi_get_system_info(DMI_BOARD_NAME); >> > + /* >> > + * BIOS on Intel DZ77RE-75K motherboard notifies OS about Thunderbolt >> > + * hotplug before devices behind Thunderbolt are ready to be >> > + * enumerated. >> > + * Let's delay enumeration by 2 seconds. >> > + */ >> > + if (board_name && !strcmp(board_name, "DZ77RE-75K")) >> > + delay = 2 * HZ; >> Linus will not be happy with those kind of delay. is there any way for kernel to retry before device is declared not there. pcie hotplug spec: need to retry several times in 1000ms before delcaring the devices is not present. http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=2f5d8e4ff947ad6673397083b48719cd6c59cd61 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 13, 2012 at 12:25 PM, Yinghai Lu <yinghai@kernel.org> wrote: > > Linus will not be happy with those kind of delay. Indeed. And the DMI check is bogus too, since the "there can be delays" is apparently part of the pcie hotplug spec. So do the sane thing. Retry a few times, with increasingly long delays (ie something like start with 10ms, then double the delay until you hit 1s, and then just give up: end result, ~2s total wait, but 10ms for any sane device that doesn't suck). No DMI checks, no hacks, not insane default delays. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 13, 2012 at 12:30:04PM -0800, Linus Torvalds wrote: > On Thu, Dec 13, 2012 at 12:25 PM, Yinghai Lu <yinghai@kernel.org> wrote: > > > > Linus will not be happy with those kind of delay. > > Indeed. And the DMI check is bogus too, since the "there can be > delays" is apparently part of the pcie hotplug spec. It's ACPI PCI hotplug, not PCIe native hotplug. PCIe hotplug spec is not relevant. IIUC, in ACPI case devices should be ready to be enumerated, before you get notification. Rafael, is it correct? > So do the sane thing. Retry a few times, with increasingly long delays > (ie something like start with 10ms, then double the delay until you > hit 1s, and then just give up: end result, ~2s total wait, but 10ms > for any sane device that doesn't suck). PCI rescan is expensive and generate noise in dmesg. We'll end up with tons of useless messages.
On Thu, Dec 13, 2012 at 12:49 PM, Kirill A. Shutemov <kirill.shutemov@linux.intel.com> wrote: > > PCI rescan is expensive and generate noise in dmesg. We'll end up with > tons of useless messages. So? That's still better than adding random DMI string checks. And christ, this is at hotplug time only. Nobody cares. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, December 13, 2012 10:49:53 PM Kirill A. Shutemov wrote: > On Thu, Dec 13, 2012 at 12:30:04PM -0800, Linus Torvalds wrote: > > On Thu, Dec 13, 2012 at 12:25 PM, Yinghai Lu <yinghai@kernel.org> wrote: > > > > > > Linus will not be happy with those kind of delay. > > > > Indeed. And the DMI check is bogus too, since the "there can be > > delays" is apparently part of the pcie hotplug spec. > > It's ACPI PCI hotplug, not PCIe native hotplug. PCIe hotplug spec is not > relevant. > > IIUC, in ACPI case devices should be ready to be enumerated, before you > get notification. Rafael, is it correct? Not necessarily. The ACPI nodes will be, the device themselves are still PCI devices. :-) Thanks, Rafael
On Thu, Dec 13, 2012 at 12:30:04PM -0800, Linus Torvalds wrote: > On Thu, Dec 13, 2012 at 12:25 PM, Yinghai Lu <yinghai@kernel.org> wrote: > > > > Linus will not be happy with those kind of delay. > > Indeed. And the DMI check is bogus too, since the "there can be > delays" is apparently part of the pcie hotplug spec. > > So do the sane thing. Retry a few times, with increasingly long delays > (ie something like start with 10ms, then double the delay until you > hit 1s, and then just give up: end result, ~2s total wait, but 10ms > for any sane device that doesn't suck). > > No DMI checks, no hacks, not insane default delays. I've realized that there's no strong criteria of hotplug success in ACPI PCI Hotplug. We can't know when we should stop retrying. In Thunderbolt case before any devices hotplugged you only see a root port. Thunderbolt host controller is powered off and kernel can't see it. On hotplug BIOS enables the host controller, initialize it and notify OS about hotplug. Normally kernel will enumerate 6 ports on Thunderbolt host controller, 2 ports on device Thunderbolt controller and target device itself. All this for simple non-chained case. With device chaining the hierarchy is even more complex. On DZ77RE-75K motherboard without the workaround kernel will discover only ports on host controller, but not device ports or device. So kernel will find devices on broken implementation, not all of them. Even worse: there's no way to distinguish between plug and unplug events and kernel uses the same code path for both cases.
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c index 1a2b3ca..c4db634 100644 --- a/drivers/pci/hotplug/acpiphp_glue.c +++ b/drivers/pci/hotplug/acpiphp_glue.c @@ -49,6 +49,7 @@ #include <linux/mutex.h> #include <linux/slab.h> #include <linux/acpi.h> +#include <linux/dmi.h> #include "../pci.h" #include "acpiphp.h" @@ -1327,6 +1328,31 @@ out: static void handle_hotplug_event_bridge(acpi_handle handle, u32 type, void *context) { + unsigned long delay = 0; + const struct dmi_system_id *match; + static const struct dmi_system_id sysids[] = { + /* + * BIOS on Intel DZ77RE-75K motherboard notifies OS about + * Thunderbolt hotplug before devices behind Thunderbolt are + * ready to be enumerated. + * Let's delay enumeration by 2 seconds. + */ + { + .ident = "DZ77RE-75K", + .matches = { + DMI_MATCH(DMI_BOARD_VENDOR, + "Intel Corporation"), + DMI_MATCH(DMI_BOARD_NAME, + "DZ77RE-75K"), + }, + .driver_data = (void *) (2 * HZ), /* delay */ + }, + }; + + match = dmi_first_match(sysids); + if (match) + delay = (unsigned long) match->driver_data; + /* * Currently the code adds all hotplug events to the kacpid_wq * queue when it should add hotplug events to the kacpi_hotplug_wq. @@ -1336,7 +1362,7 @@ static void handle_hotplug_event_bridge(acpi_handle handle, u32 type, * don't deadlock on hotplug actions. */ alloc_acpiphp_hp_work(handle, type, context, - _handle_hotplug_event_bridge, 0); + _handle_hotplug_event_bridge, delay); } static void _handle_hotplug_event_func(struct work_struct *work)