linux-kernel - Re: [PATCH] xen/pcifront: move xenstore config scanning into sub-function

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] [day] [month] [year] [list]

Date:   Thu, 6 Oct 2022 17:01:56 +0200
From:   Juergen Gross <jgross@...e.com>
To:     Jason Andryuk <jandryuk@...il.com>
Cc:     linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
        Stefano Stabellini <sstabellini@...nel.org>,
        Oleksandr Tyshchenko <oleksandr_tyshchenko@...m.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        xen-devel@...ts.xenproject.org
Subject: Re: [PATCH] xen/pcifront: move xenstore config scanning into
 sub-function

On 06.10.22 15:29, Jason Andryuk wrote:
> On Thu, Oct 6, 2022 at 5:29 AM Juergen Gross <jgross@...e.com> wrote:
>>
>> pcifront_try_connect() and pcifront_attach_devices() share a large
>> chunk of duplicated code for reading the config information from
>> Xenstore, which only differs regarding a function call.
>>
>> Put that code into a new sub-function. While at it fix the error
>> reporting in case the root-xx node had the wrong format.
>>
>> As the return value of pcifront_try_connect() and
>> pcifront_attach_devices() are not used anywhere make those functions
>> return void. As an additional bonus this removes the dubious return
>> of -EFAULT in case of an unexpected driver state.
>>
>> Signed-off-by: Juergen Gross <jgross@...e.com>
>> ---
>>   drivers/pci/xen-pcifront.c | 133 +++++++++++--------------------------
>>   1 file changed, 40 insertions(+), 93 deletions(-)
>>
>> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
>> index 689271c4245c..a68e47dcdd7e 100644
>> --- a/drivers/pci/xen-pcifront.c
>> +++ b/drivers/pci/xen-pcifront.c
>> @@ -819,76 +819,79 @@ static int pcifront_publish_info(struct pcifront_device *pdev)
> 
>>          err = xenbus_scanf(XBT_NIL, pdev->xdev->otherend,
>>                             "root_num", "%d", &num_roots);
>>          if (err == -ENOENT) {
>>                  xenbus_dev_error(pdev->xdev, err,
>>                                   "No PCI Roots found, trying 0000:00");
>> -               err = pcifront_scan_root(pdev, 0, 0);
>> +               if (rescan)
>> +                       err = pcifront_rescan_root(pdev, 0, 0);
>> +               else
>> +                       err = pcifront_scan_root(pdev, 0, 0);
> 
> Early in pcifront_rescan_root(), we have:
> 
>          b = pci_find_bus(domain, bus);
>          if (!b)
>                  /* If the bus is unknown, create it. */
>                  return pcifront_scan_root(pdev, domain, bus);
> 
> pcifront_scan_root() does some allocation, but the later scanning
> matches that of pcifront_rescan_root().  So I think we can just always
> call pcifront_rescan_root() and it should do the right thing.  That
> drops the need for the rescan boolean.

Hmm, with some more pcifront_rescan_root() adaption this will make it
possible to drop even more code (i.e. the CONFIG_PCI_DOMAINS check in
pcifront_rescan_root(), as the one in pcifront_scan_root() would be
enough then).

I'll send out V2 soon.

> 
> Regardless of the above idea:
> 
> Reviewed-by: Jason Andryuk <jandryuk@...il.com>

Thanks. As I'm about to change the patch, I'll drop the R-b.


Juergen

Download attachment "OpenPGP_0xB0DE9DD628BF132F.asc" of type "application/pgp-keys" (3099 bytes)

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (496 bytes)