| Message ID | 20250215054431.55747-8-xueshuai@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | dmaengine: idxd: fix memory leak in error handling path | expand |
Hi, Shuai, On 2/14/25 21:44, Shuai Xue wrote: > The idxd_cleanup() helper clean up perfmon, interrupts, internals and so s/clean/cleans/ > on. Refactor remove call with idxd_cleanup() helper to avoid code s/idxd_cleanup()/the idxd_cleanup()/ > duplication. Note, this also fixes the missing put_device() for idxd > groups, enginces and wqs. > > Fixes: bfe1d56091c1 ("dmaengine: idxd: Init and probe for Intel data accelerators") > Suggested-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> > Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com> > --- > drivers/dma/idxd/init.c | 13 ++----------- > 1 file changed, 2 insertions(+), 11 deletions(-) > > diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c > index f40f1c44a302..0fbfbe024c29 100644 > --- a/drivers/dma/idxd/init.c > +++ b/drivers/dma/idxd/init.c > @@ -1282,20 +1282,11 @@ static void idxd_remove(struct pci_dev *pdev) > get_device(idxd_confdev(idxd)); get_device() is called here. > device_unregister(idxd_confdev(idxd)); > idxd_shutdown(pdev); > - if (device_pasid_enabled(idxd)) > - idxd_disable_system_pasid(idxd); > idxd_device_remove_debugfs(idxd); > - > - irq_entry = idxd_get_ie(idxd, 0); > - free_irq(irq_entry->vector, irq_entry); > - pci_free_irq_vectors(pdev); > + idxd_cleanup(idxd); > pci_iounmap(pdev, idxd->reg_base); > - if (device_user_pasid_enabled(idxd)) > - idxd_disable_sva(pdev); > - pci_disable_device(pdev); > - destroy_workqueue(idxd->wq); > - perfmon_pmu_remove(idxd); > idxd_free(idxd); put_device() is called inside idxd_free(). Seems not easy to read code to match the pair. Plus idxd_free() is called only in non FLR case. Maybe it's better to change this code to: 1. call put_device() outside idxd_free() so that it's easy to match the get_device() and put_deivce in the same level of function. 2. idxd_free() called here is OK because this is not in FLR handler. But only call it in non FLR path in idxd_pci_probe_alloc() ? > + pci_disable_device(pdev); > } > > static struct pci_driver idxd_pci_driver = { Thanks. -Fenghua
在 2025/2/19 05:01, Fenghua Yu 写道: > Hi, Shuai, > > On 2/14/25 21:44, Shuai Xue wrote: >> The idxd_cleanup() helper clean up perfmon, interrupts, internals and so > > s/clean/cleans/ > > >> on. Refactor remove call with idxd_cleanup() helper to avoid code > s/idxd_cleanup()/the idxd_cleanup()/ >> duplication. Note, this also fixes the missing put_device() for idxd >> groups, enginces and wqs. >> >> Fixes: bfe1d56091c1 ("dmaengine: idxd: Init and probe for Intel data accelerators") >> Suggested-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> >> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com> >> --- >> drivers/dma/idxd/init.c | 13 ++----------- >> 1 file changed, 2 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c >> index f40f1c44a302..0fbfbe024c29 100644 >> --- a/drivers/dma/idxd/init.c >> +++ b/drivers/dma/idxd/init.c >> @@ -1282,20 +1282,11 @@ static void idxd_remove(struct pci_dev *pdev) >> get_device(idxd_confdev(idxd)); > get_device() is called here. >> device_unregister(idxd_confdev(idxd)); >> idxd_shutdown(pdev); >> - if (device_pasid_enabled(idxd)) >> - idxd_disable_system_pasid(idxd); >> idxd_device_remove_debugfs(idxd); >> - >> - irq_entry = idxd_get_ie(idxd, 0); >> - free_irq(irq_entry->vector, irq_entry); >> - pci_free_irq_vectors(pdev); >> + idxd_cleanup(idxd); >> pci_iounmap(pdev, idxd->reg_base); >> - if (device_user_pasid_enabled(idxd)) >> - idxd_disable_sva(pdev); >> - pci_disable_device(pdev); >> - destroy_workqueue(idxd->wq); >> - perfmon_pmu_remove(idxd); >> idxd_free(idxd); > > put_device() is called inside idxd_free(). Seems not easy to read code to match the pair. IMHO, idxd_free() is paired with idxd_alloc() which grap a reference count by device_initialize(). So, we should match that right pair. > * When ->release() is called for the idxd->conf_dev, it frees all the memory related > * to the idxd context. I did not figure out why you explictly grab reference count of idxd_confdev(idxd). idxd_unregister_devices() is paired with idxd_register_devices(), it only decrease reference through wqs, engines, and groups. So a refcnt of idxd->conf_dev is still hold by idxd_alloc(). Please correct me, if I missed anything. > > Plus idxd_free() is called only in non FLR case. > > Maybe it's better to change this code to: > > 1. call put_device() outside idxd_free() so that it's easy to match the get_device() and put_deivce in the same level of function. See my comments above. > > 2. idxd_free() called here is OK because this is not in FLR handler. But only call it in non FLR path in idxd_pci_probe_alloc() > Exactly, so, shoud I add a protection in idxd_free() in a fact that non FLR case will not call it. Thanks. Shuai
diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c index f40f1c44a302..0fbfbe024c29 100644 --- a/drivers/dma/idxd/init.c +++ b/drivers/dma/idxd/init.c @@ -1282,20 +1282,11 @@ static void idxd_remove(struct pci_dev *pdev) get_device(idxd_confdev(idxd)); device_unregister(idxd_confdev(idxd)); idxd_shutdown(pdev); - if (device_pasid_enabled(idxd)) - idxd_disable_system_pasid(idxd); idxd_device_remove_debugfs(idxd); - - irq_entry = idxd_get_ie(idxd, 0); - free_irq(irq_entry->vector, irq_entry); - pci_free_irq_vectors(pdev); + idxd_cleanup(idxd); pci_iounmap(pdev, idxd->reg_base); - if (device_user_pasid_enabled(idxd)) - idxd_disable_sva(pdev); - pci_disable_device(pdev); - destroy_workqueue(idxd->wq); - perfmon_pmu_remove(idxd); idxd_free(idxd); + pci_disable_device(pdev); } static struct pci_driver idxd_pci_driver = {
The idxd_cleanup() helper clean up perfmon, interrupts, internals and so on. Refactor remove call with idxd_cleanup() helper to avoid code duplication. Note, this also fixes the missing put_device() for idxd groups, enginces and wqs. Fixes: bfe1d56091c1 ("dmaengine: idxd: Init and probe for Intel data accelerators") Suggested-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com> --- drivers/dma/idxd/init.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)