| Message ID | 20240115144655.32046-11-pstanner@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Make PCI's devres API more consistent | expand |
Mon, Jan 15, 2024 at 03:46:20PM +0100, Philipp Stanner kirjoitti: > Thanks to preceding cleanup steps, pcim_release() is now not needed > anymore and can be replaced by pcim_disable_device(), which is the exact > counterpart to pcim_enable_device(). > This permits removing further parts of the old devres API. > > Replace pcim_release() with pcim_disable_device(). > Remove the now surplus get_dr() function. ... > + devm_add_action(&pdev->dev, pcim_disable_device, pdev); No error check? > + return pci_enable_device(pdev); Maybe ret = pci_enable_device(...); if (ret) return ret; return devm_add_action_or_reset(...)? I could think of side effects of this, so perhaps the commit message and/or code needs a comment on why the above proposal can _not_ be used?
On Tue, 2024-01-16 at 23:40 +0200, andy.shevchenko@gmail.com wrote: > Mon, Jan 15, 2024 at 03:46:20PM +0100, Philipp Stanner kirjoitti: > > Thanks to preceding cleanup steps, pcim_release() is now not needed > > anymore and can be replaced by pcim_disable_device(), which is the > > exact > > counterpart to pcim_enable_device(). > > This permits removing further parts of the old devres API. > > > > Replace pcim_release() with pcim_disable_device(). > > Remove the now surplus get_dr() function. > > ... > > > + devm_add_action(&pdev->dev, pcim_disable_device, pdev); > > No error check? > > > + return pci_enable_device(pdev); > > Maybe > > ret = pci_enable_device(...); > if (ret) > return ret; > > return devm_add_action_or_reset(...)? > > I could think of side effects of this, so perhaps the commit message > and/or > code needs a comment on why the above proposal can _not_ be used? > That proposal can be used, so this was simply a bug. P.
diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c index 7c4edcaeb8fe..87bc62be21eb 100644 --- a/drivers/pci/devres.c +++ b/drivers/pci/devres.c @@ -464,48 +464,26 @@ int pcim_intx(struct pci_dev *pdev, int enable) } EXPORT_SYMBOL_GPL(pcim_intx); -static void pcim_release(struct device *gendev, void *res) +static void pcim_disable_device(void *pdev_raw) { - struct pci_dev *dev = to_pci_dev(gendev); - - if (!dev->pinned) - pci_disable_device(dev); -} - -static struct pci_devres *get_pci_dr(struct pci_dev *pdev) -{ - struct pci_devres *dr, *new_dr; - - dr = devres_find(&pdev->dev, pcim_release, NULL, NULL); - if (dr) - return dr; + struct pci_dev *pdev = pdev_raw; - new_dr = devres_alloc(pcim_release, sizeof(*new_dr), GFP_KERNEL); - if (!new_dr) - return NULL; - return devres_get(&pdev->dev, new_dr, NULL, NULL); + if (!pdev->pinned) + pci_disable_device(pdev); } /** * pcim_enable_device - Managed pci_enable_device() * @pdev: PCI device to be initialized * - * Managed pci_enable_device(). + * Managed pci_enable_device(). Device will automatically be disabled on + * driver detach. */ int pcim_enable_device(struct pci_dev *pdev) { - struct pci_devres *dr; - int rc; - - dr = get_pci_dr(pdev); - if (unlikely(!dr)) - return -ENOMEM; - - rc = pci_enable_device(pdev); - if (!rc) - pdev->is_managed = 1; + devm_add_action(&pdev->dev, pcim_disable_device, pdev); - return rc; + return pci_enable_device(pdev); } EXPORT_SYMBOL(pcim_enable_device);
Thanks to preceding cleanup steps, pcim_release() is now not needed anymore and can be replaced by pcim_disable_device(), which is the exact counterpart to pcim_enable_device(). This permits removing further parts of the old devres API. Replace pcim_release() with pcim_disable_device(). Remove the now surplus get_dr() function. Signed-off-by: Philipp Stanner <pstanner@redhat.com> --- drivers/pci/devres.c | 38 ++++++++------------------------------ 1 file changed, 8 insertions(+), 30 deletions(-)