Re: [PATCH 2/4] PCI: add functionality for resizing resources v3

[Andy Shevchenko]

On Thu, May 4, 2017 at 1:15 PM, Andy Shevchenko
 wrote:
> On Thu, May 4, 2017 at 12:23 PM, Christian König
>  wrote:
>> Am 26.04.2017 um 19:00 schrieb Andy Shevchenko:

> static int ...xxx...(...)
> {
> unsigned int i;
> int ret;
>

> if (res->parent)
> release_resource(res);
>

dev_info() should be here. I commented on this earlier, just forgot.

> res->start = 0;
> res->end = 0;
> dev_info(>dev, "BAR %d: released %pR\n", i, res);
> return i;
> }
>
>
> struct pci_dev *next = bridge;
> ...
> do {
> bridge = next;
>
> ret = ...xxx...(...);

> if (ret)

Here, of course,

if (ret < 0)

> goto cleanup;
>
> next = bridge->bus ? bridge->bus->self : NULL;
> } while (next);


-- 
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] PCI: add functionality for resizing resources v3

[Andy Shevchenko]

On Thu, May 4, 2017 at 12:23 PM, Christian König
 wrote:
> Am 26.04.2017 um 19:00 schrieb Andy Shevchenko:

>>> +   while (1) {
>>
>> This raises red flag. Care to refactor?
>> Also do {} while () syntax allows faster to get that the loop body
>> goes at least once.
>
>
> I've tried to refactor this, but couldn't come up with something which works
> and is readable at the same time.
>
> Any suggestion how this should look like?

This is original code.

--- 8< --- 8< ---

while (1) {
for (i = PCI_BRIDGE_RESOURCES; i < PCI_BRIDGE_RESOURCE_END; i++) {
struct resource *res = >resource[i];

if ((res->flags & type_mask) != (type & type_mask))
continue;

/* Ignore BARs which are still in use */
if (res->child)
continue;

ret = add_to_list(, bridge, res, 0, 0);
if (ret)
goto cleanup;

if (res->parent)
release_resource(res);
res->start = 0;
res->end = 0;
dev_info(>dev, "BAR %d: released %pR\n", i, res);
break;
}
if (i == PCI_BRIDGE_RESOURCE_END)
break;

if (!bridge->bus || !bridge->bus->self)
break;

bridge = bridge->bus->self;
}

--- 8< --- 8< ---

I would think about something like below

static int ...xxx...(...)
{
unsigned int i;
int ret;

for (i = PCI_BRIDGE_RESOURCES; i < PCI_BRIDGE_RESOURCE_END; i++) {
struct resource *res = >resource[i];

/* Ignore BARs which are still in use */
if (((res->flags ^ type) & type_mask) == 0 && !res->child)
break;
}
if (i == PCI_BRIDGE_RESOURCE_END)
return -EBUSY;

ret = add_to_list(, bridge, res, 0, 0);
if (ret)
return ret;

if (res->parent)
release_resource(res);

res->start = 0;
res->end = 0;
dev_info(>dev, "BAR %d: released %pR\n", i, res);
return i;
}


struct pci_dev *next = bridge;
...
do {
bridge = next;

ret = ...xxx...(...);
if (ret)
goto cleanup;

next = bridge->bus ? bridge->bus->self : NULL;
} while (next);
...

-- 
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] PCI: add functionality for resizing resources v3

[Christian König]

Am 26.04.2017 um 19:00 schrieb Andy Shevchenko:

[SNIP]

+   while (1) {

This raises red flag. Care to refactor?
Also do {} while () syntax allows faster to get that the loop body
goes at least once.


I've tried to refactor this, but couldn't come up with something which 
works and is readable at the same time.


Any suggestion how this should look like?

Christian.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] PCI: add functionality for resizing resources v3

[Andy Shevchenko]

On Tue, May 2, 2017 at 6:51 PM, Christian König  wrote:
> Am 26.04.2017 um 19:00 schrieb Andy Shevchenko:
>> On Tue, Apr 25, 2017 at 4:19 PM, Christian König
>>  wrote:

>>> +int pci_reassign_bridge_resources(struct pci_dev *bridge, unsigned long
>>> type)
>>> +{
>>> +   const unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
>>> +   IORESOURCE_PREFETCH | IORESOURCE_MEM_64;
>>> +
>>
>> Redundant.
>
>
> Redundant, but also a reminder to myself that I wanted to ask something
> about that.

Missed context I suppose. Usually I comment in one word something
obvious, i.e. redundant empty line.
Sorry for missing my point.

> This type_mask is used already three times in this file, shouldn't we add a
> define for that?

Yes, that's wxactly what I commented somewhere (in one of the rest cases).

-- 
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] PCI: add functionality for resizing resources v3

[Christian König]

Am 26.04.2017 um 19:00 schrieb Andy Shevchenko:

On Tue, Apr 25, 2017 at 4:19 PM, Christian König
 wrote:

From: Christian König 

This allows device drivers to request resizing their BARs.

The function only tries to reprogram the windows of the bridge directly above
the requesting device and only the BAR of the same type (usually mem, 64bit,
prefetchable). This is done to make sure not to disturb other drivers by
changing the BARs of their devices.

If reprogramming the bridge BAR fails the old status is restored and -ENOSPC
returned to the calling device driver.
+int pci_reassign_bridge_resources(struct pci_dev *bridge, unsigned long type)
+{
+   const unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
+   IORESOURCE_PREFETCH | IORESOURCE_MEM_64;
+

Redundant.


Redundant, but also a reminder to myself that I wanted to ask something 
about that.


This type_mask is used already three times in this file, shouldn't we 
add a define for that?



[SNIP]

+   list_for_each_entry(dev_res, , list) {
+   /* Skip the bridge we just assigned resources for. */
+   if (bridge == dev_res->dev)
+   continue;
+
+   bridge = dev_res->dev;
+   pci_setup_bridge(bridge->subordinate);
+   }
+
+   free_list();
+   free_list();
+   return ret;

You might re-use two lines with below, but perhaps better to show
which case returns 0 explicitly and drop assignment ret = 0 above.


Good point, but actually the free_list() is superfluous here 
since when the failed list isn't empty we end up in the cleanup path.


Going to fix all other comments in the next version.

Regards,
Christian.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] PCI: add functionality for resizing resources v3

[Andy Shevchenko]

On Tue, Apr 25, 2017 at 4:19 PM, Christian König
 wrote:
> From: Christian König 
>
> This allows device drivers to request resizing their BARs.
>
> The function only tries to reprogram the windows of the bridge directly above
> the requesting device and only the BAR of the same type (usually mem, 64bit,
> prefetchable). This is done to make sure not to disturb other drivers by
> changing the BARs of their devices.
>
> If reprogramming the bridge BAR fails the old status is restored and -ENOSPC
> returned to the calling device driver.

> +int pci_reassign_bridge_resources(struct pci_dev *bridge, unsigned long type)
> +{
> +   const unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
> +   IORESOURCE_PREFETCH | IORESOURCE_MEM_64;

> +

Redundant.

> +   struct pci_dev_resource *dev_res;
> +   LIST_HEAD(saved);
> +   LIST_HEAD(added);
> +   LIST_HEAD(failed);

> +   unsigned i;

unsigned int i;

> +   int ret = 0;
> +
> +   /* Walk to the root BUS, releasing bridge BARs when possible */

> +   while (1) {

This raises red flag. Care to refactor?
Also do {} while () syntax allows faster to get that the loop body
goes at least once.

> +   for (i = PCI_BRIDGE_RESOURCES; i < PCI_BRIDGE_RESOURCE_END;
> +i++) {

> +   struct resource *res = >resource[i];
> +

> +   if ((res->flags & type_mask) != (type & type_mask))

I would rather go with

if ((res->flags ^ type) & type_mask)

> +   continue;
> +
> +   /* Ignore BARs which are still in use */
> +   if (res->child)
> +   continue;
> +
> +   ret = add_to_list(, bridge, res, 0, 0);
> +   if (ret)
> +   goto cleanup;
> +
> +   if (res->parent)
> +   release_resource(res);

> +   res->start = 0;
> +   res->end = 0;

> +   dev_info(>dev, "BAR %d: released %pR\n",
> +i, res);

I doesn't make much sense to me after zeroing a range.

> +   break;
> +   }
> +   if (i == PCI_BRIDGE_RESOURCE_END)
> +   break;
> +
> +   if (!bridge->bus || !bridge->bus->self)
> +   break;
> +
> +   bridge = bridge->bus->self;
> +   }
> +
> +   if (list_empty())
> +   return -ENOENT;
> +
> +   __pci_bus_size_bridges(bridge->subordinate, );
> +   __pci_bridge_assign_resources(bridge, , );
> +   BUG_ON(!list_empty());
> +
> +   if (!list_empty()) {
> +   ret = -ENOSPC;
> +   goto cleanup;
> +   }

> +
> +

Remove extra empty line.

> +   list_for_each_entry(dev_res, , list) {
> +   /* Skip the bridge we just assigned resources for. */
> +   if (bridge == dev_res->dev)
> +   continue;
> +
> +   bridge = dev_res->dev;
> +   pci_setup_bridge(bridge->subordinate);
> +   }
> +

> +   free_list();
> +   free_list();
> +   return ret;

You might re-use two lines with below, but perhaps better to show
which case returns 0 explicitly and drop assignment ret = 0 above.

> +
> +cleanup:
> +   /* restore size and flags */
> +   list_for_each_entry(dev_res, , list) {
> +   struct resource *res = dev_res->res;
> +
> +   res->start = dev_res->start;
> +   res->end = dev_res->end;
> +   res->flags = dev_res->flags;
> +   }
> +   free_list();
> +
> +   /* Revert to the old configuration */
> +   list_for_each_entry(dev_res, , list) {
> +   struct resource *res = dev_res->res;
> +
> +   bridge = dev_res->dev;
> +   i = res - bridge->resource;
> +
> +   res->start = dev_res->start;
> +   res->end = dev_res->end;
> +   res->flags = dev_res->flags;
> +
> +   pci_claim_resource(bridge, i);
> +   pci_setup_bridge(bridge->subordinate);
> +   }

> +   free_list();
> +
> +   return ret;

> +}

> +void pci_release_resource(struct pci_dev *dev, int resno)
> +{
> +   struct resource *res = dev->resource + resno;
> +
> +   release_resource(res);

> +   res->end = resource_size(res) - 1;
> +   res->start = 0;
> +   res->flags |= IORESOURCE_UNSET;

> +   dev_info(>dev, "BAR %d: released %pR\n", resno, res);

Makes little sense to me after you cleared information out.

> +}
> +EXPORT_SYMBOL(pci_release_resource);
> +
> +int pci_resize_resource(struct pci_dev *dev, int resno, int size)
> +{
> +   struct resource *res = dev->resource + resno;

> +   u64 bytes = 1ULL << (size + 20);

> +   

[PATCH 2/4] PCI: add functionality for resizing resources v3

[Christian König]

From: Christian König 

This allows device drivers to request resizing their BARs.

The function only tries to reprogram the windows of the bridge directly above
the requesting device and only the BAR of the same type (usually mem, 64bit,
prefetchable). This is done to make sure not to disturb other drivers by
changing the BARs of their devices.

If reprogramming the bridge BAR fails the old status is restored and -ENOSPC
returned to the calling device driver.

v2: rebase on changes in rbar support
v3: style cleanups, fail if memory decoding is enabled or resources
still allocated, resize all unused bridge BARs,
drop calling pci_reenable_device

Signed-off-by: Christian König 
---
 drivers/pci/setup-bus.c | 102 
 drivers/pci/setup-res.c |  60 
 include/linux/pci.h |   3 ++
 3 files changed, 165 insertions(+)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index f30ca75..39351cf 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1923,6 +1923,108 @@ void pci_assign_unassigned_bridge_resources(struct 
pci_dev *bridge)
 }
 EXPORT_SYMBOL_GPL(pci_assign_unassigned_bridge_resources);
 
+int pci_reassign_bridge_resources(struct pci_dev *bridge, unsigned long type)
+{
+   const unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
+   IORESOURCE_PREFETCH | IORESOURCE_MEM_64;
+
+   struct pci_dev_resource *dev_res;
+   LIST_HEAD(saved);
+   LIST_HEAD(added);
+   LIST_HEAD(failed);
+   unsigned i;
+   int ret = 0;
+
+   /* Walk to the root BUS, releasing bridge BARs when possible */
+   while (1) {
+   for (i = PCI_BRIDGE_RESOURCES; i < PCI_BRIDGE_RESOURCE_END;
+i++) {
+   struct resource *res = >resource[i];
+
+   if ((res->flags & type_mask) != (type & type_mask))
+   continue;
+
+   /* Ignore BARs which are still in use */
+   if (res->child)
+   continue;
+
+   ret = add_to_list(, bridge, res, 0, 0);
+   if (ret)
+   goto cleanup;
+
+   if (res->parent)
+   release_resource(res);
+   res->start = 0;
+   res->end = 0;
+   dev_info(>dev, "BAR %d: released %pR\n",
+i, res);
+   break;
+   }
+   if (i == PCI_BRIDGE_RESOURCE_END)
+   break;
+
+   if (!bridge->bus || !bridge->bus->self)
+   break;
+
+   bridge = bridge->bus->self;
+   }
+
+   if (list_empty())
+   return -ENOENT;
+
+   __pci_bus_size_bridges(bridge->subordinate, );
+   __pci_bridge_assign_resources(bridge, , );
+   BUG_ON(!list_empty());
+
+   if (!list_empty()) {
+   ret = -ENOSPC;
+   goto cleanup;
+   }
+
+
+   list_for_each_entry(dev_res, , list) {
+   /* Skip the bridge we just assigned resources for. */
+   if (bridge == dev_res->dev)
+   continue;
+
+   bridge = dev_res->dev;
+   pci_setup_bridge(bridge->subordinate);
+   }
+
+   free_list();
+   free_list();
+   return ret;
+
+cleanup:
+   /* restore size and flags */
+   list_for_each_entry(dev_res, , list) {
+   struct resource *res = dev_res->res;
+
+   res->start = dev_res->start;
+   res->end = dev_res->end;
+   res->flags = dev_res->flags;
+   }
+   free_list();
+
+   /* Revert to the old configuration */
+   list_for_each_entry(dev_res, , list) {
+   struct resource *res = dev_res->res;
+
+   bridge = dev_res->dev;
+   i = res - bridge->resource;
+
+   res->start = dev_res->start;
+   res->end = dev_res->end;
+   res->flags = dev_res->flags;
+
+   pci_claim_resource(bridge, i);
+   pci_setup_bridge(bridge->subordinate);
+   }
+   free_list();
+
+   return ret;
+}
+
 void pci_assign_unassigned_bus_resources(struct pci_bus *bus)
 {
struct pci_dev *dev;
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 9526e34..0d40adb 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -363,6 +363,66 @@ int pci_reassign_resource(struct pci_dev *dev, int resno, 
resource_size_t addsiz
return 0;
 }
 
+void pci_release_resource(struct pci_dev *dev, int resno)
+{
+   struct resource *res = dev->resource + resno;
+
+   release_resource(res);
+   res->end = resource_size(res) - 1;
+   res->start = 0;
+   res->flags