2024-06-10 09:35:47

by Dan Carpenter

Subject: [PATCH 0/2] PCI: endpoint: fix a couple error handling bugs

Two small error error handling and cleanup patches. The first one fixes
an incorrect error message printed on success. The other one fixes some
cleanup. Which is probably not required because PCI code is generally
required for a functioning system...

From static analysis. Untested.

2024-06-10 09:36:17

by Dan Carpenter

Subject: [PATCH 2/2] PCI: endpoint: Fix epf_ntb_epc_cleanup() a bit

There are two issues related to epf_ntb_epc_cleanup().
1) It should call epf_ntb_config_sspad_bar_clear().
2) The epf_ntb_bind() function should call epf_ntb_epc_cleanup()
to cleanup.

I also changed the ordering a bit. Unwinding should be done in the
mirror order from how they are allocated.

Fixes: e35f56bb0330 ("PCI: endpoint: Support NTB transfer between RC and EP")
Signed-off-by: Dan Carpenter <[email protected]>
---
drivers/pci/endpoint/functions/pci-epf-vntb.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index 7f05a44e9a9f..874cb097b093 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -799,8 +799,9 @@ static int epf_ntb_epc_init(struct epf_ntb *ntb)
*/
static void epf_ntb_epc_cleanup(struct epf_ntb *ntb)
{
- epf_ntb_db_bar_clear(ntb);
epf_ntb_mw_bar_clear(ntb, ntb->num_mws);
+ epf_ntb_db_bar_clear(ntb);
+ epf_ntb_config_sspad_bar_clear(ntb);
}

#define EPF_NTB_R(_name) \
@@ -1337,7 +1338,7 @@ static int epf_ntb_bind(struct pci_epf *epf)
ret = pci_register_driver(&vntb_pci_driver);
if (ret) {
dev_err(dev, "failure register vntb pci driver\n");
- goto err_bar_alloc;
+ goto err_epc_cleanup;
}

ret = vpci_scan_bus(ntb);
@@ -1348,6 +1349,8 @@ static int epf_ntb_bind(struct pci_epf *epf)

err_unregister:
pci_unregister_driver(&vntb_pci_driver);
+err_epc_cleanup:
+ epf_ntb_epc_cleanup(ntb);
err_bar_alloc:
epf_ntb_config_spad_bar_free(ntb);

--
2.43.0

2024-06-10 09:36:45

by Dan Carpenter

Subject: [PATCH 1/2] PCI: endpoint: Clean up error handling in vpci_scan_bus()

Smatch complains about inconsistent NULL checking in vpci_scan_bus():

drivers/pci/endpoint/functions/pci-epf-vntb.c:1024 vpci_scan_bus()
error: we previously assumed 'vpci_bus' could be null (see line 1021)

Instead of printing an error message and then crashing we should return
an error code and clean up. Also the NULL check is reversed so it
prints an error for success instead of failure.

Fixes: e35f56bb0330 ("PCI: endpoint: Support NTB transfer between RC and EP")
Signed-off-by: Dan Carpenter <[email protected]>
---
drivers/pci/endpoint/functions/pci-epf-vntb.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index 8e779eecd62d..7f05a44e9a9f 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -1018,8 +1018,10 @@ static int vpci_scan_bus(void *sysdata)
struct epf_ntb *ndev = sysdata;

vpci_bus = pci_scan_bus(ndev->vbus_number, &vpci_ops, sysdata);
- if (vpci_bus)
- pr_err("create pci bus\n");
+ if (!vpci_bus) {
+ pr_err("create pci bus failed\n");
+ return -EINVAL;
+ }

pci_bus_add_devices(vpci_bus);

@@ -1338,10 +1340,14 @@ static int epf_ntb_bind(struct pci_epf *epf)
goto err_bar_alloc;
}

- vpci_scan_bus(ntb);
+ ret = vpci_scan_bus(ntb);
+ if (ret)
+ goto err_unregister;

return 0;

+err_unregister:
+ pci_unregister_driver(&vntb_pci_driver);
err_bar_alloc:
epf_ntb_config_spad_bar_free(ntb);

--
2.43.0

2024-06-14 17:54:30

by Ilpo Järvinen

Subject: Re: [PATCH 2/2] PCI: endpoint: Fix epf_ntb_epc_cleanup() a bit

On Mon, 10 Jun 2024, Dan Carpenter wrote:

> There are two issues related to epf_ntb_epc_cleanup().
> 1) It should call epf_ntb_config_sspad_bar_clear().
> 2) The epf_ntb_bind() function should call epf_ntb_epc_cleanup()
> to cleanup.
>
> I also changed the ordering a bit. Unwinding should be done in the
> mirror order from how they are allocated.
>
> Fixes: e35f56bb0330 ("PCI: endpoint: Support NTB transfer between RC and EP")
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> drivers/pci/endpoint/functions/pci-epf-vntb.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index 7f05a44e9a9f..874cb097b093 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> @@ -799,8 +799,9 @@ static int epf_ntb_epc_init(struct epf_ntb *ntb)
> */
> static void epf_ntb_epc_cleanup(struct epf_ntb *ntb)
> {
> - epf_ntb_db_bar_clear(ntb);
> epf_ntb_mw_bar_clear(ntb, ntb->num_mws);
> + epf_ntb_db_bar_clear(ntb);
> + epf_ntb_config_sspad_bar_clear(ntb);
> }
>
> #define EPF_NTB_R(_name) \
> @@ -1337,7 +1338,7 @@ static int epf_ntb_bind(struct pci_epf *epf)
> ret = pci_register_driver(&vntb_pci_driver);
> if (ret) {
> dev_err(dev, "failure register vntb pci driver\n");
> - goto err_bar_alloc;
> + goto err_epc_cleanup;
> }
>
> ret = vpci_scan_bus(ntb);
> @@ -1348,6 +1349,8 @@ static int epf_ntb_bind(struct pci_epf *epf)
>
> err_unregister:
> pci_unregister_driver(&vntb_pci_driver);
> +err_epc_cleanup:
> + epf_ntb_epc_cleanup(ntb);
> err_bar_alloc:
> epf_ntb_config_spad_bar_free(ntb);

Reviewed-by: Ilpo J?rvinen <[email protected]>

--
i.

2024-06-14 19:53:18

by Ilpo Järvinen

Subject: Re: [PATCH 1/2] PCI: endpoint: Clean up error handling in vpci_scan_bus()

On Mon, 10 Jun 2024, Dan Carpenter wrote:

> Smatch complains about inconsistent NULL checking in vpci_scan_bus():
>
> drivers/pci/endpoint/functions/pci-epf-vntb.c:1024 vpci_scan_bus()
> error: we previously assumed 'vpci_bus' could be null (see line 1021)
>
> Instead of printing an error message and then crashing we should return
> an error code and clean up. Also the NULL check is reversed so it
> prints an error for success instead of failure.
>
> Fixes: e35f56bb0330 ("PCI: endpoint: Support NTB transfer between RC and EP")
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> drivers/pci/endpoint/functions/pci-epf-vntb.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index 8e779eecd62d..7f05a44e9a9f 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> @@ -1018,8 +1018,10 @@ static int vpci_scan_bus(void *sysdata)
> struct epf_ntb *ndev = sysdata;
>
> vpci_bus = pci_scan_bus(ndev->vbus_number, &vpci_ops, sysdata);
> - if (vpci_bus)
> - pr_err("create pci bus\n");
> + if (!vpci_bus) {
> + pr_err("create pci bus failed\n");
> + return -EINVAL;
> + }
>
> pci_bus_add_devices(vpci_bus);
>
> @@ -1338,10 +1340,14 @@ static int epf_ntb_bind(struct pci_epf *epf)
> goto err_bar_alloc;
> }
>
> - vpci_scan_bus(ntb);
> + ret = vpci_scan_bus(ntb);
> + if (ret)
> + goto err_unregister;
>
> return 0;
>
> +err_unregister:
> + pci_unregister_driver(&vntb_pci_driver);
> err_bar_alloc:
> epf_ntb_config_spad_bar_free(ntb);

Reviewed-by: Ilpo J?rvinen <[email protected]>


--
i.