From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751912AbdEDKP4 (ORCPT <rfc822;w@1wt.eu>);
        Thu, 4 May 2017 06:15:56 -0400
Received: from mail-qk0-f181.google.com ([209.85.220.181]:35777 "EHLO
        mail-qk0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751113AbdEDKPu (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Thu, 4 May 2017 06:15:50 -0400
MIME-Version: 1.0
In-Reply-To: <5a9a94f3-34b3-9484-a455-f5b2ee0b4d51@vodafone.de>
References: <1493126394-1239-1-git-send-email-deathsimple@vodafone.de>
 <1493126394-1239-3-git-send-email-deathsimple@vodafone.de>
 <CAHp75VfSFBYgHRsX2JSH0zdSEW8VvXbNMVvBBsD9kKgzGr-v2w@mail.gmail.com> <5a9a94f3-34b3-9484-a455-f5b2ee0b4d51@vodafone.de>
From: Andy Shevchenko <andy.shevchenko@gmail.com>
Date: Thu, 4 May 2017 13:15:49 +0300
Message-ID: <CAHp75VcLWrrRuUYwT4Mxww9e3886LqczMXTHo95wUybyCZpqTg@mail.gmail.com>
Subject: Re: [PATCH 2/4] PCI: add functionality for resizing resources v3
To: =?UTF-8?Q?Christian_K=C3=B6nig?= <deathsimple@vodafone.de>
Cc: Bjorn Helgaas <helgaas@kernel.org>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        dri-devel@lists.freedesktop.org,
        Platform Driver <platform-driver-x86@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
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 v44AG9Cm002688

On Thu, May 4, 2017 at 12:23 PM, Christian König
<deathsimple@vodafone.de> 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 = &bridge->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(&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);
                    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 = &bridge->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(&saved, bridge, res, 0, 0);
    if (ret)
            return ret;

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

    res->start = 0;
    res->end = 0;
    dev_info(&bridge->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