Bluetooth: btintel_pcie: Fix the error handling path of btintel_pcie_probe()

Commit Message

Christophe JAILLET May 20, 2024, 7:41 a.m. UTC

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 <[e-mail]>
---
Compile tested only.
Maybe incomplete.
---
 drivers/bluetooth/btintel_pcie.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

Comments

[e-mail] May 20, 2024, 8:35 a.m. UTC | #1

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=854340

---Test result---

Test Summary:
CheckPatch                    PASS      0.61 seconds
GitLint                       PASS      0.48 seconds
SubjectPrefix                 PASS      0.11 seconds
BuildKernel                   PASS      29.78 seconds
CheckAllWarning               PASS      32.49 seconds
CheckSparse                   PASS      37.82 seconds
CheckSmatch                   FAIL      34.83 seconds
BuildKernel32                 PASS      28.79 seconds
TestRunnerSetup               PASS      517.74 seconds
TestRunner_l2cap-tester       PASS      18.11 seconds
TestRunner_iso-tester         PASS      28.22 seconds
TestRunner_bnep-tester        PASS      4.73 seconds
TestRunner_mgmt-tester        PASS      111.80 seconds
TestRunner_rfcomm-tester      PASS      7.30 seconds
TestRunner_sco-tester         PASS      14.84 seconds
TestRunner_ioctl-tester       PASS      10.97 seconds
TestRunner_mesh-tester        PASS      5.82 seconds
TestRunner_smp-tester         PASS      6.83 seconds
TestRunner_userchan-tester    PASS      4.95 seconds
IncrementalBuild              PASS      28.29 seconds

Details
##############################
Test: CheckSmatch - FAIL
Desc: Run smatch tool with source
Output:

Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: net/bluetooth/hci_core.o] Error 139
make[4]: *** Deleting file 'net/bluetooth/hci_core.o'
make[3]: *** [scripts/Makefile.build:485: net/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: net] Error 2
make[2]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bcm203x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bcm203x.o'
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [scripts/Makefile.build:485: drivers/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: drivers] Error 2
make[1]: *** [/github/workspace/src/src/Makefile:1919: .] Error 2
make: *** [Makefile:240: __sub-make] Error 2


---
Regards,
Linux Bluetooth

Johan Hovold May 20, 2024, 11:02 a.m. UTC | #2

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 <[e-mail]>
> ---
> 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

Luiz Augusto von Dentz May 24, 2024, 7:39 p.m. UTC | #3

Hi Christophe,

On Mon, May 20, 2024 at 3:42 AM Christophe JAILLET
<[e-mail]> 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 <[e-mail]>
> ---
> 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
>

Christophe JAILLET May 26, 2024, 10:38 a.m. UTC | #4

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
> <[e-mail]> 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 <[e-mail]>
>> ---
>> 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
>>
> 
>

Patch

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);