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 <[email protected]>
---
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)
return err;
}
-static int pcifront_try_connect(struct pcifront_device *pdev)
+static void pcifront_connect(struct pcifront_device *pdev, bool rescan)
{
- int err = -EFAULT;
+ int err;
int i, num_roots, len;
char str[64];
unsigned int domain, bus;
-
- /* Only connect once */
- if (xenbus_read_driver_state(pdev->xdev->nodename) !=
- XenbusStateInitialised)
- goto out;
-
- err = pcifront_connect_and_init_dma(pdev);
- if (err && err != -EEXIST) {
- xenbus_dev_fatal(pdev->xdev, err,
- "Error setting up PCI Frontend");
- goto out;
- }
-
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);
if (err) {
xenbus_dev_fatal(pdev->xdev, err,
"Error scanning PCI root 0000:00");
- goto out;
+ return;
}
num_roots = 0;
} else if (err != 1) {
- if (err == 0)
- err = -EINVAL;
- xenbus_dev_fatal(pdev->xdev, err,
+ xenbus_dev_fatal(pdev->xdev, err >= 0 ? -EINVAL : err,
"Error reading number of PCI roots");
- goto out;
+ return;
}
for (i = 0; i < num_roots; i++) {
len = snprintf(str, sizeof(str), "root-%d", i);
- if (unlikely(len >= (sizeof(str) - 1))) {
- err = -ENOMEM;
- goto out;
- }
+ if (unlikely(len >= (sizeof(str) - 1)))
+ return;
err = xenbus_scanf(XBT_NIL, pdev->xdev->otherend, str,
"%x:%x", &domain, &bus);
if (err != 2) {
- if (err >= 0)
- err = -EINVAL;
- xenbus_dev_fatal(pdev->xdev, err,
+ xenbus_dev_fatal(pdev->xdev, err >= 0 ? -EINVAL : err,
"Error reading PCI root %d", i);
- goto out;
+ return;
}
- err = pcifront_scan_root(pdev, domain, bus);
+ if (rescan)
+ err = pcifront_rescan_root(pdev, domain, bus);
+ else
+ err = pcifront_scan_root(pdev, domain, bus);
if (err) {
xenbus_dev_fatal(pdev->xdev, err,
"Error scanning PCI root %04x:%02x",
domain, bus);
- goto out;
+ return;
}
}
- err = xenbus_switch_state(pdev->xdev, XenbusStateConnected);
+ xenbus_switch_state(pdev->xdev, XenbusStateConnected);
+}
-out:
- return err;
+static void pcifront_try_connect(struct pcifront_device *pdev)
+{
+ int err;
+
+ /* Only connect once */
+ if (xenbus_read_driver_state(pdev->xdev->nodename) !=
+ XenbusStateInitialised)
+ return;
+
+ err = pcifront_connect_and_init_dma(pdev);
+ if (err && err != -EEXIST) {
+ xenbus_dev_fatal(pdev->xdev, err,
+ "Error setting up PCI Frontend");
+ return;
+ }
+
+ pcifront_connect(pdev, false);
}
static int pcifront_try_disconnect(struct pcifront_device *pdev)
@@ -914,67 +917,11 @@ static int pcifront_try_disconnect(struct pcifront_device *pdev)
return err;
}
-static int pcifront_attach_devices(struct pcifront_device *pdev)
+static void pcifront_attach_devices(struct pcifront_device *pdev)
{
- int err = -EFAULT;
- int i, num_roots, len;
- unsigned int domain, bus;
- char str[64];
-
- if (xenbus_read_driver_state(pdev->xdev->nodename) !=
+ if (xenbus_read_driver_state(pdev->xdev->nodename) ==
XenbusStateReconfiguring)
- goto out;
-
- 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_rescan_root(pdev, 0, 0);
- if (err) {
- xenbus_dev_fatal(pdev->xdev, err,
- "Error scanning PCI root 0000:00");
- goto out;
- }
- num_roots = 0;
- } else if (err != 1) {
- if (err == 0)
- err = -EINVAL;
- xenbus_dev_fatal(pdev->xdev, err,
- "Error reading number of PCI roots");
- goto out;
- }
-
- for (i = 0; i < num_roots; i++) {
- len = snprintf(str, sizeof(str), "root-%d", i);
- if (unlikely(len >= (sizeof(str) - 1))) {
- err = -ENOMEM;
- goto out;
- }
-
- err = xenbus_scanf(XBT_NIL, pdev->xdev->otherend, str,
- "%x:%x", &domain, &bus);
- if (err != 2) {
- if (err >= 0)
- err = -EINVAL;
- xenbus_dev_fatal(pdev->xdev, err,
- "Error reading PCI root %d", i);
- goto out;
- }
-
- err = pcifront_rescan_root(pdev, domain, bus);
- if (err) {
- xenbus_dev_fatal(pdev->xdev, err,
- "Error scanning PCI root %04x:%02x",
- domain, bus);
- goto out;
- }
- }
-
- xenbus_switch_state(pdev->xdev, XenbusStateConnected);
-
-out:
- return err;
+ pcifront_connect(pdev, true);
}
static int pcifront_detach_devices(struct pcifront_device *pdev)
--
2.35.3
On Thu, Oct 6, 2022 at 5:29 AM Juergen Gross <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
Regards,
Jason
On 06.10.22 15:29, Jason Andryuk wrote:
> On Thu, Oct 6, 2022 at 5:29 AM Juergen Gross <[email protected]> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
Thanks. As I'm about to change the patch, I'll drop the R-b.
Juergen