diff mbox series

[v2,08/17] PCI: dwc: Add host de-initialization callback

Message ID 20220503214638.1895-9-Sergey.Semin@baikalelectronics.ru (mailing list archive)
State Superseded
Delegated to: Lorenzo Pieralisi
Headers show
Series PCI: dwc: Add dma-ranges/YAML-schema/Baikal-T1 support | expand

Commit Message

Serge Semin May 3, 2022, 9:46 p.m. UTC
Seeing the platform-specific DW PCIe host-initialization is performed from
within the generic dw_pcie_host_init() method by means of the dedicated
dw_pcie_ops.host_init() callback, there must be declared an antagonist
which would perform the corresponding cleanups. Let's add such callback
then. It will be called in the dw_pcie_host_deinit() method and in the
cleanup-on-error path in the dw_pcie_host_init() function.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 .../pci/controller/dwc/pcie-designware-host.c | 21 ++++++++++++++-----
 drivers/pci/controller/dwc/pcie-designware.h  |  1 +
 2 files changed, 17 insertions(+), 5 deletions(-)

Comments

Rob Herring (Arm) May 16, 2022, 8:48 p.m. UTC | #1
On Wed, May 04, 2022 at 12:46:29AM +0300, Serge Semin wrote:
> Seeing the platform-specific DW PCIe host-initialization is performed from
> within the generic dw_pcie_host_init() method by means of the dedicated
> dw_pcie_ops.host_init() callback, there must be declared an antagonist
> which would perform the corresponding cleanups. Let's add such callback
> then. It will be called in the dw_pcie_host_deinit() method and in the
> cleanup-on-error path in the dw_pcie_host_init() function.

I'm not really a fan of .host_init() to begin with as it isn't really 
clear by the name when it is supposed to be called and what init to do. 
The drv probe -> dw_pcie_host_init -> drv .host_init() -> return to drv 
sequence isn't great either. I'd rather see more fine grained and well 
defined hooks. So I'm hesitant to add a host_deinit()...

Rob
Serge Semin May 20, 2022, 10:20 a.m. UTC | #2
On Mon, May 16, 2022 at 03:48:28PM -0500, Rob Herring wrote:
> On Wed, May 04, 2022 at 12:46:29AM +0300, Serge Semin wrote:
> > Seeing the platform-specific DW PCIe host-initialization is performed from
> > within the generic dw_pcie_host_init() method by means of the dedicated
> > dw_pcie_ops.host_init() callback, there must be declared an antagonist
> > which would perform the corresponding cleanups. Let's add such callback
> > then. It will be called in the dw_pcie_host_deinit() method and in the
> > cleanup-on-error path in the dw_pcie_host_init() function.
> 

> I'm not really a fan of .host_init() to begin with as it isn't really 
> clear by the name when it is supposed to be called and what init to do. 
> The drv probe -> dw_pcie_host_init -> drv .host_init() -> return to drv 
> sequence isn't great either. I'd rather see more fine grained and well 
> defined hooks. So I'm hesitant to add a host_deinit()...

What you say is a matter of another change. This patch just fixes the
already defined init-hook interface. Indeed the initializations
performed in the framework of the init-method need to be cleaned up in
case of the PCIe host probe procedure failure. I can't fix the DW PCIe
platform drivers since they may have some specifics I am not aware of,
but at least the interface needs to be provided for the new drivers.
One such low-level driver is submitted for review in this patchset.

What you suggest is a lot of additional work including preliminary
design settling discussions and consequent review. Alas I don't have
time for this anymore especially seeing the need for such finer grained
hooking isn't justified by neither the current platform drivers nor
the driver submitted by me. Sorry I can't fulfill your request.

-Sergey

> 
> Rob
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index c74b4587b236..bb1c25c32f2d 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -354,13 +354,14 @@  int dw_pcie_host_init(struct pcie_port *pp)
 			pp->num_vectors = MSI_DEF_NUM_VECTORS;
 		} else if (pp->num_vectors > MAX_MSI_IRQS) {
 			dev_err(dev, "Invalid number of vectors\n");
-			return -EINVAL;
+			ret = -EINVAL;
+			goto err_deinit_host;
 		}
 
 		if (pp->ops->msi_host_init) {
 			ret = pp->ops->msi_host_init(pp);
 			if (ret < 0)
-				return ret;
+				goto err_deinit_host;
 		} else if (pp->has_msi_ctrl) {
 			u32 ctrl, num_ctrls;
 
@@ -372,8 +373,10 @@  int dw_pcie_host_init(struct pcie_port *pp)
 				pp->msi_irq = platform_get_irq_byname_optional(pdev, "msi");
 				if (pp->msi_irq < 0) {
 					pp->msi_irq = platform_get_irq(pdev, 0);
-					if (pp->msi_irq < 0)
-						return pp->msi_irq;
+					if (pp->msi_irq < 0) {
+						ret = pp->msi_irq;
+						goto err_deinit_host;
+					}
 				}
 			}
 
@@ -381,7 +384,7 @@  int dw_pcie_host_init(struct pcie_port *pp)
 
 			ret = dw_pcie_allocate_domains(pp);
 			if (ret)
-				return ret;
+				goto err_deinit_host;
 
 			if (pp->msi_irq > 0)
 				irq_set_chained_handler_and_data(pp->msi_irq,
@@ -434,6 +437,11 @@  int dw_pcie_host_init(struct pcie_port *pp)
 err_free_msi:
 	if (pp->has_msi_ctrl)
 		dw_pcie_free_msi(pp);
+
+err_deinit_host:
+	if (pp->ops->host_deinit)
+		pp->ops->host_deinit(pp);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(dw_pcie_host_init);
@@ -450,6 +458,9 @@  void dw_pcie_host_deinit(struct pcie_port *pp)
 
 	if (pp->has_msi_ctrl)
 		dw_pcie_free_msi(pp);
+
+	if (pp->ops->host_deinit)
+		pp->ops->host_deinit(pp);
 }
 EXPORT_SYMBOL_GPL(dw_pcie_host_deinit);
 
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 1868773ecb91..bca1d3e83636 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -200,6 +200,7 @@  enum dw_pcie_device_mode {
 
 struct dw_pcie_host_ops {
 	int (*host_init)(struct pcie_port *pp);
+	void (*host_deinit)(struct pcie_port *pp);
 	int (*msi_host_init)(struct pcie_port *pp);
 };