linux-kernel - Re: [PATCH 2/4] PCI: add functionality for resizing resources v2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VcY_P63T3-XcYafxhSFshQyL1dj0B2+Q4iW9hMiig3tRQ@mail.gmail.com>
Date:   Mon, 13 Mar 2017 18:43:42 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Christian König <deathsimple@...afone.de>
Cc:     helgaas@...nel.org,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        dri-devel@...ts.freedesktop.org,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        amd-gfx@...ts.freedesktop.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/4] PCI: add functionality for resizing resources v2

On Mon, Mar 13, 2017 at 2:41 PM, Christian König
<deathsimple@...afone.de> wrote:
> From: Christian König <christian.koenig@....com>
>
> 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.

Some style comments.

> v2: rebase on changes in rbar support

This kind of comments usually goes after --- delimiter below.

> Signed-off-by: Christian König <christian.koenig@....com>
> ---

...here.

> +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 resource saved;
> +       LIST_HEAD(add_list);
> +       LIST_HEAD(fail_head);

> +       struct pci_dev_resource *fail_res;

Perhaps move before definition of 'saved'?

> +       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)

> +                       continue;

> +       /* restore size and flags */
> +       list_for_each_entry(fail_res, &fail_head, list) {
> +               struct resource *res = fail_res->res;
> +
> +               res->start = fail_res->start;
> +               res->end = fail_res->end;
> +               res->flags = fail_res->flags;
> +       }
> +
> +       /* Revert to the old configuration */
> +       if (!list_empty(&fail_head)) {
> +               struct resource *res = &bridge->resource[i];
> +

> +               res->start = saved.start;
> +               res->end = saved.end;
> +               res->flags = saved.flags;

Would
 *res = saved;
work?

> +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

> +       if (!sizes)
> +               return -ENOTSUPP;
> +
> +       if (!(sizes & (1 << size)))

BIT(size) ?

> +               return -EINVAL;
> +

and
 old = pci_rbar_get_current_size(dev, resno);
here

> +       if (old < 0)
> +               return old;

> +error_resize:
> +       pci_rbar_set_size(dev, resno, old);
> +       res->end = res->start + (1ULL << (old + 20)) - 1;

BIT_ULL(old + 20) ?

-- 
With Best Regards,
Andy Shevchenko