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]

Date:   Tue, 11 Apr 2017 11:14:29 +0200
From:   Christian König <deathsimple@...afone.de>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
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

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.