[PATCH 0/3] USB: dwc3: qcom: fix resource leaks on probe deferral

[PATCH 0/3] USB: dwc3: qcom: fix resource leaks on probe deferral

[Johan Hovold]

When reviewing the recently submitted series which reworks the dwc3 qcom
glue implementation [1], I noticed that the driver's tear down handling
is currently broken, something which can lead to memory leaks and
potentially use-after-free issues on probe deferral and on driver
unbind.

Let's get this sorted before reworking driver.

Note that the last patch has only been compile tested as I don't have
access to a sdm845 device.

Johan

[1] https://lore.kernel.org/lkml/20231016-dwc3-refactor-v1-0-ab4a84165470@quicinc.com/


Johan Hovold (3):
  USB: dwc3: qcom: fix resource leaks on probe deferral
  USB: dwc3: qcom: fix software node leak on probe errors
  USB: dwc3: qcom: fix ACPI platform device leak

 drivers/usb/dwc3/dwc3-qcom.c | 57 +++++++++++++++++++++++++++---------
 1 file changed, 43 insertions(+), 14 deletions(-)

-- 
2.41.0

[PATCH 1/3] USB: dwc3: qcom: fix resource leaks on probe deferral

[Johan Hovold]

The driver needs to deregister and free the newly allocated dwc3 core
platform device on ACPI probe errors (e.g. probe deferral) and on driver
unbind but instead it leaked those resources while erroneously dropping
a reference to the parent platform device which is still in use.

For OF probing the driver takes a reference to the dwc3 core platform
device which has also always been leaked.

Fix the broken ACPI tear down and make sure to drop the dwc3 core
reference for both OF and ACPI.

Fixes: 8fd95da2cfb5 ("usb: dwc3: qcom: Release the correct resources in dwc3_qcom_remove()")
Fixes: 2bc02355f8ba ("usb: dwc3: qcom: Add support for booting with ACPI")
Fixes: a4333c3a6ba9 ("usb: dwc3: Add Qualcomm DWC3 glue driver")
Cc: stable@vger.kernel.org      # 4.18
Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc: Lee Jones <lee@kernel.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/usb/dwc3/dwc3-qcom.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 3de43df6bbe8..00c3021b43ce 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -758,6 +758,7 @@ static int dwc3_qcom_of_register_core(struct platform_device *pdev)
 	if (!qcom->dwc3) {
 		ret = -ENODEV;
 		dev_err(dev, "failed to get dwc3 platform device\n");
+		of_platform_depopulate(dev);
 	}
 
 node_put:
@@ -899,7 +900,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 
 	if (ret) {
 		dev_err(dev, "failed to register DWC3 Core, err=%d\n", ret);
-		goto depopulate;
+		goto clk_disable;
 	}
 
 	ret = dwc3_qcom_interconnect_init(qcom);
@@ -934,7 +935,8 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	if (np)
 		of_platform_depopulate(&pdev->dev);
 	else
-		platform_device_put(pdev);
+		platform_device_del(qcom->dwc3);
+	platform_device_put(qcom->dwc3);
 clk_disable:
 	for (i = qcom->num_clocks - 1; i >= 0; i--) {
 		clk_disable_unprepare(qcom->clks[i]);
@@ -957,7 +959,8 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
 	if (np)
 		of_platform_depopulate(&pdev->dev);
 	else
-		platform_device_put(pdev);
+		platform_device_del(qcom->dwc3);
+	platform_device_put(qcom->dwc3);
 
 	for (i = qcom->num_clocks - 1; i >= 0; i--) {
 		clk_disable_unprepare(qcom->clks[i]);
-- 
2.41.0

[PATCH 2/3] USB: dwc3: qcom: fix software node leak on probe errors

[Johan Hovold]

Make sure to remove the software node also on (ACPI) probe errors to
avoid leaking the underlying resources.

Note that the software node is only used for ACPI probe so the driver
unbind tear down is updated to match probe.

Fixes: 8dc6e6dd1bee ("usb: dwc3: qcom: Constify the software node")
Cc: stable@vger.kernel.org      # 5.12
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/usb/dwc3/dwc3-qcom.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 00c3021b43ce..0703f9b85cda 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -932,10 +932,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 interconnect_exit:
 	dwc3_qcom_interconnect_exit(qcom);
 depopulate:
-	if (np)
+	if (np) {
 		of_platform_depopulate(&pdev->dev);
-	else
+	} else {
+		device_remove_software_node(&qcom->dwc3->dev);
 		platform_device_del(qcom->dwc3);
+	}
 	platform_device_put(qcom->dwc3);
 clk_disable:
 	for (i = qcom->num_clocks - 1; i >= 0; i--) {
@@ -955,11 +957,12 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	int i;
 
-	device_remove_software_node(&qcom->dwc3->dev);
-	if (np)
+	if (np) {
 		of_platform_depopulate(&pdev->dev);
-	else
+	} else {
+		device_remove_software_node(&qcom->dwc3->dev);
 		platform_device_del(qcom->dwc3);
+	}
 	platform_device_put(qcom->dwc3);
 
 	for (i = qcom->num_clocks - 1; i >= 0; i--) {
-- 
2.41.0

[PATCH 3/3] USB: dwc3: qcom: fix ACPI platform device leak

[Johan Hovold]

Make sure to free the "urs" platform device, which is created for some
ACPI platforms, on probe errors and on driver unbind.

Compile-tested only.

Fixes: c25c210f590e ("usb: dwc3: qcom: add URS Host support for sdm845 ACPI boot")
Cc: Shawn Guo <shawn.guo@linaro.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/usb/dwc3/dwc3-qcom.c | 37 +++++++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 0703f9b85cda..10fb481d943b 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -767,9 +767,9 @@ static int dwc3_qcom_of_register_core(struct platform_device *pdev)
 	return ret;
 }
 
-static struct platform_device *
-dwc3_qcom_create_urs_usb_platdev(struct device *dev)
+static struct platform_device *dwc3_qcom_create_urs_usb_platdev(struct device *dev)
 {
+	struct platform_device *urs_usb = NULL;
 	struct fwnode_handle *fwh;
 	struct acpi_device *adev;
 	char name[8];
@@ -789,9 +789,26 @@ dwc3_qcom_create_urs_usb_platdev(struct device *dev)
 
 	adev = to_acpi_device_node(fwh);
 	if (!adev)
-		return NULL;
+		goto err_put_handle;
+
+	urs_usb = acpi_create_platform_device(adev, NULL);
+	if (IS_ERR_OR_NULL(urs_usb))
+		goto err_put_handle;
+
+	return urs_usb;
 
-	return acpi_create_platform_device(adev, NULL);
+err_put_handle:
+	fwnode_handle_put(fwh);
+
+	return urs_usb;
+}
+
+static void dwc3_qcom_destroy_urs_usb_platdev(struct platform_device *urs_usb)
+{
+	struct fwnode_handle *fwh = urs_usb->dev.fwnode;
+
+	platform_device_unregister(urs_usb);
+	fwnode_handle_put(fwh);
 }
 
 static int dwc3_qcom_probe(struct platform_device *pdev)
@@ -875,13 +892,13 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	qcom->qscratch_base = devm_ioremap_resource(dev, parent_res);
 	if (IS_ERR(qcom->qscratch_base)) {
 		ret = PTR_ERR(qcom->qscratch_base);
-		goto clk_disable;
+		goto free_urs;
 	}
 
 	ret = dwc3_qcom_setup_irq(pdev);
 	if (ret) {
 		dev_err(dev, "failed to setup IRQs, err=%d\n", ret);
-		goto clk_disable;
+		goto free_urs;
 	}
 
 	/*
@@ -900,7 +917,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 
 	if (ret) {
 		dev_err(dev, "failed to register DWC3 Core, err=%d\n", ret);
-		goto clk_disable;
+		goto free_urs;
 	}
 
 	ret = dwc3_qcom_interconnect_init(qcom);
@@ -939,6 +956,9 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 		platform_device_del(qcom->dwc3);
 	}
 	platform_device_put(qcom->dwc3);
+free_urs:
+	if (qcom->urs_usb)
+		dwc3_qcom_destroy_urs_usb_platdev(qcom->urs_usb);
 clk_disable:
 	for (i = qcom->num_clocks - 1; i >= 0; i--) {
 		clk_disable_unprepare(qcom->clks[i]);
@@ -965,6 +985,9 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
 	}
 	platform_device_put(qcom->dwc3);
 
+	if (qcom->urs_usb)
+		dwc3_qcom_destroy_urs_usb_platdev(qcom->urs_usb);
+
 	for (i = qcom->num_clocks - 1; i >= 0; i--) {
 		clk_disable_unprepare(qcom->clks[i]);
 		clk_put(qcom->clks[i]);
-- 
2.41.0

Re: [PATCH 1/3] USB: dwc3: qcom: fix resource leaks on probe deferral

[Andrew Halaney]

On Fri, Nov 17, 2023 at 06:36:48PM +0100, Johan Hovold wrote:
> The driver needs to deregister and free the newly allocated dwc3 core
> platform device on ACPI probe errors (e.g. probe deferral) and on driver
> unbind but instead it leaked those resources while erroneously dropping
> a reference to the parent platform device which is still in use.
> 
> For OF probing the driver takes a reference to the dwc3 core platform
> device which has also always been leaked.
> 
> Fix the broken ACPI tear down and make sure to drop the dwc3 core
> reference for both OF and ACPI.
> 
> Fixes: 8fd95da2cfb5 ("usb: dwc3: qcom: Release the correct resources in dwc3_qcom_remove()")
> Fixes: 2bc02355f8ba ("usb: dwc3: qcom: Add support for booting with ACPI")
> Fixes: a4333c3a6ba9 ("usb: dwc3: Add Qualcomm DWC3 glue driver")
> Cc: stable@vger.kernel.org      # 4.18
> Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Cc: Lee Jones <lee@kernel.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---

I foobared the review of one of the patches listed as being fixed by
this, but for what it is worth I think this makes sense.

Hopefully my eye is better this time.

Acked-by: Andrew Halaney <ahalaney@redhat.com>

>  drivers/usb/dwc3/dwc3-qcom.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 3de43df6bbe8..00c3021b43ce 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -758,6 +758,7 @@ static int dwc3_qcom_of_register_core(struct platform_device *pdev)
>  	if (!qcom->dwc3) {
>  		ret = -ENODEV;
>  		dev_err(dev, "failed to get dwc3 platform device\n");
> +		of_platform_depopulate(dev);
>  	}
>  
>  node_put:
> @@ -899,7 +900,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  
>  	if (ret) {
>  		dev_err(dev, "failed to register DWC3 Core, err=%d\n", ret);
> -		goto depopulate;
> +		goto clk_disable;
>  	}
>  
>  	ret = dwc3_qcom_interconnect_init(qcom);
> @@ -934,7 +935,8 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  	if (np)
>  		of_platform_depopulate(&pdev->dev);
>  	else
> -		platform_device_put(pdev);
> +		platform_device_del(qcom->dwc3);
> +	platform_device_put(qcom->dwc3);
>  clk_disable:
>  	for (i = qcom->num_clocks - 1; i >= 0; i--) {
>  		clk_disable_unprepare(qcom->clks[i]);
> @@ -957,7 +959,8 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
>  	if (np)
>  		of_platform_depopulate(&pdev->dev);
>  	else
> -		platform_device_put(pdev);
> +		platform_device_del(qcom->dwc3);
> +	platform_device_put(qcom->dwc3);
>  
>  	for (i = qcom->num_clocks - 1; i >= 0; i--) {
>  		clk_disable_unprepare(qcom->clks[i]);
> -- 
> 2.41.0
> 
> 

Re: [PATCH 2/3] USB: dwc3: qcom: fix software node leak on probe errors

[Andrew Halaney]

On Fri, Nov 17, 2023 at 06:36:49PM +0100, Johan Hovold wrote:
> Make sure to remove the software node also on (ACPI) probe errors to
> avoid leaking the underlying resources.
> 
> Note that the software node is only used for ACPI probe so the driver
> unbind tear down is updated to match probe.
> 
> Fixes: 8dc6e6dd1bee ("usb: dwc3: qcom: Constify the software node")
> Cc: stable@vger.kernel.org      # 5.12
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Acked-by: Andrew Halaney <ahalaney@redhat.com>

> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 00c3021b43ce..0703f9b85cda 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -932,10 +932,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  interconnect_exit:
>  	dwc3_qcom_interconnect_exit(qcom);
>  depopulate:
> -	if (np)
> +	if (np) {
>  		of_platform_depopulate(&pdev->dev);
> -	else
> +	} else {
> +		device_remove_software_node(&qcom->dwc3->dev);
>  		platform_device_del(qcom->dwc3);
> +	}
>  	platform_device_put(qcom->dwc3);
>  clk_disable:
>  	for (i = qcom->num_clocks - 1; i >= 0; i--) {
> @@ -955,11 +957,12 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	int i;
>  
> -	device_remove_software_node(&qcom->dwc3->dev);
> -	if (np)
> +	if (np) {
>  		of_platform_depopulate(&pdev->dev);
> -	else
> +	} else {
> +		device_remove_software_node(&qcom->dwc3->dev);
>  		platform_device_del(qcom->dwc3);
> +	}
>  	platform_device_put(qcom->dwc3);
>  
>  	for (i = qcom->num_clocks - 1; i >= 0; i--) {
> -- 
> 2.41.0
> 
> 

Re: [PATCH 3/3] USB: dwc3: qcom: fix ACPI platform device leak

[Andrew Halaney]

On Fri, Nov 17, 2023 at 06:36:50PM +0100, Johan Hovold wrote:
> Make sure to free the "urs" platform device, which is created for some
> ACPI platforms, on probe errors and on driver unbind.
> 
> Compile-tested only.
> 
> Fixes: c25c210f590e ("usb: dwc3: qcom: add URS Host support for sdm845 ACPI boot")
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Acked-by: Andrew Halaney <ahalaney@redhat.com>

> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 37 +++++++++++++++++++++++++++++-------
>  1 file changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 0703f9b85cda..10fb481d943b 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -767,9 +767,9 @@ static int dwc3_qcom_of_register_core(struct platform_device *pdev)
>  	return ret;
>  }
>  
> -static struct platform_device *
> -dwc3_qcom_create_urs_usb_platdev(struct device *dev)
> +static struct platform_device *dwc3_qcom_create_urs_usb_platdev(struct device *dev)
>  {
> +	struct platform_device *urs_usb = NULL;
>  	struct fwnode_handle *fwh;
>  	struct acpi_device *adev;
>  	char name[8];
> @@ -789,9 +789,26 @@ dwc3_qcom_create_urs_usb_platdev(struct device *dev)
>  
>  	adev = to_acpi_device_node(fwh);
>  	if (!adev)
> -		return NULL;
> +		goto err_put_handle;
> +
> +	urs_usb = acpi_create_platform_device(adev, NULL);
> +	if (IS_ERR_OR_NULL(urs_usb))
> +		goto err_put_handle;
> +
> +	return urs_usb;
>  
> -	return acpi_create_platform_device(adev, NULL);
> +err_put_handle:
> +	fwnode_handle_put(fwh);
> +
> +	return urs_usb;
> +}
> +
> +static void dwc3_qcom_destroy_urs_usb_platdev(struct platform_device *urs_usb)
> +{
> +	struct fwnode_handle *fwh = urs_usb->dev.fwnode;
> +
> +	platform_device_unregister(urs_usb);
> +	fwnode_handle_put(fwh);
>  }
>  
>  static int dwc3_qcom_probe(struct platform_device *pdev)
> @@ -875,13 +892,13 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  	qcom->qscratch_base = devm_ioremap_resource(dev, parent_res);
>  	if (IS_ERR(qcom->qscratch_base)) {
>  		ret = PTR_ERR(qcom->qscratch_base);
> -		goto clk_disable;
> +		goto free_urs;
>  	}
>  
>  	ret = dwc3_qcom_setup_irq(pdev);
>  	if (ret) {
>  		dev_err(dev, "failed to setup IRQs, err=%d\n", ret);
> -		goto clk_disable;
> +		goto free_urs;
>  	}
>  
>  	/*
> @@ -900,7 +917,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  
>  	if (ret) {
>  		dev_err(dev, "failed to register DWC3 Core, err=%d\n", ret);
> -		goto clk_disable;
> +		goto free_urs;
>  	}
>  
>  	ret = dwc3_qcom_interconnect_init(qcom);
> @@ -939,6 +956,9 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  		platform_device_del(qcom->dwc3);
>  	}
>  	platform_device_put(qcom->dwc3);
> +free_urs:
> +	if (qcom->urs_usb)
> +		dwc3_qcom_destroy_urs_usb_platdev(qcom->urs_usb);
>  clk_disable:
>  	for (i = qcom->num_clocks - 1; i >= 0; i--) {
>  		clk_disable_unprepare(qcom->clks[i]);
> @@ -965,6 +985,9 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
>  	}
>  	platform_device_put(qcom->dwc3);
>  
> +	if (qcom->urs_usb)
> +		dwc3_qcom_destroy_urs_usb_platdev(qcom->urs_usb);
> +
>  	for (i = qcom->num_clocks - 1; i >= 0; i--) {
>  		clk_disable_unprepare(qcom->clks[i]);
>  		clk_put(qcom->clks[i]);
> -- 
> 2.41.0
> 
> 

Re: [PATCH 0/3] USB: dwc3: qcom: fix resource leaks on probe deferral

[Konrad Dybcio]

On 17.11.2023 18:36, Johan Hovold wrote:
> When reviewing the recently submitted series which reworks the dwc3 qcom
> glue implementation [1], I noticed that the driver's tear down handling
> is currently broken, something which can lead to memory leaks and
> potentially use-after-free issues on probe deferral and on driver
> unbind.
> 
> Let's get this sorted before reworking driver.
> 
> Note that the last patch has only been compile tested as I don't have
> access to a sdm845 device.
> 
> Johan
I'll sound like a broken record, but:

is there anyone in the world that is actively benefiting from this failed
experiment of using the ACPI tables that were shipped with these SoCs?

There are so so so many shortcomings associated with it due to how Windows
drivers on these platforms know waaaay too much and largely use ACPI to
"bind driver x" and I simply think it doesn't make sense to continue
carrying this code forward given little use and no testing.

Konrad

Re: [PATCH 3/3] USB: dwc3: qcom: fix ACPI platform device leak

[Shawn Guo]

On Sat, Nov 18, 2023 at 1:38 AM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> Make sure to free the "urs" platform device, which is created for some
> ACPI platforms, on probe errors and on driver unbind.
>
> Compile-tested only.
>
> Fixes: c25c210f590e ("usb: dwc3: qcom: add URS Host support for sdm845 ACPI boot")
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Acked-by: Shawn Guo <shawn.guo@linaro.org>

Re: [PATCH 0/3] USB: dwc3: qcom: fix resource leaks on probe deferral

[Andrew Halaney]

On Sat, Nov 18, 2023 at 12:47:30AM +0100, Konrad Dybcio wrote:
> On 17.11.2023 18:36, Johan Hovold wrote:
> > When reviewing the recently submitted series which reworks the dwc3 qcom
> > glue implementation [1], I noticed that the driver's tear down handling
> > is currently broken, something which can lead to memory leaks and
> > potentially use-after-free issues on probe deferral and on driver
> > unbind.
> > 
> > Let's get this sorted before reworking driver.
> > 
> > Note that the last patch has only been compile tested as I don't have
> > access to a sdm845 device.
> > 
> > Johan
> I'll sound like a broken record, but:
> 
> is there anyone in the world that is actively benefiting from this failed
> experiment of using the ACPI tables that were shipped with these SoCs?
> 
> There are so so so many shortcomings associated with it due to how Windows
> drivers on these platforms know waaaay too much and largely use ACPI to
> "bind driver x" and I simply think it doesn't make sense to continue
> carrying this code forward given little use and no testing.
> 
> Konrad
> 

For what it is worth, I have agreed with your opinion on this every time
I've read it. I am not the target audience of the question, but I'll at
least give my personal (interpreted: uneducated? undesired?) opinion
that the ACPI support in here adds little value and extra burden.

Of course that topic is a bit independent of this series, but I'd be
curious if a patchset removing the support would be welcomed or not by
maintainers, so I'm stirring the pot by replying here :)

Thanks,
Andrew

Re: [PATCH 0/3] USB: dwc3: qcom: fix resource leaks on probe deferral

[Johan Hovold]

On Mon, Nov 20, 2023 at 09:22:54AM -0600, Andrew Halaney wrote:
> On Sat, Nov 18, 2023 at 12:47:30AM +0100, Konrad Dybcio wrote:
> > On 17.11.2023 18:36, Johan Hovold wrote:
> > > When reviewing the recently submitted series which reworks the dwc3 qcom
> > > glue implementation [1], I noticed that the driver's tear down handling
> > > is currently broken, something which can lead to memory leaks and
> > > potentially use-after-free issues on probe deferral and on driver
> > > unbind.
> > > 
> > > Let's get this sorted before reworking driver.
> > > 
> > > Note that the last patch has only been compile tested as I don't have
> > > access to a sdm845 device.

> > I'll sound like a broken record, but:
> > 
> > is there anyone in the world that is actively benefiting from this failed
> > experiment of using the ACPI tables that were shipped with these SoCs?
> > 
> > There are so so so many shortcomings associated with it due to how Windows
> > drivers on these platforms know waaaay too much and largely use ACPI to
> > "bind driver x" and I simply think it doesn't make sense to continue
> > carrying this code forward given little use and no testing.

> For what it is worth, I have agreed with your opinion on this every time
> I've read it. I am not the target audience of the question, but I'll at
> least give my personal (interpreted: uneducated? undesired?) opinion
> that the ACPI support in here adds little value and extra burden.
> 
> Of course that topic is a bit independent of this series, but I'd be
> curious if a patchset removing the support would be welcomed or not by
> maintainers, so I'm stirring the pot by replying here :)

I agree that if we can remove the ACPI hacks in here, we should try do
so (e.g. given that no one really uses it anymore).

As Andrew already mentioned, that is a separate issue not directly
related to this series, though.

Removing it before reworking the dwc3 binding [1] and adding multiport
support [2] should simplify both of those series quite a bit, however.

Johan

[1] https://lore.kernel.org/all/20231016-dwc3-refactor-v1-0-ab4a84165470@quicinc.com/
[2] https://lore.kernel.org/all/20231007154806.605-1-quic_kriskura@quicinc.com/

Re: [PATCH 2/3] USB: dwc3: qcom: fix software node leak on probe errors

[Heikki Krogerus]

On Fri, Nov 17, 2023 at 06:36:49PM +0100, Johan Hovold wrote:
> Make sure to remove the software node also on (ACPI) probe errors to
> avoid leaking the underlying resources.
> 
> Note that the software node is only used for ACPI probe so the driver
> unbind tear down is updated to match probe.
> 
> Fixes: 8dc6e6dd1bee ("usb: dwc3: qcom: Constify the software node")
> Cc: stable@vger.kernel.org      # 5.12
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 00c3021b43ce..0703f9b85cda 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -932,10 +932,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  interconnect_exit:
>  	dwc3_qcom_interconnect_exit(qcom);
>  depopulate:
> -	if (np)
> +	if (np) {
>  		of_platform_depopulate(&pdev->dev);
> -	else
> +	} else {
> +		device_remove_software_node(&qcom->dwc3->dev);
>  		platform_device_del(qcom->dwc3);
> +	}
>  	platform_device_put(qcom->dwc3);
>  clk_disable:
>  	for (i = qcom->num_clocks - 1; i >= 0; i--) {
> @@ -955,11 +957,12 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	int i;
>  
> -	device_remove_software_node(&qcom->dwc3->dev);
> -	if (np)
> +	if (np) {
>  		of_platform_depopulate(&pdev->dev);
> -	else
> +	} else {
> +		device_remove_software_node(&qcom->dwc3->dev);
>  		platform_device_del(qcom->dwc3);
> +	}
>  	platform_device_put(qcom->dwc3);
>  
>  	for (i = qcom->num_clocks - 1; i >= 0; i--) {
> -- 
> 2.41.0

-- 
heikki

Re: [PATCH 0/3] USB: dwc3: qcom: fix resource leaks on probe deferral

[Greg Kroah-Hartman]

On Mon, Nov 20, 2023 at 05:53:10PM +0100, Johan Hovold wrote:
> On Mon, Nov 20, 2023 at 09:22:54AM -0600, Andrew Halaney wrote:
> > On Sat, Nov 18, 2023 at 12:47:30AM +0100, Konrad Dybcio wrote:
> > > On 17.11.2023 18:36, Johan Hovold wrote:
> > > > When reviewing the recently submitted series which reworks the dwc3 qcom
> > > > glue implementation [1], I noticed that the driver's tear down handling
> > > > is currently broken, something which can lead to memory leaks and
> > > > potentially use-after-free issues on probe deferral and on driver
> > > > unbind.
> > > > 
> > > > Let's get this sorted before reworking driver.
> > > > 
> > > > Note that the last patch has only been compile tested as I don't have
> > > > access to a sdm845 device.
> 
> > > I'll sound like a broken record, but:
> > > 
> > > is there anyone in the world that is actively benefiting from this failed
> > > experiment of using the ACPI tables that were shipped with these SoCs?
> > > 
> > > There are so so so many shortcomings associated with it due to how Windows
> > > drivers on these platforms know waaaay too much and largely use ACPI to
> > > "bind driver x" and I simply think it doesn't make sense to continue
> > > carrying this code forward given little use and no testing.
> 
> > For what it is worth, I have agreed with your opinion on this every time
> > I've read it. I am not the target audience of the question, but I'll at
> > least give my personal (interpreted: uneducated? undesired?) opinion
> > that the ACPI support in here adds little value and extra burden.
> > 
> > Of course that topic is a bit independent of this series, but I'd be
> > curious if a patchset removing the support would be welcomed or not by
> > maintainers, so I'm stirring the pot by replying here :)
> 
> I agree that if we can remove the ACPI hacks in here, we should try do
> so (e.g. given that no one really uses it anymore).
> 
> As Andrew already mentioned, that is a separate issue not directly
> related to this series, though.
> 
> Removing it before reworking the dwc3 binding [1] and adding multiport
> support [2] should simplify both of those series quite a bit, however.
> 
> Johan
> 
> [1] https://lore.kernel.org/all/20231016-dwc3-refactor-v1-0-ab4a84165470@quicinc.com/
> [2] https://lore.kernel.org/all/20231007154806.605-1-quic_kriskura@quicinc.com/
> 

So should I apply this series now or not?

confused,

greg k-h

Re: [PATCH 0/3] USB: dwc3: qcom: fix resource leaks on probe deferral

[Johan Hovold]

On Tue, Nov 21, 2023 at 03:21:34PM +0100, Greg Kroah-Hartman wrote:
> On Mon, Nov 20, 2023 at 05:53:10PM +0100, Johan Hovold wrote:
> > On Mon, Nov 20, 2023 at 09:22:54AM -0600, Andrew Halaney wrote:
> > > On Sat, Nov 18, 2023 at 12:47:30AM +0100, Konrad Dybcio wrote:
> > > > On 17.11.2023 18:36, Johan Hovold wrote:
> > > > > When reviewing the recently submitted series which reworks the dwc3 qcom
> > > > > glue implementation [1], I noticed that the driver's tear down handling
> > > > > is currently broken, something which can lead to memory leaks and
> > > > > potentially use-after-free issues on probe deferral and on driver
> > > > > unbind.
> > > > > 
> > > > > Let's get this sorted before reworking driver.
> > > > > 
> > > > > Note that the last patch has only been compile tested as I don't have
> > > > > access to a sdm845 device.
> > 
> > > > I'll sound like a broken record, but:
> > > > 
> > > > is there anyone in the world that is actively benefiting from this failed
> > > > experiment of using the ACPI tables that were shipped with these SoCs?

> > I agree that if we can remove the ACPI hacks in here, we should try do
> > so (e.g. given that no one really uses it anymore).
> > 
> > As Andrew already mentioned, that is a separate issue not directly
> > related to this series, though.
> > 
> > Removing it before reworking the dwc3 binding [1] and adding multiport
> > support [2] should simplify both of those series quite a bit, however.

> > [1] https://lore.kernel.org/all/20231016-dwc3-refactor-v1-0-ab4a84165470@quicinc.com/
> > [2] https://lore.kernel.org/all/20231007154806.605-1-quic_kriskura@quicinc.com/
> > 
> 
> So should I apply this series now or not?

Please do. Removing ACPI support should be done later if that's at all
possible.

Johan

Re: [PATCH 0/3] USB: dwc3: qcom: fix resource leaks on probe deferral

[Greg Kroah-Hartman]

On Tue, Nov 21, 2023 at 04:04:03PM +0100, Johan Hovold wrote:
> On Tue, Nov 21, 2023 at 03:21:34PM +0100, Greg Kroah-Hartman wrote:
> > On Mon, Nov 20, 2023 at 05:53:10PM +0100, Johan Hovold wrote:
> > > On Mon, Nov 20, 2023 at 09:22:54AM -0600, Andrew Halaney wrote:
> > > > On Sat, Nov 18, 2023 at 12:47:30AM +0100, Konrad Dybcio wrote:
> > > > > On 17.11.2023 18:36, Johan Hovold wrote:
> > > > > > When reviewing the recently submitted series which reworks the dwc3 qcom
> > > > > > glue implementation [1], I noticed that the driver's tear down handling
> > > > > > is currently broken, something which can lead to memory leaks and
> > > > > > potentially use-after-free issues on probe deferral and on driver
> > > > > > unbind.
> > > > > > 
> > > > > > Let's get this sorted before reworking driver.
> > > > > > 
> > > > > > Note that the last patch has only been compile tested as I don't have
> > > > > > access to a sdm845 device.
> > > 
> > > > > I'll sound like a broken record, but:
> > > > > 
> > > > > is there anyone in the world that is actively benefiting from this failed
> > > > > experiment of using the ACPI tables that were shipped with these SoCs?
> 
> > > I agree that if we can remove the ACPI hacks in here, we should try do
> > > so (e.g. given that no one really uses it anymore).
> > > 
> > > As Andrew already mentioned, that is a separate issue not directly
> > > related to this series, though.
> > > 
> > > Removing it before reworking the dwc3 binding [1] and adding multiport
> > > support [2] should simplify both of those series quite a bit, however.
> 
> > > [1] https://lore.kernel.org/all/20231016-dwc3-refactor-v1-0-ab4a84165470@quicinc.com/
> > > [2] https://lore.kernel.org/all/20231007154806.605-1-quic_kriskura@quicinc.com/
> > > 
> > 
> > So should I apply this series now or not?
> 
> Please do. Removing ACPI support should be done later if that's at all
> possible.

Great, now queued up, thanks.

greg k-h