| Message ID | 20160816091502.GB1751@lahna.fi.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Tue, 2016-08-16 at 12:15 +0300, Mika Westerberg wrote: > On Fri, Aug 12, 2016 at 11:45:30AM -0500, Aaron Durbin wrote: > > > > Was anyone able to take a look into a solution for the current > > problem? Again, please feel free to ask if anyone would like help > > testing potential solutions. > > Below is one proposal for fixing the issue. It is just a prototype and > I'm not sure if it takes everything needed into account. Would you be > able to try it out and let us know if it works for you? Just style comments below. > --- a/drivers/acpi/acpi_platform.c > +++ b/drivers/acpi/acpi_platform.c > @@ -18,6 +18,7 @@ > #include <linux/module.h> > #include <linux/dma-mapping.h> > #include <linux/platform_device.h> > +#include <linux/pci.h> I would keep trying to arrange them in alphabetical order, i.e. put before platform_device.h. > > #include "internal.h" > > @@ -30,6 +31,35 @@ static const struct acpi_device_id > forbidden_id_list[] = { > {"", 0}, > }; > > +static struct resource *acpi_find_parent_resource(struct acpi_device > *adev, > + struct resource > *res) > +{ > + struct device *parent; > + > + parent = acpi_get_first_physical_node(adev->parent); > + if (!parent) > + return NULL; > + > +#if IS_ENABLED(CONFIG_PCI) Oh. > + if (dev_is_pci(parent)) { Has a stub. > + struct pci_dev *pdev = to_pci_dev(parent); Not needed an ugly define. > + > + if (!pci_is_bridge(pdev)) { Ditto. > + int i; > + > + for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) { Ditto. > + struct resource *r = &pdev- > >resource[i]; > + > + if (r->start && resource_contains(r, > res)) > + return r; > + } > + } > + } > +#endif > + > + return NULL; > +} > +
On Tue, Aug 16, 2016 at 1:23 PM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Tue, 2016-08-16 at 12:15 +0300, Mika Westerberg wrote: >> On Fri, Aug 12, 2016 at 11:45:30AM -0500, Aaron Durbin wrote: >> > >> > Was anyone able to take a look into a solution for the current >> > problem? Again, please feel free to ask if anyone would like help >> > testing potential solutions. >> >> Below is one proposal for fixing the issue. It is just a prototype and >> I'm not sure if it takes everything needed into account. Would you be >> able to try it out and let us know if it works for you? > > Just style comments below. Didn't Mika say it was a prototype? Meaning that it would be cleaned up (among other things)? Why do you have to confuse things by posting style comments on patches clearly described as prototypes? Is there not enough noise on the mailing lists already? Thanks, Rafael -- 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 Tue, Aug 16, 2016 at 02:23:24PM +0300, Andy Shevchenko wrote: > On Tue, 2016-08-16 at 12:15 +0300, Mika Westerberg wrote: > > On Fri, Aug 12, 2016 at 11:45:30AM -0500, Aaron Durbin wrote: > > > > > > Was anyone able to take a look into a solution for the current > > > problem? Again, please feel free to ask if anyone would like help > > > testing potential solutions. > > > > Below is one proposal for fixing the issue. It is just a prototype and > > I'm not sure if it takes everything needed into account. Would you be > > able to try it out and let us know if it works for you? > > Just style comments below. Thanks for the comments. As Rafael already mentioned, this is just a prototype. We don't even know yet if it fixes the issue. If it turns out to be working and we decide to go with this approach, I will clean up the code accordingly and submit it as a proper patch. -- 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 Tue, Aug 16, 2016 at 2:15 AM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Fri, Aug 12, 2016 at 11:45:30AM -0500, Aaron Durbin wrote: >> Was anyone able to take a look into a solution for the current >> problem? Again, please feel free to ask if anyone would like help >> testing potential solutions. > > Below is one proposal for fixing the issue. It is just a prototype and > I'm not sure if it takes everything needed into account. Would you be > able to try it out and let us know if it works for you? Yes. I'll give it a go. I'm traveling this week so it won't likely be til early next week. Thanks for the proposed fix. I'll report back on my findings. > > diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c > index 159f7f19abce..72068415f806 100644 > --- a/drivers/acpi/acpi_platform.c > +++ b/drivers/acpi/acpi_platform.c > @@ -18,6 +18,7 @@ > #include <linux/module.h> > #include <linux/dma-mapping.h> > #include <linux/platform_device.h> > +#include <linux/pci.h> > > #include "internal.h" > > @@ -30,6 +31,35 @@ static const struct acpi_device_id forbidden_id_list[] = { > {"", 0}, > }; > > +static struct resource *acpi_find_parent_resource(struct acpi_device *adev, > + struct resource *res) > +{ > + struct device *parent; > + > + parent = acpi_get_first_physical_node(adev->parent); > + if (!parent) > + return NULL; > + > +#if IS_ENABLED(CONFIG_PCI) > + if (dev_is_pci(parent)) { > + struct pci_dev *pdev = to_pci_dev(parent); > + > + if (!pci_is_bridge(pdev)) { > + int i; > + > + for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) { > + struct resource *r = &pdev->resource[i]; > + > + if (r->start && resource_contains(r, res)) > + return r; > + } > + } > + } > +#endif > + > + return NULL; > +} > + > /** > * acpi_create_platform_device - Create platform device for ACPI device node > * @adev: ACPI device node to create a platform device for. > @@ -69,8 +99,17 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev) > return ERR_PTR(-ENOMEM); > } > count = 0; > - list_for_each_entry(rentry, &resource_list, node) > - resources[count++] = *rentry->res; > + list_for_each_entry(rentry, &resource_list, node) { > + struct resource *res = &resources[count++]; > + > + *res = *rentry->res; > + /* > + * If the device has parent we need to take its > + * resources into account as well because this > + * device might consume part of those. > + */ > + res->parent = acpi_find_parent_resource(adev, res); > + } > > acpi_dev_free_resource_list(&resource_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
On Wed, Aug 17, 2016 at 6:02 PM, Aaron Durbin <adurbin@google.com> wrote: > On Tue, Aug 16, 2016 at 2:15 AM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: >> On Fri, Aug 12, 2016 at 11:45:30AM -0500, Aaron Durbin wrote: >>> Was anyone able to take a look into a solution for the current >>> problem? Again, please feel free to ask if anyone would like help >>> testing potential solutions. >> >> Below is one proposal for fixing the issue. It is just a prototype and >> I'm not sure if it takes everything needed into account. Would you be >> able to try it out and let us know if it works for you? > > Yes. I'll give it a go. I'm traveling this week so it won't likely be > til early next week. Thanks for the proposed fix. I'll report back on > my findings. > Sorry for the late response. I tried the patch and it works with ACPI devices in a hierarchy below PCI devices. I didn't have any ACPI drivers probed but I was able to hang 2 ACPI devices off of a BAR that had a driver for the PCI device without any resource conflicts (snippet from /proc/iomem): c2a43000-c2a43fff : 0000:00:1b.0 c2a43000-c2a437ff : GOOG1234:00 c2a43800-c2a43fff : GOOG1235:00 One observation that I don't think matters much, but I wanted to write it down in case anyone pulls this thread up again. In the case of finding the parent resource, the ACPI devcie's resources won't show up in the resource tree until the pci parent devices' resources are inserted. >> >> diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c >> index 159f7f19abce..72068415f806 100644 >> --- a/drivers/acpi/acpi_platform.c >> +++ b/drivers/acpi/acpi_platform.c >> @@ -18,6 +18,7 @@ >> #include <linux/module.h> >> #include <linux/dma-mapping.h> >> #include <linux/platform_device.h> >> +#include <linux/pci.h> >> >> #include "internal.h" >> >> @@ -30,6 +31,35 @@ static const struct acpi_device_id forbidden_id_list[] = { >> {"", 0}, >> }; >> >> +static struct resource *acpi_find_parent_resource(struct acpi_device *adev, >> + struct resource *res) >> +{ >> + struct device *parent; >> + >> + parent = acpi_get_first_physical_node(adev->parent); >> + if (!parent) >> + return NULL; >> + >> +#if IS_ENABLED(CONFIG_PCI) >> + if (dev_is_pci(parent)) { >> + struct pci_dev *pdev = to_pci_dev(parent); >> + >> + if (!pci_is_bridge(pdev)) { >> + int i; >> + >> + for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) { >> + struct resource *r = &pdev->resource[i]; >> + >> + if (r->start && resource_contains(r, res)) >> + return r; >> + } >> + } >> + } >> +#endif >> + >> + return NULL; >> +} >> + >> /** >> * acpi_create_platform_device - Create platform device for ACPI device node >> * @adev: ACPI device node to create a platform device for. >> @@ -69,8 +99,17 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev) >> return ERR_PTR(-ENOMEM); >> } >> count = 0; >> - list_for_each_entry(rentry, &resource_list, node) >> - resources[count++] = *rentry->res; >> + list_for_each_entry(rentry, &resource_list, node) { >> + struct resource *res = &resources[count++]; >> + >> + *res = *rentry->res; >> + /* >> + * If the device has parent we need to take its >> + * resources into account as well because this >> + * device might consume part of those. >> + */ >> + res->parent = acpi_find_parent_resource(adev, res); >> + } >> >> acpi_dev_free_resource_list(&resource_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
On Fri, Sep 9, 2016 at 9:12 AM, Aaron Durbin <adurbin@google.com> wrote: > On Wed, Aug 17, 2016 at 6:02 PM, Aaron Durbin <adurbin@google.com> wrote: >> On Tue, Aug 16, 2016 at 2:15 AM, Mika Westerberg >> <mika.westerberg@linux.intel.com> wrote: >>> On Fri, Aug 12, 2016 at 11:45:30AM -0500, Aaron Durbin wrote: >>>> Was anyone able to take a look into a solution for the current >>>> problem? Again, please feel free to ask if anyone would like help >>>> testing potential solutions. >>> >>> Below is one proposal for fixing the issue. It is just a prototype and >>> I'm not sure if it takes everything needed into account. Would you be >>> able to try it out and let us know if it works for you? >> >> Yes. I'll give it a go. I'm traveling this week so it won't likely be >> til early next week. Thanks for the proposed fix. I'll report back on >> my findings. >> > > Sorry for the late response. I tried the patch and it works with ACPI > devices in a hierarchy below PCI devices. I didn't have any ACPI > drivers probed but I was able to hang 2 ACPI devices off of a BAR that > had a driver for the PCI device without any resource conflicts > (snippet from /proc/iomem): > > c2a43000-c2a43fff : 0000:00:1b.0 > c2a43000-c2a437ff : GOOG1234:00 > c2a43800-c2a43fff : GOOG1235:00 > > One observation that I don't think matters much, but I wanted to write > it down in case anyone pulls this thread up again. In the case of > finding the parent resource, the ACPI devcie's resources won't show up > in the resource tree until the pci parent devices' resources are > inserted. > I forgot to say thanks for helping in sorting this issue out. It's very much appreciated. -Aaron > >>> >>> diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c >>> index 159f7f19abce..72068415f806 100644 >>> --- a/drivers/acpi/acpi_platform.c >>> +++ b/drivers/acpi/acpi_platform.c >>> @@ -18,6 +18,7 @@ >>> #include <linux/module.h> >>> #include <linux/dma-mapping.h> >>> #include <linux/platform_device.h> >>> +#include <linux/pci.h> >>> >>> #include "internal.h" >>> >>> @@ -30,6 +31,35 @@ static const struct acpi_device_id forbidden_id_list[] = { >>> {"", 0}, >>> }; >>> >>> +static struct resource *acpi_find_parent_resource(struct acpi_device *adev, >>> + struct resource *res) >>> +{ >>> + struct device *parent; >>> + >>> + parent = acpi_get_first_physical_node(adev->parent); >>> + if (!parent) >>> + return NULL; >>> + >>> +#if IS_ENABLED(CONFIG_PCI) >>> + if (dev_is_pci(parent)) { >>> + struct pci_dev *pdev = to_pci_dev(parent); >>> + >>> + if (!pci_is_bridge(pdev)) { >>> + int i; >>> + >>> + for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) { >>> + struct resource *r = &pdev->resource[i]; >>> + >>> + if (r->start && resource_contains(r, res)) >>> + return r; >>> + } >>> + } >>> + } >>> +#endif >>> + >>> + return NULL; >>> +} >>> + >>> /** >>> * acpi_create_platform_device - Create platform device for ACPI device node >>> * @adev: ACPI device node to create a platform device for. >>> @@ -69,8 +99,17 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev) >>> return ERR_PTR(-ENOMEM); >>> } >>> count = 0; >>> - list_for_each_entry(rentry, &resource_list, node) >>> - resources[count++] = *rentry->res; >>> + list_for_each_entry(rentry, &resource_list, node) { >>> + struct resource *res = &resources[count++]; >>> + >>> + *res = *rentry->res; >>> + /* >>> + * If the device has parent we need to take its >>> + * resources into account as well because this >>> + * device might consume part of those. >>> + */ >>> + res->parent = acpi_find_parent_resource(adev, res); >>> + } >>> >>> acpi_dev_free_resource_list(&resource_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
On Fri, Sep 09, 2016 at 09:12:49AM -0500, Aaron Durbin wrote: > On Wed, Aug 17, 2016 at 6:02 PM, Aaron Durbin <adurbin@google.com> wrote: > > On Tue, Aug 16, 2016 at 2:15 AM, Mika Westerberg > > <mika.westerberg@linux.intel.com> wrote: > >> On Fri, Aug 12, 2016 at 11:45:30AM -0500, Aaron Durbin wrote: > >>> Was anyone able to take a look into a solution for the current > >>> problem? Again, please feel free to ask if anyone would like help > >>> testing potential solutions. > >> > >> Below is one proposal for fixing the issue. It is just a prototype and > >> I'm not sure if it takes everything needed into account. Would you be > >> able to try it out and let us know if it works for you? > > > > Yes. I'll give it a go. I'm traveling this week so it won't likely be > > til early next week. Thanks for the proposed fix. I'll report back on > > my findings. > > > > Sorry for the late response. I tried the patch and it works with ACPI > devices in a hierarchy below PCI devices. I didn't have any ACPI > drivers probed but I was able to hang 2 ACPI devices off of a BAR that > had a driver for the PCI device without any resource conflicts > (snippet from /proc/iomem): > > c2a43000-c2a43fff : 0000:00:1b.0 > c2a43000-c2a437ff : GOOG1234:00 > c2a43800-c2a43fff : GOOG1235:00 Thanks for testing. > One observation that I don't think matters much, but I wanted to write > it down in case anyone pulls this thread up again. In the case of > finding the parent resource, the ACPI devcie's resources won't show up > in the resource tree until the pci parent devices' resources are > inserted. Right, that's expected as the parent PCI device resources will be allocated a bit later when pcibios_resource_survey_bus() is called. Rafael, Bjorn, What do you think? Should I clean up the code and send it out as a formal patch? -- 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 Mon, Sep 12, 2016 at 10:10 AM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Fri, Sep 09, 2016 at 09:12:49AM -0500, Aaron Durbin wrote: >> On Wed, Aug 17, 2016 at 6:02 PM, Aaron Durbin <adurbin@google.com> wrote: >> > On Tue, Aug 16, 2016 at 2:15 AM, Mika Westerberg >> > <mika.westerberg@linux.intel.com> wrote: >> >> On Fri, Aug 12, 2016 at 11:45:30AM -0500, Aaron Durbin wrote: >> >>> Was anyone able to take a look into a solution for the current >> >>> problem? Again, please feel free to ask if anyone would like help >> >>> testing potential solutions. >> >> >> >> Below is one proposal for fixing the issue. It is just a prototype and >> >> I'm not sure if it takes everything needed into account. Would you be >> >> able to try it out and let us know if it works for you? >> > >> > Yes. I'll give it a go. I'm traveling this week so it won't likely be >> > til early next week. Thanks for the proposed fix. I'll report back on >> > my findings. >> > >> >> Sorry for the late response. I tried the patch and it works with ACPI >> devices in a hierarchy below PCI devices. I didn't have any ACPI >> drivers probed but I was able to hang 2 ACPI devices off of a BAR that >> had a driver for the PCI device without any resource conflicts >> (snippet from /proc/iomem): >> >> c2a43000-c2a43fff : 0000:00:1b.0 >> c2a43000-c2a437ff : GOOG1234:00 >> c2a43800-c2a43fff : GOOG1235:00 > > Thanks for testing. > >> One observation that I don't think matters much, but I wanted to write >> it down in case anyone pulls this thread up again. In the case of >> finding the parent resource, the ACPI devcie's resources won't show up >> in the resource tree until the pci parent devices' resources are >> inserted. > > Right, that's expected as the parent PCI device resources will be > allocated a bit later when pcibios_resource_survey_bus() is called. > > Rafael, Bjorn, > > What do you think? Should I clean up the code and send it out as a > formal patch? Yes, please, unless Bjorn disagrees. :-) Thanks, Rafael -- 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/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c index 159f7f19abce..72068415f806 100644 --- a/drivers/acpi/acpi_platform.c +++ b/drivers/acpi/acpi_platform.c @@ -18,6 +18,7 @@ #include <linux/module.h> #include <linux/dma-mapping.h> #include <linux/platform_device.h> +#include <linux/pci.h> #include "internal.h" @@ -30,6 +31,35 @@ static const struct acpi_device_id forbidden_id_list[] = { {"", 0}, }; +static struct resource *acpi_find_parent_resource(struct acpi_device *adev, + struct resource *res) +{ + struct device *parent; + + parent = acpi_get_first_physical_node(adev->parent); + if (!parent) + return NULL; + +#if IS_ENABLED(CONFIG_PCI) + if (dev_is_pci(parent)) { + struct pci_dev *pdev = to_pci_dev(parent); + + if (!pci_is_bridge(pdev)) { + int i; + + for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) { + struct resource *r = &pdev->resource[i]; + + if (r->start && resource_contains(r, res)) + return r; + } + } + } +#endif + + return NULL; +} + /** * acpi_create_platform_device - Create platform device for ACPI device node * @adev: ACPI device node to create a platform device for. @@ -69,8 +99,17 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev) return ERR_PTR(-ENOMEM); } count = 0; - list_for_each_entry(rentry, &resource_list, node) - resources[count++] = *rentry->res; + list_for_each_entry(rentry, &resource_list, node) { + struct resource *res = &resources[count++]; + + *res = *rentry->res; + /* + * If the device has parent we need to take its + * resources into account as well because this + * device might consume part of those. + */ + res->parent = acpi_find_parent_resource(adev, res); + } acpi_dev_free_resource_list(&resource_list); }