Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1208939 > unrolled thread
| Started by | Bjorn Helgaas <bhelgaas@google.com> |
|---|---|
| First post | 2015-08-18 01:10 +0200 |
| Last post | 2015-08-18 21:10 +0200 |
| Articles | 2 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v3 03/51] PCI: Use correct align for optional only resources during sorting Bjorn Helgaas <bhelgaas@google.com> - 2015-08-18 01:10 +0200
Re: [PATCH v3 03/51] PCI: Use correct align for optional only resources during sorting Yinghai Lu <yinghai@kernel.org> - 2015-08-18 21:10 +0200
| From | Bjorn Helgaas <bhelgaas@google.com> |
|---|---|
| Date | 2015-08-18 01:10 +0200 |
| Subject | Re: [PATCH v3 03/51] PCI: Use correct align for optional only resources during sorting |
| Message-ID | <pYBHs-4Ns-13@gated-at.bofh.it> |
On Mon, Jul 27, 2015 at 04:29:21PM -0700, Yinghai Lu wrote:
> During sorting before assign, we only put resource with non-zero align
> in the sorted list, so for optional resources that must size is 0 and only
> have addon parts, we need to have correct align.
>
> While treating SRIOV as optional resources, we always read alignment for
> SRIOV bars, so they are ok.
> Hotplug bridge resources are using STARTALIGN so it is ok when size is 0
> if we have correct start for them.
>
> Later we want to treat the ROM BAR as optional resource, and it has
> have SIZEALIGN, we need to find a way to get align for them.
Are you saying we have a ROM resource where the ROM BAR is of non-zero
size, but the resource structure says it's zero size?
> We can use addon resource align instead in that case, and it will
> be ok for SRIOV path and hotplug bridge resource path.
>
> Sorted list will contain must resource align/size to 0/0 to hold spot for
> optional resources.
>
> We need to pass realloc_head from sizing stage to sorting stage, and
> get entry from realloc list and calculate align from the entry.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=81431
Apparently this fixes a problem? What is the problem? What is the
symptom of the problem?
> Reported-by: TJ <linux@iam.tj>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
> drivers/pci/setup-bus.c | 50 ++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 43 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 247d8fe..27cb0f0 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -145,9 +145,43 @@ static resource_size_t get_res_add_align(struct list_head *head,
> return dev_res->min_align;
> }
>
> +static resource_size_t __pci_resource_alignment(
I don't like creating new function names by adding "__" to the
beginning of an existing function name. It doesn't give enough of a
clue about what how the functions are different. I don't really
understand what's going on here, but this might be a clue that this
could be refactored differently.
> + struct pci_dev *dev,
> + struct resource *r,
> + struct list_head *realloc_head)
> +{
> + resource_size_t r_align = pci_resource_alignment(dev, r);
> + resource_size_t orig_start, orig_end;
> + struct pci_dev_resource *dev_res;
> +
> + if (r_align || !realloc_head)
> + return r_align;
> +
> + dev_res = res_to_dev_res(realloc_head, r);
> + if (!dev_res || !dev_res->add_size)
> + return r_align;
> +
> + orig_start = r->start;
> + orig_end = r->end;
> + r->end += dev_res->add_size;
> + if ((r->flags & IORESOURCE_STARTALIGN)) {
> + resource_size_t r_size = resource_size(r);
> + resource_size_t add_align = dev_res->min_align;
> +
> + r->start = add_align;
> + r->end = add_align + r_size - 1;
I think this would be slightly shorter and more obvious as:
resource_size_t r_size = resource_size(r);
r->start = dev_res->min_align;
r->end = r->start + r_size - 1;
> + }
> + r_align = pci_resource_alignment(dev, r);
> + r->start = orig_start;
> + r->end = orig_end;
> +
> + return r_align;
> +}
>
> /* Sort resources by alignment */
> -static void pdev_sort_resources(struct pci_dev *dev, struct list_head *head)
> +static void pdev_sort_resources(struct pci_dev *dev,
> + struct list_head *realloc_head,
> + struct list_head *head)
> {
> int i;
>
> @@ -165,7 +199,7 @@ static void pdev_sort_resources(struct pci_dev *dev, struct list_head *head)
> if (!(r->flags) || r->parent)
> continue;
>
> - r_align = pci_resource_alignment(dev, r);
> + r_align = __pci_resource_alignment(dev, r, realloc_head);
> if (!r_align) {
> dev_warn(&dev->dev, "BAR %d: %pR has bogus alignment\n",
> i, r);
> @@ -183,8 +217,9 @@ static void pdev_sort_resources(struct pci_dev *dev, struct list_head *head)
> list_for_each_entry(dev_res, head, list) {
> resource_size_t align;
>
> - align = pci_resource_alignment(dev_res->dev,
> - dev_res->res);
> + align = __pci_resource_alignment(dev_res->dev,
> + dev_res->res,
> + realloc_head);
>
> if (r_align > align) {
> n = &dev_res->list;
> @@ -197,6 +232,7 @@ static void pdev_sort_resources(struct pci_dev *dev, struct list_head *head)
> }
>
> static void __dev_sort_resources(struct pci_dev *dev,
> + struct list_head *realloc_head,
> struct list_head *head)
> {
> u16 class = dev->class >> 8;
> @@ -213,7 +249,7 @@ static void __dev_sort_resources(struct pci_dev *dev,
> return;
> }
>
> - pdev_sort_resources(dev, head);
> + pdev_sort_resources(dev, realloc_head, head);
> }
>
> static inline void reset_resource(struct resource *res)
> @@ -501,7 +537,7 @@ static void pdev_assign_resources_sorted(struct pci_dev *dev,
> {
> LIST_HEAD(head);
>
> - __dev_sort_resources(dev, &head);
> + __dev_sort_resources(dev, add_head, &head);
> __assign_resources_sorted(&head, add_head, fail_head);
>
> }
> @@ -514,7 +550,7 @@ static void pbus_assign_resources_sorted(const struct pci_bus *bus,
> LIST_HEAD(head);
>
> list_for_each_entry(dev, &bus->devices, bus_list)
> - __dev_sort_resources(dev, &head);
> + __dev_sort_resources(dev, realloc_head, &head);
>
> __assign_resources_sorted(&head, realloc_head, fail_head);
> }
> --
> 1.8.4.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Yinghai Lu <yinghai@kernel.org> |
|---|---|
| Date | 2015-08-18 21:10 +0200 |
| Message-ID | <pYUqK-6Po-33@gated-at.bofh.it> |
| In reply to | #1208939 |
On Mon, Aug 17, 2015 at 4:00 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Mon, Jul 27, 2015 at 04:29:21PM -0700, Yinghai Lu wrote:
>> During sorting before assign, we only put resource with non-zero align
>> in the sorted list, so for optional resources that must size is 0 and only
>> have addon parts, we need to have correct align.
>>
>> While treating SRIOV as optional resources, we always read alignment for
>> SRIOV bars, so they are ok.
>> Hotplug bridge resources are using STARTALIGN so it is ok when size is 0
>> if we have correct start for them.
>>
>> Later we want to treat the ROM BAR as optional resource, and it has
>> have SIZEALIGN, we need to find a way to get align for them.
>
> Are you saying we have a ROM resource where the ROM BAR is of non-zero
> size, but the resource structure says it's zero size?
Try to treat the ROM resource as optional, so will set the size in the
resource to 0.
aka required part size to 0. We can not get alignment from size anymore.
When we try to sort the resources before assign, we will skip resources with
bogus alignment.
>
>> We can use addon resource align instead in that case, and it will
>> be ok for SRIOV path and hotplug bridge resource path.
>>
>> Sorted list will contain must resource align/size to 0/0 to hold spot for
>> optional resources.
>>
>> We need to pass realloc_head from sizing stage to sorting stage, and
>> get entry from realloc list and calculate align from the entry.
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=81431
>
> Apparently this fixes a problem? What is the problem? What is the
> symptom of the problem?
resource allocation fail, so try to fix it by treat ROM BAR to be optional.
>
>> Reported-by: TJ <linux@iam.tj>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> ---
>> drivers/pci/setup-bus.c | 50 ++++++++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 43 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>> index 247d8fe..27cb0f0 100644
>> --- a/drivers/pci/setup-bus.c
>> +++ b/drivers/pci/setup-bus.c
>> @@ -145,9 +145,43 @@ static resource_size_t get_res_add_align(struct list_head *head,
>> return dev_res->min_align;
>> }
>>
>> +static resource_size_t __pci_resource_alignment(
>
> I don't like creating new function names by adding "__" to the
> beginning of an existing function name. It doesn't give enough of a
> clue about what how the functions are different. I don't really
> understand what's going on here, but this might be a clue that this
> could be refactored differently.
The function try to use alignment from optional part if alignment from
required part is 0.
>
>> + struct pci_dev *dev,
>> + struct resource *r,
>> + struct list_head *realloc_head)
>> +{
>> + resource_size_t r_align = pci_resource_alignment(dev, r);
>> + resource_size_t orig_start, orig_end;
>> + struct pci_dev_resource *dev_res;
>> +
>> + if (r_align || !realloc_head)
>> + return r_align;
>> +
>> + dev_res = res_to_dev_res(realloc_head, r);
>> + if (!dev_res || !dev_res->add_size)
>> + return r_align;
>> +
>> + orig_start = r->start;
>> + orig_end = r->end;
>> + r->end += dev_res->add_size;
>> + if ((r->flags & IORESOURCE_STARTALIGN)) {
>> + resource_size_t r_size = resource_size(r);
>> + resource_size_t add_align = dev_res->min_align;
>> +
>> + r->start = add_align;
>> + r->end = add_align + r_size - 1;
>
> I think this would be slightly shorter and more obvious as:
>
> resource_size_t r_size = resource_size(r);
>
> r->start = dev_res->min_align;
> r->end = r->start + r_size - 1;
ok, will update that.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web