| Message ID | 692b4749f4267436363a5a8840140da8cd8858a1.1716190895.git.christophe.jaillet@wanadoo.fr |
|---|---|
| State | New |
| Headers | show |
| Series | Bluetooth: btintel_pcie: Fix the error handling path of btintel_pcie_probe() | expand |
On Mon, May 20, 2024 at 09:41:57AM +0200, Christophe JAILLET wrote: > Some resources freed in the remove function are not handled by the error > handling path of the probe. > > Add the needed function calls. > > Fixes: c2b636b3f788 ("Bluetooth: btintel_pcie: Add support for PCIe transport") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > Compile tested only. > Maybe incomplete. > --- > drivers/bluetooth/btintel_pcie.c | 20 ++++++++++++++------ > 1 file changed, 14 insertions(+), 6 deletions(-) > > diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c > index 5b6805d87fcf..d572576d0dbc 100644 > --- a/drivers/bluetooth/btintel_pcie.c > +++ b/drivers/bluetooth/btintel_pcie.c > @@ -1280,17 +1280,17 @@ static int btintel_pcie_probe(struct pci_dev *pdev, > > err = btintel_pcie_config_pcie(pdev, data); > if (err) > - goto exit_error; > + goto exit_destroy_worqueue; typo: workqueue [...] > bt_dev_dbg(data->hdev, "cnvi: 0x%8.8x cnvr: 0x%8.8x", data->cnvi, > data->cnvr); > return 0; > > -exit_error: > +exit_free_pcie: > + btintel_pcie_free(data); > + > +exit_free_irq_vectors: > + pci_free_irq_vectors(pdev); > + > +exit_destroy_worqueue: > + destroy_workqueue(data->workqueue); > + Please use an 'err_' prefix which is shorter and clearly indicates that these are error paths. I'd also drop the newlines. > /* reset device before exit */ > btintel_pcie_reset_bt(data); Johan
Hi Christophe, On Mon, May 20, 2024 at 3:42 AM Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > > Some resources freed in the remove function are not handled by the error > handling path of the probe. > > Add the needed function calls. > > Fixes: c2b636b3f788 ("Bluetooth: btintel_pcie: Add support for PCIe transport") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > Compile tested only. > Maybe incomplete. > --- > drivers/bluetooth/btintel_pcie.c | 20 ++++++++++++++------ > 1 file changed, 14 insertions(+), 6 deletions(-) > > diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c > index 5b6805d87fcf..d572576d0dbc 100644 > --- a/drivers/bluetooth/btintel_pcie.c > +++ b/drivers/bluetooth/btintel_pcie.c > @@ -1280,17 +1280,17 @@ static int btintel_pcie_probe(struct pci_dev *pdev, > > err = btintel_pcie_config_pcie(pdev, data); > if (err) > - goto exit_error; > + goto exit_destroy_worqueue; > > pci_set_drvdata(pdev, data); > > err = btintel_pcie_alloc(data); > if (err) > - goto exit_error; > + goto exit_free_irq_vectors; > > err = btintel_pcie_enable_bt(data); > if (err) > - goto exit_error; > + goto exit_free_pcie; > > /* CNV information (CNVi and CNVr) is in CSR */ > data->cnvi = btintel_pcie_rd_reg32(data, BTINTEL_PCIE_CSR_HW_REV_REG); > @@ -1299,17 +1299,25 @@ static int btintel_pcie_probe(struct pci_dev *pdev, > > err = btintel_pcie_start_rx(data); > if (err) > - goto exit_error; > + goto exit_free_pcie; > > err = btintel_pcie_setup_hdev(data); > if (err) > - goto exit_error; > + goto exit_free_pcie; > > bt_dev_dbg(data->hdev, "cnvi: 0x%8.8x cnvr: 0x%8.8x", data->cnvi, > data->cnvr); > return 0; > > -exit_error: > +exit_free_pcie: > + btintel_pcie_free(data); > + > +exit_free_irq_vectors: > + pci_free_irq_vectors(pdev); > + > +exit_destroy_worqueue: > + destroy_workqueue(data->workqueue); > + This looks a bit messy, perhaps we should really be calling btintel_pcie_remove instead and adapt it to check if a field has been initialized or not then proceed to free/cleanup/etc. > /* reset device before exit */ > btintel_pcie_reset_bt(data); > > -- > 2.45.1 >
Le 24/05/2024 à 21:39, Luiz Augusto von Dentz a écrit : > Hi Christophe, > > On Mon, May 20, 2024 at 3:42 AM Christophe JAILLET > <christophe.jaillet@wanadoo.fr> wrote: >> >> Some resources freed in the remove function are not handled by the error >> handling path of the probe. >> >> Add the needed function calls. >> >> Fixes: c2b636b3f788 ("Bluetooth: btintel_pcie: Add support for PCIe transport") >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >> --- >> Compile tested only. >> Maybe incomplete. >> --- >> drivers/bluetooth/btintel_pcie.c | 20 ++++++++++++++------ >> 1 file changed, 14 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c >> index 5b6805d87fcf..d572576d0dbc 100644 >> --- a/drivers/bluetooth/btintel_pcie.c >> +++ b/drivers/bluetooth/btintel_pcie.c >> @@ -1280,17 +1280,17 @@ static int btintel_pcie_probe(struct pci_dev *pdev, >> >> err = btintel_pcie_config_pcie(pdev, data); >> if (err) >> - goto exit_error; >> + goto exit_destroy_worqueue; >> >> pci_set_drvdata(pdev, data); >> >> err = btintel_pcie_alloc(data); >> if (err) >> - goto exit_error; >> + goto exit_free_irq_vectors; >> >> err = btintel_pcie_enable_bt(data); >> if (err) >> - goto exit_error; >> + goto exit_free_pcie; >> >> /* CNV information (CNVi and CNVr) is in CSR */ >> data->cnvi = btintel_pcie_rd_reg32(data, BTINTEL_PCIE_CSR_HW_REV_REG); >> @@ -1299,17 +1299,25 @@ static int btintel_pcie_probe(struct pci_dev *pdev, >> >> err = btintel_pcie_start_rx(data); >> if (err) >> - goto exit_error; >> + goto exit_free_pcie; >> >> err = btintel_pcie_setup_hdev(data); >> if (err) >> - goto exit_error; >> + goto exit_free_pcie; >> >> bt_dev_dbg(data->hdev, "cnvi: 0x%8.8x cnvr: 0x%8.8x", data->cnvi, >> data->cnvr); >> return 0; >> >> -exit_error: >> +exit_free_pcie: >> + btintel_pcie_free(data); >> + >> +exit_free_irq_vectors: >> + pci_free_irq_vectors(pdev); >> + >> +exit_destroy_worqueue: >> + destroy_workqueue(data->workqueue); >> + > > This looks a bit messy, perhaps we should really be calling > btintel_pcie_remove instead and adapt it to check if a field has been > initialized or not then proceed to free/cleanup/etc. > Not sure it would be that easy / readable. It would look like something like: static void btintel_pcie_remove(struct pci_dev *pdev) { struct btintel_pcie_data *data; data = pci_get_drvdata(pdev); btintel_pcie_reset_bt(data); for (int i = 0; i < data->alloc_vecs; i++) { struct msix_entry *msix_entry; msix_entry = &data->msix_entries[i]; free_irq(msix_entry->vector, msix_entry); } if (data->alloc_vecs) pci_free_irq_vectors(pdev); btintel_pcie_release_hdev(data); flush_work(&data->rx_work); if (data->workqueue) destroy_workqueue(data->workqueue); if (data->dma_pool) btintel_pcie_free(data); pci_clear_master(pdev); pci_set_drvdata(pdev, NULL); } The added tests don't always look related to the function call just after it : - data->alloc_vecs vs pci_free_irq_vectors(), ok why not - data->dma_pool vs btintel_pcie_free() does not look that really obvious. There is also another issue in the remove function. We call free_irq() on irq allocated with devm_request_threaded_irq(). I'll try to see if more managed resources usage and/or some devm_add_action_or_reset() could help. CJ >> /* reset device before exit */ >> btintel_pcie_reset_bt(data); >> >> -- >> 2.45.1 >> > >
diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c index 5b6805d87fcf..d572576d0dbc 100644 --- a/drivers/bluetooth/btintel_pcie.c +++ b/drivers/bluetooth/btintel_pcie.c @@ -1280,17 +1280,17 @@ static int btintel_pcie_probe(struct pci_dev *pdev, err = btintel_pcie_config_pcie(pdev, data); if (err) - goto exit_error; + goto exit_destroy_worqueue; pci_set_drvdata(pdev, data); err = btintel_pcie_alloc(data); if (err) - goto exit_error; + goto exit_free_irq_vectors; err = btintel_pcie_enable_bt(data); if (err) - goto exit_error; + goto exit_free_pcie; /* CNV information (CNVi and CNVr) is in CSR */ data->cnvi = btintel_pcie_rd_reg32(data, BTINTEL_PCIE_CSR_HW_REV_REG); @@ -1299,17 +1299,25 @@ static int btintel_pcie_probe(struct pci_dev *pdev, err = btintel_pcie_start_rx(data); if (err) - goto exit_error; + goto exit_free_pcie; err = btintel_pcie_setup_hdev(data); if (err) - goto exit_error; + goto exit_free_pcie; bt_dev_dbg(data->hdev, "cnvi: 0x%8.8x cnvr: 0x%8.8x", data->cnvi, data->cnvr); return 0; -exit_error: +exit_free_pcie: + btintel_pcie_free(data); + +exit_free_irq_vectors: + pci_free_irq_vectors(pdev); + +exit_destroy_worqueue: + destroy_workqueue(data->workqueue); + /* reset device before exit */ btintel_pcie_reset_bt(data);
Some resources freed in the remove function are not handled by the error handling path of the probe. Add the needed function calls. Fixes: c2b636b3f788 ("Bluetooth: btintel_pcie: Add support for PCIe transport") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- Compile tested only. Maybe incomplete. --- drivers/bluetooth/btintel_pcie.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)