| Message ID | 20240819165148.58201-9-pstanner@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Remove pcim_iounmap_regions() | expand |
On Mon, Aug 19, 2024 at 06:51:47PM +0200, Philipp Stanner wrote: > stmicro uses PCI devres in the wrong way. Resources requested > through pcim_* functions don't need to be cleaned up manually in the > remove() callback or in the error unwind path of a probe() function. > > Moreover, there is an unnecessary loop which only requests and ioremaps > BAR 0, but iterates over all BARs nevertheless. > > Furthermore, pcim_iomap_regions() and pcim_iomap_table() have been > deprecated by the PCI subsystem in commit e354bb84a4c1 ("PCI: Deprecate > pcim_iomap_table(), pcim_iomap_regions_request_all()"). > > Replace these functions with pcim_iomap_region(). > > Remove the unnecessary manual pcim_* cleanup calls. > > Remove the unnecessary loop over all BARs. ... loongson_dwmac_probe() > + memset(&res, 0, sizeof(res)); > + res.addr = pcim_iomap_region(pdev, 0, pci_name(pdev)); > + if (IS_ERR(res.addr)) { > + ret = PTR_ERR(res.addr); > + goto err_disable_device; It seems your series reveals issues in the error paths of .probe():s in many drivers... If we use pcim variant to enable device, why do we need to explicitly disable it? > } ... loongson_dwmac_remove() > pci_disable_msi(pdev); > pci_disable_device(pdev); Not sure why we need these either...
On Mon, 2024-08-19 at 21:28 +0300, Andy Shevchenko wrote: > On Mon, Aug 19, 2024 at 06:51:47PM +0200, Philipp Stanner wrote: > > stmicro uses PCI devres in the wrong way. Resources requested > > through pcim_* functions don't need to be cleaned up manually in > > the > > remove() callback or in the error unwind path of a probe() > > function. > > > > Moreover, there is an unnecessary loop which only requests and > > ioremaps > > BAR 0, but iterates over all BARs nevertheless. > > > > Furthermore, pcim_iomap_regions() and pcim_iomap_table() have been > > deprecated by the PCI subsystem in commit e354bb84a4c1 ("PCI: > > Deprecate > > pcim_iomap_table(), pcim_iomap_regions_request_all()"). > > > > Replace these functions with pcim_iomap_region(). > > > > Remove the unnecessary manual pcim_* cleanup calls. > > > > Remove the unnecessary loop over all BARs. > > ... > > loongson_dwmac_probe() > > > + memset(&res, 0, sizeof(res)); > > + res.addr = pcim_iomap_region(pdev, 0, pci_name(pdev)); > > + if (IS_ERR(res.addr)) { > > + ret = PTR_ERR(res.addr); > > + goto err_disable_device; > > It seems your series reveals issues in the error paths of .probe():s > in many drivers... > > If we use pcim variant to enable device, why do we need to explicitly > disable it? No. > > > } > > ... > > loongson_dwmac_remove() > > > pci_disable_msi(pdev); > > pci_disable_device(pdev); > > Not sure why we need these either... It's complicated. The code uses pciM_enable_device(), but here in remove pci_disable_device(). pcim_enable_device() sets up a disable callback which only calls pci_disable_device() if pcim_pin_device() has not been called. This code doesn't seem to call pcim_pin_device(), so I think pci_disable_device() could be removed. I definitely would not feel confident touching pci_disable_msi(), though. The AFAIK biggest problem remaining in PCI devres is that the MSI code base implicitly calls into devres, see here [1] P. [1] https://lore.kernel.org/all/ee44ea7ac760e73edad3f20b30b4d2fff66c1a85.camel@redhat.com/
On Tue, Aug 20, 2024 at 09:52:40AM +0200, Philipp Stanner wrote: > On Mon, 2024-08-19 at 21:28 +0300, Andy Shevchenko wrote: > > On Mon, Aug 19, 2024 at 06:51:47PM +0200, Philipp Stanner wrote: ... > > loongson_dwmac_probe() > > > > > + memset(&res, 0, sizeof(res)); > > > + res.addr = pcim_iomap_region(pdev, 0, pci_name(pdev)); > > > + if (IS_ERR(res.addr)) { > > > + ret = PTR_ERR(res.addr); > > > + goto err_disable_device; > > > > It seems your series reveals issues in the error paths of .probe():s > > in many drivers... > > > > If we use pcim variant to enable device, why do we need to explicitly > > disable it? > > No. Can you elaborate? No issues being revealed, or no need to disable it explicitly, or...? > > > } ... > > loongson_dwmac_remove() > > > > > pci_disable_msi(pdev); > > > pci_disable_device(pdev); > > > > Not sure why we need these either... > > It's complicated. > > The code uses pciM_enable_device(), but here in remove > pci_disable_device(). > > pcim_enable_device() sets up a disable callback which only calls > pci_disable_device() if pcim_pin_device() has not been called. > > This code doesn't seem to call pcim_pin_device(), so I think > pci_disable_device() could be removed. > > > I definitely would not feel confident touching pci_disable_msi(), > though. The AFAIK biggest problem remaining in PCI devres is that the > MSI code base implicitly calls into devres, see here [1] But isn't it a busyness of PCI core to call pci_disable_msi() at the right moment? Okay, I admit that there might be devices that require a special workflow WRT MSI, is this the case here? > [1] https://lore.kernel.org/all/ee44ea7ac760e73edad3f20b30b4d2fff66c1a85.camel@redhat.com/
On Tue, 2024-08-20 at 13:37 +0300, Andy Shevchenko wrote: > On Tue, Aug 20, 2024 at 09:52:40AM +0200, Philipp Stanner wrote: > > On Mon, 2024-08-19 at 21:28 +0300, Andy Shevchenko wrote: > > > On Mon, Aug 19, 2024 at 06:51:47PM +0200, Philipp Stanner wrote: > > ... > > > > loongson_dwmac_probe() > > > > > > > + memset(&res, 0, sizeof(res)); > > > > + res.addr = pcim_iomap_region(pdev, 0, pci_name(pdev)); > > > > + if (IS_ERR(res.addr)) { > > > > + ret = PTR_ERR(res.addr); > > > > + goto err_disable_device; > > > > > > It seems your series reveals issues in the error paths of > > > .probe():s > > > in many drivers... > > > > > > If we use pcim variant to enable device, why do we need to > > > explicitly > > > disable it? > > > > No. > > Can you elaborate? No issues being revealed, or no need to disable it > explicitly, or...? Oh, my bad, I overlooked your "why" in that question. We do not explicitly have to disable it. It's wrong / unnecessary, as many of the other calls you criticized in this series. pcim_enable_device() (in pci/devres.c) calls devm_add_action(..., pcim_disable_device, ...), which will disable the device on driver detach. So the call of pci_disable_device() above is redundant. We could remove it. > > > > > } > > ... > > > > loongson_dwmac_remove() > > > > > > > pci_disable_msi(pdev); > > > > pci_disable_device(pdev); > > > > > > Not sure why we need these either... > > > > It's complicated. > > > > The code uses pciM_enable_device(), but here in remove > > pci_disable_device(). > > > > pcim_enable_device() sets up a disable callback which only calls > > pci_disable_device() if pcim_pin_device() has not been called. > > > > This code doesn't seem to call pcim_pin_device(), so I think > > pci_disable_device() could be removed. > > > > > > I definitely would not feel confident touching pci_disable_msi(), > > though. The AFAIK biggest problem remaining in PCI devres is that > > the > > MSI code base implicitly calls into devres, see here [1] > > But isn't it a busyness of PCI core to call pci_disable_msi() at the > right > moment? Okay, I admit that there might be devices that require a > special > workflow WRT MSI, is this the case here? I don't know enough about how MSI is intended to be used. From what I've seen in the code base, pcim_setup_msi_release() does register a devres callback that will indeed call pci_disable_msi() after some intermediate calls. But in my honest opinion, that code is _very_ broken. I was thinking about how we might clean it up, but couldn't come up with an idea yet. Only after the code in pci/msi/ has been cleanly separated from implicit devres I myself would start touching function calls related to MSI. That being said, I suspect that one can remove pci_disable_msi() in the line above. But the risk-benefit-ratio doesn't pay off for me. P. > > > [1] > > https://lore.kernel.org/all/ee44ea7ac760e73edad3f20b30b4d2fff66c1a85.camel@redhat.com/ >
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c index 9e40c28d453a..5d42a9fad672 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c @@ -50,7 +50,7 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id struct plat_stmmacenet_data *plat; struct stmmac_resources res; struct device_node *np; - int ret, i, phy_mode; + int ret, phy_mode; np = dev_of_node(&pdev->dev); @@ -88,14 +88,11 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id goto err_put_node; } - /* Get the base address of device */ - for (i = 0; i < PCI_STD_NUM_BARS; i++) { - if (pci_resource_len(pdev, i) == 0) - continue; - ret = pcim_iomap_regions(pdev, BIT(0), pci_name(pdev)); - if (ret) - goto err_disable_device; - break; + memset(&res, 0, sizeof(res)); + res.addr = pcim_iomap_region(pdev, 0, pci_name(pdev)); + if (IS_ERR(res.addr)) { + ret = PTR_ERR(res.addr); + goto err_disable_device; } plat->bus_id = of_alias_get_id(np, "ethernet"); @@ -116,8 +113,6 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id loongson_default_data(plat); pci_enable_msi(pdev); - memset(&res, 0, sizeof(res)); - res.addr = pcim_iomap_table(pdev)[0]; res.irq = of_irq_get_byname(np, "macirq"); if (res.irq < 0) { @@ -158,18 +153,10 @@ static void loongson_dwmac_remove(struct pci_dev *pdev) { struct net_device *ndev = dev_get_drvdata(&pdev->dev); struct stmmac_priv *priv = netdev_priv(ndev); - int i; of_node_put(priv->plat->mdio_node); stmmac_dvr_remove(&pdev->dev); - for (i = 0; i < PCI_STD_NUM_BARS; i++) { - if (pci_resource_len(pdev, i) == 0) - continue; - pcim_iounmap_regions(pdev, BIT(i)); - break; - } - pci_disable_msi(pdev); pci_disable_device(pdev); } diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c index 352b01678c22..f89a8a54c4e8 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c @@ -188,11 +188,11 @@ static int stmmac_pci_probe(struct pci_dev *pdev, return ret; } - /* Get the base address of device */ + /* Request the base address BAR of device */ for (i = 0; i < PCI_STD_NUM_BARS; i++) { if (pci_resource_len(pdev, i) == 0) continue; - ret = pcim_iomap_regions(pdev, BIT(i), pci_name(pdev)); + ret = pcim_request_region(pdev, i, pci_name(pdev)); if (ret) return ret; break; @@ -205,7 +205,10 @@ static int stmmac_pci_probe(struct pci_dev *pdev, return ret; memset(&res, 0, sizeof(res)); - res.addr = pcim_iomap_table(pdev)[i]; + /* Get the base address of device */ + res.addr = pcim_iomap(pdev, i, 0); + if (!res.addr) + return -ENOMEM; res.wol_irq = pdev->irq; res.irq = pdev->irq; @@ -231,16 +234,7 @@ static int stmmac_pci_probe(struct pci_dev *pdev, */ static void stmmac_pci_remove(struct pci_dev *pdev) { - int i; - stmmac_dvr_remove(&pdev->dev); - - for (i = 0; i < PCI_STD_NUM_BARS; i++) { - if (pci_resource_len(pdev, i) == 0) - continue; - pcim_iounmap_regions(pdev, BIT(i)); - break; - } } static int __maybe_unused stmmac_pci_suspend(struct device *dev)
stmicro uses PCI devres in the wrong way. Resources requested through pcim_* functions don't need to be cleaned up manually in the remove() callback or in the error unwind path of a probe() function. Moreover, there is an unnecessary loop which only requests and ioremaps BAR 0, but iterates over all BARs nevertheless. Furthermore, pcim_iomap_regions() and pcim_iomap_table() have been deprecated by the PCI subsystem in commit e354bb84a4c1 ("PCI: Deprecate pcim_iomap_table(), pcim_iomap_regions_request_all()"). Replace these functions with pcim_iomap_region(). Remove the unnecessary manual pcim_* cleanup calls. Remove the unnecessary loop over all BARs. Signed-off-by: Philipp Stanner <pstanner@redhat.com> --- .../ethernet/stmicro/stmmac/dwmac-loongson.c | 25 +++++-------------- .../net/ethernet/stmicro/stmmac/stmmac_pci.c | 18 +++++-------- 2 files changed, 12 insertions(+), 31 deletions(-)