| Message ID | 51BA8524.9050507@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Thu, Jun 13, 2013 at 7:51 PM, Jiang Liu (Gerry) <jiang.liu@huawei.com> wrote: > On 2013/6/14 10:30, Yinghai Lu wrote: >> >> On Thu, Jun 13, 2013 at 7:09 PM, Jiang Liu (Gerry) <jiang.liu@huawei.com> >> wrote: > > Not sure. But a little concern about check_hotplug_bridge(), it treats > dock station and devices on dock station with _EJD as hot-plug-gable > PCI bus and reserve extra resources for possible hot-adding. But I > think we should only reserve extra resource for dock station, and should > not reserve resource for devices on station with _EJD method. That is should be... if there is children hotplug slots, we should add extra... > > >> >>> Currently I'm trying to change acpiphp to respect BIOS resource >>> assignment by calling pcibios_survey_resource_bus(), as in pci_root.c. >>> The other way is to change the IOMM resource allocation algorithm, >>> but obviously it's much more risky of regressions if changing the >>> algorithm. >> >> >> that is not going to help, need to increase bridge resource. >> >> please check if BIOS have setup option about hotplug MMIO pad size. > > For the first step, I'm trying to make hotplug case work in the same way as > boot time. Do you think this patch help? > > diff --git a/drivers/pci/hotplug/acpiphp_glue.c > b/drivers/pci/hotplug/acpiphp_gl > index 270fdba..12e3f6e 100644 > --- a/drivers/pci/hotplug/acpiphp_glue.c > +++ b/drivers/pci/hotplug/acpiphp_glue.c > @@ -837,13 +837,13 @@ static int __ref enable_device(struct acpiphp_slot > *slot) > max = pci_scan_bridge(bus, dev, max, pass); > if (pass && dev->subordinate) { > check_hotplug_bridge(slot, dev); > - pci_bus_size_bridges(dev->subordinate); > + pcibios_resource_survey_bus(dev->subordi > } > } > } > } > > - pci_bus_assign_resources(bus); > + pci_assign_unassigned_bus_resources(bus); > acpiphp_sanitize_bus(bus); > acpiphp_set_hpp_values(bus); > acpiphp_set_acpi_region(slot); > --- Yes, it should help. but may need to after pcibios_add_bus move down patch. otherwise, we could enable children slot and survery them and assign unassigned for them at first and then go to parent slots. that will make children slots survey fails, that is too early and should be done after parent slots. Yinghai -- 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, Jun 13, 2013 at 8:30 PM, Yinghai Lu <yinghai@kernel.org> wrote: > On Thu, Jun 13, 2013 at 7:51 PM, Jiang Liu (Gerry) <jiang.liu@huawei.com> wrote: >> For the first step, I'm trying to make hotplug case work in the same way as >> boot time. Do you think this patch help? >> >> diff --git a/drivers/pci/hotplug/acpiphp_glue.c >> b/drivers/pci/hotplug/acpiphp_gl >> index 270fdba..12e3f6e 100644 >> --- a/drivers/pci/hotplug/acpiphp_glue.c >> +++ b/drivers/pci/hotplug/acpiphp_glue.c >> @@ -837,13 +837,13 @@ static int __ref enable_device(struct acpiphp_slot >> *slot) >> max = pci_scan_bridge(bus, dev, max, pass); >> if (pass && dev->subordinate) { >> check_hotplug_bridge(slot, dev); >> - pci_bus_size_bridges(dev->subordinate); >> + pcibios_resource_survey_bus(dev->subordi >> } >> } >> } >> } >> >> - pci_bus_assign_resources(bus); >> + pci_assign_unassigned_bus_resources(bus); can not use use pci_assign_unassigned_bus_resources directly. as bus could have devices that is there already before this enable_device and could have driver loaded before. that is why we have checking if (PCI_SLOT(dev->devfn) != slot->device) -- 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, Jun 13, 2013 at 8:30 PM, Yinghai Lu <yinghai@kernel.org> wrote: >> diff --git a/drivers/pci/hotplug/acpiphp_glue.c >> b/drivers/pci/hotplug/acpiphp_gl >> index 270fdba..12e3f6e 100644 >> --- a/drivers/pci/hotplug/acpiphp_glue.c >> +++ b/drivers/pci/hotplug/acpiphp_glue.c >> @@ -837,13 +837,13 @@ static int __ref enable_device(struct acpiphp_slot >> *slot) >> max = pci_scan_bridge(bus, dev, max, pass); >> if (pass && dev->subordinate) { >> check_hotplug_bridge(slot, dev); >> - pci_bus_size_bridges(dev->subordinate); >> + pcibios_resource_survey_bus(dev->subordi >> } >> } >> } >> } >> >> - pci_bus_assign_resources(bus); >> + pci_assign_unassigned_bus_resources(bus); >> acpiphp_sanitize_bus(bus); >> acpiphp_set_hpp_values(bus); >> acpiphp_set_acpi_region(slot); >> --- > > Yes, it should help. but may need to after pcibios_add_bus move down patch. > > otherwise, we could enable children slot and survery them and assign > unassigned for them at first > and then go to parent slots. > that will make children slots survey fails, that is too early and > should be done after parent slots. oh, it seems we should get enable_slot called children slots before parent slots finished. so should not need "pcibios_add_bus move down" patch. Yinghai -- 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 2013/6/14 11:43, Yinghai Lu wrote: > On Thu, Jun 13, 2013 at 8:30 PM, Yinghai Lu <yinghai@kernel.org> wrote: >> On Thu, Jun 13, 2013 at 7:51 PM, Jiang Liu (Gerry) <jiang.liu@huawei.com> wrote: > >>> For the first step, I'm trying to make hotplug case work in the same way as >>> boot time. Do you think this patch help? >>> >>> diff --git a/drivers/pci/hotplug/acpiphp_glue.c >>> b/drivers/pci/hotplug/acpiphp_gl >>> index 270fdba..12e3f6e 100644 >>> --- a/drivers/pci/hotplug/acpiphp_glue.c >>> +++ b/drivers/pci/hotplug/acpiphp_glue.c >>> @@ -837,13 +837,13 @@ static int __ref enable_device(struct acpiphp_slot >>> *slot) >>> max = pci_scan_bridge(bus, dev, max, pass); >>> if (pass && dev->subordinate) { >>> check_hotplug_bridge(slot, dev); >>> - pci_bus_size_bridges(dev->subordinate); >>> + pcibios_resource_survey_bus(dev->subordi >>> } >>> } >>> } >>> } >>> >>> - pci_bus_assign_resources(bus); >>> + pci_assign_unassigned_bus_resources(bus); > > can not use use pci_assign_unassigned_bus_resources directly. as bus > could have devices that is there already > before this enable_device and could have driver loaded before. > > that is why we have checking > if (PCI_SLOT(dev->devfn) != slot->device) > Thanks for reminder. This a quick solution for prove of concept and it should be OK for this Sony laptop case. On the other hand, we may need to enhance pci_bus_size_bridges(dev->subordinate) and pci_bus_assign_resources(bus) to support realloc_list. > . > -- 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
2013/6/14 Jiang Liu (Gerry) <jiang.liu@huawei.com>: > On 2013/6/14 10:30, Yinghai Lu wrote: >> >> On Thu, Jun 13, 2013 at 7:09 PM, Jiang Liu (Gerry) <jiang.liu@huawei.com> >> wrote: >>> >>> On 2013/6/14 2:42, Yinghai Lu wrote: >>>> >>>> >>>> On Thu, Jun 13, 2013 at 9:32 AM, Jiang Liu <jiang.liu@huawei.com> wrote: >>>>> >>>>> >>>>> Alexander E. Patrakov <patrakov@gmail.com> reports two bugs related to >>>>> dock station support on Sony VAIO VPCZ23A4R. Actually there are at >>>>> least >>>>> four bugs related to Sony VAIO VPCZ23A4R dock support. >>>>> 1) can't correctly detect hotplug slot for dock state >>>>> 2) resource leak on undocking >>>>> 3) resource allocation failure for dock devices >>>>> 4) one bug in intel_snd_hda driver >>>>> >>>>> The first patch fixes issue 1, and the second patch fixes issue 2. >>>>> These two patches, if accepted, should be material for stable branches >>>>> too. >>>>> Patch 3-9 are code improvement for ACPI and dock driver. >>>>> >>>>> I have found the root cause for issue three, but still working on >>>>> solutions, and seems can't be solve in short time. So please help >>>>> to review and test patches for issue 1) and 2) first. >>>> >>>> >>>> >>>> the 3) is about pci resource allocation? >>>> because pcibios_add_bus is called too early? >>>> >>>> If that is case, we should have something like attached patch for it. >>>> >>>> With that, we will not need to worry about _OSC set for 3.10 etc. >>>> >>> Hi Yinghai, >>> Seems not related to pcibios_add_bus(). According to my >>> investigation, the issue is caused by difference in PCI resource >>> assignment between boot time and runtime hotplug. On x86 platforms, >>> it respects PCI resource assignment from BIOS and only reassign >>> resources for unassigned BARs. But with acpiphp, it ignores BIOS >>> resource assignment and reassign all resources by OS. >>> If we have enough resources, reassigning all PCI resources should >>> work too, but may fail if we are under resource constraints. On the >>> other handle, current PCI IOMM align algorithm may waste huge MMIO >>> address space if we have some PCI devices with huge IOMM BAR. >>> On this Sony laptop, BIOS allocates limited IOMM resources for >>> the dock station and the dock station has a gfx which has a 256MB >>> IOMM BAR. So current acpiphp driver fails to allocate resources >>> for most devices on the dock station. >> >> >> Is it a regression? > > Not sure. But a little concern about check_hotplug_bridge(), it treats > dock station and devices on dock station with _EJD as hot-plug-gable > PCI bus and reserve extra resources for possible hot-adding. But I > think we should only reserve extra resource for dock station, and should > not reserve resource for devices on station with _EJD method. > > >> >>> Currently I'm trying to change acpiphp to respect BIOS resource >>> assignment by calling pcibios_survey_resource_bus(), as in pci_root.c. >>> The other way is to change the IOMM resource allocation algorithm, >>> but obviously it's much more risky of regressions if changing the >>> algorithm. >> >> >> that is not going to help, need to increase bridge resource. >> >> please check if BIOS have setup option about hotplug MMIO pad size. > > For the first step, I'm trying to make hotplug case work in the same way as > boot time. Do you think this patch help? > > diff --git a/drivers/pci/hotplug/acpiphp_glue.c > b/drivers/pci/hotplug/acpiphp_gl > index 270fdba..12e3f6e 100644 > --- a/drivers/pci/hotplug/acpiphp_glue.c > +++ b/drivers/pci/hotplug/acpiphp_glue.c > @@ -837,13 +837,13 @@ static int __ref enable_device(struct acpiphp_slot > *slot) > max = pci_scan_bridge(bus, dev, max, pass); > if (pass && dev->subordinate) { > check_hotplug_bridge(slot, dev); > - pci_bus_size_bridges(dev->subordinate); > + pcibios_resource_survey_bus(dev->subordi > } > } > } > } > > - pci_bus_assign_resources(bus); > + pci_assign_unassigned_bus_resources(bus); > acpiphp_sanitize_bus(bus); > acpiphp_set_hpp_values(bus); > acpiphp_set_acpi_region(slot); > --- The patch helped, thanks. Note: I have tested it together with pci_move_pcibios_add_bus_down.patch, I don't know yet if pci_move_pcibios_add_bus_down.patch is needed. -- Alexander E. Patrakov -- 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 2013/6/14 12:07, Alexander E. Patrakov wrote: > 2013/6/14 Jiang Liu (Gerry) <jiang.liu@huawei.com>: >> On 2013/6/14 10:30, Yinghai Lu wrote: >>> >>> On Thu, Jun 13, 2013 at 7:09 PM, Jiang Liu (Gerry) <jiang.liu@huawei.com> >>> wrote: >>>> >>>> On 2013/6/14 2:42, Yinghai Lu wrote: >>>>> >>>>> >>>>> On Thu, Jun 13, 2013 at 9:32 AM, Jiang Liu <jiang.liu@huawei.com> wrote: >>>>>> >>>>>> >>>>>> Alexander E. Patrakov <patrakov@gmail.com> reports two bugs related to >>>>>> dock station support on Sony VAIO VPCZ23A4R. Actually there are at >>>>>> least >>>>>> four bugs related to Sony VAIO VPCZ23A4R dock support. >>>>>> 1) can't correctly detect hotplug slot for dock state >>>>>> 2) resource leak on undocking >>>>>> 3) resource allocation failure for dock devices >>>>>> 4) one bug in intel_snd_hda driver >>>>>> >>>>>> The first patch fixes issue 1, and the second patch fixes issue 2. >>>>>> These two patches, if accepted, should be material for stable branches >>>>>> too. >>>>>> Patch 3-9 are code improvement for ACPI and dock driver. >>>>>> >>>>>> I have found the root cause for issue three, but still working on >>>>>> solutions, and seems can't be solve in short time. So please help >>>>>> to review and test patches for issue 1) and 2) first. >>>>> >>>>> >>>>> >>>>> the 3) is about pci resource allocation? >>>>> because pcibios_add_bus is called too early? >>>>> >>>>> If that is case, we should have something like attached patch for it. >>>>> >>>>> With that, we will not need to worry about _OSC set for 3.10 etc. >>>>> >>>> Hi Yinghai, >>>> Seems not related to pcibios_add_bus(). According to my >>>> investigation, the issue is caused by difference in PCI resource >>>> assignment between boot time and runtime hotplug. On x86 platforms, >>>> it respects PCI resource assignment from BIOS and only reassign >>>> resources for unassigned BARs. But with acpiphp, it ignores BIOS >>>> resource assignment and reassign all resources by OS. >>>> If we have enough resources, reassigning all PCI resources should >>>> work too, but may fail if we are under resource constraints. On the >>>> other handle, current PCI IOMM align algorithm may waste huge MMIO >>>> address space if we have some PCI devices with huge IOMM BAR. >>>> On this Sony laptop, BIOS allocates limited IOMM resources for >>>> the dock station and the dock station has a gfx which has a 256MB >>>> IOMM BAR. So current acpiphp driver fails to allocate resources >>>> for most devices on the dock station. >>> >>> >>> Is it a regression? >> >> Not sure. But a little concern about check_hotplug_bridge(), it treats >> dock station and devices on dock station with _EJD as hot-plug-gable >> PCI bus and reserve extra resources for possible hot-adding. But I >> think we should only reserve extra resource for dock station, and should >> not reserve resource for devices on station with _EJD method. >> >> >>> >>>> Currently I'm trying to change acpiphp to respect BIOS resource >>>> assignment by calling pcibios_survey_resource_bus(), as in pci_root.c. >>>> The other way is to change the IOMM resource allocation algorithm, >>>> but obviously it's much more risky of regressions if changing the >>>> algorithm. >>> >>> >>> that is not going to help, need to increase bridge resource. >>> >>> please check if BIOS have setup option about hotplug MMIO pad size. >> >> For the first step, I'm trying to make hotplug case work in the same way as >> boot time. Do you think this patch help? >> >> diff --git a/drivers/pci/hotplug/acpiphp_glue.c >> b/drivers/pci/hotplug/acpiphp_gl >> index 270fdba..12e3f6e 100644 >> --- a/drivers/pci/hotplug/acpiphp_glue.c >> +++ b/drivers/pci/hotplug/acpiphp_glue.c >> @@ -837,13 +837,13 @@ static int __ref enable_device(struct acpiphp_slot >> *slot) >> max = pci_scan_bridge(bus, dev, max, pass); >> if (pass && dev->subordinate) { >> check_hotplug_bridge(slot, dev); >> - pci_bus_size_bridges(dev->subordinate); >> + pcibios_resource_survey_bus(dev->subordi >> } >> } >> } >> } >> >> - pci_bus_assign_resources(bus); >> + pci_assign_unassigned_bus_resources(bus); >> acpiphp_sanitize_bus(bus); >> acpiphp_set_hpp_values(bus); >> acpiphp_set_acpi_region(slot); >> --- > > > The patch helped, thanks. Note: I have tested it together with > pci_move_pcibios_add_bus_down.patch, I don't know yet if > pci_move_pcibios_add_bus_down.patch is needed. What's the situation now? Could you please send out dmesgs\ioports\iomem? Thanks! > > -- > Alexander E. Patrakov > > . > -- 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
2013/6/14 Jiang Liu (Gerry) <jiang.liu@huawei.com>: > What's the situation now? Could you please send out dmesgs\ioports\iomem? > Thanks! The requested information has been added to https://bugzilla.kernel.org/show_bug.cgi?id=56531 The changes in pci_move_pcibios_add_bus_down.patch are not needed. The radeon card in the dock station works after docking and restarting the X server, and its HD audio device works, too. Just for completeness, I have tested the CD-ROM drive, the USB adapters and the Ethernet card - they all work. IOW, everything works after docking. I did not test undocking, as that would surely hit https://bugzilla.kernel.org/show_bug.cgi?id=59681 and the hda_intel bug you mentioned in the first mail in this thread. Note that, for the initial series of patches, I still don't have any response to the lockdep warnings in https://lkml.org/lkml/2013/6/13/398
On 2013/6/14 12:43, Alexander E. Patrakov wrote: > 2013/6/14 Jiang Liu (Gerry) <jiang.liu@huawei.com>: > >> What's the situation now? Could you please send out dmesgs\ioports\iomem? >> Thanks! > > The requested information has been added to > https://bugzilla.kernel.org/show_bug.cgi?id=56531 > > The changes in pci_move_pcibios_add_bus_down.patch are not needed. > > The radeon card in the dock station works after docking and restarting > the X server, and its HD audio device works, too. Just for > completeness, I have tested the CD-ROM drive, the USB adapters and the > Ethernet card - they all work. IOW, everything works after docking. > > I did not test undocking, as that would surely hit > https://bugzilla.kernel.org/show_bug.cgi?id=59681 and the hda_intel > bug you mentioned in the first mail in this thread. > > Note that, for the initial series of patches, I still don't have any > response to the lockdep warnings in > https://lkml.org/lkml/2013/6/13/398 Sounds great, I will rework the lockdep related patch tonight. Thanks for testing. > -- 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
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_gl index 270fdba..12e3f6e 100644 --- a/drivers/pci/hotplug/acpiphp_glue.c +++ b/drivers/pci/hotplug/acpiphp_glue.c @@ -837,13 +837,13 @@ static int __ref enable_device(struct acpiphp_slot *slot) max = pci_scan_bridge(bus, dev, max, pass); if (pass && dev->subordinate) { check_hotplug_bridge(slot, dev); - pci_bus_size_bridges(dev->subordinate); + pcibios_resource_survey_bus(dev->subordi } } } } - pci_bus_assign_resources(bus); + pci_assign_unassigned_bus_resources(bus);