[PATCH 1/5] Bluetooth: hci_bcm: Add missing acpi_dev_free_resource_list()

[PATCH 1/5] Bluetooth: hci_bcm: Add missing acpi_dev_free_resource_list()

[Jarkko Nikula]

Caller of acpi_dev_get_resources() should free the constructed resource
list by calling the acpi_dev_free_resource_list() in order to avoid memory
leak.

Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
Call to acpi_dev_get_resources() was introduced by the commit ae056908862b
("Bluetooth: hci_bcm: Retrieve UART speed from ACPI") so fix should go to
v4.3-rc.
---
 drivers/bluetooth/hci_bcm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 0c791ac279d0..1a538ad6bf2b 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -706,6 +706,7 @@ static int bcm_acpi_probe(struct bcm_device *dev)
 		return 0;
 
 	acpi_dev_get_resources(adev, &resources, bcm_resource, dev);
+	acpi_dev_free_resource_list(&resources);
 
 	dmi_id = dmi_first_match(bcm_wrong_irq_dmi_table);
 	if (dmi_id) {
-- 
2.5.3

[PATCH 2/5] Bluetooth: hci_bcm: Handle possible error from acpi_dev_get_resources()

[Jarkko Nikula]

Driver doesn't handle possible error from acpi_dev_get_resources(). Test it
and return the error code in case of error.

Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/bluetooth/hci_bcm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 1a538ad6bf2b..5375c9c04fda 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -705,7 +705,9 @@ static int bcm_acpi_probe(struct bcm_device *dev)
 	if (!adev)
 		return 0;
 
-	acpi_dev_get_resources(adev, &resources, bcm_resource, dev);
+	ret = acpi_dev_get_resources(adev, &resources, bcm_resource, dev);
+	if (ret < 0)
+		return ret;
 	acpi_dev_free_resource_list(&resources);
 
 	dmi_id = dmi_first_match(bcm_wrong_irq_dmi_table);
-- 
2.5.3

[PATCH 3/5] Bluetooth: hci_bcm: Remove needless acpi_match_device() call

[Jarkko Nikula]

There is no need to call acpi_match_device() in driver's probe path and
verify does it find a match to given ACPI _HIDs in .acpi_match_table as
driver/platform/acpi core code has found the match prior calling the probe.

Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/bluetooth/hci_bcm.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 5375c9c04fda..a1b9bbcbcb79 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -647,16 +647,11 @@ static int bcm_resource(struct acpi_resource *ares, void *data)
 static int bcm_acpi_probe(struct bcm_device *dev)
 {
 	struct platform_device *pdev = dev->pdev;
-	const struct acpi_device_id *id;
 	struct acpi_device *adev;
 	LIST_HEAD(resources);
 	const struct dmi_system_id *dmi_id;
 	int ret;
 
-	id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
-	if (!id)
-		return -ENODEV;
-
 	/* Retrieve GPIO data */
 	dev->name = dev_name(&pdev->dev);
 	ret = acpi_dev_add_driver_gpios(ACPI_COMPANION(&pdev->dev),
-- 
2.5.3

[PATCH 4/5] Bluetooth: hci_bcm: Remove needless looking code

[Jarkko Nikula]

Tree wide grep for "hci_bcm" doesn't reveal there is any code registering
this platform device and "struct acpi_device_id" use for passing the
platform data looks a debug/test code leftover to me.

I'm assuming this driver effectively supports only ACPI enumeration and
thus test for ACPI_HANDLE() and platform data can be removed.

Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/bluetooth/hci_bcm.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index a1b9bbcbcb79..967d16692925 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -724,7 +724,6 @@ static int bcm_acpi_probe(struct bcm_device *dev)
 static int bcm_probe(struct platform_device *pdev)
 {
 	struct bcm_device *dev;
-	struct acpi_device_id *pdata = pdev->dev.platform_data;
 	int ret;
 
 	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
@@ -733,15 +732,9 @@ static int bcm_probe(struct platform_device *pdev)
 
 	dev->pdev = pdev;
 
-	if (ACPI_HANDLE(&pdev->dev)) {
-		ret = bcm_acpi_probe(dev);
-		if (ret)
-			return ret;
-	} else if (pdata) {
-		dev->name = pdata->id;
-	} else {
-		return -ENODEV;
-	}
+	ret = bcm_acpi_probe(dev);
+	if (ret)
+		return ret;
 
 	platform_set_drvdata(pdev, dev);
 
-- 
2.5.3

[PATCH 5/5] Bluetooth: hci_bcm: Do not test ACPI companion in bcm_acpi_probe()

[Jarkko Nikula]

This device has always ACPI companion because driver supports only ACPI
enumeration. Therefore there is no need to test it in bcm_acpi_probe() and
we can pass it directly to acpi_dev_get_resources() (which will return
-EINVAL in case of NULL argument is passed).

Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/bluetooth/hci_bcm.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 967d16692925..512873262055 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -647,7 +647,6 @@ static int bcm_resource(struct acpi_resource *ares, void *data)
 static int bcm_acpi_probe(struct bcm_device *dev)
 {
 	struct platform_device *pdev = dev->pdev;
-	struct acpi_device *adev;
 	LIST_HEAD(resources);
 	const struct dmi_system_id *dmi_id;
 	int ret;
@@ -696,11 +695,8 @@ static int bcm_acpi_probe(struct bcm_device *dev)
 	}
 
 	/* Retrieve UART ACPI info */
-	adev = ACPI_COMPANION(&dev->pdev->dev);
-	if (!adev)
-		return 0;
-
-	ret = acpi_dev_get_resources(adev, &resources, bcm_resource, dev);
+	ret = acpi_dev_get_resources(ACPI_COMPANION(&dev->pdev->dev),
+				     &resources, bcm_resource, dev);
 	if (ret < 0)
 		return ret;
 	acpi_dev_free_resource_list(&resources);
-- 
2.5.3

Re: [PATCH 1/5] Bluetooth: hci_bcm: Add missing acpi_dev_free_resource_list()

[Marcel Holtmann]

Hi Jarkko,

> Caller of acpi_dev_get_resources() should free the constructed resource
> list by calling the acpi_dev_free_resource_list() in order to avoid memory
> leak.
> 
> Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
> Call to acpi_dev_get_resources() was introduced by the commit ae056908862b
> ("Bluetooth: hci_bcm: Retrieve UART speed from ACPI") so fix should go to
> v4.3-rc.

if this should go into 4.3 then you need to send it against bluetooth tree actually and not bluetooth-next. And you might want to include patch 2/5 in there as well. Sending them as combined set with bluetooth-next patches is not really helping.

In case this is not an urgent fix that has to make it into 4.3, then I am just applying all 5 to bluetooth-next tree. Your choice.

Regards

Marcel

Re: [PATCH 1/5] Bluetooth: hci_bcm: Add missing acpi_dev_free_resource_list()

[Marcel Holtmann]

Hi Jarkko,

>> Caller of acpi_dev_get_resources() should free the constructed resource
>> list by calling the acpi_dev_free_resource_list() in order to avoid memory
>> leak.
>> 
>> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
>> ---
>> Call to acpi_dev_get_resources() was introduced by the commit ae056908862b
>> ("Bluetooth: hci_bcm: Retrieve UART speed from ACPI") so fix should go to
>> v4.3-rc.
> 
> if this should go into 4.3 then you need to send it against bluetooth tree actually and not bluetooth-next. And you might want to include patch 2/5 in there as well. Sending them as combined set with bluetooth-next patches is not really helping.
> 
> In case this is not an urgent fix that has to make it into 4.3, then I am just applying all 5 to bluetooth-next tree. Your choice.

I changed my mind here and applied all 5 patches to bluetooth-next tree.

Regards

Marcel

Re: [PATCH 1/5] Bluetooth: hci_bcm: Add missing acpi_dev_free_resource_list()

[Jarkko Nikula]

On 10/01/2015 11:05 AM, Marcel Holtmann wrote:
> Hi Jarkko,
>
>>> Caller of acpi_dev_get_resources() should free the constructed resource
>>> list by calling the acpi_dev_free_resource_list() in order to avoid memory
>>> leak.
>>>
>>> Signed-off-by: Jarkko Nikula <jarkko.nikula-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>>> ---
>>> Call to acpi_dev_get_resources() was introduced by the commit ae056908862b
>>> ("Bluetooth: hci_bcm: Retrieve UART speed from ACPI") so fix should go to
>>> v4.3-rc.
>>
>> if this should go into 4.3 then you need to send it against bluetooth tree actually and not bluetooth-next. And you might want to include patch 2/5 in there as well. Sending them as combined set with bluetooth-next patches is not really helping.
>>
>> In case this is not an urgent fix that has to make it into 4.3, then I am just applying all 5 to bluetooth-next tree. Your choice.
>
> I changed my mind here and applied all 5 patches to bluetooth-next tree.
>
Ah, sorry, brain fart from me to send these together.

Anyway, issue doesn't look fatal as leak happens only during probe time 
and may not actually even exists. At least on Asus T100TA the 
acpi_dev_get_resources() for the "BCM2E39" device returns 0, i.e. it 
doesn't build the list but calls the bcm_resource() a few times still.

Looks like acpi_dev_get_resources() builds the list for certain types of 
resources and probably not for these GPIO and serial bus that 
bcm_resource() is handling.

-- 
Jarkko