Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221006092929.30041-1-jgross@suse.com>
In-Reply-To: <20221006092929.30041-1-jgross@suse.com>
From:   Jason Andryuk <jandryuk@gmail.com>
Date:   Thu, 6 Oct 2022 09:29:41 -0400
Message-ID: <CAKf6xpvS20J0oz6vn+g47OBbKxEhAC8f2gyQ9otj+rOn+L9FOw@mail.gmail.com>
Subject: Re: [PATCH] xen/pcifront: move xenstore config scanning into sub-function
To:     Juergen Gross <jgross@suse.com>
Cc:     linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,
        Stefano Stabellini <sstabellini@kernel.org>,
        Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        xen-devel@lists.xenproject.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Oct 6, 2022 at 5:29 AM Juergen Gross <jgross@suse.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@suse.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.

Regardless of the above idea:

Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

Regards,
Jason