Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754602AbdDKJRl (ORCPT ); Tue, 11 Apr 2017 05:17:41 -0400 Received: from pegasos-out.vodafone.de ([80.84.1.38]:40072 "EHLO pegasos-out.vodafone.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754446AbdDKJRj (ORCPT ); Tue, 11 Apr 2017 05:17:39 -0400 X-Spam-Flag: NO X-Spam-Score: 0.2 Authentication-Results: rohrpostix1.prod.vfnet.de (amavisd-new); dkim=pass header.i=@vodafone.de X-DKIM: OpenDKIM Filter v2.6.8 pegasos-out.vodafone.de F2386261EF0 Subject: Re: [PATCH 2/4] PCI: add functionality for resizing resources v2 To: Andy Shevchenko References: <1489408896-25039-1-git-send-email-deathsimple@vodafone.de> <1489408896-25039-3-git-send-email-deathsimple@vodafone.de> Cc: helgaas@kernel.org, "linux-pci@vger.kernel.org" , dri-devel@lists.freedesktop.org, Platform Driver , amd-gfx@lists.freedesktop.org, "linux-kernel@vger.kernel.org" From: =?UTF-8?Q?Christian_K=c3=b6nig?= Message-ID: Date: Tue, 11 Apr 2017 11:14:29 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2003 Lines: 74 Sorry for the delayed response, have been busy with other stuff recently. Am 13.03.2017 um 17:43 schrieb Andy Shevchenko: >> v2: rebase on changes in rbar support > This kind of comments usually goes after --- delimiter below. That would remove it from the commit message which I don't want. >> + unsigned i; >> + int ret = 0; >> + >> + /* Release all children from the matching bridge resource */ >> + 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)) > IIUC it would be > if ((res->flags ^ type) & type_mask) > > (I consider 'diff' as XOR operation is more understandable, but it's up to you) I think like it is is easier to read. >> + res->start = saved.start; >> + res->end = saved.end; >> + res->flags = saved.flags; > Would > *res = saved; > work? No, res also contains a bunch of pointers into the tree which we should not override. >> +int pci_resize_resource(struct pci_dev *dev, int resno, int size) >> +{ >> + struct resource *res = dev->resource + resno; >> + u32 sizes = pci_rbar_get_possible_sizes(dev, resno); >> + int old = pci_rbar_get_current_size(dev, resno); >> + u64 bytes = 1ULL << (size + 20); >> + int ret = 0; >> + > I would put > sizes = pci_rbar_get_possible_sizes(dev, resno); > here Good idea, done. >> + if (!sizes) >> + return -ENOTSUPP; >> + >> + if (!(sizes & (1 << size))) > BIT(size) ? Done. > and > old = pci_rbar_get_current_size(dev, resno); > here Good idea as well. >> +error_resize: >> + pci_rbar_set_size(dev, resno, old); >> + res->end = res->start + (1ULL << (old + 20)) - 1; > BIT_ULL(old + 20) ? Nope, that is actually a size in bytes. Not a bitfield. So while BIT_ULL yields the right result it would be harder to understand. Christian.