Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752521AbdDZRA5 (ORCPT ); Wed, 26 Apr 2017 13:00:57 -0400 Received: from mail-qk0-f194.google.com ([209.85.220.194]:35502 "EHLO mail-qk0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751432AbdDZRAg (ORCPT ); Wed, 26 Apr 2017 13:00:36 -0400 MIME-Version: 1.0 In-Reply-To: <1493126394-1239-3-git-send-email-deathsimple@vodafone.de> References: <1493126394-1239-1-git-send-email-deathsimple@vodafone.de> <1493126394-1239-3-git-send-email-deathsimple@vodafone.de> From: Andy Shevchenko Date: Wed, 26 Apr 2017 20:00:34 +0300 Message-ID: Subject: Re: [PATCH 2/4] PCI: add functionality for resizing resources v3 To: =?UTF-8?Q?Christian_K=C3=B6nig?= Cc: helgaas@kernel.org, "linux-pci@vger.kernel.org" , dri-devel@lists.freedesktop.org, Platform Driver , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id v3QH1o7g030692 Content-Length: 5303 Lines: 188 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 = &bridge->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(&saved, bridge, res, 0, 0); > + if (ret) > + goto cleanup; > + > + if (res->parent) > + release_resource(res); > + res->start = 0; > + res->end = 0; > + dev_info(&bridge->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(&saved)) > + return -ENOENT; > + > + __pci_bus_size_bridges(bridge->subordinate, &added); > + __pci_bridge_assign_resources(bridge, &added, &failed); > + BUG_ON(!list_empty(&added)); > + > + if (!list_empty(&failed)) { > + ret = -ENOSPC; > + goto cleanup; > + } > + > + Remove extra empty line. > + list_for_each_entry(dev_res, &saved, 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(&saved); > + free_list(&failed); > + 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, &failed, list) { > + struct resource *res = dev_res->res; > + > + res->start = dev_res->start; > + res->end = dev_res->end; > + res->flags = dev_res->flags; > + } > + free_list(&failed); > + > + /* Revert to the old configuration */ > + list_for_each_entry(dev_res, &saved, 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(&saved); > + > + 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->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); > + res->end = res->start + (1ULL << (old + 20)) - 1; Perhaps macro or helper? static inline u64 rbar_size_to_bytes(size) { return 1ULL << (size + 20); } -- With Best Regards, Andy Shevchenko