PCI: dwc: Support remove
v2 cover PCI: dwc: Support remove
Currently, the PCI dwc host users don't support the remove, but nothing prevent us from supporting it. To achieve this goal, we need to ensure we can do necessary clean up work.
Changes since v1:
- address Bjorn's comments, I.E Capitalize, s/irq/IRQ/, s/msi/MSI/
Jisheng Zhang (5):
PCI: dwc: Fix dw_pcie_free_msi() if msi_irq is invalid
PCI: dwc: Free the page for MSI IRQ in dw_pcie_free_msi()
PCI: dwc: Free MSI in the error code path of dw_pcie_host_init()
PCI: dwc: Use devm_pci_alloc_host_bridge() to simplify the code
PCI: dwc: Save root bus for driver remove
.../pci/controller/dwc/pcie-designware-host.c | 38 ++++++++++--------- drivers/pci/controller/dwc/pcie-designware.h | 2 + 2 files changed, 23 insertions(+), 17 deletions(-)
Signature · 1 line
2.20.1
PATCH v2 1/5 PCI: dwc: Fix dw_pcie_free_msi() if msi_irq is invalid
We should check msi_irq before calling irq_set_chained_handler() and irq_set_handler_data().
--- drivers/pci/controller/dwc/pcie-designware-host.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/pci/controller/dwc/pcie-designware-host.c +4 -2
@@ -298,8 +298,10 @@ int dw_pcie_allocate_domains(struct pcie_port *pp)void dw_pcie_free_msi (struct pcie_port *pp) { -irq_set_chained_handler (pp->msi_irq,NULL ); -irq_set_handler_data (pp->msi_irq,NULL ); +if (pp->msi_irq) { +irq_set_chained_handler (pp->msi_irq,NULL ); +irq_set_handler_data (pp->msi_irq,NULL ); + }irq_domain_remove (pp->msi_domain);irq_domain_remove (pp->irq_domain);
Signature · 1 line
2.20.1
Re: [PATCH v2 1/5] PCI: dwc: Fix dw_pcie_free_msi() if msi_irq is invalid
Quoted 16 lines · Jisheng Zhang
We should check
msi_irqbefore callingirq_set_chained_handler()andirq_set_handler_data().Signed-off-by Jisheng Zhang <[email protected]>--- drivers/pci/controller/dwc/pcie-designware-host.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)drivers/pci/controller/dwc/pcie-designware-host.c +0
-0@@ -298,8 +298,10 @@ int dw_pcie_allocate_domains(struct pcie_port *pp)void dw_pcie_free_msi (struct pcie_port *pp)
drivers/pci/controller/dwc/pcie-designware-host.c int dw_pcie_allocate_domains(struct pcie_port *pp) · line 307
Quoted 10 lines
drivers/pci/controller/dwc/pcie-designware-host.c +4
-2{ -irq_set_chained_handler (pp->msi_irq,NULL ); -irq_set_handler_data (pp->msi_irq,NULL ); +if (pp->msi_irq) { +irq_set_chained_handler (pp->msi_irq,NULL ); +irq_set_handler_data (pp->msi_irq,NULL ); + }irq_domain_remove (pp->msi_domain);irq_domain_remove (pp->irq_domain);
Sounds good.
PATCH v2 2/5 PCI: dwc: Free the page for MSI IRQ in dw_pcie_free_msi()
In reply to Jisheng ZhangTo avoid memory leak, we need to free the page for MSI in dw_pcie_free_msi().
--- drivers/pci/controller/dwc/pcie-designware-host.c | 3 +++ drivers/pci/controller/dwc/pcie-designware.h | 1 + 2 files changed, 4 insertions(+)
drivers/pci/controller/dwc/pcie-designware-host.c +3 -0
@@ -305,6 +305,8 @@ void dw_pcie_free_msi(struct pcie_port *pp)irq_domain_remove (pp->msi_domain);irq_domain_remove (pp->irq_domain); + +__free_page (pp->msi_page); }void dw_pcie_msi_init (struct pcie_port *pp)
@@ -322,6 +324,7 @@ void dw_pcie_msi_init(struct pcie_port *pp)return ; } msi_target = (u64 )pp->msi_data; + pp->msi_page = page;/* Program the msi_data */ dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4,
drivers/pci/controller/dwc/pcie-designware.h +1 -0
@@ -179,6 +179,7 @@ struct pcie_port {struct irq_domain *irq_domain;struct irq_domain *msi_domain;dma_addr_t msi_data; +struct page *msi_page;u32 num_vectors;u32 irq_mask[MAX_MSI_CTRLS ];raw_spinlock_t lock;
Signature · 1 line
2.20.1
Re: [PATCH v2 2/5] PCI: dwc: Free the page for MSI IRQ in dw_pcie_free_msi()
Hi,
Quoted 14 lines · Jisheng Zhang
To avoid memory leak, we need to free the page for MSI in
dw_pcie_free_msi().Signed-off-by Jisheng Zhang <[email protected]>--- drivers/pci/controller/dwc/pcie-designware-host.c | 3 +++ drivers/pci/controller/dwc/pcie-designware.h | 1 + 2 files changed, 4 insertions(+)drivers/pci/controller/dwc/pcie-designware-host.c +0
-0
drivers/pci/controller/dwc/pcie-designware-host.c void dw_pcie_free_msi(struct pcie_port *pp) · line 309
Quoted 5 lines
drivers/pci/controller/dwc/pcie-designware-host.c +2
-0@@ -305,6 +305,8 @@ void dw_pcie_free_msi(struct pcie_port *pp)irq_domain_remove (pp->msi_domain);irq_domain_remove (pp->irq_domain); + +__free_page (pp->msi_page);
Perhaps you could test is msi_page is null or not before the free.
drivers/pci/controller/dwc/pcie-designware-host.c void dw_pcie_msi_init(struct pcie_port *pp) · line 327
Quoted 8 lines
drivers/pci/controller/dwc/pcie-designware-host.c +1
-0}void dw_pcie_msi_init (struct pcie_port *pp)@@ -322,6 +324,7 @@ void dw_pcie_msi_init(struct pcie_port *pp)return ; } msi_target = (u64 )pp->msi_data; + pp->msi_page = page;
And maybe use the pp->msi_page variable from the beginning of the function, that way is one less variable.
Regards,
Gustavo
Quoted 16 lines
drivers/pci/controller/dwc/pcie-designware-host.c +0
-0/* Program the msi_data */ dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4,drivers/pci/controller/dwc/pcie-designware.h +1
-0@@ -179,6 +179,7 @@ struct pcie_port {struct irq_domain *irq_domain;struct irq_domain *msi_domain;dma_addr_t msi_data; +struct page *msi_page;u32 num_vectors;u32 irq_mask[MAX_MSI_CTRLS ];raw_spinlock_t lock;
PATCH v2 3/5 PCI: dwc: Free MSI in the error code path of dw_pcie_host_init()
In reply to Jisheng ZhangIf we ever did some msi related initializations, we need to call dw_pcie_free_msi() in the error code path.
--- drivers/pci/controller/dwc/pcie-designware-host.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/pci/controller/dwc/pcie-designware-host.c +5 -2
@@ -482,7 +482,7 @@ int dw_pcie_host_init(struct pcie_port *pp)if (pp->ops->host_init) { ret = pp->ops->host_init (pp);if (ret) - goto error; + goto err_free_msi; } pp->root_bus_nr = pp->busn->start;
@@ -496,7 +496,7 @@ int dw_pcie_host_init(struct pcie_port *pp) ret =pci_scan_root_bus_bridge (bridge);if (ret) - goto error; + goto err_free_msi; bus = bridge->bus;
@@ -512,6 +512,9 @@ int dw_pcie_host_init(struct pcie_port *pp)pci_bus_add_devices (bus);return 0 ; +err_free_msi: +if (IS_ENABLED (CONFIG_PCI_MSI ) && !pp->ops->msi_host_init) +dw_pcie_free_msi (pp); error:pci_free_host_bridge (bridge);return ret;
Signature · 1 line
2.20.1
Re: [PATCH v2 3/5] PCI: dwc: Free MSI in the error code path of dw_pcie_host_init()
Hi,
Quoted 31 lines · Jisheng Zhang
If we ever did some msi related initializations, we need to call
dw_pcie_free_msi()in the error code path.Signed-off-by Jisheng Zhang <[email protected]>--- drivers/pci/controller/dwc/pcie-designware-host.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)drivers/pci/controller/dwc/pcie-designware-host.c +2
-2@@ -482,7 +482,7 @@ int dw_pcie_host_init(struct pcie_port *pp)if (pp->ops->host_init) { ret = pp->ops->host_init (pp);if (ret) - goto error; + goto err_free_msi; } pp->root_bus_nr = pp->busn->start;@@ -496,7 +496,7 @@ int dw_pcie_host_init(struct pcie_port *pp) ret =pci_scan_root_bus_bridge (bridge);if (ret) - goto error; + goto err_free_msi; bus = bridge->bus;
drivers/pci/controller/dwc/pcie-designware-host.c int dw_pcie_host_init(struct pcie_port *pp) · line 516
Quoted 5 lines
drivers/pci/controller/dwc/pcie-designware-host.c +2
-0@@ -512,6 +512,9 @@ int dw_pcie_host_init(struct pcie_port *pp)pci_bus_add_devices (bus);return 0 ; +err_free_msi: +if (IS_ENABLED (CONFIG_PCI_MSI ) && !pp->ops->msi_host_init)
Look to Lucas Stach patch 3afc8299f39a ("PCI: dwc: skip MSI init if MSIs have been explicitly disabled")
You need to change this to:
if (IS_ENABLED(CONFIG_PCI_MSI) && pci_msi_enabled() && !pp->ops->msi_host_init)
drivers/pci/controller/dwc/pcie-designware-host.c int dw_pcie_host_init(struct pcie_port *pp) · line 520
Quoted 5 lines
drivers/pci/controller/dwc/pcie-designware-host.c +1
-0+dw_pcie_free_msi (pp); error:pci_free_host_bridge (bridge);return ret;
Sounds good, thanks.
PATCH v2 4/5 PCI: dwc: Use devm_pci_alloc_host_bridge() to simplify the code
In reply to Jisheng ZhangUse devm_pci_alloc_host_bridge() to simplify the error code path.
--- .../pci/controller/dwc/pcie-designware-host.c | 21 +++++++------------ 1 file changed, 8 insertions(+), 13 deletions(-)
drivers/pci/controller/dwc/pcie-designware-host.c +8 -13
@@ -357,7 +357,7 @@ int dw_pcie_host_init(struct pcie_port *pp)dev_err (dev,"Missing *config* reg space\n" ); } - bridge =pci_alloc_host_bridge (0 ); + bridge =devm_pci_alloc_host_bridge (dev,0 );if (!bridge)return -ENOMEM ;
@@ -368,7 +368,7 @@ int dw_pcie_host_init(struct pcie_port *pp) ret =devm_request_pci_bus_resources (dev, &bridge->windows);if (ret) - goto error; +return ret;/* Get the I/O and memory ranges from DT */ resource_list_for_each_entry_safe (win, tmp, &bridge->windows) {
@@ -412,8 +412,7 @@ int dw_pcie_host_init(struct pcie_port *pp) resource_size(pp->cfg));if (!pci->dbi_base) {dev_err (dev,"Error with ioremap\n" ); - ret = -ENOMEM ; - goto error; +return -ENOMEM ; } }
@@ -424,8 +423,7 @@ int dw_pcie_host_init(struct pcie_port *pp) pp->cfg0_base, pp->cfg0_size);if (!pp->va_cfg0_base) {dev_err (dev,"Error with ioremap in function\n" ); - ret = -ENOMEM ; - goto error; +return -ENOMEM ; } }
@@ -435,8 +433,7 @@ int dw_pcie_host_init(struct pcie_port *pp) pp->cfg1_size);if (!pp->va_cfg1_base) {dev_err (dev,"Error with ioremap\n" ); - ret = -ENOMEM ; - goto error; +return -ENOMEM ; } }
@@ -459,14 +456,14 @@ int dw_pcie_host_init(struct pcie_port *pp) pp->num_vectors == 0) {dev_err (dev,"Invalid number of vectors\n" ); - goto error; +return -EINVAL ; } }if (!pp->ops->msi_host_init) { ret =dw_pcie_allocate_domains (pp);if (ret) - goto error; +return ret;if (pp->msi_irq) irq_set_chained_handler_and_data(pp->msi_irq,
@@ -475,7 +472,7 @@ int dw_pcie_host_init(struct pcie_port *pp) }else { ret = pp->ops->msi_host_init (pp);if (ret <0 ) - goto error; +return ret; } }
@@ -515,8 +512,6 @@ int dw_pcie_host_init(struct pcie_port *pp) err_free_msi:if (IS_ENABLED (CONFIG_PCI_MSI ) && !pp->ops->msi_host_init)dw_pcie_free_msi (pp); -error: -pci_free_host_bridge (bridge);return ret; }
Signature · 1 line
2.20.1
Re: [PATCH v2 4/5] PCI: dwc: Use devm_pci_alloc_host_bridge() to simplify the code
Hi,
Quoted 87 lines · Jisheng Zhang
Use
devm_pci_alloc_host_bridge()to simplify the error code path.Signed-off-by Jisheng Zhang <[email protected]>--- .../pci/controller/dwc/pcie-designware-host.c | 21 +++++++------------ 1 file changed, 8 insertions(+), 13 deletions(-)drivers/pci/controller/dwc/pcie-designware-host.c +8
-11@@ -357,7 +357,7 @@ int dw_pcie_host_init(struct pcie_port *pp)dev_err (dev,"Missing *config* reg space\n" ); } - bridge =pci_alloc_host_bridge (0 ); + bridge =devm_pci_alloc_host_bridge (dev,0 );if (!bridge)return -ENOMEM ;@@ -368,7 +368,7 @@ int dw_pcie_host_init(struct pcie_port *pp) ret =devm_request_pci_bus_resources (dev, &bridge->windows);if (ret) - goto error; +return ret;/* Get the I/O and memory ranges from DT */ resource_list_for_each_entry_safe (win, tmp, &bridge->windows) {@@ -412,8 +412,7 @@ int dw_pcie_host_init(struct pcie_port *pp) resource_size(pp->cfg));if (!pci->dbi_base) {dev_err (dev,"Error with ioremap\n" ); - ret = -ENOMEM ; - goto error; +return -ENOMEM ; } }@@ -424,8 +423,7 @@ int dw_pcie_host_init(struct pcie_port *pp) pp->cfg0_base, pp->cfg0_size);if (!pp->va_cfg0_base) {dev_err (dev,"Error with ioremap in function\n" ); - ret = -ENOMEM ; - goto error; +return -ENOMEM ; } }@@ -435,8 +433,7 @@ int dw_pcie_host_init(struct pcie_port *pp) pp->cfg1_size);if (!pp->va_cfg1_base) {dev_err (dev,"Error with ioremap\n" ); - ret = -ENOMEM ; - goto error; +return -ENOMEM ; } }@@ -459,14 +456,14 @@ int dw_pcie_host_init(struct pcie_port *pp) pp->num_vectors == 0) {dev_err (dev,"Invalid number of vectors\n" ); - goto error; +return -EINVAL ; } }if (!pp->ops->msi_host_init) { ret =dw_pcie_allocate_domains (pp);if (ret) - goto error; +return ret;if (pp->msi_irq) irq_set_chained_handler_and_data(pp->msi_irq,@@ -475,7 +472,7 @@ int dw_pcie_host_init(struct pcie_port *pp) }else { ret = pp->ops->msi_host_init (pp);if (ret <0 ) - goto error; +return ret; } }
drivers/pci/controller/dwc/pcie-designware-host.c int dw_pcie_host_init(struct pcie_port *pp) · line 517
Quoted 8 lines
drivers/pci/controller/dwc/pcie-designware-host.c +0
-2@@ -515,8 +512,6 @@ int dw_pcie_host_init(struct pcie_port *pp) err_free_msi:if (IS_ENABLED (CONFIG_PCI_MSI ) && !pp->ops->msi_host_init)dw_pcie_free_msi (pp); -error: -pci_free_host_bridge (bridge);return ret; }
Nice!
PATCH v2 5/5 PCI: dwc: Save root bus for driver remove
In reply to Jisheng ZhangCurrently dwc host doesn't support the remove, but nothing prevent us from supporting it. Save the root bus for clean up work in driver remove code path.
After this patch, the dwc host users could implement its remove as:
static int foo_pcie_remove(struct platform_device *pdev)
{
...
pci_stop_root_bus(pp->root_bus);
pci_remove_root_bus(pp->root_bus);
dw_pcie_free_msi(pp);
...
}--- drivers/pci/controller/dwc/pcie-designware-host.c | 1 + drivers/pci/controller/dwc/pcie-designware.h | 1 + 2 files changed, 2 insertions(+)
drivers/pci/controller/dwc/pcie-designware-host.c +1 -0
@@ -496,6 +496,7 @@ int dw_pcie_host_init(struct pcie_port *pp) goto err_free_msi; bus = bridge->bus; + pp->root_bus = bus;if (pp->ops->scan_bus) pp->ops->scan_bus (pp);
drivers/pci/controller/dwc/pcie-designware.h +1 -0
@@ -182,6 +182,7 @@ struct pcie_port {struct page *msi_page;u32 num_vectors;u32 irq_mask[MAX_MSI_CTRLS ]; +struct pci_bus *root_bus;raw_spinlock_t lock;DECLARE_BITMAP (msi_irq_in_use,MAX_MSI_IRQS ); };
Signature · 1 line
2.20.1
Re: [PATCH v2 5/5] PCI: dwc: Save root bus for driver remove
Hi,
Quoted 26 lines · Jisheng Zhang
Currently dwc host doesn't support the remove, but nothing prevent us from supporting it. Save the root bus for clean up work in driver remove code path.
After this patch, the dwc host users could implement its remove as:
static int
foo_pcie_remove(struct platform_device *pdev){ ... pci_stop_root_bus(pp->root_bus); pci_remove_root_bus(pp->root_bus); dw_pcie_free_msi(pp); ... }Signed-off-by Jisheng Zhang <[email protected]>--- drivers/pci/controller/dwc/pcie-designware-host.c | 1 + drivers/pci/controller/dwc/pcie-designware.h | 1 + 2 files changed, 2 insertions(+)drivers/pci/controller/dwc/pcie-designware-host.c +0
-0
drivers/pci/controller/dwc/pcie-designware-host.c int dw_pcie_host_init(struct pcie_port *pp) · line 499
Quoted 4 lines
drivers/pci/controller/dwc/pcie-designware-host.c +1
-0@@ -496,6 +496,7 @@ int dw_pcie_host_init(struct pcie_port *pp) goto err_free_msi; bus = bridge->bus; + pp->root_bus = bus;
Why you don't use from the begging the pp->root_bus variable instead of bus variable? That way we can remove the bus variable.
Regards,
Gustavo
Quoted 16 lines
drivers/pci/controller/dwc/pcie-designware-host.c +0
-0if (pp->ops->scan_bus) pp->ops->scan_bus (pp);drivers/pci/controller/dwc/pcie-designware.h +1
-0@@ -182,6 +182,7 @@ struct pcie_port {struct page *msi_page;u32 num_vectors;u32 irq_mask[MAX_MSI_CTRLS ]; +struct pci_bus *root_bus;raw_spinlock_t lock;DECLARE_BITMAP (msi_irq_in_use,MAX_MSI_IRQS ); };