ACPI device using sub-resource of PCI device
diff mbox

Message ID 20160816091502.GB1751@lahna.fi.intel.com
State Not Applicable
Headers show

Commit Message

Mika Westerberg Aug. 16, 2016, 9:15 a.m. UTC
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?

--
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

Comments

Andy Shevchenko Aug. 16, 2016, 11:23 a.m. UTC | #1
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;
> +}
> +
Rafael J. Wysocki Aug. 16, 2016, 11:27 a.m. UTC | #2
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
Mika Westerberg Aug. 16, 2016, 12:20 p.m. UTC | #3
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
Aaron Durbin Aug. 17, 2016, 11:02 p.m. UTC | #4
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
Aaron Durbin Sept. 9, 2016, 2:12 p.m. UTC | #5
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
Aaron Durbin Sept. 9, 2016, 2:16 p.m. UTC | #6
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
Mika Westerberg Sept. 12, 2016, 8:10 a.m. UTC | #7
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
Rafael J. Wysocki Sept. 12, 2016, 12:26 p.m. UTC | #8
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

Patch
diff mbox

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);
 	}