[PATCH] drm/loongson: clean up KMS polling on probe failure

[PATCH] drm/loongson: clean up KMS polling on probe failure

[Myeonghun Pak]

lsdc_pci_probe() initializes KMS polling before setting up vblank support,
requesting the IRQ and registering the DRM device. If any of those later
steps fails, probe returns without finalizing polling. The remove path has
the same lifetime gap when tearing down a successfully registered device.

Route those probe failures through a poll cleanup label. Also finalize
polling from remove before unregistering the DRM device.

This issue was identified during our ongoing static-analysis research while
reviewing kernel code.

Fixes: f39db26c5428 ("drm: Add kms driver for loongson display controller")
Cc: stable@vger.kernel.org
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
 drivers/gpu/drm/loongson/lsdc_drv.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/loongson/lsdc_drv.c b/drivers/gpu/drm/loongson/lsdc_drv.c
index abf5bf68ee..3db1f8690a 100644
--- a/drivers/gpu/drm/loongson/lsdc_drv.c
+++ b/drivers/gpu/drm/loongson/lsdc_drv.c
@@ -297,7 +297,7 @@ static int lsdc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (loongson_vblank) {
 		ret = drm_vblank_init(ddev, descp->num_of_crtc);
 		if (ret)
-			return ret;
+			goto err_poll_fini;
 
 		ret = devm_request_irq(&pdev->dev, pdev->irq,
 				       descp->funcs->irq_handler,
@@ -305,7 +305,7 @@ static int lsdc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 				       dev_name(&pdev->dev), ddev);
 		if (ret) {
 			drm_err(ddev, "Failed to register interrupt: %d\n", ret);
-			return ret;
+			goto err_poll_fini;
 		}
 
 		drm_info(ddev, "registered irq: %u\n", pdev->irq);
@@ -313,17 +313,22 @@ static int lsdc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	ret = drm_dev_register(ddev, 0);
 	if (ret)
-		return ret;
+		goto err_poll_fini;
 
 	drm_client_setup(ddev, NULL);
 
 	return 0;
+
+err_poll_fini:
+	drm_kms_helper_poll_fini(ddev);
+	return ret;
 }
 
 static void lsdc_pci_remove(struct pci_dev *pdev)
 {
 	struct drm_device *ddev = pci_get_drvdata(pdev);
 
+	drm_kms_helper_poll_fini(ddev);
 	drm_dev_unregister(ddev);
 	drm_atomic_helper_shutdown(ddev);
 }
-- 
2.47.1

Re: [PATCH] drm/loongson: clean up KMS polling on probe failure

[Icenowy Zheng]

在 2026-05-12二的 02:01 +0900，Myeonghun Pak写道：
> lsdc_pci_probe() initializes KMS polling before setting up vblank
> support,
> requesting the IRQ and registering the DRM device. If any of those
> later
> steps fails, probe returns without finalizing polling. The remove
> path has
> the same lifetime gap when tearing down a successfully registered
> device.
> 
> Route those probe failures through a poll cleanup label. Also
> finalize
> polling from remove before unregistering the DRM device.

Interesting, but it looks like a `drmm_kms_helper_poll_init` function
exists (while rarely used).

Maybe it's better to switch to this? Or maybe there's some reason not
to use this?

Thanks,
Icenowy

> 
> This issue was identified during our ongoing static-analysis research
> while
> reviewing kernel code.
> 
> Fixes: f39db26c5428 ("drm: Add kms driver for loongson display
> controller")
> Cc: stable@vger.kernel.org
> Co-developed-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
> ---
>  drivers/gpu/drm/loongson/lsdc_drv.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/loongson/lsdc_drv.c
> b/drivers/gpu/drm/loongson/lsdc_drv.c
> index abf5bf68ee..3db1f8690a 100644
> --- a/drivers/gpu/drm/loongson/lsdc_drv.c
> +++ b/drivers/gpu/drm/loongson/lsdc_drv.c
> @@ -297,7 +297,7 @@ static int lsdc_pci_probe(struct pci_dev *pdev,
> const struct pci_device_id *ent)
>  	if (loongson_vblank) {
>  		ret = drm_vblank_init(ddev, descp->num_of_crtc);
>  		if (ret)
> -			return ret;
> +			goto err_poll_fini;
>  
>  		ret = devm_request_irq(&pdev->dev, pdev->irq,
>  				       descp->funcs->irq_handler,
> @@ -305,7 +305,7 @@ static int lsdc_pci_probe(struct pci_dev *pdev,
> const struct pci_device_id *ent)
>  				       dev_name(&pdev->dev), ddev);
>  		if (ret) {
>  			drm_err(ddev, "Failed to register interrupt:
> %d\n", ret);
> -			return ret;
> +			goto err_poll_fini;
>  		}
>  
>  		drm_info(ddev, "registered irq: %u\n", pdev->irq);
> @@ -313,17 +313,22 @@ static int lsdc_pci_probe(struct pci_dev *pdev,
> const struct pci_device_id *ent)
>  
>  	ret = drm_dev_register(ddev, 0);
>  	if (ret)
> -		return ret;
> +		goto err_poll_fini;
>  
>  	drm_client_setup(ddev, NULL);
>  
>  	return 0;
> +
> +err_poll_fini:
> +	drm_kms_helper_poll_fini(ddev);
> +	return ret;
>  }
>  
>  static void lsdc_pci_remove(struct pci_dev *pdev)
>  {
>  	struct drm_device *ddev = pci_get_drvdata(pdev);
>  
> +	drm_kms_helper_poll_fini(ddev);
>  	drm_dev_unregister(ddev);
>  	drm_atomic_helper_shutdown(ddev);
>  }

Re: [PATCH] drm/loongson: clean up KMS polling on probe failure

[Thomas Zimmermann]

Hi

Am 11.05.26 um 19:22 schrieb Icenowy Zheng:
> 在 2026-05-12二的 02:01 +0900，Myeonghun Pak写道：
>> lsdc_pci_probe() initializes KMS polling before setting up vblank
>> support,
>> requesting the IRQ and registering the DRM device. If any of those
>> later
>> steps fails, probe returns without finalizing polling. The remove
>> path has
>> the same lifetime gap when tearing down a successfully registered
>> device.
>>
>> Route those probe failures through a poll cleanup label. Also
>> finalize
>> polling from remove before unregistering the DRM device.
> Interesting, but it looks like a `drmm_kms_helper_poll_init` function
> exists (while rarely used).
>
> Maybe it's better to switch to this? Or maybe there's some reason not
> to use this?

Agreed. DRM drivers are advised to use managed cleanup for their data 
structures. So rather switch to drmm_kms_helper_poll_init(). That also 
resolves the problem that loongson never calls poll_fini on regular 
removals.

Best regards
Thomas

>
> Thanks,
> Icenowy
>
>> This issue was identified during our ongoing static-analysis research
>> while
>> reviewing kernel code.
>>
>> Fixes: f39db26c5428 ("drm: Add kms driver for loongson display
>> controller")
>> Cc: stable@vger.kernel.org
>> Co-developed-by: Ijae Kim <ae878000@gmail.com>
>> Signed-off-by: Ijae Kim <ae878000@gmail.com>
>> Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
>> ---
>>   drivers/gpu/drm/loongson/lsdc_drv.c | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/loongson/lsdc_drv.c
>> b/drivers/gpu/drm/loongson/lsdc_drv.c
>> index abf5bf68ee..3db1f8690a 100644
>> --- a/drivers/gpu/drm/loongson/lsdc_drv.c
>> +++ b/drivers/gpu/drm/loongson/lsdc_drv.c
>> @@ -297,7 +297,7 @@ static int lsdc_pci_probe(struct pci_dev *pdev,
>> const struct pci_device_id *ent)
>>   	if (loongson_vblank) {
>>   		ret = drm_vblank_init(ddev, descp->num_of_crtc);
>>   		if (ret)
>> -			return ret;
>> +			goto err_poll_fini;
>>   
>>   		ret = devm_request_irq(&pdev->dev, pdev->irq,
>>   				       descp->funcs->irq_handler,
>> @@ -305,7 +305,7 @@ static int lsdc_pci_probe(struct pci_dev *pdev,
>> const struct pci_device_id *ent)
>>   				       dev_name(&pdev->dev), ddev);
>>   		if (ret) {
>>   			drm_err(ddev, "Failed to register interrupt:
>> %d\n", ret);
>> -			return ret;
>> +			goto err_poll_fini;
>>   		}
>>   
>>   		drm_info(ddev, "registered irq: %u\n", pdev->irq);
>> @@ -313,17 +313,22 @@ static int lsdc_pci_probe(struct pci_dev *pdev,
>> const struct pci_device_id *ent)
>>   
>>   	ret = drm_dev_register(ddev, 0);
>>   	if (ret)
>> -		return ret;
>> +		goto err_poll_fini;
>>   
>>   	drm_client_setup(ddev, NULL);
>>   
>>   	return 0;
>> +
>> +err_poll_fini:
>> +	drm_kms_helper_poll_fini(ddev);
>> +	return ret;
>>   }
>>   
>>   static void lsdc_pci_remove(struct pci_dev *pdev)
>>   {
>>   	struct drm_device *ddev = pci_get_drvdata(pdev);
>>   
>> +	drm_kms_helper_poll_fini(ddev);
>>   	drm_dev_unregister(ddev);
>>   	drm_atomic_helper_shutdown(ddev);
>>   }

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)

Re: [PATCH] drm/loongson: clean up KMS polling on probe failure

[Myeonghun Pak]

[-- Attachment #1: Type: text/plain, Size: 4261 bytes --]

Hi Icenowy, Thomas,

Thanks for the review.

I agree that using drmm_kms_helper_poll_init() is the better fix here. It
keeps the polling cleanup tied to the DRM device lifetime and avoids adding
manual unwind and remove-path cleanup.

I sent a v2 that switches lsdc_pci_probe() to drmm_kms_helper_poll_init().

Thanks,
Myeonghun


2026년 5월 12일 (화) 오후 3:26, Thomas Zimmermann <tzimmermann@suse.de>님이 작성:

> Hi
>
> Am 11.05.26 um 19:22 schrieb Icenowy Zheng:
> > 在 2026-05-12二的 02:01 +0900，Myeonghun Pak写道：
> >> lsdc_pci_probe() initializes KMS polling before setting up vblank
> >> support,
> >> requesting the IRQ and registering the DRM device. If any of those
> >> later
> >> steps fails, probe returns without finalizing polling. The remove
> >> path has
> >> the same lifetime gap when tearing down a successfully registered
> >> device.
> >>
> >> Route those probe failures through a poll cleanup label. Also
> >> finalize
> >> polling from remove before unregistering the DRM device.
> > Interesting, but it looks like a `drmm_kms_helper_poll_init` function
> > exists (while rarely used).
> >
> > Maybe it's better to switch to this? Or maybe there's some reason not
> > to use this?
>
> Agreed. DRM drivers are advised to use managed cleanup for their data
> structures. So rather switch to drmm_kms_helper_poll_init(). That also
> resolves the problem that loongson never calls poll_fini on regular
> removals.
>
> Best regards
> Thomas
>
> >
> > Thanks,
> > Icenowy
> >
> >> This issue was identified during our ongoing static-analysis research
> >> while
> >> reviewing kernel code.
> >>
> >> Fixes: f39db26c5428 ("drm: Add kms driver for loongson display
> >> controller")
> >> Cc: stable@vger.kernel.org
> >> Co-developed-by: Ijae Kim <ae878000@gmail.com>
> >> Signed-off-by: Ijae Kim <ae878000@gmail.com>
> >> Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
> >> ---
> >>   drivers/gpu/drm/loongson/lsdc_drv.c | 11 ++++++++---
> >>   1 file changed, 8 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/loongson/lsdc_drv.c
> >> b/drivers/gpu/drm/loongson/lsdc_drv.c
> >> index abf5bf68ee..3db1f8690a 100644
> >> --- a/drivers/gpu/drm/loongson/lsdc_drv.c
> >> +++ b/drivers/gpu/drm/loongson/lsdc_drv.c
> >> @@ -297,7 +297,7 @@ static int lsdc_pci_probe(struct pci_dev *pdev,
> >> const struct pci_device_id *ent)
> >>      if (loongson_vblank) {
> >>              ret = drm_vblank_init(ddev, descp->num_of_crtc);
> >>              if (ret)
> >> -                    return ret;
> >> +                    goto err_poll_fini;
> >>
> >>              ret = devm_request_irq(&pdev->dev, pdev->irq,
> >>                                     descp->funcs->irq_handler,
> >> @@ -305,7 +305,7 @@ static int lsdc_pci_probe(struct pci_dev *pdev,
> >> const struct pci_device_id *ent)
> >>                                     dev_name(&pdev->dev), ddev);
> >>              if (ret) {
> >>                      drm_err(ddev, "Failed to register interrupt:
> >> %d\n", ret);
> >> -                    return ret;
> >> +                    goto err_poll_fini;
> >>              }
> >>
> >>              drm_info(ddev, "registered irq: %u\n", pdev->irq);
> >> @@ -313,17 +313,22 @@ static int lsdc_pci_probe(struct pci_dev *pdev,
> >> const struct pci_device_id *ent)
> >>
> >>      ret = drm_dev_register(ddev, 0);
> >>      if (ret)
> >> -            return ret;
> >> +            goto err_poll_fini;
> >>
> >>      drm_client_setup(ddev, NULL);
> >>
> >>      return 0;
> >> +
> >> +err_poll_fini:
> >> +    drm_kms_helper_poll_fini(ddev);
> >> +    return ret;
> >>   }
> >>
> >>   static void lsdc_pci_remove(struct pci_dev *pdev)
> >>   {
> >>      struct drm_device *ddev = pci_get_drvdata(pdev);
> >>
> >> +    drm_kms_helper_poll_fini(ddev);
> >>      drm_dev_unregister(ddev);
> >>      drm_atomic_helper_shutdown(ddev);
> >>   }
>
> --
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
> GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG
> Nürnberg)
>
>
>

[-- Attachment #2: Type: text/html, Size: 5915 bytes --]

Claude review: drm/loongson: clean up KMS polling on probe failure

[Claude Code Review Bot]

Overall Series Review

Subject: drm/loongson: clean up KMS polling on probe failure
Author: Myeonghun Pak <mhun512@gmail.com>
Patches: 4
Reviewed: 2026-05-16T14:50:22.210155

---

This is a single patch that addresses a real bug: `drm_kms_helper_poll_init()` is called during probe but `drm_kms_helper_poll_fini()` is never called on either the error paths or the remove path. The analysis is correct — the polling timer/work would leak on probe failure and on device removal.

However, **the proposed fix is not the right approach**. The loongson driver already uses `devm_drm_dev_alloc()` and `drmm_mode_config_init()` — it follows the DRM managed resource pattern. The proper fix is to replace `drm_kms_helper_poll_init()` with `drmm_kms_helper_poll_init()` (a one-line change), which registers a managed cleanup action so that `drm_kms_helper_poll_fini()` is called automatically when the DRM device is torn down. This eliminates the need for any manual `goto` cleanup labels or explicit `poll_fini` calls in remove.

Additionally, the remove-path change has an ordering concern: the patch places `drm_kms_helper_poll_fini()` **before** `drm_dev_unregister()`, but the standard tear-down order (as seen in e.g. `verisilicon/vs_drm.c`) is to unregister first, then finalize polling. Calling `poll_fini` while the device is still registered could race with connector probing triggered by userspace.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/loongson: clean up KMS polling on probe failure

[Claude Code Review Bot]

Patch Review

**The bug is real, the fix is suboptimal.**

The commit message correctly identifies that `drm_kms_helper_poll_init()` at line 296 of `lsdc_drv.c` starts a polling timer that is never cleaned up on failure or removal. However:

1. **Prefer `drmm_kms_helper_poll_init()` instead.** The driver already uses managed DRM resources (`devm_drm_dev_alloc`, `drmm_mode_config_init`, `drmm_add_action_or_reset` for TTM/PLL/I2C). The idiomatic fix is a one-liner — change:

```c
drm_kms_helper_poll_init(ddev);
```
to:
```c
drmm_kms_helper_poll_init(ddev);
```

This automatically handles cleanup on both error paths and device removal, with no `goto` labels or manual `poll_fini` calls needed. The managed variant exists specifically for this purpose (see `drm_probe_helper.c:963`).

2. **Remove-path ordering is wrong.** The patch adds:
```c
+	drm_kms_helper_poll_fini(ddev);
 	drm_dev_unregister(ddev);
```
Calling `poll_fini` before `drm_dev_unregister` means the polling work is cancelled while the device is still registered with userspace. The conventional order (used by verisilicon and others) is `drm_dev_unregister` first, then `drm_kms_helper_poll_fini`. With the `drmm_` approach this ordering question goes away entirely.

3. **The `err_poll_fini` label itself is fine** in terms of correctness for the error paths — all three failure points (vblank init, IRQ request, dev register) happen after `poll_init`, so jumping to the cleanup label is correct. But again, this whole error-path dance is unnecessary with the managed API.

**Recommendation:** Request a v2 that simply switches from `drm_kms_helper_poll_init()` to `drmm_kms_helper_poll_init()` and drops the `err_poll_fini` label and the explicit `poll_fini` in remove. This is a smaller, cleaner, and more correct fix.

---
Generated by Claude Code Patch Reviewer